Enforce DER rules for BIT STRING values.

DER requires BIT STRING padding bits be zero.

Bug: 354
Change-Id: Id59154cc4e77f91df8b9ff1eb1b09514116808da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50288
Reviewed-by: Adam Langley <agl@google.com>

5 files changed, 37 insertions, 10 deletions

diff --git a/crypto/asn1/a_bitstr.c b/crypto/asn1/a_bitstr.c
index 7ce8cca..1bb58e2 100644
--- a/crypto/asn1/a_bitstr.c
+++ b/crypto/asn1/a_bitstr.c
@@ -159,11 +159,20 @@ ASN1_BIT_STRING *c2i_ASN1_BIT_STRING(ASN1_BIT_STRING **a,
 
     p = *pp;
     padding = *(p++);
+    len--;
     if (padding > 7) {
         OPENSSL_PUT_ERROR(ASN1, ASN1_R_INVALID_BIT_STRING_BITS_LEFT);
         goto err;
     }
 
+    /* Unused bits in a BIT STRING must be zero. */
+    uint8_t padding_mask = (1 << padding) - 1;
+    if (padding != 0 &&
+        (len < 1 || (p[len - 1] & padding_mask) != 0)) {
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_INVALID_BIT_STRING_PADDING);
+        goto err;
+    }
+
     /*
      * We do this to preserve the settings.  If we modify the settings, via
      * the _set_bit function, we will recalculate on output
@@ -171,21 +180,19 @@ ASN1_BIT_STRING *c2i_ASN1_BIT_STRING(ASN1_BIT_STRING **a,
     ret->flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07); /* clear */
     ret->flags |= (ASN1_STRING_FLAG_BITS_LEFT | padding); /* set */
 
-    if (len-- > 1) {            /* using one because of the bits left byte */
-        s = (unsigned char *)OPENSSL_malloc((int)len);
+    if (len > 0) {
+        s = OPENSSL_memdup(p, len);
         if (s == NULL) {
             OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
             goto err;
         }
-        OPENSSL_memcpy(s, p, (int)len);
-        s[len - 1] &= (0xff << padding);
         p += len;
-    } else
+    } else {
         s = NULL;
+    }
 
     ret->length = (int)len;
-    if (ret->data != NULL)
-        OPENSSL_free(ret->data);
+    OPENSSL_free(ret->data);
     ret->data = s;
     ret->type = V_ASN1_BIT_STRING;
     if (a != NULL)
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 4d8ed1b..1b0c11c 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -337,11 +337,10 @@ TEST(ASN1Test, BitString) {
       // Leading byte too high
       {0x03, 0x02, 0x08, 0x00},
       {0x03, 0x02, 0xff, 0x00},
-      // TODO(https://crbug.com/boringssl/354): Reject these inputs.
       // Empty bit strings must have a zero leading byte.
-      // {0x03, 0x01, 0x01},
+      {0x03, 0x01, 0x01},
       // Unused bits must all be zero.
-      // {0x03, 0x02, 0x06, 0xc1 /* 0b11000001 */},
+      {0x03, 0x02, 0x06, 0xc1 /* 0b11000001 */},
   };
   for (const auto &test : kInvalidInputs) {
     SCOPED_TRACE(Bytes(test));
diff --git a/crypto/err/asn1.errordata b/crypto/err/asn1.errordata
index 5674bda..9344621 100644
--- a/crypto/err/asn1.errordata
+++ b/crypto/err/asn1.errordata
@@ -41,6 +41,7 @@ ASN1,138,ILLEGAL_TIME_VALUE
 ASN1,139,INTEGER_NOT_ASCII_FORMAT
 ASN1,140,INTEGER_TOO_LARGE_FOR_LONG
 ASN1,141,INVALID_BIT_STRING_BITS_LEFT
+ASN1,194,INVALID_BIT_STRING_PADDING
 ASN1,142,INVALID_BMPSTRING
 ASN1,143,INVALID_DIGIT
 ASN1,144,INVALID_MODIFIER
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 46b7b3f..77c383a 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -3507,10 +3507,29 @@ xWIAAA==
 -----END CERTIFICATE-----
 )";
 
+// kNonZeroPadding is an X.09 certificate where the BIT STRING signature field
+// has non-zero padding values.
+static const char kNonZeroPadding[] = R"(
+-----BEGIN CERTIFICATE-----
+MIIB0DCCAXagAwIBAgIJANlMBNpJfb/rMAkGByqGSM49BAEwRTELMAkGA1UEBhMC
+QVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0IFdpZGdp
+dHMgUHR5IEx0ZDAeFw0xNDA0MjMyMzIxNTdaFw0xNDA1MjMyMzIxNTdaMEUxCzAJ
+BgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5l
+dCBXaWRnaXRzIFB0eSBMdGQwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATmK2ni
+v2Wfl74vHg2UikzVl2u3qR4NRvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYa
+HPUdfvGULUvPciLBo1AwTjAdBgNVHQ4EFgQUq4TSrKuV8IJOFngHVVdf5CaNgtEw
+HwYDVR0jBBgwFoAUq4TSrKuV8IJOFngHVVdf5CaNgtEwDAYDVR0TBAUwAwEB/zAJ
+BgcqhkjOPQQBA0kBMEUCIQDyoDVeUTo2w4J5m+4nUIWOcAZ0lVfSKXQA9L4Vh13E
+BwIgfB55FGohg/B6dGh5XxSZmmi08cueFV7mHzJSYV51yRQB
+-----END CERTIFICATE-----
+)";
+
 TEST(X509Test, BER) {
   // Constructed strings are forbidden in DER.
   EXPECT_FALSE(CertFromPEM(kConstructedBitString));
   EXPECT_FALSE(CertFromPEM(kConstructedOctetString));
   // Indefinite lengths are forbidden in DER.
   EXPECT_FALSE(CertFromPEM(kIndefiniteLength));
+  // Padding bits in BIT STRINGs must be zero in BER.
+  EXPECT_FALSE(CertFromPEM(kNonZeroPadding));
 }
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index a186701..d8a371c 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -2034,5 +2034,6 @@ BSSL_NAMESPACE_END
 #define ASN1_R_WRONG_TYPE 191
 #define ASN1_R_NESTED_TOO_DEEP 192
 #define ASN1_R_BAD_TEMPLATE 193
+#define ASN1_R_INVALID_BIT_STRING_PADDING 194
 
 #endif