Remove ASN1_BIT_STRING implicit truncation The implicit truncation behavior is surprising and makes an ASN1_BIT_STRING's in-memory representation messy. Matching OpenSSL 4.x, remove this. See bug for more context. This immediately fixes one API sharp edge: if you create an ASN1_BIT_STRING and then set a byte string in it, it does not actually set what one would expect. Bug: 42290311 Change-Id: I85693f0dddb136f55d8a7c38fb5c957bb56d1d5c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/92568 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Lily Chen <chlily@google.com>
diff --git a/crypto/asn1/a_bitstr.cc b/crypto/asn1/a_bitstr.cc index b5e3483..17ba486 100644 --- a/crypto/asn1/a_bitstr.cc +++ b/crypto/asn1/a_bitstr.cc
@@ -45,27 +45,8 @@ int bssl::asn1_bit_string_length(const ASN1_BIT_STRING *str, uint8_t *out_padding_bits) { int len = str->length; - if (str->flags & ASN1_STRING_FLAG_BITS_LEFT) { - // If the string is already empty, it cannot have padding bits. - *out_padding_bits = len == 0 ? 0 : str->flags & 0x07; - return len; - } - - // TODO(https://crbug.com/42290311): Remove this representation. - while (len > 0 && str->data[len - 1] == 0) { - len--; - } - uint8_t padding_bits = 0; - if (len > 0) { - uint8_t last = str->data[len - 1]; - assert(last != 0); - for (; padding_bits < 7; padding_bits++) { - if (last & (1 << padding_bits)) { - break; - } - } - } - *out_padding_bits = padding_bits; + // If the string is already empty, it cannot have padding bits. + *out_padding_bits = len == 0 ? 0 : str->flags & 0x07; return len; }
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 4cce03d..9d938d1 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc
@@ -940,12 +940,26 @@ EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 63)); EXPECT_EQ(0, ASN1_BIT_STRING_get_bit(val.get(), 64)); - // By default, a BIT STRING implicitly truncates trailing zeros. + // A BIT STRING may be manually set with trailing zeros. val.reset(ASN1_BIT_STRING_new()); ASSERT_TRUE(val); - static const uint8_t kZeros[64] = {0}; + static const uint8_t kZeros[5] = {0}; ASSERT_TRUE(ASN1_STRING_set(val.get(), kZeros, sizeof(kZeros))); - TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitStringEmpty); + static const uint8_t kBitStringZeros[] = {0x03, 0x06, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00}; + TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitStringZeros); +} + +TEST(ASN1Test, SetBitString) { + // Creating an ASN1_BIT_STRING and then filling in a byte string should assign + // the byte string, not a silently truncated version. + UniquePtr<ASN1_BIT_STRING> val(ASN1_BIT_STRING_new()); + ASSERT_TRUE(val); + const uint8_t kBytesf000[] = {0xf0, 0x00}; + ASSERT_TRUE( + ASN1_STRING_set(val.get(), kBytesf000, sizeof(kBytesf000))); + static const uint8_t kBitStringf000[] = {0x03, 0x03, 0x00, 0xf0, 0x00}; + TestSerialize(val.get(), i2d_ASN1_BIT_STRING, kBitStringf000); } TEST(ASN1Test, StringToUTF8) { @@ -2325,9 +2339,6 @@ const Input kInputs[] = { {V_ASN1_BIT_STRING, {}, ASN1_STRING_FLAG_BITS_LEFT | 0, false}, {V_ASN1_BIT_STRING, {}, 0, true}, - // When |ASN1_STRING_FLAG_BITS_LEFT| is unset, BIT STRINGs implicitly - // drop trailing zeros. - {V_ASN1_BIT_STRING, {0x00, 0x00, 0x00, 0x00}, 0, true}, {V_ASN1_OCTET_STRING, {}, 0, false}, {V_ASN1_UTF8STRING, {}, 0, false}, @@ -2345,8 +2356,6 @@ {V_ASN1_BIT_STRING, {0xe0}, ASN1_STRING_FLAG_BITS_LEFT | 5, false}, // 4-bit inputs. {V_ASN1_BIT_STRING, {0xf0}, ASN1_STRING_FLAG_BITS_LEFT | 4, false}, - {V_ASN1_BIT_STRING, {0xf0}, 0, true}, // 4 trailing zeros dropped. - {V_ASN1_BIT_STRING, {0xf0, 0x00}, 0, true}, // 12 trailing zeros dropped. // 5-bit inputs. {V_ASN1_BIT_STRING, {0x00}, ASN1_STRING_FLAG_BITS_LEFT | 3, false}, {V_ASN1_BIT_STRING, {0xf0}, ASN1_STRING_FLAG_BITS_LEFT | 3, false},
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h index c4a4941..a68fcf3 100644 --- a/crypto/asn1/internal.h +++ b/crypto/asn1/internal.h
@@ -107,8 +107,9 @@ // asn1_bit_string_length returns the number of bytes in |str| and sets // |*out_padding_bits| to the number of padding bits. // -// This function should be used instead of |ASN1_STRING_length| to correctly -// handle the non-|ASN1_STRING_FLAG_BITS_LEFT| case. +// TODO(crbug.com/42290311): This function dates to BIT STRING implicit +// truncation and tries to account for the byte length differing from +// |ASN1_STRING_length|. Remove this and replace with a simpler API. int asn1_bit_string_length(const ASN1_BIT_STRING *str, uint8_t *out_padding_bits);
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index dbbefbb..933029a 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h
@@ -488,12 +488,6 @@ long flags; }; -// ASN1_STRING_FLAG_BITS_LEFT indicates, in a BIT STRING |ASN1_STRING|, that -// flags & 0x7 contains the number of padding bits added to the BIT STRING -// value. When not set, all trailing zero bits in the last byte are implicitly -// treated as padding. This behavior is deprecated and should not be used. -#define ASN1_STRING_FLAG_BITS_LEFT 0x08 - // ASN1_STRING_type_new returns a newly-allocated empty |ASN1_STRING| object of // type |type|, or NULL on error. OPENSSL_EXPORT ASN1_STRING *ASN1_STRING_type_new(int type); @@ -858,14 +852,10 @@ // This library represents BIT STRINGs as |ASN1_STRING|s with type // |V_ASN1_BIT_STRING|. The data contains the encoded form of the BIT STRING, // including any padding bits added to round to a whole number of bytes, but -// excluding the leading byte containing the number of padding bits. If -// |ASN1_STRING_FLAG_BITS_LEFT| is set, the bottom three bits contains the -// number of padding bits. For example, DER encodes the BIT STRING {1, 0} as -// {0x06, 0x80 = 0b10_000000}. The |ASN1_STRING| representation has data of -// {0x80} and flags of ASN1_STRING_FLAG_BITS_LEFT | 6. If -// |ASN1_STRING_FLAG_BITS_LEFT| is unset, trailing zero bits are implicitly -// removed. Callers should not rely this representation when constructing bit -// strings. The padding bits in the |ASN1_STRING| data must be zero. +// excluding the leading byte containing the number of padding bits. Instead, +// the bottom three bits of |flags| contain the number of padding bits. For +// example, DER encodes the BIT STRING {1, 0} as {0x06, 0x80 = 0b10_000000}. The +// |ASN1_STRING| representation has data of {0x80} and flags of 6. // ASN1_BIT_STRING_new calls |ASN1_STRING_type_new| with |V_ASN1_BIT_STRING|. OPENSSL_EXPORT ASN1_BIT_STRING *ASN1_BIT_STRING_new(void); @@ -917,14 +907,18 @@ // // This function may be used with |ASN1_STRING_get0_data| to interpret |str| as // a byte string. +// +// TODO(crbug.com/42290311): This function dates to BIT STRING implicit +// truncation and tries to account for the byte length differing from +// |ASN1_STRING_length|. Remove this and replace with a simpler API. OPENSSL_EXPORT int ASN1_BIT_STRING_num_bytes(const ASN1_BIT_STRING *str, size_t *out); // ASN1_BIT_STRING_set calls |ASN1_STRING_set|. It leaves flags unchanged, so // the caller must set the number of unused bits. // -// TODO(davidben): Maybe it should? Wrapping a byte string in a bit string is a -// common use case. +// TODO(crbug.com/42290311): This function, and |ASN1_STRING_set|, should reset +// the number of unused bits. OPENSSL_EXPORT int ASN1_BIT_STRING_set(ASN1_BIT_STRING *str, const unsigned char *d, ossl_ssize_t length); @@ -1780,6 +1774,15 @@ // Deprecated functions. +// ASN1_STRING_FLAG_BITS_LEFT does nothing. Historically, when a BIT STRING +// lacked this flag, it enabled an implicit truncation behavior. This has since +// been removed. +// +// TODO(crbug.com/443769299): Remove this when |ASN1_STRING| is opaque. For now, +// we continue to set it in various codepaths, in case code is querying |flags| +// manually, even though it does nothing. +#define ASN1_STRING_FLAG_BITS_LEFT 0x08 + // ASN1_STRING_set_default_mask does nothing. OPENSSL_EXPORT void ASN1_STRING_set_default_mask(unsigned long mask);