commit a61e7475d3145836cc11469f328ad7c98e7cf527
author David Benjamin <davidben@google.com> Fri Sep 23 15:04:20 2022 -0400
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Sep 23 19:32:20 2022 +0000
tree 2c7c089e96238a1720d5b65bbbdef9756d7c9f17
parent 04989786e9ab16cef5261bbd05a2b1a8cb312dbf

Also check for V_ASN1_NEG_INTEGER when checking types.

ASN1_STRING's representation is confusing. For specifically INTEGER and
ENUMERATED, it lifts the sign bit into the type. While negative serial
numbers aren't actually valid, we do accept them and test code sometimes
uses these APIs to construct them, so amend
https://boringssl-review.googlesource.com/c/boringssl/+/54286 to allow
them.

I've also switched the CRL one to an assert. On reflection, returning 0
for a CRL lookup is failing closed, so it seems better to just continue
to accept the ASN1_STRING, even if it's the wrong type.

Change-Id: I1e81a89700ef14407a78bd3798cdae28a80640cd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54525
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/x509/x509_cmp.c

diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c
index b80d682..c9966e9 100644
--- a/crypto/x509/x509_cmp.c
+++ b/crypto/x509/x509_cmp.c
@@ -210,7 +210,7 @@
 
 X509 *X509_find_by_issuer_and_serial(const STACK_OF(X509) *sk, X509_NAME *name,
                                      const ASN1_INTEGER *serial) {
-  if (serial->type != V_ASN1_INTEGER) {
+  if (serial->type != V_ASN1_INTEGER && serial->type != V_ASN1_NEG_INTEGER) {
     return NULL;
   }
 

crypto/x509/x509_set.c

diff --git a/crypto/x509/x509_set.c b/crypto/x509/x509_set.c
index 8c920be..29fe722 100644
--- a/crypto/x509/x509_set.c
+++ b/crypto/x509/x509_set.c
@@ -98,7 +98,7 @@
 }
 
 int X509_set_serialNumber(X509 *x, const ASN1_INTEGER *serial) {
-  if (serial->type != V_ASN1_INTEGER) {
+  if (serial->type != V_ASN1_INTEGER && serial->type != V_ASN1_NEG_INTEGER) {
     OPENSSL_PUT_ERROR(ASN1, ASN1_R_WRONG_TYPE);
     return 0;
   }

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index c639ef0..24ee3bc 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -4816,6 +4816,15 @@
   // Passing an IA5String to X509_set_serialNumber() should fail.
   bssl::UniquePtr<ASN1_IA5STRING> str(ASN1_IA5STRING_new());
   ASSERT_TRUE(str);
-  int r = X509_set_serialNumber(root.get(), str.get());
-  ASSERT_EQ(r, 0);
+  EXPECT_FALSE(X509_set_serialNumber(root.get(), str.get()));
+
+  // Passing a negative serial number is allowed. While invalid, we do accept
+  // them and some callers rely in this for tests.
+  bssl::UniquePtr<ASN1_INTEGER> serial(ASN1_INTEGER_new());
+  ASSERT_TRUE(serial);
+  ASSERT_TRUE(ASN1_INTEGER_set(serial.get(), -1));
+  ASSERT_TRUE(X509_set_serialNumber(root.get(), serial.get()));
+  int64_t val;
+  ASSERT_TRUE(ASN1_INTEGER_get_int64(&val, X509_get0_serialNumber(root.get())));
+  EXPECT_EQ(-1, val);
 }

crypto/x509/x509cset.c

diff --git a/crypto/x509/x509cset.c b/crypto/x509/x509cset.c
index 408723f..5b9dea9 100644
--- a/crypto/x509/x509cset.c
+++ b/crypto/x509/x509cset.c
@@ -216,7 +216,7 @@
                                   const ASN1_INTEGER *serial) {
   ASN1_INTEGER *in;
 
-  if (serial->type != V_ASN1_INTEGER) {
+  if (serial->type != V_ASN1_INTEGER && serial->type != V_ASN1_NEG_INTEGER) {
     OPENSSL_PUT_ERROR(ASN1, ASN1_R_WRONG_TYPE);
     return 0;
   }

crypto/x509/x_crl.c

diff --git a/crypto/x509/x_crl.c b/crypto/x509/x_crl.c
index 573c115..c153b5d 100644
--- a/crypto/x509/x_crl.c
+++ b/crypto/x509/x_crl.c
@@ -65,6 +65,8 @@
 #include <openssl/x509.h>
 #include <openssl/x509v3.h>
 
+#include <assert.h>
+
 #include "../internal.h"
 #include "internal.h"
 
@@ -430,9 +432,9 @@
 
 static int crl_lookup(X509_CRL *crl, X509_REVOKED **ret, ASN1_INTEGER *serial,
                       X509_NAME *issuer) {
-  if (serial->type != V_ASN1_INTEGER) {
-    return 0;
-  }
+  // Use an assert, rather than a runtime error, because returning nothing for a
+  // CRL is arguably failing open, rather than closed.
+  assert(serial->type == V_ASN1_INTEGER || serial->type == V_ASN1_NEG_INTEGER);
   X509_REVOKED rtmp, *rev;
   size_t idx;
   rtmp.serialNumber = serial;