Clear unused bits in ASN1_STRING_set/set0 Previously, they left the bit count alone, even if it was inconsistent with the string, which mostly meant callers needed to go back and fix up the flags field afterwards. Bump BORINGSSL_API_VERSION. Between this and other fixes in this series, callers should not need to mess with ASN1_STRING_FLAG_BITS_LEFT anymore. BORINGSSL_API_VERSION can be used to gate such code that may still be needed in older BoringSSLs. Update-Note: ASN1_STRING_set/set0 on a BIT STRING will now cleanly set the BIT STRING to containing the specified bytes, without carrying over the unused bit count from the previous value. To set a BIT STRING that isn't a whole number of bytes, use ASN1_BIT_STRING_set1. Bug: 42290311 Change-Id: I8dfd6ca6485de7a3bae0be055e4e624f5ff9b340 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/92571 Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Commit-Queue: Lily Chen <chlily@google.com> Reviewed-by: Lily Chen <chlily@google.com> Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/asn1_lib.cc b/crypto/asn1/asn1_lib.cc index 9ec8f6d..c129c6b 100644 --- a/crypto/asn1/asn1_lib.cc +++ b/crypto/asn1/asn1_lib.cc
@@ -266,6 +266,7 @@ } } str->length = (int)len; + str->flags &= ~0x07; // Clear unused bits if this is a BIT STRING. if (data != nullptr) { OPENSSL_memcpy(str->data, data, len); // Historically, OpenSSL would NUL-terminate most (but not all) @@ -281,6 +282,7 @@ OPENSSL_free(str->data); str->data = reinterpret_cast<uint8_t *>(data); str->length = len; + str->flags &= ~0x07; // Clear unused bits if this is a BIT STRING. } ASN1_STRING *ASN1_STRING_new() {
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index b106a46..12604c5 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc
@@ -47,6 +47,14 @@ BSSL_NAMESPACE_BEGIN +static Span<const uint8_t> ASN1StringToBytes(const ASN1_STRING *str) { + return Span(ASN1_STRING_get0_data(str), ASN1_STRING_length(str)); +} + +static std::string_view ASN1StringToStringView(const ASN1_STRING *str) { + return BytesAsStringView(ASN1StringToBytes(str)); +} + // |obj| and |i2d_func| require different template parameters because C++ may // deduce, say, |ASN1_STRING*| via |obj| and |const ASN1_STRING*| via // |i2d_func|. Template argument deduction then fails. The language is not able @@ -996,6 +1004,21 @@ EXPECT_FALSE(set1(val.get(), {0x00, 0x00}, 8)); EXPECT_FALSE(set1(val.get(), {0x00, 0x00}, -1)); EXPECT_FALSE(set1(val.get(), {}, 1)); + + // |ASN1_STRING_set| and |ASN1_STRING_set0| should clear the count of unused + // bits, rather then carry it over. + ASSERT_TRUE(set1(val.get(), {0xf0}, 4)); + static const uint8_t kBytes[] = {0x00, 0x01, 0x02}; + ASSERT_TRUE(ASN1_STRING_set(val.get(), kBytes, sizeof(kBytes))); + EXPECT_EQ(ASN1_BIT_STRING_unused_bits(val.get()), 0); + EXPECT_EQ(Bytes(ASN1StringToBytes(val.get())), Bytes(kBytes)); + + ASSERT_TRUE(set1(val.get(), {0xf0}, 4)); + void *copy = OPENSSL_memdup(kBytes, sizeof(kBytes)); + ASSERT_NE(copy, nullptr); + ASN1_STRING_set0(val.get(), copy, sizeof(kBytes)); + EXPECT_EQ(ASN1_BIT_STRING_unused_bits(val.get()), 0); + EXPECT_EQ(Bytes(ASN1StringToBytes(val.get())), Bytes(kBytes)); } TEST(ASN1Test, StringToUTF8) { @@ -1059,12 +1082,6 @@ } } -static std::string_view ASN1StringToStringView(const ASN1_STRING *str) { - return std::string_view( - reinterpret_cast<const char *>(ASN1_STRING_get0_data(str)), - ASN1_STRING_length(str)); -} - static bool ASN1Time_check_posix(const ASN1_TIME *s, int64_t t) { struct tm stm, ttm; int day, sec;
diff --git a/crypto/x509/a_sign.cc b/crypto/x509/a_sign.cc index 2bc5437..a430bff 100644 --- a/crypto/x509/a_sign.cc +++ b/crypto/x509/a_sign.cc
@@ -96,7 +96,5 @@ uint8_t *sig_data; sig.Release(&sig_data, &sig_len); ASN1_STRING_set0(out, sig_data, static_cast<int>(sig_len)); - out->flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07); - out->flags |= ASN1_STRING_FLAG_BITS_LEFT; return static_cast<int>(sig_len); }
diff --git a/crypto/x509/x509cset.cc b/crypto/x509/x509cset.cc index 3f4e75e..19e938a 100644 --- a/crypto/x509/x509cset.cc +++ b/crypto/x509/x509cset.cc
@@ -218,10 +218,5 @@ int X509_CRL_set1_signature_value(X509_CRL *crl, const uint8_t *sig, size_t sig_len) { - if (!ASN1_STRING_set(crl->signature, sig, sig_len)) { - return 0; - } - crl->signature->flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07); - crl->signature->flags |= ASN1_STRING_FLAG_BITS_LEFT; - return 1; + return ASN1_STRING_set(crl->signature, sig, sig_len); }
diff --git a/crypto/x509/x509rset.cc b/crypto/x509/x509rset.cc index 15a147f..076a3bf 100644 --- a/crypto/x509/x509rset.cc +++ b/crypto/x509/x509rset.cc
@@ -51,10 +51,5 @@ int X509_REQ_set1_signature_value(X509_REQ *req, const uint8_t *sig, size_t sig_len) { - if (!ASN1_STRING_set(req->signature, sig, sig_len)) { - return 0; - } - req->signature->flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07); - req->signature->flags |= ASN1_STRING_FLAG_BITS_LEFT; - return 1; + return ASN1_STRING_set(req->signature, sig, sig_len); }
diff --git a/crypto/x509/x_pubkey.cc b/crypto/x509/x_pubkey.cc index d929036..828dec0 100644 --- a/crypto/x509/x_pubkey.cc +++ b/crypto/x509/x_pubkey.cc
@@ -191,10 +191,6 @@ } ASN1_STRING_set0(&pub->public_key, key, key_len); - // Set the number of unused bits to zero. - pub->public_key.flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07); - pub->public_key.flags |= ASN1_STRING_FLAG_BITS_LEFT; - x509_pubkey_changed(pub, GetDefaultEVPAlgorithms()); return 1; }
diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc index 206521a..0c3842e 100644 --- a/crypto/x509/x_x509.cc +++ b/crypto/x509/x_x509.cc
@@ -500,13 +500,7 @@ } int X509_set1_signature_value(X509 *x509, const uint8_t *sig, size_t sig_len) { - auto *impl = FromOpaque(x509); - if (!ASN1_STRING_set(&impl->signature, sig, sig_len)) { - return 0; - } - impl->signature.flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07); - impl->signature.flags |= ASN1_STRING_FLAG_BITS_LEFT; - return 1; + return ASN1_STRING_set(&FromOpaque(x509)->signature, sig, sig_len); } void X509_get0_signature(const ASN1_BIT_STRING **psig, const X509_ALGOR **palg,
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index b83e3cf..e891382 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h
@@ -555,12 +555,20 @@ // |data|. It returns one on success and zero on error. If |data| is NULL, it // updates the length and allocates the buffer as needed, but does not // initialize the contents. +// +// If |str| is a BIT STRING, this function sets the number of unused bits to +// zero. |ASN1_BIT_STRING_set1| may be used to set a BIT STRING that is not a +// whole number of bytes. OPENSSL_EXPORT int ASN1_STRING_set(ASN1_STRING *str, const void *data, ossl_ssize_t len); // ASN1_STRING_set0 sets the contents of |str| to |len| bytes from |data|. It // takes ownership of |data|, which must have been allocated with // |OPENSSL_malloc|. +// +// If |str| is a BIT STRING, this function sets the number of unused bits to +// zero. |ASN1_BIT_STRING_set1| may be used to set a BIT STRING that is not a +// whole number of bytes. OPENSSL_EXPORT void ASN1_STRING_set0(ASN1_STRING *str, void *data, int len); // The following functions call |ASN1_STRING_type_new| with the corresponding @@ -906,11 +914,7 @@ // always returns zero. Otherwise it returns a number between 0 and 7. OPENSSL_EXPORT uint8_t ASN1_BIT_STRING_unused_bits(const ASN1_BIT_STRING *str); -// ASN1_BIT_STRING_set calls |ASN1_STRING_set|. It leaves flags unchanged, so -// the caller must set the number of unused bits. -// -// TODO(crbug.com/42290311): This function, and |ASN1_STRING_set|, should reset -// the number of unused bits. +// ASN1_BIT_STRING_set calls |ASN1_STRING_set|. OPENSSL_EXPORT int ASN1_BIT_STRING_set(ASN1_BIT_STRING *str, const uint8_t *data, ossl_ssize_t length);
diff --git a/include/openssl/base.h b/include/openssl/base.h index c9a85c3..961e542 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h
@@ -73,7 +73,7 @@ // A consumer may use this symbol in the preprocessor to temporarily build // against multiple revisions of BoringSSL at the same time. It is not // recommended to do so for longer than is necessary. -#define BORINGSSL_API_VERSION 39 +#define BORINGSSL_API_VERSION 40 #if defined(BORINGSSL_SHARED_LIBRARY)