Remove d2i_FOO object reuse

The "object reuse" mode in d2i_FOO is extremely problematic. See bug for
details. When we rewrite d2i_RSAPublicKey, etc., with CBS, we switched
dropped this fragile behavior and replaced it with freeing the old value
and replacing it with the new value. Extend this behavior to all functions
generated by crypto/asn1 templates too.

In particular, tasn_dec.c already frees the original object on error,
which means callers must already be prepared for OpenSSL to free their
reused object. The one caller I found that would be incompatible (via
both running tests and auditing callers) was actually broken due to this
error case, and has been fixed.

Update-Note: This slightly changes the calling convention of the d2i_FOO
functions. The change should be compatible with almost all valid calls.
If something goes wrong, it should hopefully be quite obvious. If
affected (or unaffected), prefer to set the output parameter to NULL
and use the return value instead.

Fixed: 550
Change-Id: Ie54cdf17f8f5a4d76fdbcddeaa27e6afd3fa9d8e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56647
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

1 files changed, 23 insertions, 7 deletions

diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c
index bdde8e3..39329b6 100644
--- a/crypto/asn1/tasn_dec.c
+++ b/crypto/asn1/tasn_dec.c
@@ -61,6 +61,7 @@
 #include <openssl/mem.h>
 #include <openssl/pool.h>
 
+#include <assert.h>
 #include <limits.h>
 #include <string.h>
 
@@ -144,20 +145,35 @@ unsigned long ASN1_tag2bit(int tag) {
 
 ASN1_VALUE *ASN1_item_d2i(ASN1_VALUE **pval, const unsigned char **in, long len,
                           const ASN1_ITEM *it) {
-  ASN1_VALUE *ptmpval = NULL;
-  if (!pval) {
-    pval = &ptmpval;
+  ASN1_VALUE *ret = NULL;
+  if (asn1_item_ex_d2i(&ret, in, len, it, /*tag=*/-1, /*aclass=*/0, /*opt=*/0,
+                       /*buf=*/NULL, /*depth=*/0) <= 0) {
+    // Clean up, in case the caller left a partial object.
+    //
+    // TODO(davidben): I don't think it can leave one, but the codepaths below
+    // are a bit inconsistent. Revisit this when rewriting this function.
+    ASN1_item_ex_free(&ret, it);
   }
 
-  if (asn1_item_ex_d2i(pval, in, len, it, /*tag=*/-1, /*aclass=*/0, /*opt=*/0,
-                       /*buf=*/NULL, /*depth=*/0) > 0) {
-    return *pval;
+  // If the caller supplied an output pointer, free the old one and replace it
+  // with |ret|. This differs from OpenSSL slightly in that we don't support
+  // object reuse. We run this on both success and failure. On failure, even
+  // with object reuse, OpenSSL destroys the previous object.
+  if (pval != NULL) {
+    ASN1_item_ex_free(pval, it);
+    *pval = ret;
   }
-  return NULL;
+  return ret;
 }
 
 // Decode an item, taking care of IMPLICIT tagging, if any. If 'opt' set and
 // tag mismatch return -1 to handle OPTIONAL
+//
+// TODO(davidben): Historically, all functions in this file had to account for
+// |*pval| containing an arbitrary existing value. This is no longer the case
+// because |ASN1_item_d2i| now always starts from NULL. As part of rewriting
+// this function, take the simplified assumptions into account. Though we must
+// still account for the internal calls to |ASN1_item_ex_new|.
 
 static int asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in,
                             long len, const ASN1_ITEM *it, int tag, int aclass,