Rewrite ASN1_OBJECT and ASN1_BOOLEAN d2i/i2d functions.
These functions already don't go through tasn_*.c. Rewrite them to use
CBS and CBB. This removes some dependencies on ASN1_get_object and
ASN1_put_object.
Update-Note: d2i_ASN1_OBJECT and d2i_ASN1_BOOLEAN will no longer accept
non-minimal length prefixes (forbidden in DER). d2i_ASN1_BOOLEAN will
also no longer accept non-canonical representations of TRUE (also
forbidden in DER). This does not affect certificate parsing, as that
still goes through the old template system, though we will make a
similar change to those functions later.
Bug: 354, 548
Change-Id: I0b7aa96f47aca5c31ec4f702e27108b4106311f2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58145
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/a_bool.c b/crypto/asn1/a_bool.c
index 67d6813..e227b7f 100644
--- a/crypto/asn1/a_bool.c
+++ b/crypto/asn1/a_bool.c
@@ -56,64 +56,40 @@
#include <openssl/asn1.h>
+#include <openssl/bytestring.h>
#include <openssl/err.h>
-#include <openssl/mem.h>
-int i2d_ASN1_BOOLEAN(ASN1_BOOLEAN a, unsigned char **pp) {
- int r;
- unsigned char *p, *allocated = NULL;
+#include "../bytestring/internal.h"
- r = ASN1_object_size(0, 1, V_ASN1_BOOLEAN);
- if (pp == NULL) {
- return r;
+
+int i2d_ASN1_BOOLEAN(ASN1_BOOLEAN a, unsigned char **outp) {
+ CBB cbb;
+ if (!CBB_init(&cbb, 3) || //
+ !CBB_add_asn1_bool(&cbb, a != ASN1_BOOLEAN_FALSE)) {
+ CBB_cleanup(&cbb);
+ return -1;
}
-
- if (*pp == NULL) {
- if ((p = allocated = OPENSSL_malloc(r)) == NULL) {
- return -1;
- }
- } else {
- p = *pp;
- }
-
- ASN1_put_object(&p, 0, 1, V_ASN1_BOOLEAN, V_ASN1_UNIVERSAL);
- *p = a ? ASN1_BOOLEAN_TRUE : ASN1_BOOLEAN_FALSE;
-
- // If a new buffer was allocated, just return it back.
- // If not, return the incremented buffer pointer.
- *pp = allocated != NULL ? allocated : p + 1;
- return r;
+ return CBB_finish_i2d(&cbb, outp);
}
-ASN1_BOOLEAN d2i_ASN1_BOOLEAN(ASN1_BOOLEAN *a, const unsigned char **pp,
- long length) {
- const unsigned char *p = *pp;
- long len;
- int inf, tag, xclass;
- inf = ASN1_get_object(&p, &len, &tag, &xclass, length);
- if (inf & 0x80) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_OBJECT_HEADER);
+ASN1_BOOLEAN d2i_ASN1_BOOLEAN(ASN1_BOOLEAN *out, const unsigned char **inp,
+ long len) {
+ if (len < 0) {
return ASN1_BOOLEAN_NONE;
}
- if (inf & V_ASN1_CONSTRUCTED) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_TYPE_NOT_PRIMITIVE);
+ CBS cbs;
+ CBS_init(&cbs, *inp, (size_t)len);
+ int val;
+ if (!CBS_get_asn1_bool(&cbs, &val)) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
return ASN1_BOOLEAN_NONE;
}
- if (tag != V_ASN1_BOOLEAN || xclass != V_ASN1_UNIVERSAL) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_EXPECTING_A_BOOLEAN);
- return ASN1_BOOLEAN_NONE;
+ ASN1_BOOLEAN ret = val ? ASN1_BOOLEAN_TRUE : ASN1_BOOLEAN_FALSE;
+ if (out != NULL) {
+ *out = ret;
}
-
- if (len != 1) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_BOOLEAN_IS_WRONG_LENGTH);
- return ASN1_BOOLEAN_NONE;
- }
- ASN1_BOOLEAN ret = (ASN1_BOOLEAN) * (p++);
- if (a != NULL) {
- (*a) = ret;
- }
- *pp = p;
+ *inp = CBS_data(&cbs);
return ret;
}
diff --git a/crypto/asn1/a_object.c b/crypto/asn1/a_object.c
index 56219e1..77bd175 100644
--- a/crypto/asn1/a_object.c
+++ b/crypto/asn1/a_object.c
@@ -59,46 +59,36 @@
#include <limits.h>
#include <string.h>
+#include <openssl/bytestring.h>
#include <openssl/err.h>
#include <openssl/mem.h>
#include <openssl/obj.h>
+#include "../bytestring/internal.h"
#include "../internal.h"
#include "internal.h"
-int i2d_ASN1_OBJECT(const ASN1_OBJECT *a, unsigned char **pp) {
- if (a == NULL) {
+int i2d_ASN1_OBJECT(const ASN1_OBJECT *in, unsigned char **outp) {
+ if (in == NULL) {
OPENSSL_PUT_ERROR(ASN1, ERR_R_PASSED_NULL_PARAMETER);
return -1;
}
- if (a->length == 0) {
+ if (in->length <= 0) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_ILLEGAL_OBJECT);
return -1;
}
- int objsize = ASN1_object_size(0, a->length, V_ASN1_OBJECT);
- if (pp == NULL || objsize == -1) {
- return objsize;
+ CBB cbb, child;
+ if (!CBB_init(&cbb, (size_t)in->length + 2) ||
+ !CBB_add_asn1(&cbb, &child, CBS_ASN1_OBJECT) ||
+ !CBB_add_bytes(&child, in->data, in->length)) {
+ CBB_cleanup(&cbb);
+ return -1;
}
- unsigned char *p, *allocated = NULL;
- if (*pp == NULL) {
- if ((p = allocated = OPENSSL_malloc(objsize)) == NULL) {
- return -1;
- }
- } else {
- p = *pp;
- }
-
- ASN1_put_object(&p, 0, a->length, V_ASN1_OBJECT, V_ASN1_UNIVERSAL);
- OPENSSL_memcpy(p, a->data, a->length);
-
- // If a new buffer was allocated, just return it back.
- // If not, return the incremented buffer pointer.
- *pp = allocated != NULL ? allocated : p + a->length;
- return objsize;
+ return CBB_finish_i2d(&cbb, outp);
}
int i2t_ASN1_OBJECT(char *buf, int buf_len, const ASN1_OBJECT *a) {
@@ -140,29 +130,25 @@
return ret;
}
-ASN1_OBJECT *d2i_ASN1_OBJECT(ASN1_OBJECT **a, const unsigned char **pp,
- long length) {
- long len;
- int tag, xclass;
- const unsigned char *p = *pp;
- int inf = ASN1_get_object(&p, &len, &tag, &xclass, length);
- if (inf & 0x80) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_OBJECT_HEADER);
+ASN1_OBJECT *d2i_ASN1_OBJECT(ASN1_OBJECT **out, const unsigned char **inp,
+ long len) {
+ if (len < 0) {
return NULL;
}
- if (inf & V_ASN1_CONSTRUCTED) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_TYPE_NOT_PRIMITIVE);
+ CBS cbs, child;
+ CBS_init(&cbs, *inp, (size_t)len);
+ if (!CBS_get_asn1(&cbs, &child, CBS_ASN1_OBJECT)) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
return NULL;
}
- if (tag != V_ASN1_OBJECT || xclass != V_ASN1_UNIVERSAL) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_EXPECTING_AN_OBJECT);
- return NULL;
- }
- ASN1_OBJECT *ret = c2i_ASN1_OBJECT(a, &p, len);
- if (ret) {
- *pp = p;
+ const uint8_t *contents = CBS_data(&child);
+ ASN1_OBJECT *ret = c2i_ASN1_OBJECT(out, &contents, CBS_len(&child));
+ if (ret != NULL) {
+ // |c2i_ASN1_OBJECT| should have consumed the entire input.
+ assert(CBS_data(&cbs) == contents);
+ *inp = CBS_data(&cbs);
}
return ret;
}
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 36ad676..640b726 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -552,8 +552,10 @@
{0x81, 0x01, 0x00},
// Element is constructed.
{0x21, 0x01, 0x00},
- // TODO(https://crbug.com/boringssl/354): Reject non-DER encodings of TRUE
- // and test this.
+ // Not a DER encoding of TRUE.
+ {0x01, 0x01, 0x01},
+ // Non-minimal tag length.
+ {0x01, 0x81, 0x01, 0xff},
};
for (const auto &invalid : kInvalidBooleans) {
SCOPED_TRACE(Bytes(invalid));
@@ -690,6 +692,8 @@
{0x86, 0x03, 0x2b, 0x65, 0x70},
// Element is constructed.
{0x26, 0x03, 0x2b, 0x65, 0x70},
+ // Non-minimal tag length.
+ {0x06, 0x81, 0x03, 0x2b, 0x65, 0x70},
};
for (const auto &invalid : kInvalidObjects) {
SCOPED_TRACE(Bytes(invalid));
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 26634a8..ff77955 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -443,9 +443,6 @@
//
// WARNING: This function's is slightly different from other |d2i_*| functions
// because |ASN1_BOOLEAN| is not a pointer type.
-//
-// TODO(https://crbug.com/boringssl/354): This function currently also accepts
-// BER, but this will be removed in the future.
OPENSSL_EXPORT ASN1_BOOLEAN d2i_ASN1_BOOLEAN(ASN1_BOOLEAN *out,
const unsigned char **inp,
long len);
@@ -1444,15 +1441,12 @@
// d2i_ASN1_OBJECT parses a DER-encoded ASN.1 OBJECT IDENTIFIER from up to |len|
// bytes at |*inp|, as described in |d2i_SAMPLE|.
-//
-// TODO(https://crbug.com/boringssl/354): This function currently also accepts
-// BER, but this will be removed in the future.
OPENSSL_EXPORT ASN1_OBJECT *d2i_ASN1_OBJECT(ASN1_OBJECT **out,
const uint8_t **inp, long len);
// i2d_ASN1_OBJECT marshals |in| as a DER-encoded ASN.1 OBJECT IDENTIFIER, as
// described in |i2d_SAMPLE|.
-OPENSSL_EXPORT int i2d_ASN1_OBJECT(const ASN1_OBJECT *a, uint8_t **outp);
+OPENSSL_EXPORT int i2d_ASN1_OBJECT(const ASN1_OBJECT *in, uint8_t **outp);
// c2i_ASN1_OBJECT decodes |len| bytes from |*inp| as the contents of a
// DER-encoded OBJECT IDENTIFIER, excluding the tag and length. It behaves like
