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 ...
^To assignee: is this aging PR still relevant?
(In reply to Mark Linimon from comment #6) I see no indication that this has been addressed, unfortunately.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=f021e3573519ff192fc708cda9ca4bba264c96f7 commit f021e3573519ff192fc708cda9ca4bba264c96f7 Author: Shteryana Shopova <syrinx@FreeBSD.org> AuthorDate: 2025-01-10 20:30:21 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2025-01-10 20:47:06 +0000 bsnmpwalk: Fix crash on invalid data PR: 258570 Reported by: Robert Morris <rtm@lcs.mit.edu> Reviewed by: emaste, markj Differential Revision: https://reviews.freebsd.org/D48422 usr.sbin/bsnmpd/tools/bsnmptools/bsnmpget.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=7fbbab2d326bcbfa1592c6104b745cd5973f5aaa commit 7fbbab2d326bcbfa1592c6104b745cd5973f5aaa Author: Shteryana Shopova <syrinx@FreeBSD.org> AuthorDate: 2025-01-10 20:30:21 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2025-01-12 19:31:44 +0000 bsnmpwalk: Fix crash on invalid data PR: 258570 Reported by: Robert Morris <rtm@lcs.mit.edu> Reviewed by: emaste, markj Differential Revision: https://reviews.freebsd.org/D48422 (cherry picked from commit f021e3573519ff192fc708cda9ca4bba264c96f7) usr.sbin/bsnmpd/tools/bsnmptools/bsnmpget.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=748e7bc1521c87d292e8762c59e415ac687f69a2 commit 748e7bc1521c87d292e8762c59e415ac687f69a2 Author: Shteryana Shopova <syrinx@FreeBSD.org> AuthorDate: 2025-01-10 20:30:21 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2025-01-12 19:32:38 +0000 bsnmpwalk: Fix crash on invalid data PR: 258570 Reported by: Robert Morris <rtm@lcs.mit.edu> Reviewed by: emaste, markj Differential Revision: https://reviews.freebsd.org/D48422 (cherry picked from commit f021e3573519ff192fc708cda9ca4bba264c96f7) (cherry picked from commit 7fbbab2d326bcbfa1592c6104b745cd5973f5aaa) usr.sbin/bsnmpd/tools/bsnmptools/bsnmpget.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)