Skip to content

SNMP: Fix parsing of truncated ASN.1 length fields #2450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rousskov wants to merge 4 commits into
base: master
Choose a base branch
from
+127 −73
Open

SNMP: Fix parsing of truncated ASN.1 length fields #2450

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3848ea0
SNMP: Fix parsing of truncated ASN.1 length fields
rousskov Jun 26, 2026
ac0bde1
fixup: Refactored to supply input length to asn_parse_length()
rousskov Jun 26, 2026
6de6e51
Converted asn_parse_length() to a local static helper
rousskov Jun 29, 2026
3d34b05
fixup: Spell dataLength like other functions do
rousskov Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/asn1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 *);

@yadij yadij Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This being an ABI breaking change in a bundled third-party library please just remove the dropped function and make the custom one a static like the new type parser (shuffle it up higher in the file as needed).

Suggested change
u_char *asn_parse_length(u_char *, int *, u_int *);

@rousskov rousskov Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6de6e51.

To save time, I am not going to argue about the premise of this change request.

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);
Expand Down
143 changes: 99 additions & 44 deletions lib/snmplib/asn1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -197,26 +218,24 @@ 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)) {
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)
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -473,21 +492,24 @@ 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 */
if (IS_EXTENSION_ID(*bufp)) {
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);

bufp = asn_parse_length(bufp, datalength, &asn_length);
if (bufp == NULL)
return (NULL);
assert(asn_length <= *datalength);

header_len = bufp - data;
if (header_len + asn_length > *datalength || asn_length > (u_int)(2 << 18) ) {
if (asn_length > (u_int)(2 << 18)) {
snmp_set_api_error(SNMPERR_ASN_DECODE);
return (NULL);
}
Expand Down Expand Up @@ -528,20 +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 *

@yadij yadij Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
u_char *
static u_char *

and shuffle the whole changed function up above first caller.

@rousskov rousskov Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6de6e51.

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 */
{
// ASN.1 length ::= short-form | long-form
// short-form ::= octet ; bit 8 = 0
// long-form ::= first-octet <N subsequent octets>
// 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;

@rousskov rousskov Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example of input access that lacks length checks in official code.

To add the missing check, I had to add a dataLength parameter to asn_parse_length(). Most (if not all) ASN parsing functions update their data length parameter, and such updates are a good idea, so new PR code follows that pattern. Naturally, all that required caller adjustments. While adjusting caller code, I added a few other "obviously" missing checks. I did not perform a full audit though!

This comment does not request any changes.


// 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 */

Expand All @@ -553,17 +594,32 @@ asn_parse_length(u_char * data, u_int * length)
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 + 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 *
Expand Down Expand Up @@ -655,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)
Expand Down
Loading