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

@@ -210,7 +210,7 @@ unsigned long X509_NAME_hash_old(X509_NAME *x) {
 
 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

@@ -98,7 +98,7 @@ int X509_set_version(X509 *x, long version) {
 }
 
 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

@@ -4816,6 +4816,15 @@ TEST(X509Test, SetSerialNumberChecksASN1StringType) {
   // 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());
+  EXPECT_FALSE(X509_set_serialNumber(root.get(), str.get()));
-  ASSERT_EQ(r, 0);
+
+  // 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

@@ -216,7 +216,7 @@ int X509_REVOKED_set_serialNumber(X509_REVOKED *revoked,
                                   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

@@ -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 struct CRYPTO_STATIC_MUTEX g_crl_sort_lock = CRYPTO_STATIC_MUTEX_INIT;
 
 static int crl_lookup(X509_CRL *crl, X509_REVOKED **ret, ASN1_INTEGER *serial,
                       X509_NAME *issuer) {
-  if (serial->type != V_ASN1_INTEGER) {
+  // Use an assert, rather than a runtime error, because returning nothing for a
-    return 0;
+  // 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;