Bug 258570 - bsnmpwalk can crash due to bug in snmp_parse_resp()
Summary: bsnmpwalk can crash due to bug in snmp_parse_resp()
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 13.0-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Shteryana Shopova
Depends on:
Reported: 2021-09-18 10:58 UTC by Robert Morris
Modified: 2022-09-23 17:06 UTC (History)
2 users (show)

See Also:

Fake snmp server to demonstrate bsnmpwalk crash. (1.50 KB, text/plain)
2021-09-18 10:58 UTC, Robert Morris
no flags Details
Proposed fix for the bsnmpwalk crash (946 bytes, patch)
2022-09-16 13:05 UTC, Shteryana Shopova
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2021-09-18 10:58:00 UTC
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:

                &(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)
Comment 1 Shteryana Shopova freebsd_committer 2022-09-16 13:05:21 UTC
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
Comment 2 Ed Maste freebsd_committer 2022-09-23 14:57:08 UTC
I am curious why SNMP_ERR_NOSUCHNAME should not just return -1.
Comment 3 Shteryana Shopova freebsd_committer 2022-09-23 15:03:16 UTC
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)
Comment 4 Ed Maste freebsd_committer 2022-09-23 15:20:46 UTC
(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?
Comment 5 Shteryana Shopova freebsd_committer 2022-09-23 17:06:04 UTC
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 ...