Skip to content

Commit efab69b

Browse files
davidbenBoringssl LUCI CQ
authored andcommitted
Return 0x80 in all ASN1_get_object error paths.
If the header is valid, but the body is truncated, ASN1_get_object intentionally preserves the indefinite-length and constructed output bits. This means callers who check for error with == 0x80 may read off the end of the buffer on accident. This is unlikely to break callers: 0x80 was already a possible error value, so callers already needed to handle it. The original function's aim in returning more information is unlikely to matter because callers cannot distinguish 0x80 (could not parse header) and 0x80 (header was valid, definite-length, and primitive, but length was too long). Update-Note: ASN1_get_object's calling convention is slightly simplified. Bug: 451 Change-Id: If2b45c47e6b8864aef9fd5e04f313219639991ed Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50005 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
1 parent 471e631 commit efab69b

File tree

3 files changed

+16
-13
lines changed

3 files changed

+16
-13
lines changed

crypto/asn1/asn1_lib.c

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -167,17 +167,13 @@ int ASN1_get_object(const unsigned char **pp, long *plength, int *ptag,
167167
#endif
168168
if (*plength > (omax - (p - *pp))) {
169169
OPENSSL_PUT_ERROR(ASN1, ASN1_R_TOO_LONG);
170-
/*
171-
* Set this so that even if things are not long enough the values are
172-
* set correctly
173-
*/
174-
ret |= 0x80;
170+
return 0x80;
175171
}
176172
*pp = p;
177173
return (ret | inf);
178174
err:
179175
OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG);
180-
return (0x80);
176+
return 0x80;
181177
}
182178

183179
static int asn1_get_length(const unsigned char **pp, int *inf, long *rl,

crypto/asn1/asn1_test.cc

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1669,6 +1669,17 @@ TEST(ASN1Test, PrintASN1Object) {
16691669
std::string(reinterpret_cast<const char *>(bio_data), bio_len));
16701670
}
16711671

1672+
TEST(ASN1, GetObject) {
1673+
// The header is valid, but there are not enough bytes for the length.
1674+
static const uint8_t kTruncated[] = {0x30, 0x01};
1675+
const uint8_t *ptr = kTruncated;
1676+
long length;
1677+
int tag;
1678+
int tag_class;
1679+
EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class,
1680+
sizeof(kTruncated)));
1681+
}
1682+
16721683
// The ASN.1 macros do not work on Windows shared library builds, where usage of
16731684
// |OPENSSL_EXPORT| is a bit stricter.
16741685
#if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY)

include/openssl/asn1.h

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1413,13 +1413,11 @@ OPENSSL_EXPORT int i2t_ASN1_OBJECT(char *buf, int buf_len,
14131413

14141414
// Low-level encoding functions.
14151415

1416-
// ASN1_get_object parses a BER element from up to |max_len| bytes at |*inp| and
1417-
// returns a bitmask with status bits.
1416+
// ASN1_get_object parses a BER element from up to |max_len| bytes at |*inp|.
14181417
//
1419-
// If the return value has 0x80 set, the function failed to parse an element.
1418+
// If the return value is 0x80, the function failed to parse an element.
14201419
// The contents of |*inp|, |*out_length|, |*out_tag|, and |*out_class| are
1421-
// undefined. Note callers must check for bitwise AND with 0x80, not equality.
1422-
// The return value on error may have other bits set,
1420+
// undefined.
14231421
//
14241422
// Otherwise, the function successfully parsed a element. The return value has
14251423
// bit 0x01 set if the length is indefinite, and |V_ASN1_CONSTRUCTED| set if the
@@ -1432,8 +1430,6 @@ OPENSSL_EXPORT int i2t_ASN1_OBJECT(char *buf, int buf_len,
14321430
// This function is very difficult to use correctly. Use |CBS_get_asn1| and
14331431
// related functions from bytestring.h.
14341432
//
1435-
// TODO(https://crbug.com/boringssl/451): Simplify the return value on error.
1436-
//
14371433
// TODO(https://crbug.com/boringssl/354): Remove support for indefinite lengths
14381434
// and non-minimal lengths.
14391435
OPENSSL_EXPORT int ASN1_get_object(const unsigned char **inp, long *out_length,

0 commit comments

Comments
 (0)