sm: Adding missing stuff to the PKCS#12 parser rewrite.

* sm/minip12.c (struct bufferlist_s): New.
(struct tlv_ctx_s): Add bufferlist.
(tlv_register_buffer): New.
(tlv_release): Release bufferlist.
(tlv_expect_object): Handle octet string cramming.
(tlv_expect_octet_string): Ditto.
(cram_octet_string): Changed interface.  We don't need the
input_consumed value anymore.

* sm/minip12.c (parse_shrouded_key_bag): Also parse the attribute set.

* sm/t-minip12.c (main): Add option --no-extra.
(cert_collect_cb, run_tests_from_file): Fix memory leak

* tests/cms/samplekeys/t5793-openssl.pfx: New from T5793.
* tests/cms/samplekeys/t5793-test.pfx: Ditto.
* tests/cms/samplekeys/Description-p12: Add them.
* tests/cms/Makefile.am (EXTRA_DIST): Add samplekeys.
--

This should finish the rewrite of the pkcsc#12 parser for now.  More
fun is likely to come.

GnuPG-bug-id: 6536, 5793

6 files changed, 190 insertions, 90 deletions

diff --git a/sm/minip12.c b/sm/minip12.c
index 69e23455a..eafebfe67 100644
--- a/sm/minip12.c
+++ b/sm/minip12.c
@@ -148,6 +148,14 @@ struct tag_info
 
 #define TLV_MAX_DEPTH 20
 
+
+struct bufferlist_s
+{
+  struct bufferlist_s *next;
+  char *buffer;
+};
+
+
 /* An object to control the ASN.1 parsing.  */
 struct tlv_ctx_s
 {
@@ -163,6 +171,8 @@ struct tlv_ctx_s
   gpg_error_t lasterr; /* Last error from tlv function.  */
   const char *lastfunc;/* Name of last called function.  */
 
+  struct bufferlist_s *bufferlist;  /* To keep track of amlloced buffers. */
+
   unsigned int pop_count;/* Number of pops by tlv_next.  */
   unsigned int stacklen; /* Used size of the stack.  */
   struct {
@@ -198,6 +208,12 @@ struct p12_parse_ctx_s
 static int opt_verbose;
 
 
+static unsigned char *cram_octet_string (const unsigned char *input,
+                                         size_t length, size_t *r_newlength);
+
+
+
+
 void
 p12_set_verbosity (int verbose, int debug)
 {
@@ -354,9 +370,36 @@ tlv_new (const unsigned char *buffer, size_t bufsize)
 }
 
 
+/* This function can be used to store a malloced buffer into the TLV
+ * object.  Ownership of BUFFER is thus transferred to TLV.  This
+ * buffer will then only be released by tlv_release. */
+static gpg_error_t
+tlv_register_buffer (struct tlv_ctx_s *tlv, char *buffer)
+{
+  struct bufferlist_s *item;
+
+  item = xtrycalloc (1, sizeof *item);
+  if (!item)
+    return gpg_error_from_syserror ();
+  item->buffer = buffer;
+  item->next = tlv->bufferlist;
+  tlv->bufferlist = item;
+  return 0;
+}
+
+
 static void
 tlv_release (struct tlv_ctx_s *tlv)
 {
+  if (!tlv)
+    return;
+  while (tlv->bufferlist)
+    {
+      struct bufferlist_s *save = tlv->bufferlist->next;
+      xfree (tlv->bufferlist->buffer);
+      xfree (tlv->bufferlist);
+      tlv->bufferlist = save;
+    }
   xfree (tlv);
 }
 
@@ -457,7 +500,8 @@ tlv_next (struct tlv_ctx_s *tlv)
       /* End tag while in ndef container.  Skip the tag, and pop.  */
       tlv->offset += n - (tlv->bufsize - tlv->offset);
       err = _tlv_pop (tlv);
-      // FIXME see above and run peek again.
+      /* FIXME: We need to peek whether there is another end tag and
+       *        pop again.  We can't modify the TLV object, though.  */
       if (err)
         return (tlv->lasterr = err);
     }
@@ -558,24 +602,42 @@ tlv_expect_set (struct tlv_ctx_s *tlv)
 
 /* Expect an object of CLASS with TAG and store its value at
  * (R_DATA,R_DATALEN).  Then skip over its value to the next tag.
- * Note that the stored value are not allocated but point into
+ * Note that the stored value is not allocated but points into
  * TLV.  */
 static gpg_error_t
 tlv_expect_object (struct tlv_ctx_s *tlv, int class, int tag,
                    unsigned char const **r_data, size_t *r_datalen)
 {
+  gpg_error_t err;
   const unsigned char *p;
 
   tlv->lastfunc = __func__;
-  if (!(tlv->ti.class == class && tlv->ti.tag == tag
-        && !tlv->ti.is_constructed))
+  if (!(tlv->ti.class == class && tlv->ti.tag == tag))
     return (tlv->lasterr = gpg_error (GPG_ERR_INV_OBJ));
   p = tlv->buffer + tlv->offset;
   if (!tlv->ti.length)
     return (tlv->lasterr = gpg_error (GPG_ERR_TOO_SHORT));
 
-  *r_data = p;
-  *r_datalen = tlv->ti.length;
+  if (class == CLASS_CONTEXT && tag == 0 && tlv->ti.is_constructed)
+    {
+      char *newbuffer;
+
+      newbuffer = cram_octet_string (p, tlv->ti.length, r_datalen);
+      if (!newbuffer)
+        return (tlv->lasterr = gpg_error (GPG_ERR_BAD_BER));
+      err = tlv_register_buffer (tlv, newbuffer);
+      if (err)
+        {
+          xfree (newbuffer);
+          return (tlv->lasterr = err);
+        }
+      *r_data = newbuffer;
+    }
+  else
+    {
+      *r_data = p;
+      *r_datalen = tlv->ti.length;
+    }
 
   tlv->offset += tlv->ti.length;
   return 0;
@@ -591,21 +653,40 @@ static gpg_error_t
 tlv_expect_octet_string (struct tlv_ctx_s *tlv, int encapsulates,
                          unsigned char const **r_data, size_t *r_datalen)
 {
+  gpg_error_t err;
   const unsigned char *p;
   size_t n;
 
   tlv->lastfunc = __func__;
   if (!(tlv->ti.class == CLASS_UNIVERSAL && tlv->ti.tag == TAG_OCTET_STRING
-        && !tlv->ti.is_constructed))
+        && (!tlv->ti.is_constructed || encapsulates)))
     return (tlv->lasterr = gpg_error (GPG_ERR_INV_OBJ));
   p = tlv->buffer + tlv->offset;
   if (!(n=tlv->ti.length))
     return (tlv->lasterr = gpg_error (GPG_ERR_TOO_SHORT));
 
-  if (r_data)
-    *r_data = p;
-  if (r_datalen)
-    *r_datalen = tlv->ti.length;
+  if (encapsulates && tlv->ti.is_constructed)
+    {
+      char *newbuffer;
+
+      newbuffer = cram_octet_string (p, n, r_datalen);
+      if (!newbuffer)
+        return (tlv->lasterr = gpg_error (GPG_ERR_BAD_BER));
+      err = tlv_register_buffer (tlv, newbuffer);
+      if (err)
+        {
+          xfree (newbuffer);
+          return (tlv->lasterr = err);
+        }
+      *r_data = newbuffer;
+    }
+  else
+    {
+      if (r_data)
+        *r_data = p;
+      if (r_datalen)
+        *r_datalen = tlv->ti.length;
+    }
   if (encapsulates)
     return _tlv_push (tlv);
 
@@ -716,37 +797,37 @@ tlv_expect_object_id (struct tlv_ctx_s *tlv,
 
 
 /* Given an ASN.1 chunk of a structure like:
-
-     24 NDEF:       OCTET STRING  -- This is not passed to us
-     04    1:         OCTET STRING  -- INPUT point s to here
-            :           30
-     04    1:         OCTET STRING
-            :           80
-          [...]
-     04    2:         OCTET STRING
-            :           00 00
-            :         } -- This denotes a Null tag and are the last
-                        -- two bytes in INPUT.
-
-   Create a new buffer with the content of that octet string.  INPUT
-   is the original buffer with a length as stored at LENGTH.  Returns
-   NULL on error or a new malloced buffer with the length of this new
-   buffer stored at LENGTH and the number of bytes parsed from input
-   are added to the value stored at INPUT_CONSUMED.  INPUT_CONSUMED is
-   allowed to be passed as NULL if the caller is not interested in
-   this value. */
+ *
+ *   24 NDEF:       OCTET STRING  -- This is not passed to us
+ *   04    1:         OCTET STRING  -- INPUT point s to here
+ *          :           30
+ *   04    1:         OCTET STRING
+ *          :           80
+ *        [...]
+ *   04    2:         OCTET STRING
+ *          :           00 00
+ *          :         } -- This denotes a Null tag and are the last
+ *                      -- two bytes in INPUT.
+ *
+ * The example is from Mozilla Firefox 1.0.4 which actually exports
+ * certs as single byte chunks of octet strings.
+ *
+ * Create a new buffer with the content of that octet string.  INPUT
+ * is the original buffer with a LENGTH.  Returns
+ * NULL on error or a new malloced buffer with its actual used length
+ * stored at R_NEWLENGTH.    */
 static unsigned char *
-cram_octet_string (const unsigned char *input, size_t *length,
-                   size_t *input_consumed)
+cram_octet_string (const unsigned char *input, size_t length,
+                   size_t *r_newlength)
 {
   const unsigned char *s = input;
-  size_t n = *length;
+  size_t n = length;
   unsigned char *output, *d;
   struct tag_info ti;
 
   /* Allocate output buf.  We know that it won't be longer than the
      input buffer. */
-  d = output = gcry_malloc (n);
+  d = output = gcry_malloc (length);
   if (!output)
     goto bailout;
 
@@ -769,20 +850,15 @@ cram_octet_string (const unsigned char *input, size_t *length,
     }
 
 
-  *length = d - output;
-  if (input_consumed)
-    *input_consumed += s - input;
+  *r_newlength = d - output;
   return output;
 
  bailout:
-  if (input_consumed)
-    *input_consumed += s - input;
   gcry_free (output);
   return NULL;
 }
 
 
-
 static int
 string_to_key (int id, char *salt, size_t saltlen, int iter, const char *pw,
                int req_keylen, unsigned char *keybuf)
@@ -1331,38 +1407,6 @@ parse_bag_encrypted_data (struct p12_parse_ctx_s *ctx, struct tlv_ctx_s *tlv)
   if (tlv_next (tlv))
     goto bailout;
 
-  /* consumed = p - p_start; */
-  /* if (ti.class == CLASS_CONTEXT && ti.tag == 0 && ti.is_constructed && ti.ndef) */
-  /*   { */
-  /*     /\* Mozilla exported certs now come with single byte chunks of */
-  /*        octet strings.  (Mozilla Firefox 1.0.4).  Arghh. *\/ */
-  /*     where = "cram-rc2or3des-ciphertext"; */
-  /*     cram_buffer = cram_octet_string ( p, &n, &consumed); */
-  /*     if (!cram_buffer) */
-  /*       goto bailout; */
-  /*     p = p_start = cram_buffer; */
-  /*     if (r_consumed) */
-  /*       *r_consumed = consumed; */
-  /*     r_consumed = NULL; /\* Donot update that value on return. *\/ */
-  /*     ti.length = n; */
-  /*   } */
-  /* else if (ti.class == CLASS_CONTEXT && ti.tag == 0 && ti.is_constructed) */
-  /*   { */
-  /*     where = "octets-rc2or3des-ciphertext"; */
-  /*     n = ti.length; */
-  /*     cram_buffer = cram_octet_string ( p, &n, &consumed); */
-  /*     if (!cram_buffer) */
-  /*       goto bailout; */
-  /*     p = p_start = cram_buffer; */
-  /*     if (r_consumed) */
-  /*       *r_consumed = consumed; */
-  /*     r_consumed = NULL; /\* Do not update that value on return. *\/ */
-  /*     ti.length = n; */
-  /*   } */
-  /* else if (ti.class == CLASS_CONTEXT && ti.tag == 0 && ti.length ) */
-  /*   ; */
-  /* else */
-  /*   goto bailout; */
   if (tlv_expect_object (tlv, CLASS_CONTEXT, 0, &data, &datalen))
     goto bailout;
 
@@ -1538,7 +1582,8 @@ parse_bag_encrypted_data (struct p12_parse_ctx_s *ctx, struct tlv_ctx_s *tlv)
                              keyelem_count, gpg_strerror (err));
                   goto bailout;
                 }
-              log_debug ("RSA key parameter %d found\n", keyelem_count);
+              if (opt_verbose > 1)
+                log_debug ("RSA key parameter %d found\n", keyelem_count);
               keyelem_count++;
             }
           if (err && gpg_err_code (err) != GPG_ERR_EOF)
@@ -1683,6 +1728,7 @@ parse_shrouded_key_bag (struct p12_parse_ctx_s *ctx, struct tlv_ctx_s *tlv)
   size_t saltlen;
   char iv[16];
   unsigned int iter;
+  struct tlv_ctx_s *saved_tlv = NULL;
   int renewed_tlv = 0;  /* True if the TLV must be released.  */
   unsigned char *plain = NULL;
   int is_pbes2 = 0;
@@ -1865,8 +1911,10 @@ parse_shrouded_key_bag (struct p12_parse_ctx_s *ctx, struct tlv_ctx_s *tlv)
                           : GCRY_CIPHER_3DES,
                  bag_data_p);
 
+
   /* We do not need the TLV anymore and allocated a new one.  */
   where = "shrouded_key_bag.decrypted-text";
+  saved_tlv = tlv;
   tlv = tlv_new (plain, datalen);
   if (!tlv)
     {
@@ -1874,6 +1922,8 @@ parse_shrouded_key_bag (struct p12_parse_ctx_s *ctx, struct tlv_ctx_s *tlv)
       goto bailout;
     }
   renewed_tlv = 1;
+  if (opt_verbose > 1)
+    log_debug ("new parser context\n");
 
   if (tlv_next (tlv))
     {
@@ -2037,10 +2087,39 @@ parse_shrouded_key_bag (struct p12_parse_ctx_s *ctx, struct tlv_ctx_s *tlv)
       err = 0;
     }
 
+  if (opt_verbose > 1)
+    log_debug ("restoring parser context\n");
+  tlv_release (tlv);
+  renewed_tlv = 0;
+  tlv = saved_tlv;
+
+  where = "shrouded_key_bag.attribute_set";
+  err = tlv_next (tlv);
+  if (gpg_err_code (err) == GPG_ERR_EOF)
+    goto leave;
+  if (err)
+    goto bailout;
+  err = tlv_expect_set (tlv);
+  if (!err)
+    { /* This is the optional set of attributes.  Skip it. */
+      tlv_skip (tlv);
+      if (opt_verbose)
+        log_info ("skipping %s\n", where);
+    }
+  else if (gpg_err_code (err) == GPG_ERR_INV_OBJ)
+    tlv_set_pending (tlv); /* The next tlv_next will be skipped.  */
+  else /* Other error.  */
+    goto bailout;
+
+
  leave:
   gcry_free (plain);
   if (renewed_tlv)
-   tlv_release (tlv);
+    {
+      tlv_release (tlv);
+      if (opt_verbose > 1)
+        log_debug ("parser context released\n");
+    }
   return err;
 
  bailout:
@@ -2322,17 +2401,6 @@ p12_parse (const unsigned char *buffer, size_t length, const char *pw,
 
   if (tlv_next (tlv))
     goto bailout;
-  if (tlv->ti.is_constructed && tlv->ti.ndef)
-    {
-      log_debug ("FIXME Put this into our TLV machinery.\n");
-      /* /\* Mozilla exported certs now come with single byte chunks of */
-      /*    octet strings.  (Mozilla Firefox 1.0.4).  Arghh. *\/ */
-      /* where = "cram-bags"; */
-      /* cram_buffer = cram_octet_string ( p, &n, NULL); */
-      /* if (!cram_buffer) */
-      /*   goto bailout; */
-      /* p = p_start = cram_buffer; */
-    }
   if (tlv_expect_octet_string (tlv, 1, NULL, NULL))
     goto bailout;
 
diff --git a/sm/t-minip12.c b/sm/t-minip12.c
index 80ba4c69e..de6b7e5cc 100644
--- a/sm/t-minip12.c
+++ b/sm/t-minip12.c
@@ -444,12 +444,14 @@ static void
 cert_collect_cb (void *opaque, const unsigned char *cert, size_t certlen)
 {
   char **certstr = opaque;
-  char *hash;
+  char *hash, *save;
 
   hash = hash_buffer (cert, certlen);
   if (*certstr)
     {
-      *certstr = xstrconcat (*certstr, ",", hash, NULL);
+      save = *certstr;
+      *certstr = xstrconcat (save, ",", hash, NULL);
+      xfree (save);
       xfree (hash);
     }
   else
@@ -645,7 +647,12 @@ run_tests_from_file (const char *descfname)
           copy_data (&p, line, lineno);
           hexdowncase (p);
           if (p && cert)
-            cert = xstrconcat (cert, ",", p, NULL);
+            {
+              char *save = cert;
+              cert = xstrconcat (save, ",", p, NULL);
+              xfree (save);
+              xfree (p);
+            }
           else
             cert = p;
         }
@@ -681,6 +688,7 @@ main (int argc, char **argv)
   char const *name = NULL;
   char const *pass = NULL;
   int ret;
+  int no_extra = 0;
 
   if (argc)
     { argc--; argv++; }
@@ -697,12 +705,18 @@ main (int argc, char **argv)
           fputs ("usage: " PGM " <pkcs12file> [<passphrase>]\n"
                  "Without <pkcs12file> a regression test is run\n"
                  "Options:\n"
+                 "  --no-extra          do not run extra tests\n"
                  "  --verbose           print timings etc.\n"
                  "                      given twice shows more\n"
                  "  --debug             flyswatter\n"
                  , stdout);
           exit (0);
         }
+      else if (!strcmp (*argv, "--no-extra"))
+        {
+          no_extra = 1;
+          argc--; argv++;
+        }
       else if (!strcmp (*argv, "--verbose"))
         {
           verbose++;
@@ -761,7 +775,7 @@ main (int argc, char **argv)
       xfree (descfname);
 
       /* Check whether we have non-public regression test cases. */
-      p = value_from_gnupg_autogen_rc ("GNUPG_EXTRA_TESTS_DIR");
+      p = no_extra? NULL:value_from_gnupg_autogen_rc ("GNUPG_EXTRA_TESTS_DIR");
       if (p)
         {
           descfname = xstrconcat  (p, "/pkcs12/Description", NULL);
diff --git a/tests/cms/Makefile.am b/tests/cms/Makefile.am
index 60fdf0281..7efdf37b1 100644
--- a/tests/cms/Makefile.am
+++ b/tests/cms/Makefile.am
@@ -86,13 +86,19 @@ TEST_FILES = plain-1.cms.asc \
 testscripts = sm-sign+verify sm-verify
 
 EXTRA_DIST = $(XTESTS) $(KEYS) $(CERTS) $(TEST_FILES) \
+             samplemsgs/README \
+	     samplekeys/Description-p12 \
 	     samplekeys/steed-self-signing-nonthority.pem \
 	     samplekeys/68A638998DFABAC510EA645CE34F9686B2EDF7EA.key \
              samplekeys/32100C27173EF6E9C4E9A25D3D69F86D37A4F939.key \
              samplekeys/cert_g10code_pete1.pem \
              samplekeys/cert_g10code_test1.pem \
              samplekeys/cert_g10code_theo1.pem \
-             samplemsgs/README \
+             samplekeys/ov-user.p12 \
+             samplekeys/ov-server.p12 \
+             samplekeys/opensc-test.p12 \
+             samplekeys/t5793-openssl.pfx \
+             samplekeys/t5793-test.pfx \
              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 bd1e35c91..f882de9ea 100644
--- a/tests/cms/samplekeys/Description-p12
+++ b/tests/cms/samplekeys/Description-p12
@@ -18,3 +18,15 @@ Pass: password
 Cert: 115abfc3ae554092a57ade74177fedf9459af5d2
 Cert: a0d6d318952c313ff8c33cd3f629647ff1de76b3
 Key:  5a36c61706367ecdb52e8779e3a32bbac1069fa1
+
+Name: t5793-openssl.pfx
+Desc: self-signed key issued keys
+Pass: test
+Cert: 80348a438e4b803b99e708da0b7fdd0659dedd15
+Key:  c271e44ab4fb19ca1aae71102ea4d7292ccc981d
+
+Name: t5793-test.pfx
+Desc: QuaVadis format of t5793-openssl
+Pass: test
+Cert: 80348a438e4b803b99e708da0b7fdd0659dedd15
+Key:  c271e44ab4fb19ca1aae71102ea4d7292ccc981d
diff --git a/tests/cms/samplekeys/t5793-openssl.pfx b/tests/cms/samplekeys/t5793-openssl.pfx
new file mode 100644
index 000000000..0f1beed0f
--- /dev/null
+++ b/tests/cms/samplekeys/t5793-openssl.pfx
diff --git a/tests/cms/samplekeys/t5793-test.pfx b/tests/cms/samplekeys/t5793-test.pfx
new file mode 100644
index 000000000..c8b256f19
--- /dev/null
+++ b/tests/cms/samplekeys/t5793-test.pfx