Page MenuHome GnuPG
Create Task
Maniphest T4975

undefined-shift in block_filter
Closed, ResolvedPublic
Actions

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                     }

Revisions and Commits

rG GnuPG
AcceptedD503 fix undefined-shift in block_filter
rG8abf065307ff common: Avoid undefined behavior of left shift operator.

Related Objects

Event Timeline

ltx created this task.Jun 12 2020, 5:19 AM
ltx removed External Link.
ltx added a revision: D503: fix undefined-shift in block_filter.
• 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 mentioned this in D503: fix undefined-shift in block_filter.Jul 10 2020, 4:26 AM
• 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
• gniibe added a commit: rG8abf065307ff: common: Avoid undefined behavior of left shift operator..Jul 16 2020, 4:20 AM
• werner added a comment.Jul 16 2020, 11:30 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