From c1f78634ec3927ddcfdc4687bc6e408c658a0ece Mon Sep 17 00:00:00 2001 From: Werner Koch Date: Thu, 5 Oct 2023 10:02:59 +0200 Subject: [PATCH] sm: Improve the octet string cramming for pkcs#12 * sm/minip12.c (need_octet_string_cramming): New. (tlv_expect_object, tlv_expect_octet_string): Run the test before cramming. * sm/minip12.c (ENABLE_DER_STRUCT_DUMPING): New but undefined macro for debug purposes. (bag_decrypted_data_p, bag_data_p): Use macro to allow dumping. -- This bug was exhibited by importing a gpgsm exported EC certificate. We use an extra test instead of retrying to allow retruning an error from malloc failure. And well, for easier reading of the code. GnuPG-bug-id: 6536 --- sm/minip12.c | 79 ++++++++++++++---- tests/cms/Makefile.am | 1 + tests/cms/samplekeys/Description-p12 | 10 +++ .../edward.tester@demo.gnupg.com.p12 | Bin 0 -> 1561 bytes 4 files changed, 73 insertions(+), 17 deletions(-) create mode 100644 tests/cms/samplekeys/edward.tester@demo.gnupg.com.p12 diff --git a/sm/minip12.c b/sm/minip12.c index 265243f3e..ed80534b6 100644 --- a/sm/minip12.c +++ b/sm/minip12.c @@ -50,6 +50,8 @@ #define DIM(v) (sizeof(v)/sizeof((v)[0])) #endif +/* Enable the next macro to dump stuff for debugging. */ +#undef ENABLE_DER_STRUCT_DUMPING static unsigned char const oid_data[9] = { @@ -111,6 +113,8 @@ static unsigned char const data_mactemplate[51] = { #define DATA_MACTEMPLATE_MAC_OFF 17 #define DATA_MACTEMPLATE_SALT_OFF 39 +/* Note that the BMP String in this template reads: + * "GnuPG exported certificate ffffffff" */ static unsigned char const data_attrtemplate[106] = { 0x31, 0x7c, 0x30, 0x55, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x09, 0x14, 0x31, @@ -210,6 +214,8 @@ static int opt_verbose; static unsigned char *cram_octet_string (const unsigned char *input, size_t length, size_t *r_newlength); +static int need_octet_string_cramming (const unsigned char *input, + size_t length); @@ -560,7 +566,7 @@ tlv_expect_sequence (struct tlv_ctx_s *tlv) return _tlv_push (tlv); } -/* Variant of tlv_expect_sequence to be used for the ouyter sequence +/* Variant of tlv_expect_sequence to be used for the outer sequence * of an object which might have padding after the ASN.1 data. */ static gpg_error_t tlv_expect_top_sequence (struct tlv_ctx_s *tlv) @@ -618,7 +624,8 @@ tlv_expect_object (struct tlv_ctx_s *tlv, int class, int tag, if (!tlv->ti.length) return (tlv->lasterr = gpg_error (GPG_ERR_TOO_SHORT)); - if (class == CLASS_CONTEXT && tag == 0 && tlv->ti.is_constructed) + if (class == CLASS_CONTEXT && tag == 0 && tlv->ti.is_constructed + && need_octet_string_cramming (p, tlv->ti.length)) { char *newbuffer; @@ -665,7 +672,8 @@ tlv_expect_octet_string (struct tlv_ctx_s *tlv, int encapsulates, if (!(n=tlv->ti.length)) return (tlv->lasterr = gpg_error (GPG_ERR_TOO_SHORT)); - if (encapsulates && tlv->ti.is_constructed) + if (encapsulates && tlv->ti.is_constructed + && need_octet_string_cramming (p, n)) { char *newbuffer; @@ -859,6 +867,39 @@ cram_octet_string (const unsigned char *input, size_t length, } +/* Return true if (INPUT,LENGTH) is a structure which should be passed + * to cram_octet_string. This is basically the same loop as in + * cram_octet_string but without any actual copying. */ +static int +need_octet_string_cramming (const unsigned char *input, size_t length) +{ + const unsigned char *s = input; + size_t n = length; + struct tag_info ti; + + if (!length) + return 0; + + while (n) + { + if (parse_tag (&s, &n, &ti)) + return 0; + if (ti.class == CLASS_UNIVERSAL && ti.tag == TAG_OCTET_STRING + && !ti.ndef && !ti.is_constructed) + { + s += ti.length; + n -= ti.length; + } + else if (ti.class == CLASS_UNIVERSAL && !ti.tag && !ti.is_constructed) + break; /* Ready */ + else + return 0; + } + + return 1; +} + + static int string_to_key (int id, char *salt, size_t saltlen, int iter, const char *pw, int req_keylen, unsigned char *keybuf) @@ -1173,13 +1214,15 @@ bag_decrypted_data_p (const void *plaintext, size_t length) const unsigned char *p = plaintext; size_t n = length; - /* { */ - /* # warning debug code is enabled */ - /* FILE *fp = fopen ("tmp-minip12-plain-data.der", "wb"); */ - /* if (!fp || fwrite (p, n, 1, fp) != 1) */ - /* exit (2); */ - /* fclose (fp); */ - /* } */ +#ifdef ENABLE_DER_STRUCT_DUMPING + { + # warning debug code is enabled + FILE *fp = fopen ("tmp-minip12-plain-data.der", "wb"); + if (!fp || fwrite (p, n, 1, fp) != 1) + exit (2); + fclose (fp); + } +#endif /*ENABLE_DER_STRUCT_DUMPING*/ if (parse_tag (&p, &n, &ti)) return 0; @@ -1696,13 +1739,15 @@ bag_data_p (const void *plaintext, size_t length) const unsigned char *p = plaintext; size_t n = length; -/* { */ -/* # warning debug code is enabled */ -/* FILE *fp = fopen ("tmp-minip12-plain-key.der", "wb"); */ -/* if (!fp || fwrite (p, n, 1, fp) != 1) */ -/* exit (2); */ -/* fclose (fp); */ -/* } */ +#ifdef ENABLE_DER_STRUCT_DUMPING + { +# warning debug code is enabled + FILE *fp = fopen ("tmp-minip12-plain-key.der", "wb"); + if (!fp || fwrite (p, n, 1, fp) != 1) + exit (2); + fclose (fp); + } +#endif /*ENABLE_DER_STRUCT_DUMPING*/ if (parse_tag (&p, &n, &ti) || ti.class || ti.tag != TAG_SEQUENCE) return 0; diff --git a/tests/cms/Makefile.am b/tests/cms/Makefile.am index 7efdf37b1..d5d753902 100644 --- a/tests/cms/Makefile.am +++ b/tests/cms/Makefile.am @@ -99,6 +99,7 @@ EXTRA_DIST = $(XTESTS) $(KEYS) $(CERTS) $(TEST_FILES) \ samplekeys/opensc-test.p12 \ samplekeys/t5793-openssl.pfx \ samplekeys/t5793-test.pfx \ + samplekeys/edward.tester@demo.gnupg.com.p12 \ samplemsgs/pwri-sample.cbc.p7m \ samplemsgs/pwri-sample.cbc-2.p7m \ samplemsgs/pwri-sample.gcm.p7m \ diff --git a/tests/cms/samplekeys/Description-p12 b/tests/cms/samplekeys/Description-p12 index f882de9ea..6fbbd82cf 100644 --- a/tests/cms/samplekeys/Description-p12 +++ b/tests/cms/samplekeys/Description-p12 @@ -1,4 +1,6 @@ # Description-p12 - Machine readable description of our P12 test vectors +# The Cert line gives the SHA1 fingerprint of the certificate +# The Key line gives a hash of the key parameters as returned by minip12.c Name: ov-user.p12 Desc: Private test key from www.openvalidation.org @@ -30,3 +32,11 @@ Desc: QuaVadis format of t5793-openssl Pass: test Cert: 80348a438e4b803b99e708da0b7fdd0659dedd15 Key: c271e44ab4fb19ca1aae71102ea4d7292ccc981d + +Name: edward.tester@demo.gnupg.com.p12 +Desc: GnuPG exported Brainpool certificate +Pass: abc,123456 +Cert: ff810b9281a43c394aa138e9c7fd4c0193216fa6 +Key: 94c6d0b067370a8f2a09ae43cfe8d700bbd61e75 + +# eof # diff --git a/tests/cms/samplekeys/edward.tester@demo.gnupg.com.p12 b/tests/cms/samplekeys/edward.tester@demo.gnupg.com.p12 new file mode 100644 index 0000000000000000000000000000000000000000..a6f983780f6ca06c9be9cef235d46fa945430bfd GIT binary patch literal 1561 zcmY+Ec{G%39LL|6*VrOqG8Yv}$|&#ThRHfeldWm&-Lfy)W;8~cO3Vz}kfKZ3#uCO7 zQ{#>pTlN~n4TVN&Q#WM`t?szz-rK!@{Lc4zKHqbG=RD6JADDqaih<%VLt+det($~T zT9N}LK^8+o05K%GMYtViK;Azp1d9O?M94u51Vpj-M+3s@2${b(tN{_QJS2svr)|=m zXowOMlLF!y5bC#sLZVkTReZX8&qUSxi_My6vA!!roY*}T@#+H3*@VoaW6FKzE;|Mb zmo(&6X+6vM0r;)S$fvjj^wb(2LCTYBKSJt@+#Xy;ZoEI=o6NO(MMzPCVx@D}FJ@rM z5>Q=68)e`?qF*DU17o`vpeP&`eE9HqDh1FIO(ArY-O{uO-_Stt5 zeM86wllQr?PDY8zmhb-1u?VJ83*8h=cQX`!^^Kj>WqW;I;JlT-_7n5-sdsaFk~E=V zUcpO?<(4luR5$uTIO&x3oI%B?tgrp0j^FD)=8bQO{L(thQ`d??3blGC`;n^JTPHd~ zj13RZI@*~>jw__y)poHGs(0P-9avYJk63>}{KUj#LiC6~ABm?z^^r6!Gn7JP&7&MH zdF>RuWy*@$@BTJkjwbS#XMwxthbct1C<_Z<1U5NHn zGs&n{R4v95E{CL@h%4pw+YBQeSh_A=?Al(*^aq-)d~aWIS{_ z^qRpX*6+Wp{v2%+J=dJHRNQp zPEzm5ZP=-saMtsiC3$)9{ixJzLw#dJj{5WxBww8w!sxD?iQX}8`nDo9dpYO;N=?b2 zxG~!PgA0B_*U6nm3!S8#PQ&|W zhlK10y=}B5;lSJiey5Mjym~G{HV&FUD2~qwD-r;RgOLkSc=38- zcl06`AD8TB8|R_WuQG@jXJ#KysJ(QLb!#%qM#pVo<;=CE%1Z{6snZ~zTxD9`U5aQ6-u$8242812P=A@KBo7rh z%gzTXmI=_Uud#quW1~xiq_@Lu$7-ENv*mavUd%O!jV6Mg)F9(7X6iJiThj*K^wfXa zgh|zFj;qUOa_DoU`TWTq!OQdEtSZ8EOKP%oZEm9Glcc5(6dymewU!Nx2IsvhO<_CQ zv6$1a>%UKwR>t5r+5-Ck5)cg#M5zG;0w;h-fDBLos^}abBL1RWy@dclqVWfO|7tV< zEMNfa0x*D~i2DMD@a7+Dk(l+6@(Nw{3cc7AYgb_Wrt6ynn661KfibWqERB#r%ZrPt oN&t{@c(F$V$7b0aj9<|~l_+pDVo;5c)T>djsne-Jb@dPb1p3sgLjV8( literal 0 HcmV?d00001