Skip to content

Commit

Permalink
Merge pull request #443 from TomSellers/address_panics_from_fuzzing
Browse files Browse the repository at this point in the history 
Address 8 panics, add tests and fuzzing
  • Loading branch information
@SuperQ
SuperQ committed Sep 5, 2023
2 parents 2dfb865 + 245a809 commit 8f14baf
Show file tree
Hide file tree
Showing 4 changed files with 217 additions and 5 deletions.
28 changes: 28 additions & 0 deletions helper.go
View file
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)
}
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
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 {
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
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) {
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
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 {
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) {
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) {
return nil, 0, errors.New("error parsing SNMPV3: truncated packet")
}

Expand Down
9 changes: 7 additions & 2 deletions v3_usm.go
View file
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:
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:
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

0 comments on commit 8f14baf

Please sign in to comment.