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 <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
index 9def017..1954bea 100644
--- a/crypto/asn1/asn1_lib.c
+++ b/crypto/asn1/asn1_lib.c
@@ -167,17 +167,13 @@
#endif
if (*plength > (omax - (p - *pp))) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_TOO_LONG);
- /*
- * Set this so that even if things are not long enough the values are
- * set correctly
- */
- ret |= 0x80;
+ return 0x80;
}
*pp = p;
return (ret | inf);
err:
OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG);
- return (0x80);
+ return 0x80;
}
static int asn1_get_length(const unsigned char **pp, int *inf, long *rl,
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 1381169..96f9f9c 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1669,6 +1669,17 @@
std::string(reinterpret_cast<const char *>(bio_data), bio_len));
}
+TEST(ASN1, GetObject) {
+ // The header is valid, but there are not enough bytes for the length.
+ static const uint8_t kTruncated[] = {0x30, 0x01};
+ const uint8_t *ptr = kTruncated;
+ long length;
+ int tag;
+ int tag_class;
+ EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class,
+ sizeof(kTruncated)));
+}
+
// The ASN.1 macros do not work on Windows shared library builds, where usage of
// |OPENSSL_EXPORT| is a bit stricter.
#if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY)
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 84f64db..d376c1a 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -1413,13 +1413,11 @@
// Low-level encoding functions.
-// ASN1_get_object parses a BER element from up to |max_len| bytes at |*inp| and
-// returns a bitmask with status bits.
+// ASN1_get_object parses a BER element from up to |max_len| bytes at |*inp|.
//
-// If the return value has 0x80 set, the function failed to parse an element.
+// If the return value is 0x80, the function failed to parse an element.
// The contents of |*inp|, |*out_length|, |*out_tag|, and |*out_class| are
-// undefined. Note callers must check for bitwise AND with 0x80, not equality.
-// The return value on error may have other bits set,
+// undefined.
//
// Otherwise, the function successfully parsed a element. The return value has
// bit 0x01 set if the length is indefinite, and |V_ASN1_CONSTRUCTED| set if the
@@ -1432,8 +1430,6 @@
// This function is very difficult to use correctly. Use |CBS_get_asn1| and
// related functions from bytestring.h.
//
-// TODO(https://crbug.com/boringssl/451): Simplify the return value on error.
-//
// TODO(https://crbug.com/boringssl/354): Remove support for indefinite lengths
// and non-minimal lengths.
OPENSSL_EXPORT int ASN1_get_object(const unsigned char **inp, long *out_length,
