From ff2814fb9ecf040cdf978d2388d7e507e702fb0b Mon Sep 17 00:00:00 2001 From: Theo Buehler Date: Thu, 22 Jan 2026 02:31:07 +0100 Subject: [PATCH] Remove implicit truncation of i2c_ASN1_BIT_STRING() This removes the implicit truncation dance from i2c_ASN1_BIT_STRING(), still respecting ASN1_STRING_FLAG_BITS_LEFT and only using the bits from a->flags if that is set. Clean up and reorder the implementation. This changes the encoding of bit strings set via ASN1_BIT_STRING_set() and ASN1_STRING_set() unless that flag is set by the application. The ASN1_BIT_STRING_set() is only used without ASN1_STRING_FLAG_BITS_LEFT by the yubico-piv-tool, where this results in erroneous behavior due to incorrect porting of the code to OpenSSL 1.1. It adds (perhaps somewhat too clever) code to ensure the truncation behavior of ASN1_BIT_STRING_set_bit() is unmodified. It now sets ASN1_STRING_FLAG_BITS_LEFT and so that the resulting DER encoding remains the same, which is needed for things relying on that such as the code setting the keyUsage bits. This is required for correct DER encoding, because: X.680, 22.7: When a "NamedBitList" is used in defining a bitstring type ASN.1 encoding rules are free to add (or remove) arbitrarily any trailing 0 bits to (or from) values that are being encoded or decoded. Application designers should therefore ensure that different semantics are not associated with such vlaues which differ only in the number of trailing 0 bits. X.690, 11.2.2: Where ITU-T Rec. X.680 | ISO/IEC 8824-1, 22.7, applies, the bitstring shall have all trailing 0 bits removed before it is encoded. Note 1 - In the case where a size constraint has been applied, the abstract value delivered by a decoder to the application will be one of those satisfying the size constraint and differing from the transmitted value only in the number of trailing zero bits. Note 2 - If a bitstring value has no 1 bits, then an encoder shall encode the value with a length of 1 and an initial octet set to 0. See https://github.com/openssl/openssl/pull/29711 --- lib/libcrypto/asn1/a_bitstr.c | 81 ++++++++++++-------------- regress/lib/libcrypto/asn1/asn1basic.c | 66 ++++++++++++++++++++- 2 files changed, 100 insertions(+), 47 deletions(-) diff --git a/lib/libcrypto/asn1/a_bitstr.c b/lib/libcrypto/asn1/a_bitstr.c index e656c43f0c2a..53cee6059801 100644 --- a/lib/libcrypto/asn1/a_bitstr.c +++ b/lib/libcrypto/asn1/a_bitstr.c @@ -151,6 +151,23 @@ ASN1_BIT_STRING_set_bit(ASN1_BIT_STRING *a, int n, int value) while (a->length > 0 && a->data[a->length - 1] == 0) a->length--; + if (a->length > 0) { + uint8_t u8 = a->data[a->length - 1]; + uint8_t unused_bits = 7; + + /* Only keep least significant bit; count trailing zeroes. */ + u8 &= 0x100 - u8; + if ((u8 & 0x0f) != 0) + unused_bits -= 4; + if ((u8 & 0x33) != 0) + unused_bits -= 2; + if ((u8 & 0x55) != 0) + unused_bits -= 1; + + if (!asn1_abs_set_unused_bits(a, unused_bits)) + return 0; + } + return 1; } LCRYPTO_ALIAS(ASN1_BIT_STRING_set_bit); @@ -178,64 +195,38 @@ LCRYPTO_ALIAS(ASN1_BIT_STRING_get_bit); int i2c_ASN1_BIT_STRING(ASN1_BIT_STRING *a, unsigned char **pp) { - int ret, j, bits, len; - unsigned char *p, *d; + unsigned char *p; + unsigned char bits = 0; + int len; + int ret = 0; if (a == NULL) - return 0; - - len = a->length; - if (len > 0) { - if (a->flags & ASN1_STRING_FLAG_BITS_LEFT) { - bits = (int)a->flags & 0x07; - } else { - j = 0; - for (; len > 0; len--) { - if (a->data[len - 1]) - break; - } - if (len > 0) - j = a->data[len - 1]; - if (j & 0x01) - bits = 0; - else if (j & 0x02) - bits = 1; - else if (j & 0x04) - bits = 2; - else if (j & 0x08) - bits = 3; - else if (j & 0x10) - bits = 4; - else if (j & 0x20) - bits = 5; - else if (j & 0x40) - bits = 6; - else if (j & 0x80) - bits = 7; - else - bits = 0; - } - } else - bits = 0; - - if (len > INT_MAX - 1) - return 0; + goto err; - ret = len + 1; + if ((len = a->length) > INT_MAX - 1) + goto err; if (pp == NULL) - return ret; + goto done; + + /* XXX - call ASN1_BIT_STRING_get_length instead? */ + if (len > 0 && (a->flags & ASN1_STRING_FLAG_BITS_LEFT) != 0) + bits = a->flags & 0x07; p = *pp; - *(p++) = (unsigned char)bits; - d = a->data; + *(p++) = bits; if (len > 0) { - memcpy(p, d, len); + memcpy(p, a->data, len); p += len; p[-1] &= 0xff << bits; } *pp = p; + + done: + ret = len + 1; + + err: return ret; } diff --git a/regress/lib/libcrypto/asn1/asn1basic.c b/regress/lib/libcrypto/asn1/asn1basic.c index 0666e5b061e4..7369c8e98b59 100644 --- a/regress/lib/libcrypto/asn1/asn1basic.c +++ b/regress/lib/libcrypto/asn1/asn1basic.c @@ -66,12 +66,17 @@ const uint8_t asn1_bit_string_primitive[] = { 0x04, 0x0a, 0x3b, 0x5f, 0x29, 0x1c, 0xd0, }; +const uint8_t asn1_bit_string_primitive_no_bits_left_flag[] = { + 0x03, 0x07, + 0x00, 0x0a, 0x3b, 0x5f, 0x29, 0x1c, 0xd0, +}; + static const uint8_t asn1_bit_string_trailing_zeroes[] = { 0x04, 0x00 }; static const uint8_t asn1_bit_string_trailing_zeroes_encoded[] = { - 0x03, 0x02, 0x02, 0x04, + 0x03, 0x03, 0x00, 0x04, 0x00, }; static int @@ -84,14 +89,19 @@ asn1_bit_string_test(void) int bit, i, len; int failed = 1; + /* First test primitive encoding with ASN1_STRING_FLAG_BITS_LEFT. */ + if ((abs = ASN1_BIT_STRING_new()) == NULL) { fprintf(stderr, "FAIL: ASN1_BIT_STRING_new() == NULL\n"); goto failed; } + + /* XXX - use ASN1_BIT_STRING_set1() once it's available. */ if (!ASN1_BIT_STRING_set(abs, bs, sizeof(bs))) { fprintf(stderr, "FAIL: failed to set bit string\n"); goto failed; } + abs->flags |= ASN1_STRING_FLAG_BITS_LEFT | 0x04; if ((len = i2d_ASN1_BIT_STRING(abs, NULL)) < 0) { fprintf(stderr, "FAIL: i2d_ASN1_BIT_STRING with NULL\n"); @@ -129,6 +139,56 @@ asn1_bit_string_test(void) goto failed; } + /* Now repeat the above test without ASN1_STRING_FLAG_BITS_LEFT. */ + + ASN1_BIT_STRING_free(abs); + if ((abs = ASN1_BIT_STRING_new()) == NULL) { + fprintf(stderr, "FAIL: ASN1_BIT_STRING_new() == NULL\n"); + goto failed; + } + /* XXX - use ASN1_STRING_set() on removing ASN1_BIT_STRING_set(). */ + if (!ASN1_BIT_STRING_set(abs, bs, sizeof(bs))) { + fprintf(stderr, "FAIL: failed to set bit string\n"); + goto failed; + } + + if ((len = i2d_ASN1_BIT_STRING(abs, NULL)) < 0) { + fprintf(stderr, "FAIL: i2d_ASN1_BIT_STRING with NULL\n"); + goto failed; + } + if ((p = malloc(len)) == NULL) + errx(1, "malloc"); + memset(p, 0xbd, len); + pp = p; + if ((i2d_ASN1_BIT_STRING(abs, &pp)) != len) { + fprintf(stderr, "FAIL: i2d_ASN1_BIT_STRING\n"); + goto failed; + } + if (!asn1_compare_bytes("BIT_STRING", p, len, + asn1_bit_string_primitive_no_bits_left_flag, + sizeof(asn1_bit_string_primitive_no_bits_left_flag))) + goto failed; + if (pp != p + len) { + fprintf(stderr, "FAIL: i2d_ASN1_BIT_STRING pp = %p, want %p\n", + pp, p + len); + goto failed; + } + + /* Test primitive decoding. */ + q = p; + if (d2i_ASN1_BIT_STRING(&abs, &q, len) == NULL) { + fprintf(stderr, "FAIL: d2i_ASN1_BIT_STRING primitive\n"); + goto failed; + } + if (!asn1_compare_bytes("BIT_STRING primitive data", abs->data, abs->length, + bs, sizeof(bs))) + goto failed; + if (q != p + len) { + fprintf(stderr, "FAIL: d2i_ASN1_BIT_STRING q = %p, want %p\n", + q, p + len); + goto failed; + } + /* Test ASN1_BIT_STRING_get_bit(). */ for (i = 0; i < ((int)sizeof(bs) * 8); i++) { bit = (bs[i / 8] >> (7 - i % 8)) & 1; @@ -239,6 +299,8 @@ static const uint8_t asn1_bit_string_10010[] = { static const uint8_t asn1_bit_string_zeroes[64] = { 0 }; +static const uint8_t asn1_bit_string_zeroes_encoded[67] = { 0x03, 0x41, }; + static int asn1_bit_string_set_bit_test(void) { @@ -653,7 +715,7 @@ asn1_bit_string_set_bit_test(void) goto failed; } if (!asn1_compare_bytes("BIT STRING all zeroes", der, der_len, - asn1_bit_string_empty, sizeof(asn1_bit_string_empty))) + asn1_bit_string_zeroes_encoded, sizeof(asn1_bit_string_zeroes_encoded))) goto failed; failed = 0;