From 3848ea06872438ae4df2364260c45f44bc687f01 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 26 Jun 2026 13:42:07 -0400 Subject: [PATCH 1/4] SNMP: Fix parsing of truncated ASN.1 length fields XXX: This implementation assumes that asn_parse_length() receives input length in `*length`. It does not. `*length` is undefined in current callers, making reasonable asn_parse_length() implementation impossible. --- lib/snmplib/asn1.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/snmplib/asn1.c b/lib/snmplib/asn1.c index d2acb7d682f..459ff9f5694 100644 --- a/lib/snmplib/asn1.c +++ b/lib/snmplib/asn1.c @@ -540,6 +540,13 @@ asn_parse_length(u_char * data, u_int * length) /* u_char *data; IN - pointer to start of length field */ /* u_int *length; OUT - value of length field */ { + const auto dataLength = *length; + + if (!dataLength) { + snmp_set_api_error(SNMPERR_ASN_DECODE); + return (NULL); + } + u_char lengthbyte = *data; if (lengthbyte & ASN_LONG_LEN) { @@ -553,6 +560,13 @@ asn_parse_length(u_char * data, u_int * length) snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL); } + + // account for the lengthbyte itself (i.e. "+ 1" in memcpy() below) + if (dataLength <= lengthbyte) { + snmp_set_api_error(SNMPERR_ASN_DECODE); + return (NULL); + } + *length = (u_int) 0; memcpy((char *) (length), (char *) data + 1, (int) lengthbyte); *length = ntohl(*length); From ac0bde1ce28b13140e2ec20bd3c7b4a2ac20a6fd Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 26 Jun 2026 17:54:24 -0400 Subject: [PATCH 2/4] fixup: Refactored to supply input length to asn_parse_length() Also encapsulated duplicated ASN `type` parsing code into a local helper function. --- include/asn1.h | 2 +- lib/snmplib/asn1.c | 137 +++++++++++++++++++++++++++++---------------- 2 files changed, 90 insertions(+), 49 deletions(-) diff --git a/include/asn1.h b/include/asn1.h index d54e33f411f..2ffccebb3ea 100644 --- a/include/asn1.h +++ b/include/asn1.h @@ -86,7 +86,7 @@ u_char *asn_build_string(u_char *, int *, u_char, u_char *, int); u_char *asn_parse_header(u_char *, int *, u_char *); u_char *asn_build_header_with_truth(u_char *, int *, u_char, int, int); -u_char *asn_parse_length(u_char *, u_int *); +u_char *asn_parse_length(u_char *, int *, u_int *); u_char *asn_build_length(u_char *, int *, int, int); u_char *asn_parse_objid(u_char *, int *, u_char *, oid *, int *); u_char *asn_build_objid(u_char *, int *, u_char, oid *, int); diff --git a/lib/snmplib/asn1.c b/lib/snmplib/asn1.c index 459ff9f5694..6910f169d9a 100644 --- a/lib/snmplib/asn1.c +++ b/lib/snmplib/asn1.c @@ -100,6 +100,29 @@ asn_build_header(u_char * data, /* IN - ptr to start of object */ return (asn_build_header_with_truth(data, datalength, type, length, 0)); } +/* + * asn_parse_type - extracts a one-byte type field, advancing input + * + * Returns NULL on any error. + * On success, returns a pointer to the first byte after the type field + * and decrements `datalength` by one. + */ +static u_char * +asn_parse_type(u_char * data, int *datalength, u_char * type) +/* u_char *data; IN - input buffer */ +/* int *datalength; IN/OUT - number of bytes in input buffer */ +/* u_char *type; OUT - ASN type of object */ +{ + if (*datalength < 1) { + // not enough input for `type` + snmp_set_api_error(SNMPERR_ASN_DECODE); + return (NULL); + } + *type = *data++; + --*datalength; + return data; +} + /* * asn_parse_int - pulls an int out of an ASN int type. * On entry, datalength is input as the number of valid bytes following @@ -132,25 +155,23 @@ asn_parse_int(u_char * data, int *datalength, return (NULL); } /* Type */ - *type = *bufp++; + bufp = asn_parse_type(bufp, datalength, type); + if (bufp == NULL) + return (NULL); /* Extract length */ - bufp = asn_parse_length(bufp, &asn_length); + bufp = asn_parse_length(bufp, datalength, &asn_length); if (bufp == NULL) return (NULL); + assert(asn_length <= *datalength); - /* Make sure the entire int is in the buffer */ - if (asn_length + (bufp - data) > *datalength) { - snmp_set_api_error(SNMPERR_ASN_DECODE); - return (NULL); - } /* Can we store this int? */ if (asn_length > intsize) { snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL); } /* Remaining data */ - *datalength -= (int) asn_length + (bufp - data); + *datalength -= (int) asn_length; /* Is the int negative? */ if (*bufp & 0x80) @@ -197,18 +218,16 @@ asn_parse_unsigned_int(u_char * data, int *datalength, return (NULL); } /* Type */ - *type = *bufp++; + bufp = asn_parse_type(bufp, datalength, type); + if (bufp == NULL) + return (NULL); /* Extract length */ - bufp = asn_parse_length(bufp, &asn_length); + bufp = asn_parse_length(bufp, datalength, &asn_length); if (bufp == NULL) return (NULL); + assert(asn_length <= *datalength); - /* Make sure the entire int is in the buffer */ - if (asn_length + (bufp - data) > *datalength) { - snmp_set_api_error(SNMPERR_ASN_DECODE); - return (NULL); - } /* Can we store this int? */ if ((asn_length > (intsize + 1)) || ((asn_length == intsize + 1) && *bufp != 0x00)) { @@ -216,7 +235,7 @@ asn_parse_unsigned_int(u_char * data, int *datalength, return (NULL); } /* Remaining data */ - *datalength -= (int) asn_length + (bufp - data); + *datalength -= (int) asn_length; /* Is the int negative? */ if (*bufp & 0x80) @@ -400,22 +419,22 @@ asn_parse_string(u_char * data, int *datalength, u_char *bufp = data; u_int asn_length; - *type = *bufp++; - bufp = asn_parse_length(bufp, &asn_length); + bufp = asn_parse_type(bufp, datalength, type); if (bufp == NULL) return (NULL); - if (asn_length + (bufp - data) > *datalength) { - snmp_set_api_error(SNMPERR_ASN_DECODE); + bufp = asn_parse_length(bufp, datalength, &asn_length); + if (bufp == NULL) return (NULL); - } + assert(asn_length <= *datalength); + if (asn_length > *strlength) { snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL); } memcpy((char *) string, (char *) bufp, (int) asn_length); *strlength = (int) asn_length; - *datalength -= (int) asn_length + (bufp - data); + *datalength -= (int) asn_length; return (bufp + asn_length); } @@ -473,7 +492,6 @@ asn_parse_header(u_char * data, int *datalength, u_char * type) /* u_char *type; OUT - ASN type of object */ { u_char *bufp = data; - int header_len; u_int asn_length; /* this only works on data types < 30, i.e. no extension octets */ @@ -481,13 +499,17 @@ asn_parse_header(u_char * data, int *datalength, u_char * type) snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL); } - *type = *bufp; - bufp = asn_parse_length(bufp + 1, &asn_length); + + bufp = asn_parse_type(bufp, datalength, type); if (bufp == NULL) return (NULL); - header_len = bufp - data; - if (header_len + asn_length > *datalength || asn_length > (u_int)(2 << 18) ) { + bufp = asn_parse_length(bufp, datalength, &asn_length); + if (bufp == NULL) + return (NULL); + assert(asn_length <= *datalength); + + if (asn_length > (u_int)(2 << 18)) { snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL); } @@ -528,27 +550,39 @@ asn_build_header_with_truth(u_char * data, int *datalength, } /* - * asn_parse_length - interprets the length of the current object. - * On exit, length contains the value of this length field. + * asn_parse_length - parses the length field in front of a variable-length ASN object. * - * Returns a pointer to the first byte after this length - * field (aka: the start of the data field). - * Returns NULL on any error. + * On success, returns a pointer to the first byte after this length field + * (i.e. the start of the variable-length ASN object). + * + * On error, returns NULL. Errors include "indefinite length" (i.e. 0x80) and + * truncated input (e.g., does not contain either all of the expected length + * field bytes or all of the expected variable-length object bytes). OUT + * values documented below are only meaningful for successful outcomes. */ u_char * -asn_parse_length(u_char * data, u_int * length) +asn_parse_length(u_char *data, int *dataLength, u_int *length) /* u_char *data; IN - pointer to start of length field */ +/* int *datalength; IN/OUT - # of valid bytes in returned buffer */ /* u_int *length; OUT - value of length field */ { - const auto dataLength = *length; + // ASN.1 length ::= short-form | long-form + // short-form ::= octet ; bit 8 = 0 + // long-form ::= first-octet + // first-octet ::= octet ; bit 8 = 1, bits 7..1 = N - if (!dataLength) { + if (!*dataLength) { + // not enough data, even for the short-form snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL); } u_char lengthbyte = *data; + // consume either the entire short-form value or the first-octet of the long-form value + ++data; + --*dataLength; + if (lengthbyte & ASN_LONG_LEN) { lengthbyte &= ~ASN_LONG_LEN; /* turn MSb off */ @@ -561,23 +595,31 @@ asn_parse_length(u_char * data, u_int * length) return (NULL); } - // account for the lengthbyte itself (i.e. "+ 1" in memcpy() below) - if (dataLength <= lengthbyte) { + if (lengthbyte > *dataLength) { + // not enough data for "N subsequent octets" snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL); } *length = (u_int) 0; - memcpy((char *) (length), (char *) data + 1, (int) lengthbyte); + memcpy((char *) (length), (char *) data, (int) lengthbyte); *length = ntohl(*length); *length >>= (8 * ((sizeof *length) - lengthbyte)); - return (data + lengthbyte + 1); + // consume "N subsequent octets" + data += lengthbyte; + *dataLength -= lengthbyte; + } else { + /* short-form */ + *length = lengthbyte; } - /* short asnlength */ - *length = (int) lengthbyte; - return (data + 1); + if (*length > *dataLength) { + // not enough data for the variable-length object after this length field + snmp_set_api_error(SNMPERR_ASN_DECODE); + return (NULL); + } + return data; } u_char * @@ -669,16 +711,15 @@ asn_parse_objid(u_char * data, int *datalength, int length; u_int asn_length; - *type = *bufp++; - bufp = asn_parse_length(bufp, &asn_length); + bufp = asn_parse_type(bufp, datalength, type); if (bufp == NULL) return (NULL); - if (asn_length + (bufp - data) > *datalength) { - snmp_set_api_error(SNMPERR_ASN_DECODE); + bufp = asn_parse_length(bufp, datalength, &asn_length); + if (bufp == NULL) return (NULL); - } - *datalength -= (int) asn_length + (bufp - data); + assert(asn_length <= *datalength); + *datalength -= (int) asn_length; /* Handle invalid object identifier encodings of the form 06 00 robustly */ if (asn_length == 0) From 6de6e51ce259d1ef87078fd290c193c99994dec2 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 29 Jun 2026 11:01:12 -0400 Subject: [PATCH 3/4] Converted asn_parse_length() to a local static helper Context: https://github.com/squid-cache/squid/pull/2450#pullrequestreview-4584347399 --- include/asn1.h | 1 - lib/snmplib/asn1.c | 146 ++++++++++++++++++++++----------------------- 2 files changed, 73 insertions(+), 74 deletions(-) diff --git a/include/asn1.h b/include/asn1.h index 2ffccebb3ea..fb2d54d60d8 100644 --- a/include/asn1.h +++ b/include/asn1.h @@ -86,7 +86,6 @@ u_char *asn_build_string(u_char *, int *, u_char, u_char *, int); u_char *asn_parse_header(u_char *, int *, u_char *); u_char *asn_build_header_with_truth(u_char *, int *, u_char, int, int); -u_char *asn_parse_length(u_char *, int *, u_int *); u_char *asn_build_length(u_char *, int *, int, int); u_char *asn_parse_objid(u_char *, int *, u_char *, oid *, int *); u_char *asn_build_objid(u_char *, int *, u_char, oid *, int); diff --git a/lib/snmplib/asn1.c b/lib/snmplib/asn1.c index 6910f169d9a..425b212a703 100644 --- a/lib/snmplib/asn1.c +++ b/lib/snmplib/asn1.c @@ -123,6 +123,79 @@ asn_parse_type(u_char * data, int *datalength, u_char * type) return data; } +/* + * asn_parse_length - parses the length field in front of a variable-length ASN object. + * + * On success, returns a pointer to the first byte after this length field + * (i.e. the start of the variable-length ASN object). + * + * On error, returns NULL. Errors include "indefinite length" (i.e. 0x80) and + * truncated input (e.g., does not contain either all of the expected length + * field bytes or all of the expected variable-length object bytes). OUT + * values documented below are only meaningful for successful outcomes. + */ +static u_char * +asn_parse_length(u_char *data, int *dataLength, u_int *length) +/* u_char *data; IN - pointer to start of length field */ +/* int *datalength; IN/OUT - # of valid bytes in returned buffer */ +/* u_int *length; OUT - value of length field */ +{ + // ASN.1 length ::= short-form | long-form + // short-form ::= octet ; bit 8 = 0 + // long-form ::= first-octet + // first-octet ::= octet ; bit 8 = 1, bits 7..1 = N + + if (!*dataLength) { + // not enough data, even for the short-form + snmp_set_api_error(SNMPERR_ASN_DECODE); + return (NULL); + } + + u_char lengthbyte = *data; + + // consume either the entire short-form value or the first-octet of the long-form value + ++data; + --*dataLength; + + if (lengthbyte & ASN_LONG_LEN) { + lengthbyte &= ~ASN_LONG_LEN; /* turn MSb off */ + + if (lengthbyte == 0) { + snmp_set_api_error(SNMPERR_ASN_DECODE); + return (NULL); + } + if (lengthbyte > sizeof(int)) { + snmp_set_api_error(SNMPERR_ASN_DECODE); + return (NULL); + } + + if (lengthbyte > *dataLength) { + // not enough data for "N subsequent octets" + snmp_set_api_error(SNMPERR_ASN_DECODE); + return (NULL); + } + + *length = (u_int) 0; + memcpy((char *) (length), (char *) data, (int) lengthbyte); + *length = ntohl(*length); + *length >>= (8 * ((sizeof *length) - lengthbyte)); + + // consume "N subsequent octets" + data += lengthbyte; + *dataLength -= lengthbyte; + } else { + /* short-form */ + *length = lengthbyte; + } + + if (*length > *dataLength) { + // not enough data for the variable-length object after this length field + snmp_set_api_error(SNMPERR_ASN_DECODE); + return (NULL); + } + return data; +} + /* * asn_parse_int - pulls an int out of an ASN int type. * On entry, datalength is input as the number of valid bytes following @@ -549,79 +622,6 @@ asn_build_header_with_truth(u_char * data, int *datalength, return (asn_build_length(data, datalength, length, truth)); } -/* - * asn_parse_length - parses the length field in front of a variable-length ASN object. - * - * On success, returns a pointer to the first byte after this length field - * (i.e. the start of the variable-length ASN object). - * - * On error, returns NULL. Errors include "indefinite length" (i.e. 0x80) and - * truncated input (e.g., does not contain either all of the expected length - * field bytes or all of the expected variable-length object bytes). OUT - * values documented below are only meaningful for successful outcomes. - */ -u_char * -asn_parse_length(u_char *data, int *dataLength, u_int *length) -/* u_char *data; IN - pointer to start of length field */ -/* int *datalength; IN/OUT - # of valid bytes in returned buffer */ -/* u_int *length; OUT - value of length field */ -{ - // ASN.1 length ::= short-form | long-form - // short-form ::= octet ; bit 8 = 0 - // long-form ::= first-octet - // first-octet ::= octet ; bit 8 = 1, bits 7..1 = N - - if (!*dataLength) { - // not enough data, even for the short-form - snmp_set_api_error(SNMPERR_ASN_DECODE); - return (NULL); - } - - u_char lengthbyte = *data; - - // consume either the entire short-form value or the first-octet of the long-form value - ++data; - --*dataLength; - - if (lengthbyte & ASN_LONG_LEN) { - lengthbyte &= ~ASN_LONG_LEN; /* turn MSb off */ - - if (lengthbyte == 0) { - snmp_set_api_error(SNMPERR_ASN_DECODE); - return (NULL); - } - if (lengthbyte > sizeof(int)) { - snmp_set_api_error(SNMPERR_ASN_DECODE); - return (NULL); - } - - if (lengthbyte > *dataLength) { - // not enough data for "N subsequent octets" - snmp_set_api_error(SNMPERR_ASN_DECODE); - return (NULL); - } - - *length = (u_int) 0; - memcpy((char *) (length), (char *) data, (int) lengthbyte); - *length = ntohl(*length); - *length >>= (8 * ((sizeof *length) - lengthbyte)); - - // consume "N subsequent octets" - data += lengthbyte; - *dataLength -= lengthbyte; - } else { - /* short-form */ - *length = lengthbyte; - } - - if (*length > *dataLength) { - // not enough data for the variable-length object after this length field - snmp_set_api_error(SNMPERR_ASN_DECODE); - return (NULL); - } - return data; -} - u_char * asn_build_length(u_char * data, int *datalength, int length, int truth) From 3d34b05ef7e7fd39d909cf413da66127fb890c74 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 29 Jun 2026 11:03:05 -0400 Subject: [PATCH 4/4] fixup: Spell dataLength like other functions do --- lib/snmplib/asn1.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/snmplib/asn1.c b/lib/snmplib/asn1.c index 425b212a703..c6319030bb1 100644 --- a/lib/snmplib/asn1.c +++ b/lib/snmplib/asn1.c @@ -135,7 +135,7 @@ asn_parse_type(u_char * data, int *datalength, u_char * type) * values documented below are only meaningful for successful outcomes. */ static u_char * -asn_parse_length(u_char *data, int *dataLength, u_int *length) +asn_parse_length(u_char *data, int *datalength, u_int *length) /* u_char *data; IN - pointer to start of length field */ /* int *datalength; IN/OUT - # of valid bytes in returned buffer */ /* u_int *length; OUT - value of length field */ @@ -145,7 +145,7 @@ asn_parse_length(u_char *data, int *dataLength, u_int *length) // long-form ::= first-octet // first-octet ::= octet ; bit 8 = 1, bits 7..1 = N - if (!*dataLength) { + if (!*datalength) { // not enough data, even for the short-form snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL); @@ -155,7 +155,7 @@ asn_parse_length(u_char *data, int *dataLength, u_int *length) // consume either the entire short-form value or the first-octet of the long-form value ++data; - --*dataLength; + --*datalength; if (lengthbyte & ASN_LONG_LEN) { lengthbyte &= ~ASN_LONG_LEN; /* turn MSb off */ @@ -169,7 +169,7 @@ asn_parse_length(u_char *data, int *dataLength, u_int *length) return (NULL); } - if (lengthbyte > *dataLength) { + if (lengthbyte > *datalength) { // not enough data for "N subsequent octets" snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL); @@ -182,13 +182,13 @@ asn_parse_length(u_char *data, int *dataLength, u_int *length) // consume "N subsequent octets" data += lengthbyte; - *dataLength -= lengthbyte; + *datalength -= lengthbyte; } else { /* short-form */ *length = lengthbyte; } - if (*length > *dataLength) { + if (*length > *datalength) { // not enough data for the variable-length object after this length field snmp_set_api_error(SNMPERR_ASN_DECODE); return (NULL);