commit 62ab404cb560a6886196fe65cd3381f2ae3166ca
author David Benjamin <davidben@google.com> Tue Jan 31 21:06:58 2023 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Feb 01 14:52:55 2023 +0000
tree 34a5e1a866075740cff7827dca7257b8cdecebb0
parent 07d353680fa7d96e77ba93382deddd030793def4

Make X509V3_get_value_int free the old value before overwriting it.

This is an unexported API, so it's okay to change it. Many extension
types work by parsing a list of key:value pairs and then setting fields
based on it. If a key appears twice, it'll just overwrite the old value.

But X509V3_get_value_int forgot to free the old value when doing so.

Bug: oss-fuzz:55572
Change-Id: I2b39aa7e9214e82fb40ee2e3481697338fe88e1a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56745
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 0928d46..9abe953 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -5706,6 +5706,19 @@
        {0x30, 0x0f, 0x06, 0x03, 0x55, 0x1d, 0x13, 0x01, 0x01, 0xff, 0x04, 0x05,
         0x30, 0x03, 0x01, 0x01, 0xff}},
 
+      {"basicConstraints",
+       "critical,CA:true,pathlen:1",
+       nullptr,
+       {0x30, 0x12, 0x06, 0x03, 0x55, 0x1d, 0x13, 0x01, 0x01, 0xff,
+        0x04, 0x08, 0x30, 0x06, 0x01, 0x01, 0xff, 0x02, 0x01, 0x01}},
+
+      // key:value tuples can be repeated and just override the previous value.
+      {"basicConstraints",
+       "critical,CA:true,pathlen:100,pathlen:1",
+       nullptr,
+       {0x30, 0x12, 0x06, 0x03, 0x55, 0x1d, 0x13, 0x01, 0x01, 0xff,
+        0x04, 0x08, 0x30, 0x06, 0x01, 0x01, 0xff, 0x02, 0x01, 0x01}},
+
       // Extension contents may be referenced from a config section.
       {"basicConstraints",
        "critical,@section",

crypto/x509v3/internal.h

diff --git a/crypto/x509v3/internal.h b/crypto/x509v3/internal.h
index 05a7026..0c068a0 100644
--- a/crypto/x509v3/internal.h
+++ b/crypto/x509v3/internal.h
@@ -138,8 +138,18 @@
 // one and sets |*out_bool| to resulting value. Otherwise, it returns zero.
 int X509V3_bool_from_string(const char *str, ASN1_BOOLEAN *out_bool);
 
+// X509V3_get_value_bool decodes |value| as a boolean. On success, it returns
+// one and sets |*out_bool| to the resulting value. Otherwise, it returns zero.
 int X509V3_get_value_bool(const CONF_VALUE *value, ASN1_BOOLEAN *out_bool);
+
+// X509V3_get_value_int decodes |value| as an integer. On success, it returns
+// one and sets |*aint| to the resulting value. Otherwise, it returns zero. If
+// |*aint| was non-NULL at the start of the function, it frees the previous
+// value before writing a new one.
 int X509V3_get_value_int(const CONF_VALUE *value, ASN1_INTEGER **aint);
+
+// X509V3_get_section behaves like |NCONF_get_section| but queries |ctx|'s
+// config database.
 const STACK_OF(CONF_VALUE) *X509V3_get_section(const X509V3_CTX *ctx,
                                                const char *section);
 

crypto/x509v3/v3_utl.c

diff --git a/crypto/x509v3/v3_utl.c b/crypto/x509v3/v3_utl.c
index 96ad229..c5bf823 100644
--- a/crypto/x509v3/v3_utl.c
+++ b/crypto/x509v3/v3_utl.c
@@ -337,6 +337,7 @@
     X509V3_conf_err(value);
     return 0;
   }
+  ASN1_INTEGER_free(*aint);
   *aint = itmp;
   return 1;
 }