undefined-shift in block_filter
Closed, ResolvedPublic

Description

UndefinedBehaviorSanitizer error report while fuzzing gnupg-2.2.17(same issue on master-branch):

iobuf.c:859:43: runtime error: left shift of 255 by 24 places cannot be represented in type 'int'
    #0 0x6e9caf in block_filter /src/gnupg/common/iobuf.c:859:43
    #1 0x6e6ae3 in underflow_target /src/gnupg/common/iobuf.c:1841:7
    #2 0x6e5215 in underflow /src/gnupg/common/iobuf.c:1750:10
    #3 0x6e5041 in iobuf_readbyte /src/gnupg/common/iobuf.c:1974:17
    #4 0x4f8693 in parse_plaintext /src/gnupg/g10/parse-packet.c:3125:10
    #5 0x4df33a in parse /src/gnupg/g10/parse-packet.c:848:12
    #6 0x4dcb6f in dbg_parse_packet /src/gnupg/g10/parse-packet.c:302:12
    #7 0x5d8b4e in do_proc_packets /src/gnupg/g10/mainproc.c:1441:14
    #8 0x5d89ee in proc_packets /src/gnupg/g10/mainproc.c:1300:8
    #9 0x51cacf in decrypt_messages /src/gnupg/g10/decrypt.c:264:12
    #10 0x4b5ebe in LLVMFuzzerTestOneInput /src/gnupg/tests/fuzz/fuzz_decrypt.c:161:5
    #11 0x444c31 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:556:15
    #12 0x42f851 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:292:6
    #13 0x43550e in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:774:9
    #14 0x45f682 in main /src/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10
    #15 0x7f8b2d50482f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #16 0x408c08 in _start (/out/gnupg/fuzz_decrypt+0x408c08)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior iobuf.c:859:43 in

The problem occurs in line 859, function block_filter common/iobuf.c. when iobuf_get_noeof (chain) return 0xff, after left shift 24 will be 0xff00000, it be treated as an 'int' num, after force to type 'size_t' the value will be 0xffffffffff000000 (my os: ubuntu16.04 , arch is x86_64). That is not as expect
So 'iobuf_get_noeof (chain) ' may should force to size_t before shift.

857                   else if (c == 255)
858                     {
859                       a->size = iobuf_get_noeof (chain) << 24;
860                       a->size |= iobuf_get_noeof (chain) << 16;
861                       a->size |= iobuf_get_noeof (chain) << 8;
862                       if ((c = iobuf_get (chain)) == -1)
863                         {
864                           log_error ("block_filter: invalid 4 byte length\n");
865                           rc = GPG_ERR_BAD_DATA;
866                           break;
867                         }
868                       a->size |= c;
869                       a->partial = 2;
870                       if (!a->size)
871                         {
872                           a->eof = 1;
873                           if (!n)
874                             rc = -1;
875                           break;
876                         }
877                     }
ltx created this task.Jun 12 2020, 5:19 AM
ltx removed External Link.
werner added a subscriber: werner.Jun 12 2020, 11:05 PM

Please describe the problem and don't just paste compiler output.

ltx updated the task description. (Show Details)Jun 13 2020, 5:17 AM
ltx updated the task description. (Show Details)Jun 13 2020, 5:23 AM
werner triaged this task as Normal priority.Jun 13 2020, 3:02 PM
werner added a project: gnupg (gpg22).

Thanks for explaining; this may indeed lead to a followup processing error of correct data. However, I don't expect to ever see a fixed length header of 2GiB or more because the sender would have had to buffer all that data in the first place.

gniibe added a subscriber: gniibe.Jul 10 2020, 4:34 AM

While I see that it's not the matter of actual use case (but how gpg can be immune to fuzzing), code clean up would be good here.

I think that possible performance loss is low, and a change like this is better (than just adding size_t cast):

diff --git a/common/iobuf.c b/common/iobuf.c
index 43f2e101c..691b81880 100644
--- a/common/iobuf.c
+++ b/common/iobuf.c
@@ -909,16 +909,22 @@ block_filter (void *opaque, int control, iobuf_t chain, byte * buffer,
 		    }
 		  else if (c == 255)
 		    {
-		      a->size = iobuf_get_noeof (chain) << 24;
-		      a->size |= iobuf_get_noeof (chain) << 16;
-		      a->size |= iobuf_get_noeof (chain) << 8;
-		      if ((c = iobuf_get (chain)) == -1)
+                      size_t len = 0;
+                      int i;
+
+                      for (i = 0; i < 4; i++)
+                        {
+                          if ((c = iobuf_get (chain)) == -1)
+                            break;
+                          len = ((len << 8) | c);
+                        }
+		      if (i < 4)
 			{
 			  log_error ("block_filter: invalid 4 byte length\n");
 			  rc = GPG_ERR_BAD_DATA;
 			  break;
 			}
-		      a->size |= c;
+                      a->size = len;
                       a->partial = 2;
                       if (!a->size)
                         {
gniibe claimed this task.Jul 13 2020, 4:18 AM

Well, it changes the behaviour on error and thus it should not be backported to 2.2 so that existsing error reports about corrupted data don't change. Fine for master.

werner closed this task as Resolved.Aug 4 2020, 10:19 AM