Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Address 8 panics, add tests and fuzzing #443

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Address 8 panics, add tests and fuzzing #443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 28 additions & 0 deletions helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type variable struct {
var (
ErrBase128IntegerTooLarge = errors.New("base 128 integer too large")
ErrBase128IntegerTruncated = errors.New("base 128 integer truncated")
ErrFloatBufferTooShort = errors.New("float buffer too short")
ErrFloatTooLarge = errors.New("float too large")
ErrIntegerTooLarge = errors.New("integer too large")
ErrInvalidOidLength = errors.New("invalid OID length")
Expand Down Expand Up @@ -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)
}
Copy link
Contributor Author

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.

retVal.Type = OpaqueFloat
retVal.Value, err = parseFloat32(data[cursor:length])
if err != nil {
Expand Down Expand Up @@ -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
Copy link
Contributor Author

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

Copy link
Contributor Author

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.

return 0, 0, ErrInvalidPacketLength
}
}
length += 2 + numOctets
cursor += 2 + numOctets
Expand Down Expand Up @@ -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 {
Copy link
Contributor Author

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.

return nil, 0, fmt.Errorf("invalid cursor position for Integer %x (data %d length %d cursor %d)", data, len(data), length, cursor)
}
i, err := parseInt(data[cursor:length])
if err != nil {
return nil, 0, fmt.Errorf("unable to parse raw INTEGER: %x err: %w", data, err)
Expand All @@ -682,6 +693,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 OctetString (%d vs %d): %x", length, len(data), data)
}
if cursor > length {
return nil, 0, fmt.Errorf("invalid cursor position for OctetString %x (data %d length %d cursor %d)", data, len(data), length, cursor)
}
return string(data[cursor:length]), length, nil
case ObjectIdentifier:
length, cursor, err := parseLength(data)
Expand All @@ -691,6 +705,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 OID (%d vs %d): %x", length, len(data), data)
}
if cursor > length {
return nil, 0, fmt.Errorf("invalid cursor position for OID %x (data %d length %d cursor %d)", data, len(data), length, cursor)
}
oid, err := parseObjectIdentifier(data[cursor:length])
return oid, length, err
case IPAddress:
Expand Down Expand Up @@ -721,6 +738,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 TimeTicks (%d vs %d): %x", length, len(data), data)
}
if cursor > length {
return nil, 0, fmt.Errorf("invalid cursor position for TimeTicks %x (data %d length %d cursor %d)", data, len(data), length, cursor)
}
ret, err := parseUint(data[cursor:length])
if err != nil {
return nil, 0, fmt.Errorf("error in parseUint: %w", err)
Expand Down Expand Up @@ -774,6 +794,10 @@ func parseFloat32(bytes []byte) (float32, error) {
// We'll overflow a uint64 in this case.
return 0, ErrFloatTooLarge
}
if len(bytes) < 4 {
// We'll cause a panic in binary.BigEndian.Uint32() in this case
return 0, ErrFloatBufferTooShort
}
return math.Float32frombits(binary.BigEndian.Uint32(bytes)), nil
}

Expand All @@ -782,6 +806,10 @@ func parseFloat64(bytes []byte) (float64, error) {
// We'll overflow a uint64 in this case.
return 0, ErrFloatTooLarge
}
if len(bytes) < 8 {
// We'll cause a panic in binary.BigEndian.Uint64() in this case
return 0, ErrFloatBufferTooShort
}
return math.Float64frombits(binary.BigEndian.Uint64(bytes)), nil
}

Expand Down
179 changes: 179 additions & 0 deletions marshal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,30 @@ var testsUnmarshalErr = []struct {
{
panicUnmarshalHeader,
},
{
panicUnmarshalV3Header,
},
{
panicUnmarshalUserSecurityModelPacketLen,
},
{
panicUnmarshalV3HeaderFlagLen,
},
{
panicUnmarshalParseFloat32,
},
{
panicUnmarshalParseFloat64,
},
{
panicUnmarshalParseRawFieldTimeTicks,
},
{
panicUnmarshalDecryptPacketIndex,
},
{
panicUnmarshalDecryptNoPriv,
},
}

var testsUnmarshal = []struct {
Expand Down Expand Up @@ -758,6 +782,27 @@ func TestUnmarshalErrors(t *testing.T) {
}
}

func FuzzUnmarshal(f *testing.F) {
Copy link
Contributor Author

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$'

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

for _, test := range testsUnmarshalErr {
f.Add(test.in())
}

for _, test := range testsUnmarshal {
f.Add(test.in())
}

vhandle := GoSNMP{}
vhandle.Logger = Default.Logger
f.Fuzz(func(t *testing.T, data []byte) {
stime := time.Now()
_, _ = vhandle.SnmpDecodePacket(data)

if e := time.Since(stime); e > (time.Second * 1) {
t.Errorf("SnmpDecodePacket() took too long: %s", e)
}
})
}

func TestUnmarshal(t *testing.T) {
Default.Logger = NewLogger(log.New(io.Discard, "", 0))

Expand Down Expand Up @@ -877,6 +922,140 @@ func panicUnmarshalHeader() []byte {
return []byte("0\x04\x02\x020\x03")
}

/*
panicUnmarshalV3Header tests a boundary condition that results in a panic
when unmarshalling the SNMPv3.
*/
func panicUnmarshalV3Header() []byte {
return []byte{
0x30, 0x81, 0x95, 0x02, 0x01, 0x03, 0x30, 0x30, 0x43, 0x04, 0x30, 0x30, 0x30, 0x30, 0x43, 0x03,
0x30, 0x30, 0x30, 0x04, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x04, 0x51, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
}
}

/*
panicUnmarshalUserSecurityModelPacketLen() tests a boundary condition that results in a panic
when indexing into the packet when processing the User Security Model.
*/
func panicUnmarshalUserSecurityModelPacketLen() []byte {
return []byte{
0x30, 0x81, 0x95, 0x02, 0x01, 0x03, 0x30, 0x30, 0x43, 0x04, 0x30, 0x30, 0x30, 0x30, 0x43, 0x03,
0x30, 0x30, 0x30, 0x43, 0x01, 0x30, 0x43, 0x01, 0x30, 0x04, 0xfd, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
}
}

/*
panicUnmarshalV3HeaderFlagLen tests a boundary condition that results in a panic
when indexing into Flags without checking the length.
*/
func panicUnmarshalV3HeaderFlagLen() []byte {
return []byte{
0x30, 0x7e, 0x02, 0x01, 0x03, 0x30, 0x30, 0x43, 0x04, 0x30, 0x30, 0x30, 0x30, 0x43, 0x03, 0x30,
0x30, 0x30, 0x04, 0x00, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
}
}

/*
panicUnmarshalParseFloat32() tests a boundary condition that results in a panic
in parseFloat32 when handling malformed data.
*/
func panicUnmarshalParseFloat32() []byte {
return []byte{
0x30, 0x34, 0x43, 0x01, 0x30, 0x43, 0x06, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0xa2, 0x27, 0x43,
0x04, 0x30, 0x30, 0x30, 0x30, 0x43, 0x01, 0x30, 0x43, 0x01, 0x30, 0x30, 0x19, 0x30, 0x30, 0x06,
0x0c, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x44, 0x07, 0x9f,
0x78, 0x00, 0x30, 0x30, 0x30, 0x30,
}
}

/*
panicUnmarshalParseFloat64() tests a boundary condition that results in a panic
in parseFloat64 when handling malformed data.
*/
func panicUnmarshalParseFloat64() []byte {
return []byte{
0x30, 0x38, 0x43, 0x01, 0x30, 0x43, 0x06, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0xa2, 0x2b, 0x43,
0x04, 0x30, 0x30, 0x30, 0x30, 0x43, 0x01, 0x30, 0x43, 0x01, 0x30, 0x30, 0x1d, 0x30, 0x30, 0x06,
0x0c, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x44, 0x0b, 0x9f,
0x79, 0x80, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
}
}

/*
panicUnmarshalParseRawFieldTimeTicks() tests a boundary condition that results in a panic
in parseRawField TimeTicks type when parseLength overflows the length value returning a value
for cursor that is higher than length.
*/
func panicUnmarshalParseRawFieldTimeTicks() []byte {
return []byte{
0x30, 0x81, 0xc5, 0x43, 0x01, 0x30, 0x43, 0x06, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0xa2, 0x81,
0xb7, 0x43, 0x04, 0x30, 0x30, 0x30, 0x30, 0x43, 0x01, 0x30, 0x43, 0xeb, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
}
}

/*
panicUnmarshalDecryptPacketIndex() tests a boundary condition that results in a panic
in decryptPacket when handling malformed data.
*/
func panicUnmarshalDecryptPacketIndex() []byte {
return []byte{
0x30, 0x52, 0x02, 0x01, 0x03, 0x30, 0x30, 0x43, 0x04, 0x30, 0x30, 0x30, 0x30, 0x43, 0x03, 0x30,
0x30, 0x30, 0x43, 0x01, 0x30, 0x43, 0x01, 0x30, 0x04, 0x30, 0x30, 0x30, 0x43, 0x00, 0x43, 0x01,
0x30, 0x43, 0x01, 0x30, 0x43, 0x00, 0x43, 0x00, 0x04, 0x2a, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30,
}
}

/*
panicUnmarshalDecryptPacketIndex() tests a boundary condition that results in a panic
in UsmSecurityParameters.decryptPacket() when handling malformed data.
*/
func panicUnmarshalDecryptNoPriv() []byte {
return []byte{
0x30, 0x52, 0x02, 0x01, 0x03, 0x30, 0x30, 0x43, 0x04, 0x30, 0x30, 0x30, 0x30, 0x43, 0x03, 0x30,
0x30, 0x30, 0x43, 0x01, 0x30, 0x43, 0x01, 0x30, 0x04, 0x30, 0x30, 0x30, 0x43, 0x00, 0x43, 0x01,
0x30, 0x43, 0x01, 0x30, 0x43, 0x00, 0x43, 0x00, 0x43, 0x00, 0x04, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
0x30, 0x30, 0x30, 0x30,
}
}

/*
kyoceraResponseBytes corresponds to the response section of this snmpget

Expand Down
6 changes: 3 additions & 3 deletions v3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

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

response.MsgFlags = SnmpV3MsgFlags(MsgFlags[0])
x.Logger.Printf("parsed msg flags %s", MsgFlags)
}
Expand All @@ -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) {
Copy link
Contributor Author

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.

return 0, errors.New("error parsing SNMPV3 message ID: truncted packet")
}

Expand Down Expand Up @@ -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) {
Copy link
Contributor Author

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

return nil, 0, errors.New("error parsing SNMPV3: truncated packet")
}

Expand Down
9 changes: 7 additions & 2 deletions v3_usm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ func (sp *UsmSecurityParameters) Copy() SnmpV3SecurityParameters {
func (sp *UsmSecurityParameters) getDefaultContextEngineID() string {
return sp.AuthoritativeEngineID
}

func (sp *UsmSecurityParameters) initSecurityKeys() error {
sp.mu.Lock()
defer sp.mu.Unlock()
Expand Down Expand Up @@ -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:
Copy link
Contributor Author

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.

preiv := sp.PrivacyKey[8:]
var iv [8]byte
for i := 0; i < len(iv); i++ {
Expand Down Expand Up @@ -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:
Copy link
Contributor Author

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

if len(packet[cursorTmp:])%des.BlockSize != 0 {
return nil, errors.New("error decrypting ScopedPDU: not multiple of des block size")
}
Expand Down Expand Up @@ -927,6 +928,10 @@ func (sp *UsmSecurityParameters) marshal(flags SnmpV3MsgFlags) ([]byte, error) {
func (sp *UsmSecurityParameters) unmarshal(flags SnmpV3MsgFlags, packet []byte, cursor int) (int, error) {
var err error

if cursor >= len(packet) {
return 0, errors.New("error parsing SNMPV3 User Security Model parameters: end of packet")
}

if PDUType(packet[cursor]) != Sequence {
return 0, errors.New("error parsing SNMPV3 User Security Model parameters")
}
Expand Down