New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address 8 panics, add tests and fuzzing #443
Conversation
@@ -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) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the cursor is greater than length then a panic would be observed on line 513 (517) below.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and similar code that follows: Avoid a panic due to referencing data[cursor:length]
when cursor
is a higher value than length
.
@@ -758,6 +782,27 @@ func TestUnmarshalErrors(t *testing.T) { | |||
} | |||
} | |||
|
|||
func FuzzUnmarshal(f *testing.F) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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$'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, we should run this in CI.
When you did your fuzzing, how long did it typically take to reproduce issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, since FuzzUnmarshal
includes the test data from testsUnmarshal
this is likely good enough since testsUnmarshal
contains many valid SNMP messages.
Very nice, I'll do a longer review soon This needs a DCO sign-off. You can use |
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid panic: runtime error: index out of range [0] with length 0
in the following line when MsgFlags
is empty.
Tested by panicUnmarshalV3HeaderFlagLen
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid a panic on line 416 (parseLength(packet[cursor:])
) when cursor
== len(packet)
Similar changes were made below.
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
697d469
to
245a809
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
I'm not sure. Based on my limited understanding you'd want |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
tocase DES
one. Should we add a newdefault
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.
@SuperQ - Do you think you'd release a new version when this lands or would you wait for more changes? |
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. |
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>
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 updatedmarshal_test.go
file into a fresh checkout of themaster
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$'