Undefined shift in parse_symkeyenc
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	catenacyber
	Aug 6 2018, 9:25 AM

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

clusterfuzz-testcase-minimized-fuzz_list-4890449025171456.dms81 BDownload

Revisions and Commits

rG GnuPG
	rG0383e7fed7b2 common: Use iobuf_get_noeof to avoid undefined behaviors.
	rGf80346f42df4 common: Use iobuf_get_noeof to avoid undefined behaviors.
	rG1b309d9f6199 g10: Fix undefined behavior when EOF in parsing packet for S2K.
	rG822c63384506 g10: Fix undefined behavior when EOF in parsing packet for S2K.

Event Timeline

catenacyber created this task.Aug 6 2018, 9:25 AM

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",

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

• gniibe added a commit: rG822c63384506: g10: Fix undefined behavior when EOF in parsing packet for S2K..Aug 10 2018, 8:42 AM

• gniibe added a commit: rG1b309d9f6199: g10: Fix undefined behavior when EOF in parsing packet for S2K..

• gniibe added a commit: rGf80346f42df4: common: Use iobuf_get_noeof to avoid undefined behaviors..Sep 10 2018, 6:58 AM

• gniibe added a commit: rG0383e7fed7b2: common: Use iobuf_get_noeof to avoid undefined behaviors..

@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.

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.

Asked in https://github.com/google/oss-fuzz/pull/1806

gouttegd added a subscriber: gouttegd.Sep 12 2018, 10:08 AM

@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.

Undefined shift in parse_symkeyencClosed, ResolvedPublicActions

Description

Revisions and Commits

Event Timeline

Undefined shift in parse_symkeyenc
Closed, ResolvedPublic
Actions