Detect truncated packets when handling extended option lengths

dubek · dubek · commit 979f9a1787fc · 2015-10-12T16:21:17.000-04:00
When parsing extending option lengths, verify that enough bytes are left in the packet. This fixes a panic that occured when a specially crafted truncated packet was processed. The TestInvalidMessageParsing was refactored to allow easy additions of such cases to the test suite. The bug was found with the help of go-fuzz (https://github.com/dvyukov/go-fuzz).
diff --git a/message.go b/message.go
@@ -580,16 +580,22 @@ func (m *Message) UnmarshalBinary(data []byte) error {
 	b := data[4+tokenLen:]
 	prev := 0
 
-	parseExtOpt := func(opt int) int {
+	parseExtOpt := func(opt int) (int, error) {
 		switch opt {
 		case extoptByteCode:
+			if len(b) < 1 {
+				return -1, errors.New("truncated")
+			}
 			opt = int(b[0]) + extoptByteAddend
 			b = b[1:]
 		case extoptWordCode:
+			if len(b) < 2 {
+				return -1, errors.New("truncated")
+			}
 			opt = int(binary.BigEndian.Uint16(b[:2])) + extoptWordAddend
 			b = b[2:]
 		}
-		return opt
+		return opt, nil
 	}
 
 	for len(b) > 0 {
@@ -607,8 +613,14 @@ func (m *Message) UnmarshalBinary(data []byte) error {
 
 		b = b[1:]
 
-		delta = parseExtOpt(delta)
-		length = parseExtOpt(length)
+		delta, err := parseExtOpt(delta)
+		if err != nil {
+			return err
+		}
+		length, err = parseExtOpt(length)
+		if err != nil {
+			return err
+		}
 
 		if len(b) < length {
 			return errors.New("truncated")
diff --git a/message_test.go b/message_test.go
@@ -190,25 +190,23 @@ func TestEncodeMessageSmallWithPayload(t *testing.T) {
 }
 
 func TestInvalidMessageParsing(t *testing.T) {
-	msg, err := parseMessage(nil)
-	if err == nil {
-		t.Errorf("Unexpected success parsing short message: %v", msg)
-	}
-
-	msg, err = parseMessage([]byte{0xff, 0, 0, 0, 0, 0})
-	if err == nil {
-		t.Errorf("Unexpected success parsing invalid message: %v", msg)
-	}
-
-	msg, err = parseMessage([]byte{0x4f, 0, 0, 0, 0, 0})
-	if err == nil {
-		t.Errorf("Unexpected success parsing invalid message: %v", msg)
-	}
-
-	// TKL=5 but packet is truncated
-	msg, err = parseMessage([]byte{0x45, 0, 0, 0, 0, 0})
-	if err == nil {
-		t.Errorf("Unexpected success parsing invalid message: %v", msg)
+	var invalidPackets = [][]byte{
+		nil,
+		{0x40},
+		{0x40, 0},
+		{0x40, 0, 0},
+		{0xff, 0, 0, 0, 0, 0},
+		{0x4f, 0, 0, 0, 0, 0},
+		{0x45, 0, 0, 0, 0, 0},                // TKL=5 but packet is truncated
+		{0x40, 0x01, 0x30, 0x39, 0x4d},       // Extended word length but no extra length byte
+		{0x40, 0x01, 0x30, 0x39, 0x4e, 0x01}, // Extended word length but no full extra length word
+	}
+
+	for _, data := range invalidPackets {
+		msg, err := parseMessage(data)
+		if err == nil {
+			t.Errorf("Unexpected success parsing short message (%#v): %v", data, msg)
+		}
 	}
 }
 
