From 3140e4598ad1f0960d33dbf0815eeed9c8ea1166 Mon Sep 17 00:00:00 2001 From: Pierre Chifflier Date: Thu, 1 Mar 2018 18:58:41 +0100 Subject: [PATCH] DER parser: ensure errcode is set for every return path --- src/util-decode-der.c | 93 ++++++++++++++++++++++++++++++++++--------- src/util-decode-der.h | 8 +++- 2 files changed, 80 insertions(+), 21 deletions(-) diff --git a/src/util-decode-der.c b/src/util-decode-der.c index acc79f3e77..2d57d416ad 100644 --- a/src/util-decode-der.c +++ b/src/util-decode-der.c @@ -158,8 +158,10 @@ static Asn1Generic * DecodeAsn1DerGeneric(const unsigned char *buffer, el_type = el.tag; - if (el.tag == 0x1f) + if (el.tag == 0x1f) { + *errcode = ERR_DER_INVALID_TAG; return NULL; + } switch (el.cls) { case ASN1_CLASS_CONTEXTSPEC: @@ -279,6 +281,7 @@ static Asn1Generic * DecodeAsn1DerGeneric(const unsigned char *buffer, child->length += (d_ptr - save_d_ptr); if (child->length > max_size - (d_ptr - buffer)) { + *errcode = ERR_DER_ELEMENT_SIZE_TOO_BIG; SCFree(child); return NULL; } @@ -286,11 +289,14 @@ static Asn1Generic * DecodeAsn1DerGeneric(const unsigned char *buffer, break; }; - if (child == NULL) + if (child == NULL) { + *errcode = ERR_DER_INVALID_OBJECT; return NULL; + } /* child length should never be zero */ if (child->length == 0) { + *errcode = ERR_DER_INVALID_OBJECT; SCFree(child); return NULL; } @@ -333,8 +339,10 @@ static Asn1Generic * DecodeAsn1DerInteger(const unsigned char *buffer, } a = Asn1GenericNew(); - if (a == NULL) + if (a == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } a->type = ASN1_INTEGER; a->length = (d_ptr - buffer) + numbytes; @@ -342,6 +350,7 @@ static Asn1Generic * DecodeAsn1DerInteger(const unsigned char *buffer, a->str = SCMalloc(2*numbytes + 1); if (a->str == NULL) { + *errcode = ERR_DER_GENERIC; SCFree(a); return NULL; } @@ -389,6 +398,7 @@ static Asn1Generic * DecodeAsn1DerBoolean(const unsigned char *buffer, numbytes = d_ptr[1]; if ((uint32_t)(numbytes + 2) > size) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; } d_ptr += 2; @@ -398,8 +408,10 @@ static Asn1Generic * DecodeAsn1DerBoolean(const unsigned char *buffer, } a = Asn1GenericNew(); - if (a == NULL) + if (a == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } a->type = ASN1_BOOLEAN; a->length = (d_ptr - buffer); @@ -419,6 +431,7 @@ static Asn1Generic * DecodeAsn1DerNull(const unsigned char *buffer, numbytes = d_ptr[1]; if ((uint32_t)(numbytes + 2) > size) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; } d_ptr += 2; @@ -428,8 +441,10 @@ static Asn1Generic * DecodeAsn1DerNull(const unsigned char *buffer, } a = Asn1GenericNew(); - if (a == NULL) + if (a == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } a->type = ASN1_NULL; a->length = (d_ptr - buffer); @@ -459,6 +474,7 @@ static Asn1Generic * DecodeAsn1DerBitstring(const unsigned char *buffer, } else { numbytes = c & 0x7f; if ((uint32_t)(numbytes + 2) > max_size) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; } d_ptr++; @@ -467,18 +483,23 @@ static Asn1Generic * DecodeAsn1DerBitstring(const unsigned char *buffer, } } - if ((d_ptr-buffer) + length > max_size) + if ((d_ptr-buffer) + length > max_size) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; + } a = Asn1GenericNew(); - if (a == NULL) + if (a == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } a->type = ASN1_BITSTRING; a->strlen = length; a->str = SCMalloc(length); if (a->str == NULL) { + *errcode = ERR_DER_GENERIC; SCFree(a); return NULL; } @@ -512,6 +533,7 @@ static Asn1Generic * DecodeAsn1DerOid(const unsigned char *buffer, } else { numbytes = c & 0x7f; if ((uint32_t)(numbytes + 2) > max_size) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; } d_ptr++; @@ -520,17 +542,22 @@ static Asn1Generic * DecodeAsn1DerOid(const unsigned char *buffer, } } - if (oid_length == 0 || (d_ptr-buffer) + oid_length > max_size) + if (oid_length == 0 || (d_ptr-buffer) + oid_length > max_size) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; + } a = Asn1GenericNew(); - if (a == NULL) + if (a == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } a->type = ASN1_OID; a->str = SCMalloc(MAX_OID_LENGTH); if (a->str == NULL) { + *errcode = ERR_DER_GENERIC; SCFree(a); return NULL; } @@ -540,6 +567,7 @@ static Asn1Generic * DecodeAsn1DerOid(const unsigned char *buffer, d_ptr++; if (oid_length + (d_ptr-buffer) > max_size) { + *errcode = ERR_DER_INVALID_SIZE; SCFree(a->str); SCFree(a); return NULL; @@ -590,6 +618,7 @@ static Asn1Generic * DecodeAsn1DerIA5String(const unsigned char *buffer, } else { numbytes = c & 0x7f; if (max_size < 1 + numbytes) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; } max_size -= 1 + numbytes; @@ -605,14 +634,17 @@ static Asn1Generic * DecodeAsn1DerIA5String(const unsigned char *buffer, } a = Asn1GenericNew(); - if (a == NULL) + if (a == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } a->type = ASN1_IA5STRING; a->strlen = length; a->str = SCMalloc(length+1); if (a->str == NULL) { + *errcode = ERR_DER_GENERIC; SCFree(a); return NULL; } @@ -648,6 +680,7 @@ static Asn1Generic * DecodeAsn1DerOctetString(const unsigned char *buffer, } else { numbytes = c & 0x7f; if (max_size < 1 + numbytes) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; } max_size -= 1 + numbytes; @@ -662,8 +695,10 @@ static Asn1Generic * DecodeAsn1DerOctetString(const unsigned char *buffer, return NULL; } a = Asn1GenericNew(); - if (a == NULL) + if (a == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } a->type = ASN1_OCTETSTRING; a->strlen = length; @@ -672,6 +707,7 @@ static Asn1Generic * DecodeAsn1DerOctetString(const unsigned char *buffer, use the string in printf */ a->str = SCMalloc(length + 1); if (a->str == NULL) { + *errcode = ERR_DER_GENERIC; SCFree(a); return NULL; } @@ -718,6 +754,7 @@ static Asn1Generic * DecodeAsn1DerPrintableString(const unsigned char *buffer, numbytes = c & 0x7f; d_ptr++; if (2 + numbytes > max_size) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; } if (DecodeAsn1BuildValue(&d_ptr, &length, numbytes, errcode) == -1) { @@ -731,14 +768,17 @@ static Asn1Generic * DecodeAsn1DerPrintableString(const unsigned char *buffer, } a = Asn1GenericNew(); - if (a == NULL) + if (a == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } a->type = ASN1_PRINTSTRING; a->strlen = length; a->str = SCMalloc(length+1); if (a->str == NULL) { + *errcode = ERR_DER_GENERIC; SCFree(a); return NULL; } @@ -764,8 +804,10 @@ static Asn1Generic * DecodeAsn1DerSequence(const unsigned char *buffer, d_ptr++; node = Asn1GenericNew(); - if (node == NULL) + if (node == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } node->type = ASN1_SEQUENCE; @@ -780,6 +822,7 @@ static Asn1Generic * DecodeAsn1DerSequence(const unsigned char *buffer, numbytes = c & 0x7f; d_ptr++; if ( 2 + numbytes > max_size ) { + *errcode = ERR_DER_INVALID_SIZE; SCFree(node); return NULL; } @@ -840,8 +883,10 @@ static Asn1Generic * DecodeAsn1DerSet(const unsigned char *buffer, d_ptr++; node = Asn1GenericNew(); - if (node == NULL) + if (node == NULL) { + *errcode = ERR_DER_GENERIC; return NULL; + } node->type = ASN1_SET; node->data = NULL; @@ -855,6 +900,7 @@ static Asn1Generic * DecodeAsn1DerSet(const unsigned char *buffer, } else { numbytes = c & 0x7f; if (2 + numbytes > max_size) { + *errcode = ERR_DER_INVALID_SIZE; SCFree(node); return NULL; } @@ -940,31 +986,40 @@ Asn1Generic * DecodeDer(const unsigned char *buffer, uint32_t size, uint8_t c; DEBUG_VALIDATE_BUG_ON(errcode == NULL); + *errcode = 0; - if (size < 2) + if (size < 2) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; + } /* check that buffer is an ASN.1 structure (basic checks) */ - if (d_ptr[0] != 0x30 && d_ptr[1] != 0x82) /* Sequence */ + if (d_ptr[0] != 0x30 && d_ptr[1] != 0x82) { /* Sequence */ + *errcode = ERR_DER_UNKNOWN_ELEMENT; return NULL; + } c = d_ptr[1]; - if ((c & (1<<7))>>7 != 1) + if ((c & (1<<7))>>7 != 1) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; + } numbytes = c & 0x7f; d_ptr += 2; if (size < numbytes + 2) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; } if (DecodeAsn1BuildValue(&d_ptr, &d_length, numbytes, errcode) == -1) { return NULL; } - if (d_length+(d_ptr-buffer) != size) + if (d_length+(d_ptr-buffer) != size) { + *errcode = ERR_DER_INVALID_SIZE; return NULL; + } - *errcode = 0; cert = DecodeAsn1DerGeneric(buffer, size, 0 /* depth */, 0, errcode); return cert; diff --git a/src/util-decode-der.h b/src/util-decode-der.h index 9710f68c98..5110a4e0f1 100644 --- a/src/util-decode-der.h +++ b/src/util-decode-der.h @@ -78,13 +78,13 @@ typedef struct Asn1Generic_ { struct Asn1Generic_ *next; /* only if type is sequence */ } Asn1Generic; -/* Generic error */ +/* Generic error (failed allocation, etc.) */ #define ERR_DER_GENERIC 0x01 /* Unknown ASN.1 element type */ #define ERR_DER_UNKNOWN_ELEMENT 0x02 /* One element requires to read more bytes than available */ #define ERR_DER_ELEMENT_SIZE_TOO_BIG 0x03 -/* One element size is invalid (more than 4 bytes long) */ +/* One element size is invalid (e.g more than 4 bytes long) */ #define ERR_DER_INVALID_SIZE 0x04 /* Unsupported string type */ #define ERR_DER_UNSUPPORTED_STRING 0x05 @@ -92,6 +92,10 @@ typedef struct Asn1Generic_ { #define ERR_DER_MISSING_ELEMENT 0x06 /* Generic error */ #define ERR_DER_RECURSION_LIMIT 0x07 +/* Unexpected or unknown tag */ +#define ERR_DER_INVALID_TAG 0x08 +/* Invalid element: empty object, etc. */ +#define ERR_DER_INVALID_OBJECT 0x09 Asn1Generic * DecodeDer(const unsigned char *buffer, uint32_t size, uint32_t *errcode) __attribute__((nonnull)); void DerFree(Asn1Generic *a); -- 2.47.2