From: Pierre Chifflier Date: Thu, 15 Feb 2018 14:00:35 +0000 (+0100) Subject: DER parser: fix undefined behaviors and add missing length tests X-Git-Tag: suricata-4.1.0-beta1~22 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2d34e402c017f395f225e99874f5ca4a0c3b22f9;p=thirdparty%2Fsuricata.git DER parser: fix undefined behaviors and add missing length tests Fix several undefined behaviors, caused by possible use or read of uninitialized memory. --- diff --git a/src/util-decode-der.c b/src/util-decode-der.c index 02c16723d9..acc79f3e77 100644 --- a/src/util-decode-der.c +++ b/src/util-decode-der.c @@ -147,6 +147,10 @@ static Asn1Generic * DecodeAsn1DerGeneric(const unsigned char *buffer, *errcode = ERR_DER_RECURSION_LIMIT; return NULL; } + if (max_size < 2) { + *errcode = ERR_DER_INVALID_SIZE; + return NULL; + } el.cls = (d_ptr[0] & 0xc0) >> 6; el.pc = (d_ptr[0] & 0x20) >> 5; @@ -247,6 +251,7 @@ static Asn1Generic * DecodeAsn1DerGeneric(const unsigned char *buffer, /* total sequence length */ const unsigned char * save_d_ptr = d_ptr; d_ptr++; + el_max_size--; c = d_ptr[0]; /* short form 8.1.3.4 */ @@ -256,7 +261,7 @@ static Asn1Generic * DecodeAsn1DerGeneric(const unsigned char *buffer, /* long form 8.1.3.5 */ } else { numbytes = c & 0x7f; - if (numbytes > el_max_size) { + if (2 + numbytes > el_max_size) { SCFree(child); *errcode = ERR_DER_ELEMENT_SIZE_TOO_BIG; return NULL; @@ -306,7 +311,7 @@ static Asn1Generic * DecodeAsn1DerInteger(const unsigned char *buffer, numbytes = d_ptr[1]; - if (numbytes > size) { + if ((uint32_t)(numbytes + 2) > size) { *errcode = ERR_DER_ELEMENT_SIZE_TOO_BIG; return NULL; } @@ -383,6 +388,9 @@ static Asn1Generic * DecodeAsn1DerBoolean(const unsigned char *buffer, Asn1Generic *a; numbytes = d_ptr[1]; + if ((uint32_t)(numbytes + 2) > size) { + return NULL; + } d_ptr += 2; if (DecodeAsn1BuildValue(&d_ptr, &value, numbytes, errcode) == -1) { @@ -410,6 +418,9 @@ static Asn1Generic * DecodeAsn1DerNull(const unsigned char *buffer, Asn1Generic *a; numbytes = d_ptr[1]; + if ((uint32_t)(numbytes + 2) > size) { + return NULL; + } d_ptr += 2; if (DecodeAsn1BuildValue(&d_ptr, &value, numbytes, errcode) == -1) { @@ -447,13 +458,16 @@ static Asn1Generic * DecodeAsn1DerBitstring(const unsigned char *buffer, /* long form 8.1.3.5 */ } else { numbytes = c & 0x7f; + if ((uint32_t)(numbytes + 2) > max_size) { + return NULL; + } d_ptr++; if (DecodeAsn1BuildValue(&d_ptr, &length, numbytes, errcode) == -1) { return NULL; } } - if (length > max_size) + if ((d_ptr-buffer) + length > max_size) return NULL; a = Asn1GenericNew(); @@ -497,13 +511,16 @@ static Asn1Generic * DecodeAsn1DerOid(const unsigned char *buffer, /* long form 8.1.3.5 */ } else { numbytes = c & 0x7f; + if ((uint32_t)(numbytes + 2) > max_size) { + return NULL; + } d_ptr++; if (DecodeAsn1BuildValue(&d_ptr, &oid_length, numbytes, errcode) == -1) { return NULL; } } - if (oid_length > max_size) + if (oid_length == 0 || (d_ptr-buffer) + oid_length > max_size) return NULL; a = Asn1GenericNew(); @@ -522,6 +539,12 @@ static Asn1Generic * DecodeAsn1DerOid(const unsigned char *buffer, snprintf(a->str, MAX_OID_LENGTH, "%d.%d", (d_ptr[0]/40), (d_ptr[0]%40)); d_ptr++; + if (oid_length + (d_ptr-buffer) > max_size) { + SCFree(a->str); + SCFree(a); + return NULL; + } + /* sub-identifiers are multi-valued, coded and 7 bits, first bit of the 8bits is used to indicate, if a new value is starting */ for (i=1; i>7 == 0) { length = c; d_ptr++; + max_size--; /* long form 8.1.3.5 */ } else { numbytes = c & 0x7f; + if (max_size < 1 + numbytes) { + return NULL; + } + max_size -= 1 + numbytes; d_ptr++; if (DecodeAsn1BuildValue(&d_ptr, &length, numbytes, errcode) == -1) { return NULL; } } - if (length == UINT32_MAX || length > max_size) { + if (length == UINT32_MAX || (d_ptr-buffer) + length > max_size) { *errcode = ERR_DER_ELEMENT_SIZE_TOO_BIG; return NULL; } @@ -606,6 +635,7 @@ static Asn1Generic * DecodeAsn1DerOctetString(const unsigned char *buffer, unsigned char c; d_ptr++; + max_size--; /* total sequence length */ c = d_ptr[0]; @@ -613,20 +643,24 @@ static Asn1Generic * DecodeAsn1DerOctetString(const unsigned char *buffer, if ((c & (1<<7))>>7 == 0) { length = c; d_ptr++; + max_size--; /* long form 8.1.3.5 */ } else { numbytes = c & 0x7f; + if (max_size < 1 + numbytes) { + return NULL; + } + max_size -= 1 + numbytes; d_ptr++; if (DecodeAsn1BuildValue(&d_ptr, &length, numbytes, errcode) == -1) { return NULL; } } - if (length == UINT32_MAX || length > max_size) { + if (length == UINT32_MAX || (d_ptr-buffer) + length > max_size) { *errcode = ERR_DER_ELEMENT_SIZE_TOO_BIG; return NULL; } - a = Asn1GenericNew(); if (a == NULL) return NULL; @@ -683,12 +717,15 @@ static Asn1Generic * DecodeAsn1DerPrintableString(const unsigned char *buffer, } else { numbytes = c & 0x7f; d_ptr++; + if (2 + numbytes > max_size) { + return NULL; + } if (DecodeAsn1BuildValue(&d_ptr, &length, numbytes, errcode) == -1) { return NULL; } } - if (length == UINT32_MAX || length > max_size) { + if (length == UINT32_MAX || (d_ptr-buffer) + length > max_size) { *errcode = ERR_DER_ELEMENT_SIZE_TOO_BIG; return NULL; } @@ -742,6 +779,10 @@ static Asn1Generic * DecodeAsn1DerSequence(const unsigned char *buffer, } else { numbytes = c & 0x7f; d_ptr++; + if ( 2 + numbytes > max_size ) { + SCFree(node); + return NULL; + } if (DecodeAsn1BuildValue(&d_ptr, &d_length, numbytes, errcode) == -1) { SCFree(node); return NULL; @@ -813,6 +854,10 @@ static Asn1Generic * DecodeAsn1DerSet(const unsigned char *buffer, /* long form 8.1.3.5 */ } else { numbytes = c & 0x7f; + if (2 + numbytes > max_size) { + SCFree(node); + return NULL; + } d_ptr++; if (DecodeAsn1BuildValue(&d_ptr, &d_length, numbytes, errcode) == -1) { SCFree(node); @@ -909,6 +954,9 @@ Asn1Generic * DecodeDer(const unsigned char *buffer, uint32_t size, numbytes = c & 0x7f; d_ptr += 2; + if (size < numbytes + 2) { + return NULL; + } if (DecodeAsn1BuildValue(&d_ptr, &d_length, numbytes, errcode) == -1) { return NULL; }