Created attachment 227978 [details] Fake snmp server to demonstrate bsnmpwalk crash. snmp_parse_resp() in libbsnmptools contains: if (resp->error_status == SNMP_ERR_NOSUCHNAME) { warnx("Error - No Such Name"); return (0); } It should be return(-1). If the name is bad, the return 0 will cause bsnmpwalk to continue with an unchecked reply, so (for example) if resp.nbindings is zero or huge, this line will generate a wild pointer: snmpwalk_nextpdu_create(op, &(resp.bindings[resp.nbindings - 1].var), &req); The attached fake snmp server demonstrates the problem: % cc bsnmpwalk1.c % ./a.out & waiting on port 1610 for a request % bsnmpwalk -s localhost:1610 SNMP: ignoring trailing junk in message bsnmpwalk: Error - No Such Name Bus error (core dumped)
Created attachment 236591 [details] Proposed fix for the bsnmpwalk crash Proposed fix for the bsnmpwalk crash - do not try to process a bogus response PDU
I am curious why SNMP_ERR_NOSUCHNAME should not just return -1.
SNMP_ERR_NOSUCHNAME is a legacy thing from SNMPv1, all over the bsnmp code it is treated like a non-fatal error - ignored, and operations continued. My reasoning is to stay compatible with the rest of the bsnmpd/libbsnmp behavior and not accidentally break compatibility with some legacy SNMPv1 server (as found in some UPSes, legacy devices etc)
(In reply to Shteryana Shopova from comment #3) Ok, I see. Should we do something with resp->nbindings in snmp_parse_resp, e.g. moving the resp->nbindings != req->nbindings error case earlier? Is it possible that resp->nbindings could be invalid/large otherwise?
I believe the length is checked in snmp_dialog() - as early as here - https://cgit.freebsd.org/src/tree/contrib/bsnmp/lib/snmp.c#n261 - I am not sure if we want to add an extra check if resp.nbindings is less than 128 ...