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>
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 @@
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 @@
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 @@
// 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,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 @@
-----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 @@
#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
