SNMP: Fix parsing of truncated ASN.1 length fields#2450
Conversation
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.
Also encapsulated duplicated ASN `type` parsing code into a local helper function.
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Disclaimer: My confidence in this ugly code is low. It passes an SNMP walk test, but some buggy variations of this code passed that test as well, so that test alone is not a good indication that these changes did not make things worse somewhere!
If you can test this code, please do.
| return (NULL); | ||
| } | ||
|
|
||
| u_char lengthbyte = *data; |
There was a problem hiding this comment.
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.
rousskov
added
S-waiting-for-more-reviewers
| 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 *); |
There was a problem hiding this comment.
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).
| u_char *asn_parse_length(u_char *, int *, u_int *); |
| * field bytes or all of the expected variable-length object bytes). OUT | ||
| * values documented below are only meaningful for successful outcomes. | ||
| */ | ||
| u_char * |
There was a problem hiding this comment.
| u_char * | |
| static u_char * |
and shuffle the whole changed function up above first caller.
|
FTR; The current official code for this library is published as the "ucd-snmp" legacy API by the NetSNMP project ( |
2 participants
Also improved parsing of truncated input for a few other ASN.1 fields.
Also encapsulated heavily duplicated ASN.1
typefield parsing code.