Bad SNMP client can cause infinite loop/OOM problems

[dgjustice]

I will do my best to replicate this as soon as I can, but the device was fixed before I could get a proper packet capture. We had a Juniper router go bonkers and SNMP was returning a repeating series of incrementing OID's (e.g. [578.1, 578.2, 578.3, 578.1, 578.2, ...]). This check is not sufficient to guarantee loop termination: https://github.com/gosnmp/gosnmp/blob/master/walk.go#L147. My workaround was to use bulkwalk with a custom walkfn. snmpbulkwalk was able to catch it, but the gosnmp client can be put into an infinite loop in this corner case. To be clear, AppOpts is nil in my client code.

[TimRots]

Hi @dgjustice. Thank you for taking the time to reach out about this issue, it's very much appreciated.

I would like to help to have the mentioned corner case fixed, but will need some additional pointers on how to trigger/replicate this behavior without the help from an actual pcap.

You mentioned snmpbulkwalk was able to catch it . Was snmpbulkwalk throwing something along the lines of Error: OID not increasing: xxx before returning with a non-zero exit status?

Any additional info that comes to mind would be very much welcomed.

[dgjustice]

Tim, thank you for being understanding given my absolute lack of supporting data. I was able to dig up the output of one of the bulkwalks we ran. It behaves as you indicated. I will try to create a test case to replicate this.

snmpbulkwalk -v3  -cr5 somehost .1.3.6.1.4.1.2636.3.60.1.2.1.1.6
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.577.0 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.0 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.1 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.2 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.3 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.32 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.33 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.34 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.35 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.64 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.65 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.66 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.67 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.96 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.97 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.98 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.99 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.128 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.129 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.130 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.131 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.160 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.161 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.162 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.163 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.192 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.193 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.194 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.195 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.224 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.225 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.226 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227 = INTEGER: 0
iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.0 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.0

iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.1 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.1

iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.2 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.2

iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.3 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.3

iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.32 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.32

iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.33 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.33

iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.34 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.34

iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.35 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.35

iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.64 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.64

iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.65 = INTEGER: 0
Error: OID not increasing: iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.227
 >= iso.3.6.1.4.1.2636.3.60.1.2.1.1.6.578.65

[TimRots]

Thank you Daniel for sharing this additional information. That helped a lot to better understand the issue at hand.
I will try my best to have it resolved soon.

[dgjustice]

The way I worked around it was to create a map of PDU's and check if a new value exists in the map. I am not terribly familiar with the low-level expectations of the protocol nor the library, so any ideas are greatly appreciated. Do you think the library should return an array of value and an error, or just an error? Ultimately, if someone wants particular behavior, they can implement their own walk function. I am also not sure how this should play with the max repetitions flag.

[dgjustice]

Oh boy, I got to go protocol diving today. I was able to replicate the issue once I figured out how to craft the packets correctly.
You can run this test with go test github.com/gosnmp/gosnmp -run TestWalk. It should send your shell into oblivion. 😝 This was a quick-and-dirty effort. I am happy to receive feedback on how to clean it up and integrate it into the overall suite of tests if you feel it is appropriate. I don't mind spending some time on a solution, but I wanted to nail down the problem before throwing spaghetti at the wall. 😄

[dgjustice]

Any updates on this? The simplest approach might be simply mentioning the edge case in the docs and pointing users to implement their own walk function.

[TimRots]

Hi @dgjustice , sorry for my slow reply. Have been busy in private life.
I have a feature branch locally that fixed the issue, will try to work on it over the coming week and ask for your review.

[dgjustice]

Awesome, no worries! I was mostly curious if my data was enough to validate the issue. Thanks for the quick follow-up!