From f65bd896414ed2e25bad00527fa4f1c5fabdad09 Mon Sep 17 00:00:00 2001
From: tb <>
Date: Sun, 4 Jan 2026 09:51:42 +0000
Subject: i2c_ASN1_BIT_STRING() vs ASN1_STRING_FLAG_BITS_LEFT

A nasty quirk in the bit string handling is that the serialization
produced by i2d_ASN1_BIT_STRING() depends on whether the the magic
ASN1_STRING_FLAG_BITS_LEFT is set.

If ASN1_STRING_FLAG_BITS_LEFT is set, the number of unused bits is
carried in a->flags & 0x07 and the remainder of the bit string is
in a->data. This is terrible and undocumented but handled correctly.

If ASN1_STRING_FLAG_BITS_LEFT is not set, all trailing zero bits are
(intended to be) chopped off with all sorts of hilarious side effects.
I broke this quite thoroughly when I incorrectly ported an overflow
check from BoringSSL in:
https://github.com/openbsd/src/commit/f81cc285d2aed8b36615119a306533696f3eb66c

The result is that we currently return ret = a->length + 1 for both NULL
and non-NULL pp. The calls to asn1_ex_i2c() in asn1_i2d_ex_primitive()
thus report consistent lengths back, making it succeed.

asn1_i2d_ex_primitive() therefore skips a->length + 1 bytes, while
i2c_ASN1_BIT_STRING() only overwrites len + 1 bytes, which are possibly
fewer. So a caller passing in an output buffer containing garbage
(malloc) will get some of that garbage back in the encoding. Further,
i2c_ASN1_BIT_STRING() also advances that pointer by the possibly reduced
len + 1, but that fortunately doesn't matter since that's an effect
local to asn1_ex_i2c(), the only caller of i2c_ASN1_BIT_STRING().

The last bit is that the current behavior may set bogus unused bits
coming from the scanning backward madness. I added such an example in
the parent commit.

The fix is simple: use len after the truncation effect was established,
not the original a->length, turning this commit into what my backport
should have been.

This fixes the two currently failing regress tests, so remove expected
failure marker again.

ok jsing kenjiro
---
 src/lib/libcrypto/asn1/a_bitstr.c       | 19 +++++++++----------
 src/regress/lib/libcrypto/asn1/Makefile |  4 +---
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/src/lib/libcrypto/asn1/a_bitstr.c b/src/lib/libcrypto/asn1/a_bitstr.c
index 82fd53231f..9eee97e33d 100644
--- a/src/lib/libcrypto/asn1/a_bitstr.c
+++ b/src/lib/libcrypto/asn1/a_bitstr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: a_bitstr.c,v 1.46 2026/01/04 09:30:57 tb Exp $ */
+/* $OpenBSD: a_bitstr.c,v 1.47 2026/01/04 09:51:42 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -184,16 +184,7 @@ i2c_ASN1_BIT_STRING(ASN1_BIT_STRING *a, unsigned char **pp)
 	if (a == NULL)
 		return (0);
 
-	if (a->length == INT_MAX)
-		return (0);
-
-	ret = a->length + 1;
-
-	if (pp == NULL)
-		return (ret);
-
 	len = a->length;
-
 	if (len > 0) {
 		if (a->flags & ASN1_STRING_FLAG_BITS_LEFT) {
 			bits = (int)a->flags & 0x07;
@@ -227,6 +218,14 @@ i2c_ASN1_BIT_STRING(ASN1_BIT_STRING *a, unsigned char **pp)
 	} else
 		bits = 0;
 
+	if (len > INT_MAX - 1)
+		return (0);
+
+	ret = len + 1;
+
+	if (pp == NULL)
+		return (ret);
+
 	p = *pp;
 
 	*(p++) = (unsigned char)bits;
diff --git a/src/regress/lib/libcrypto/asn1/Makefile b/src/regress/lib/libcrypto/asn1/Makefile
index 1bb9da8585..23a01ab600 100644
--- a/src/regress/lib/libcrypto/asn1/Makefile
+++ b/src/regress/lib/libcrypto/asn1/Makefile
@@ -1,4 +1,4 @@
-#	$OpenBSD: Makefile,v 1.29 2026/01/04 09:42:32 tb Exp $
+#	$OpenBSD: Makefile,v 1.30 2026/01/04 09:51:42 tb Exp $
 
 PROGS = \
 	asn1api \
@@ -22,8 +22,6 @@ CFLAGS+=	-Wall -Wundef -Werror
 CFLAGS+=	-I${.CURDIR}/../../../../lib/libcrypto/asn1
 CFLAGS+=	-I${.CURDIR}/../../../../lib/libcrypto/bytestring
 
-REGRESS_EXPECTED_FAILURES += run-regress-asn1basic
-
 LDADD_asn1basic = ${CRYPTO_INT}
 LDADD_asn1object = ${CRYPTO_INT}
 LDADD_asn1time = ${CRYPTO_INT}
-- 
cgit v1.2.3-55-g6feb