commit 866b88dfe72f70095b7cde71f72d99f4f684abd8
author David Benjamin <davidben@google.com> Wed Dec 15 12:02:12 2021 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Dec 16 15:31:35 2021 +0000
tree a4c8e035bc201a0eb1dcbdda77ac2b68d4f31370
parent 4f1fae3043f22d3a2a0c7fcd7d0244cd91b60bdf

Don't print small, negative serial numbers in decimal.

X509_print_ex tries to print negative serial numbers in decimal. In
doing so, it ends up passing a signed long to %lx and trips
-Wformat-signed.

A minimal fix would be to cast to unsigned long, but this unsigned long
is the absolute value of a signed long (l = -l). This is tricky because
-LONG_MIN does not fit in long. It all works because the length check
only allows one bit short of sizeof(long)*8 bits (ASN1_INTEGER is
sign-and-magnitude).

Still, this is a whole lot of subtlety to account for an invalid case.
Instead, send negative serial numbers down the generic path.

Bug: 450
Change-Id: Ib215fd23863de27e01f7ededf95578f9c800da37
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50766
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/x509/t_x509.c

diff --git a/crypto/x509/t_x509.c b/crypto/x509/t_x509.c
index 7c32a87..486c4ec 100644
--- a/crypto/x509/t_x509.c
+++ b/crypto/x509/t_x509.c
@@ -54,6 +54,8 @@
  * copied and put under another distribution licence
  * [including the GNU Public Licence.] */
 
+#include <assert.h>
+
 #include <openssl/asn1.h>
 #include <openssl/bio.h>
 #include <openssl/digest.h>
@@ -98,7 +100,6 @@
     char *m = NULL, mlch = ' ';
     int nmindent = 0;
     X509_CINF *ci;
-    ASN1_INTEGER *bs;
     EVP_PKEY *pkey = NULL;
     const char *neg;
 
@@ -123,33 +124,32 @@
             goto err;
     }
     if (!(cflag & X509_FLAG_NO_SERIAL)) {
-
-        if (BIO_write(bp, "        Serial Number:", 22) <= 0)
+        if (BIO_write(bp, "        Serial Number:", 22) <= 0) {
             goto err;
-
-        bs = X509_get_serialNumber(x);
-        if (bs->length < (int)sizeof(long)
-            || (bs->length == sizeof(long) && (bs->data[0] & 0x80) == 0)) {
-            l = ASN1_INTEGER_get(bs);
-            if (bs->type == V_ASN1_NEG_INTEGER) {
-                l = -l;
-                neg = "-";
-            } else
-                neg = "";
-            if (BIO_printf(bp, " %s%lu (%s0x%lx)\n", neg, l, neg, l) <= 0)
-                goto err;
-        } else {
-            neg = (bs->type == V_ASN1_NEG_INTEGER) ? " (Negative)" : "";
-            if (BIO_printf(bp, "\n%12s%s", "", neg) <= 0)
-                goto err;
-
-            for (i = 0; i < bs->length; i++) {
-                if (BIO_printf(bp, "%02x%c", bs->data[i],
-                               ((i + 1 == bs->length) ? '\n' : ':')) <= 0)
-                    goto err;
-            }
         }
 
+        const ASN1_INTEGER *serial = X509_get0_serialNumber(x);
+        /* |ASN1_INTEGER_get| returns -1 on overflow, so this check skips
+         * negative and large serial numbers. */
+        l = ASN1_INTEGER_get(serial);
+        if (l >= 0) {
+            assert(serial->type != V_ASN1_NEG_INTEGER);
+            if (BIO_printf(bp, " %ld (0x%lx)\n", l, (unsigned long)l) <= 0) {
+                goto err;
+            }
+        } else {
+            neg = (serial->type == V_ASN1_NEG_INTEGER) ? " (Negative)" : "";
+            if (BIO_printf(bp, "\n%12s%s", "", neg) <= 0) {
+                goto err;
+            }
+
+            for (i = 0; i < serial->length; i++) {
+                if (BIO_printf(bp, "%02x%c", serial->data[i],
+                               ((i + 1 == serial->length) ? '\n' : ':')) <= 0) {
+                    goto err;
+                }
+            }
+        }
     }
 
     if (!(cflag & X509_FLAG_NO_SIGNAME)) {