Mailing List Archive

On Thu, 21 Apr 2022 22:19, Tavis Ormandy said:
> Hello, I noticed that if there are two opaque identical public subkey
> packets, but one is truncated, gpg crashes on import in gcry_mpi_cmp()

Thanks. Tracked at https://dev.gnupg.org/T5940


Shalom-Salam,

Werner

--
The pioneers of a warless world are the youth that
refuse military service. - A. Einstein

On Thu, 2022-04-21 at 22:19:21 -0000, Tavis Ormandy via Gnupg-devel wrote:
> Hello, I noticed that if there are two opaque identical public subkey
> packets, but one is truncated, gpg crashes on import in gcry_mpi_cmp()
>
> I just did this to repro:
>
> $ gpgcompose --public-key taviso --public-subkey taviso \
> --user-id anything --public-subkey taviso \
> | perl -p -e 's/(\xb9..\x04....)\x01/\1\xff/g' \
> | head -c -1 | gpg --import
> gpg: premature eof while reading rest of packet
> gpg: signal Segmentation fault caught ... exiting
> Segmentation fault
>
> That ugly horrible regex is:
>
> \xb9 : Find old-style public-subkey with 2 byte length
> .. : skip over the length bytes
> \x04 : looking for version 4
> .... : skip over the timestamp
> \x01 : change the algorithm so it's not recognized by gcry_mpi_cmp.
>
> Then piping it into head to truncate the last packet.
>
> I think it should work on any RSA public key, e.g. just replace
> the --public-subkey taviso with the id, 4B092E28 works.

Had time to reproduce this issue today with gpg 2.3.5 and valgrind:

# off=528 ctb=b9 tag=14 hlen=3 plen=525
:public sub key packet:
version 4, algo 255, created 1576527848, expires 0
unknown algorithm 255
gpg: can't handle public key algorithm 255
# off=1056 ctb=b4 tag=13 hlen=2 plen=32
:user ID packet: "Robert Bartel <r.bartel@gmx.net>"
# off=1090 ctb=b9 tag=14 hlen=3 plen=525
:public sub key packet:
version 4, algo 255, created 1576527848, expires 0
unknown algorithm 255
gpg: premature eof while reading rest of packet
gpg: pub rsa4096/0xABD4234D9E682972 2019-12-16 Robert Bartel <r.bartel@gmx.net>
==3331== Invalid read of size 8
==3331== at 0x484E9B5: bcmp (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==3331== by 0x42444D: cmp_public_keys (free-packet.c:514)
==3331== by 0x467952: collapse_subkeys (import.c:4137)
==3331== by 0x468149: import_one_real (import.c:1977)
==3331== by 0x46926F: import_one (import.c:2418)
==3331== by 0x469C67: import (import.c:673)
==3331== by 0x46A17A: import_keys_internal (import.c:569)
==3331== by 0x46A22D: import_keys (import.c:609)
==3331== by 0x412F32: main (gpg.c:4847)
==3331== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==3331==

One robustness fix would be to introduce NULL pointer checks in
cmp_public_keys (free-packet.c:514), as the second subkey's opaque mpi
value is introduced as NULL, because it was truncated.

But the real fix seems to be adding error checks for read_rest() in
g10/parse-packet.c:2584: when it returns NULL then set an error so the
import fails because of an invalid packet and does not continue
potentially using the NULL pointer later on.

The same pattern should probably be applied to the similar calls of
read_rest() in lines 2355 and 2897, just to be sure.

Hope that helps a bit,
Robert