Address 8 panics, add tests and fuzzing #443

TomSellers · 2023-08-28T19:06:18Z

This PR addresses at least 8 panics, adds tests for those cases, and introduces a fuzzer in order to enable finding additional issues when passing data to SnmpDecodePacket. I will add commentary about each panic in context. I can add reproducers and traces if desired though the best way to reproduce them is to copy the updated marshal_test.go file into a fresh checkout of the master branch and run the tests. This should reproduce all panics.

NOTE: As I'm unfamiliar with the logic here most of the changes are local and defensive in nature. There may be more logical places to add guardrails and/or similar code that might need these changes.

Testing:
go test -v -tags all

Fuzzing
go test -v -tags marshal -fuzz '^FuzzUnmarshal$'

TomSellers · 2023-08-28T19:11:40Z

helper.go

@@ -509,6 +510,9 @@ func parseOpaque(logger Logger, data []byte, retVal *variable) error {
 			if length > len(data) {
 				return fmt.Errorf("not enough data for OpaqueFloat %x (data %d length %d)", data, len(data), length)
 			}
+			if cursor > length {
+				return fmt.Errorf("invalid cursor position for OpaqueFloat %x (data %d length %d cursor %d)", data, len(data), length, cursor)
+			}


If the cursor is greater than length then a panic would be observed on line 513 (517) below.

TomSellers · 2023-08-28T19:14:51Z

helper.go

@@ -616,6 +620,10 @@ func parseLength(bytes []byte) (int, int, error) {
 				return 0, 0, ErrInvalidPacketLength
 			}
 			length += int(bytes[2+i])
+			if length < 0 {
+				// Invalid length due to overflow, return an error


An integer overflow here would result in length being a negative value. In those cases the returned value of length could be less than the value of cursor and result in a panic in the calling code when using these values to slice data.

panic: runtime error: slice bounds out of range [109:108] goroutine 1 [running]: github.com/gosnmp/gosnmp.parseRawField({{0x0, 0x0}}, {0x140000c001a, 0xae, 0xae}, {0x104ea7f86, 0xb}) /Users/username/git/gosnmp/helper.go:724 +0xa34 github.com/gosnmp/gosnmp.(*GoSNMP).unmarshalResponse(0x14000054de0, {0x140000c000e, 0xba, 0xba}, 0x140000c2000) /Users/username/git/gosnmp/marshal.go:1167 +0x648 github.com/gosnmp/gosnmp.(*GoSNMP).unmarshalPayload(0x14000054de0, {0x140000c0000, 0xc8, 0xc8}, 0xe, 0x140000c2000) /Users/username/git/gosnmp/marshal.go:1078 +0x164 github.com/gosnmp/gosnmp.(*GoSNMP).SnmpDecodePacket(0x14000054de0, {0x140000c0000, 0xc8, 0xc8}) /Users/username/git/gosnmp/gosnmp.go:554 +0x174 main.main() /Users/username/git/gosnmp/examples/example-r0/main.go:12 +0x84 exit status 2

Note that this was also brought up in PR #403. I just added some commentary there. Adding this guard on parseLength probably avoids panics in various callers of parseLength.

As a note, this is tested by panicUnmarshalParseRawFieldTimeTicks.

TomSellers · 2023-08-28T19:17:26Z

helper.go

@@ -669,6 +677,9 @@ func parseRawField(logger Logger, data []byte, msg string) (interface{}, int, er
 		if length > len(data) {
 			return nil, 0, fmt.Errorf("not enough data for Integer (%d vs %d): %x", length, len(data), data)
 		}
+		if cursor > length {


Here and similar code that follows: Avoid a panic due to referencing data[cursor:length] when cursor is a higher value than length.

TomSellers · 2023-08-28T19:20:14Z

marshal_test.go

@@ -758,6 +782,27 @@ func TestUnmarshalErrors(t *testing.T) {
 	}
 }

+func FuzzUnmarshal(f *testing.F) {


You can execute this using go test -v -tags marshal -fuzz '^FuzzUnmarshal$'. It will not be run when performing normal tests so it should have no negative impact on CI. If you chose to run this during CI I would recommend adding a timeout to limit how long it runs.

go test -timeout 30s -v -tags marshal -fuzz '^FuzzUnmarshal$'

Yea, we should run this in CI.

When you did your fuzzing, how long did it typically take to reproduce issues?

Most of these were discovered in less than 2 minutes though my original test corpus in my code included more valid packets. You will see better results with more tests because the fuzzer engine will have more to work with.

Actually, since FuzzUnmarshal includes the test data from testsUnmarshal this is likely good enough since testsUnmarshal contains many valid SNMP messages.

SuperQ · 2023-08-28T19:23:29Z

Very nice, I'll do a longer review soon

This needs a DCO sign-off. You can use git commit -s --amend to add it.

TomSellers · 2023-08-28T19:24:15Z

v3.go

@@ -391,7 +391,7 @@ func (x *GoSNMP) unmarshalV3Header(packet []byte,
 		return 0, errors.New("error parsing SNMPV3 message ID: truncted packet")
 	}

-	if MsgFlags, ok := rawMsgFlags.(string); ok {
+	if MsgFlags, ok := rawMsgFlags.(string); ok && len(MsgFlags) > 0 {


Avoid panic: runtime error: index out of range [0] with length 0 in the following line when MsgFlags is empty.

Tested by panicUnmarshalV3HeaderFlagLen

TomSellers · 2023-08-28T19:27:02Z

v3.go

@@ -401,7 +401,7 @@ func (x *GoSNMP) unmarshalV3Header(packet []byte,
 		return 0, fmt.Errorf("error parsing SNMPV3 msgSecModel: %w", err)
 	}
 	cursor += count
-	if cursor > len(packet) {
+	if cursor >= len(packet) {


Avoid a panic on line 416 (parseLength(packet[cursor:])) when cursor == len(packet)

Similar changes were made below.

TomSellers · 2023-08-28T19:31:20Z

v3_usm.go

@@ -840,7 +841,7 @@ func (sp *UsmSecurityParameters) decryptPacket(packet []byte, cursor int) ([]byt
 		stream.XORKeyStream(plaintext, packet[cursorTmp:])
 		copy(packet[cursor:], plaintext)
 		packet = packet[:cursor+len(plaintext)]
-	default:
+	case DES:


Avoid a panic on line 847 (848) caused by trying to decrypt a packet where sp.PrivacyProtocol was set to NoPriv and so sp.PrivacyKey is empty.

panic: runtime error: slice bounds out of range [8:0] goroutine 1 [running]: github.com/gosnmp/gosnmp.(*UsmSecurityParameters).decryptPacket(0x140001088f0, {0x1400007c0c0, 0x54, 0x54}, 0x2a) /Users/username/git/gosnmp/v3_usm.go:847 +0x490 github.com/gosnmp/gosnmp.(*GoSNMP).decryptPacket(0x14000059de0, {0x1400007c0c0?, 0x54?, 0x54?}, 0x2a, 0x14000114000) /Users/username/git/gosnmp/v3.go:448 +0xa8 github.com/gosnmp/gosnmp.(*GoSNMP).SnmpDecodePacket(0x14000059de0, {0x1400007c0c0, 0x54, 0x54}) /Users/username/git/gosnmp/gosnmp.go:548 +0x154 main.main() /Users/username/git/gosnmp/examples/example-r0/main.go:12 +0x84 exit status 2

Tested by panicUnmarshalDecryptNoPriv

TomSellers · 2023-08-28T19:33:48Z

v3_usm.go

@@ -786,7 +787,7 @@ func (sp *UsmSecurityParameters) encryptPacket(scopedPdu []byte) ([]byte, error)
 		}
 		b = append([]byte{byte(OctetString)}, pduLen...)
 		scopedPdu = append(b, ciphertext...) //nolint:gocritic
-	default:
+	case DES:


Found while addressing the change below: the default case includes when sp.PrivacyProtocol is NoPriv. If I understand correctly, DES encryption should NOT be applied in this case and the packet should be returned. This and the change below likely need additional scrutiny since I'm not familiar with how crypto is applied here.

TomSellers · 2023-08-28T19:39:30Z

v3.go

@@ -438,7 +438,7 @@ func (x *GoSNMP) decryptPacket(packet []byte, cursor int, response *SnmpPacket)
 	var err error
 	var decrypted = false

-	if cursor > len(packet) {
+	if cursor >= len(packet) {


Avoid panic on line 445 when cursor == len(packet)

panic: runtime error: index out of range [84] with length 84 goroutine 1 [running]: github.com/gosnmp/gosnmp.(*GoSNMP).decryptPacket(0x14000059de0?, {0x1400007c0c0?, 0x54?, 0x54?}, 0x14000114000?, 0x102e4edec?) /Users/username/git/gosnmp/v3.go:445 +0x6a4 github.com/gosnmp/gosnmp.(*GoSNMP).SnmpDecodePacket(0x14000059de0, {0x1400007c0c0, 0x54, 0x54}) /Users/username/git/gosnmp/gosnmp.go:548 +0x154 main.main() /Users/username/git/gosnmp/examples/example-r0/main.go:19 +0xe4 exit status 2

Tested by panicUnmarshalDecryptPacketIndex

Signed-off-by: Tom Sellers <tom.sellers@runzero.com>

SuperQ

These mostly look sane to me. The only one I'm not completely sure about is the default to case DES one. Should we add a new default case to catch incorrect fallthrough?

TomSellers · 2023-08-29T13:11:11Z

The only one I'm not completely sure about is the default to case DES one. Should we add a new default case to catch incorrect fallthrough?

I'm not sure. Based on my limited understanding you'd want case NoPriv to just return the unmodified packet and a nil error but I could be way off base there.

TimRots

These mostly look sane to me.

+1, looks sane to me as well.

The only one I'm not completely sure about is the default to case DES one. Should we add a new default case to catch incorrect fallthrough?

Took an extra look at both these functions to ensure we don't regress and think we should be good as-is. We could be more explicit by adding both a case for NoPriv with a comment mentioning the no-op, and a fall-trough case to guard for cases we didnt take into account. I wouldn't mind adding this later tho.

TomSellers · 2023-08-30T12:52:03Z

@SuperQ - Do you think you'd release a new version when this lands or would you wait for more changes?

SuperQ · 2023-08-30T12:58:25Z

Yes, should be fine with a new release right after. We cut a minor release recently, so I think this is the only pending change I think.

TomSellers · 2023-08-31T15:36:32Z

Thanks! Do you have a sense for when this might happen?

* [BUGFIX] address panics, add tests, fuzzing #443 Signed-off-by: SuperQ <superq@gmail.com>

TomSellers commented Aug 28, 2023

View reviewed changes

address panics, add tests, fuzzing

245a809

Signed-off-by: Tom Sellers <tom.sellers@runzero.com>

TomSellers force-pushed the address_panics_from_fuzzing branch from 697d469 to 245a809 Compare August 28, 2023 19:46

This was referenced Aug 29, 2023

Fix potential panic due to slice out of index issue, it may happen if… #403

Closed

V3 Index out of range #250

Open

SuperQ approved these changes Aug 29, 2023

View reviewed changes

SuperQ requested a review from TimRots August 29, 2023 13:06

TimRots approved these changes Aug 29, 2023

View reviewed changes

SuperQ merged commit 8f14baf into gosnmp:master Sep 5, 2023
9 checks passed

SuperQ added a commit that referenced this pull request Sep 5, 2023

Release v1.36.1

661f357

* [BUGFIX] address panics, add tests, fuzzing #443 Signed-off-by: SuperQ <superq@gmail.com>

SuperQ mentioned this pull request Sep 5, 2023

Release v1.36.1 #445

Merged

TomSellers deleted the address_panics_from_fuzzing branch September 5, 2023 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address 8 panics, add tests and fuzzing #443

Address 8 panics, add tests and fuzzing #443

TomSellers commented Aug 28, 2023 •

edited

TomSellers Aug 28, 2023

TomSellers Aug 28, 2023

TomSellers Aug 29, 2023

TomSellers Aug 28, 2023

TomSellers Aug 28, 2023

SuperQ Aug 29, 2023

TomSellers Aug 29, 2023

TomSellers Aug 29, 2023 •

edited

SuperQ commented Aug 28, 2023

TomSellers Aug 28, 2023

TomSellers Aug 28, 2023 •

edited

TomSellers Aug 28, 2023 •

edited

TomSellers Aug 28, 2023

TomSellers Aug 28, 2023

SuperQ left a comment

TomSellers commented Aug 29, 2023

TimRots left a comment

TomSellers commented Aug 30, 2023

SuperQ commented Aug 30, 2023

TomSellers commented Aug 31, 2023

Address 8 panics, add tests and fuzzing #443

Address 8 panics, add tests and fuzzing #443

Conversation

TomSellers commented Aug 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSellers Aug 29, 2023 • edited

Choose a reason for hiding this comment

SuperQ commented Aug 28, 2023

Choose a reason for hiding this comment

TomSellers Aug 28, 2023 • edited

Choose a reason for hiding this comment

TomSellers Aug 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperQ left a comment

Choose a reason for hiding this comment

TomSellers commented Aug 29, 2023

TimRots left a comment

Choose a reason for hiding this comment

TomSellers commented Aug 30, 2023

SuperQ commented Aug 30, 2023

TomSellers commented Aug 31, 2023

TomSellers commented Aug 28, 2023 •

edited

TomSellers Aug 29, 2023 •

edited

TomSellers Aug 28, 2023 •

edited

TomSellers Aug 28, 2023 •

edited