Undefined shift in parse_symkeyenc
Closed, ResolvedPublic

Description

Found using oss-fuzz

in file g10/parse-packet.c, function parse_symkeyenc, there is an undefined shift with -1 as exponent in macro S2K_DECODE_COUNT

Patch could be so simple as using iobuf_get_noeof instead of iobuf_getas done elsewhere in this function

diff --git a/g10/parse-packet.c b/g10/parse-packet.c
index 7f4c7b5c6..0d28e7ac1 100644
--- a/g10/parse-packet.c
+++ b/g10/parse-packet.c
@@ -1191,7 +1191,7 @@ parse_symkeyenc (IOBUF inp, int pkttype, unsigned long pktlen,
     }
   if (s2kmode == 3)
     {
-      k->s2k.count = iobuf_get (inp);
+      k->s2k.count = iobuf_get_noeof (inp);
       pktlen--;
     }
   k->seskeylen = seskeylen;

Bug can be reproduced running
gpg --list-packets clusterfuzz-testcase-minimized-fuzz_list-4890449025171456.dms

There is the same bug and fix in function parse_key :

diff --git a/g10/parse-packet.c b/g10/parse-packet.c
index 0d28e7ac1..b147179e2 100644
--- a/g10/parse-packet.c
+++ b/g10/parse-packet.c
@@ -2533,7 +2533,7 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
                      err = gpg_error (GPG_ERR_INV_PACKET);
                      goto leave;
                    }
-                 ski->s2k.count = iobuf_get (inp);
+                 ski->s2k.count = iobuf_get_noeof (inp);
                  pktlen--;
                  if (list_mode)
                    es_fprintf (listfp, "\tprotect count: %lu (%lu)\n",
aheinecke assigned this task to gniibe.Aug 9 2018, 8:41 AM
aheinecke added a subscriber: aheinecke.

Thanks for the tests and the report.

Gniibe in the absence of Werner could you please take this?

OK, I take this ticket.

@catenacyber , thanks for your report. Good catch. Your suggested patch looks correct.
I'm going to apply now.

gniibe triaged this task as Normal priority.Aug 10 2018, 8:24 AM
werner added a subscriber: werner.Sep 10 2018, 7:36 AM

@catenacyber thanks fo this bug report.

BTW, I would like to ask you to remove me from that Google bug distribution mailing list. I get all kind of mails about your fuzzing changes but the actual content of these mails is not readable to me because I don't have a Google account. In fact, I removed my Google account some years ago for a reason and I won't register a new one. So pretty please stop sending me these arbitrary messages but instead post actual report here at dev.gnupg.org. Thanks.

gniibe closed this task as Resolved.Sep 10 2018, 7:47 AM

I confirmed: Now, all use cases of iobuf_get check against negative value or are using iobuf_get_eof.
So, closing.

ok @werner
Should I change it to another mail address (@gniibe if you are interested) or should I just use mine ?

Another address does not help as long as we are forced to use a Google account. That is not subject to discussion. sorry.

@gniibe there seems to be one remaining issue.
Even with iobuf_get_noeof, we have to cast to an unsigned integer before shifting 24 places to avoid undefined behavior :

diff --git a/common/iobuf.c b/common/iobuf.c
index 5eeba8fe6..1b9722d0a 100644
--- a/common/iobuf.c
+++ b/common/iobuf.c
@@ -878,7 +878,7 @@ block_filter (void *opaque, int control, iobuf_t chain, byte * buffer,
                    }
                  else if (c == 255)
                    {
-                     a->size = iobuf_get_noeof (chain) << 24;
+                     a->size = (size_t)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)
``

In this case the data is taken from a byte buffer, (unsigned char *). I can't see why iobuf_readbyte should be invoked here.