Repository: fxamacker/cbor (go)

Issue:
bug: Unmarshal and Valid should check following extraneous data
### What version of fxamacker/cbor are you using?
v2.4.0

### Does this issue reproduce with the latest release?
Yes

### What OS and CPU architecture are you using (`go env`)?
MacOS, m1
<details><summary><code>go env</code> Output</summary><br><pre>
$ go env

</pre></details>

### What did you do?

```go
data := []byte{0, 1, 2, 3}
err := cbor.Valid(data) // should return a error

var val interface{}
err = cbor.Unmarshal(data, &val) // should return a error
```

<!--
If possible, provide steps and/or code to reproduce the problem. 
-->



### What did you expect to see?



### What did you see instead?


Pull request:
Fix handling of extra data in Unmarshal() & Valid()
Previously, Unmarshal() and Valid() ignored extra data (if any) after the current CBOR data item. This matched the behavior of Decoder.Decode().

However, Unmarshal() and Valid() are typically used for single CBOR data item, so extra data (if present) should return ExtraneousDataError.

This is a bug fix because the previous behavior of ignoring extra data didn't exactly match RFC 8949 regarding this and was also different from how encoding/json handles this.

The behavior for Decoder.Decode() is unchanged because extra data after reading a single CBOR data item is expected for Decoder with io.Reader.

Special thanks to @zensh for reporting this issue and proposing a solution.

Closes #359
 

Patch:
diff --git a/decode.go b/decode.go
index 0db569a..bc0e689 100644
--- a/decode.go
+++ b/decode.go
@@ -549,13 +549,13 @@ func (dm *decMode) DecOptions() DecOptions {
 // See the documentation for Unmarshal for details.
 func (dm *decMode) Unmarshal(data []byte, v interface{}) error {
 	d := decoder{data: data, dm: dm}
-	return d.value(v)
+	return d.value(v, false)
 }
 
 // Valid checks whether the CBOR data is complete and well-formed.
 func (dm *decMode) Valid(data []byte) error {
 	d := decoder{data: data, dm: dm}
-	return d.valid()
+	return d.valid(false)
 }
 
 // NewDecoder returns a new decoder that reads from r using dm DecMode.
@@ -573,7 +573,7 @@ type decoder struct {
 // If CBOR data item is invalid, error is returned and offset isn't changed.
 // If CBOR data item is valid but fails to be decode into v for other reasons,
 // error is returned and offset is moved to the next CBOR data item.
-func (d *decoder) value(v interface{}) error {
+func (d *decoder) value(v interface{}, allowExtraData bool) error {
 	// v can't be nil, non-pointer, or nil pointer value.
 	if v == nil {
 		return &InvalidUnmarshalError{"cbor: Unmarshal(nil)"}
@@ -586,7 +586,7 @@ func (d *decoder) value(v interface{}) error {
 	}
 
 	off := d.off // Save offset before data validation
-	err := d.valid()
+	err := d.valid(allowExtraData)
 	d.off = off // Restore offset
 	if err != nil {
 		return err
diff --git a/stream.go b/stream.go
index dd5a377..9337940 100644
--- a/stream.go
+++ b/stream.go
@@ -34,7 +34,7 @@ func (dec *Decoder) Decode(v interface{}) error {
 	}
 	for {
 		dec.d.reset(dec.buf[dec.off:])
-		err := dec.d.value(v)
+		err := dec.d.value(v, true)
 		// Increment dec.off even if err is not nil because
 		// dec.d.off points to the next CBOR data item if current
 		// CBOR data item is valid but failed to be decoded into v.
diff --git a/valid.go b/valid.go
index f775afb..9bf5fdb 100644
--- a/valid.go
+++ b/valid.go
@@ -68,12 +68,30 @@ func (e *TagsMdError) Error() string {
 	return "cbor: CBOR tag isn't allowed"
 }
 
+// ExtraneousDataError indicates found extraneous data following valid CBOR message.
+type ExtraneousDataError struct {
+	numOfBytes int // number of bytes of extraneous data
+	index      int // location of extraneous data
+}
+
+func (e *ExtraneousDataError) Error() string {
+	return "cbor: " + strconv.Itoa(e.numOfBytes) + " bytes of extraneous data starting at index " + strconv.Itoa(e.index)
+}
+
 // valid checks whether the CBOR data is complete and well-formed.
-func (d *decoder) valid() error {
+// allowExtraData indicates if extraneous data is allowed after the CBOR data item.
+// - use allowExtraData = true when using Decoder.Decode()
+// - use allowExtraData = false when using Unmarshal()
+func (d *decoder) valid(allowExtraData bool) error {
 	if len(d.data) == d.off {
 		return io.EOF
 	}
 	_, err := d.validInternal(0)
+	if err == nil {
+		if !allowExtraData && d.off != len(d.data) {
+			err = &ExtraneousDataError{len(d.data) - d.off, d.off}
+		}
+	}
 	return err
 }
 


Test patch:
diff --git a/decode_test.go b/decode_test.go
index f0dcc9e..0e585d3 100644
--- a/decode_test.go
+++ b/decode_test.go
@@ -1369,6 +1369,10 @@ var invalidCBORUnmarshalTests = []struct {
 	{"Major type 0 with additional information 31", hexDecode("1f"), "cbor: invalid additional information 31 for type positive integer", true},
 	{"Major type 1 with additional information 31", hexDecode("3f"), "cbor: invalid additional information 31 for type negative integer", true},
 	{"Major type 6 with additional information 31", hexDecode("df"), "cbor: invalid additional information 31 for type tag", true},
+	// Extraneous data
+	{"two ints", hexDecode("0001"), "cbor: 1 bytes of extraneous data starting at index 1", false},
+	{"two arrays", hexDecode("830102038104"), "cbor: 2 bytes of extraneous data starting at index 4", false},
+	{"int and partial array", hexDecode("00830102"), "cbor: 3 bytes of extraneous data starting at index 1", false},
 }
 
 func TestInvalidCBORUnmarshal(t *testing.T) {
diff --git a/stream_test.go b/stream_test.go
index 7165039..6dfe339 100644
--- a/stream_test.go
+++ b/stream_test.go
@@ -192,6 +192,25 @@ func TestDecoderReadError(t *testing.T) {
 	}
 }
 
+func TestDecoderInvalidData(t *testing.T) {
+	data := []byte{0x01, 0x83, 0x01, 0x02}
+	decoder := NewDecoder(bytes.NewReader(data))
+
+	var v1 interface{}
+	err := decoder.Decode(&v1)
+	if err != nil {
+		t.Errorf("Decode() returned error %v when decoding valid data item", err)
+	}
+
+	var v2 interface{}
+	err = decoder.Decode(&v2)
+	if err == nil {
+		t.Errorf("Decode() didn't return error when decoding invalid data item")
+	} else if err != io.ErrUnexpectedEOF {
+		t.Errorf("Decode() error %q, want %q", err, io.ErrUnexpectedEOF)
+	}
+}
+
 func TestDecoderStructTag(t *testing.T) {
 	type strc struct {
 		A string `json:"x" cbor:"a"`
diff --git a/valid_test.go b/valid_test.go
index 109ca2c..74fddc0 100644
--- a/valid_test.go
+++ b/valid_test.go
@@ -25,6 +25,37 @@ func TestValid2(t *testing.T) {
 	}
 }
 
+func TestValidExtraneousData(t *testing.T) {
+	testCases := []struct {
+		name                     string
+		cborData                 []byte
+		extraneousDataNumOfBytes int
+		extraneousDataIndex      int
+	}{
+		{"two numbers", []byte{0x00, 0x01}, 1, 1},                                // 0, 1
+		{"bytestring and int", []byte{0x44, 0x01, 0x02, 0x03, 0x04, 0x00}, 1, 5}, // h'01020304', 0
+		{"int and partial array", []byte{0x00, 0x83, 0x01, 0x02}, 3, 1},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := Valid(tc.cborData)
+			if err == nil {
+				t.Errorf("Valid(0x%x) didn't return an error", tc.cborData)
+			} else {
+				ederr, ok := err.(*ExtraneousDataError)
+				if !ok {
+					t.Errorf("Valid(0x%x) error type %T, want *ExtraneousDataError", tc.cborData, err)
+				} else if ederr.numOfBytes != tc.extraneousDataNumOfBytes {
+					t.Errorf("Valid(0x%x) returned %d bytes of extraneous data, want %d", tc.cborData, ederr.numOfBytes, tc.extraneousDataNumOfBytes)
+				} else if ederr.index != tc.extraneousDataIndex {
+					t.Errorf("Valid(0x%x) returned extraneous data index %d, want %d", tc.cborData, ederr.index, tc.extraneousDataIndex)
+				}
+			}
+		})
+	}
+}
+
 func TestValidOnStreamingData(t *testing.T) {
 	var buf bytes.Buffer
 	for _, t := range marshalTests {
@@ -32,7 +63,7 @@ func TestValidOnStreamingData(t *testing.T) {
 	}
 	d := decoder{data: buf.Bytes(), dm: defaultDecMode}
 	for i := 0; i < len(marshalTests); i++ {
-		if err := d.valid(); err != nil {
+		if err := d.valid(true); err != nil {
 			t.Errorf("valid() returned error %v", err)
 		}
 	}