Repository: fxamacker/cbor
Pull request #323 (2022-03-03 16:56:16)

[stream-mode branch] Add decoding option to allow invalid UTF-8
#### Description

Add DecOptions.UTF8 to specify how to decode invalid UTF-8:
- UTF8RejectInvalid (default) returns SemanticError on invalid UTF-8
- UTF8DecodeInvalid decodes invalid UTF-8

Since CBOR text containing invalid UTF-8 isn't valid, decoder by default rejects it and returns error.  This is backward compatible by default.

See RFC 8949 Section 5.3 (Validity of Items) for more info.

This change passed 200+ million execs fuzz testing the stream-mode branch.

<!-- For code contributions, please complete all the items below this line. -->
<!-- For documentation-only contributions, please delete everything below this line. -->

#### PR Was Proposed and Welcomed in Currently Open Issue
This PR was proposed and welcomed by maintainer(s) in issue #320

Updates #320 

#### Checklist (for code PR only, ignore for docs PR)

- [x] Include unit tests that cover the new code
- [x] Pass all unit tests 
- [x] Pass all 18 ci linters (golint, gosec, staticcheck, etc.)



Patch:
diff --git a/decode.go b/decode.go
index c9792e9..60ae519 100644
--- a/decode.go
+++ b/decode.go
@@ -258,6 +258,26 @@ func (ec ExtraDecErrorCond) valid() bool {
 	return ec < maxExtraDecError
 }
 
+// UTF8Mode option specifies if decoder should
+// decode CBOR Text containing invalid UTF-8 string.
+type UTF8Mode int
+
+const (
+	// UTF8RejectInvalid rejects CBOR Text containing
+	// invalid UTF-8 string.
+	UTF8RejectInvalid UTF8Mode = iota
+
+	// UTF8DecodeInvalid allows decoding CBOR Text containing
+	// invalid UTF-8 string.
+	UTF8DecodeInvalid
+
+	maxUTF8Mode
+)
+
+func (um UTF8Mode) valid() bool {
+	return um < maxUTF8Mode
+}
+
 // DecOptions specifies decoding options.
 type DecOptions struct {
 	// DupMapKey specifies whether to enforce duplicate map key.
@@ -298,6 +318,10 @@ type DecOptions struct {
 	// when unmarshalling CBOR into an empty interface value.
 	// By default, unmarshal uses map[interface{}]interface{}.
 	DefaultMapType reflect.Type
+
+	// UTF8 specifies if decoder should decode CBOR Text containing invalid UTF-8.
+	// By default, unmarshal rejects CBOR text containing invalid UTF-8.
+	UTF8 UTF8Mode
 }
 
 // DecMode returns DecMode with immutable options and no tags (safe for concurrency).
@@ -409,6 +433,9 @@ func (opts DecOptions) decMode() (*decMode, error) {
 	if opts.DefaultMapType != nil && opts.DefaultMapType.Kind() != reflect.Map {
 		return nil, fmt.Errorf("cbor: invalid DefaultMapType %s", opts.DefaultMapType)
 	}
+	if !opts.UTF8.valid() {
+		return nil, errors.New("cbor: invalid UTF8 " + strconv.Itoa(int(opts.UTF8)))
+	}
 	dm := decMode{
 		dupMapKey:         opts.DupMapKey,
 		timeTag:           opts.TimeTag,
@@ -420,6 +447,7 @@ func (opts DecOptions) decMode() (*decMode, error) {
 		intDec:            opts.IntDec,
 		extraReturnErrors: opts.ExtraReturnErrors,
 		defaultMapType:    opts.DefaultMapType,
+		utf8:              opts.UTF8,
 	}
 	return &dm, nil
 }
@@ -459,6 +487,7 @@ type decMode struct {
 	intDec            IntDecMode
 	extraReturnErrors ExtraDecErrorCond
 	defaultMapType    reflect.Type
+	utf8              UTF8Mode
 }
 
 var defaultDecMode, _ = DecOptions{}.decMode()
@@ -475,6 +504,7 @@ func (dm *decMode) DecOptions() DecOptions {
 		TagsMd:            dm.tagsMd,
 		IntDec:            dm.intDec,
 		ExtraReturnErrors: dm.extraReturnErrors,
+		UTF8:              dm.utf8,
 	}
 }
 
@@ -1101,7 +1131,7 @@ func (d *decoder) parseTextString() ([]byte, error) {
 	if ai != 31 {
 		b := d.data[d.off : d.off+int(val)]
 		d.off += int(val)
-		if !utf8.Valid(b) {
+		if d.dm.utf8 == UTF8RejectInvalid && !utf8.Valid(b) {
 			return nil, &SemanticError{"cbor: invalid UTF-8 string"}
 		}
 		return b, nil
@@ -1112,7 +1142,7 @@ func (d *decoder) parseTextString() ([]byte, error) {
 		_, _, val = d.getHead()
 		x := d.data[d.off : d.off+int(val)]
 		d.off += int(val)
-		if !utf8.Valid(x) {
+		if d.dm.utf8 == UTF8RejectInvalid && !utf8.Valid(x) {
 			for !d.foundBreak() {
 				d.skip() // Skip remaining chunk on error
 			}
diff --git a/stream_decode.go b/stream_decode.go
index 38488ad..0929c83 100644
--- a/stream_decode.go
+++ b/stream_decode.go
@@ -423,7 +423,7 @@ func (sd *StreamDecoder) DecodeString() (string, error) {
 
 	sd.updateState(end - start)
 
-	if !utf8.Valid(b) {
+	if d.dm.utf8 == UTF8RejectInvalid && !utf8.Valid(b) {
 		return "", &SemanticError{"cbor: invalid UTF-8 string"}
 	}
 


Test patch:
diff --git a/decode_test.go b/decode_test.go
index 8036def..178068e 100644
--- a/decode_test.go
+++ b/decode_test.go
@@ -1389,33 +1389,157 @@ func TestInvalidCBORUnmarshal(t *testing.T) {
 	}
 }
 
-func TestInvalidUTF8TextString(t *testing.T) {
-	invalidUTF8TextStringTests := []struct {
+func TestValidUTF8String(t *testing.T) {
+	dmRejectInvalidUTF8, err := DecOptions{UTF8: UTF8RejectInvalid}.DecMode()
+	if err != nil {
+		t.Errorf("DecMode() returned an error %+v", err)
+	}
+	dmDecodeInvalidUTF8, err := DecOptions{UTF8: UTF8DecodeInvalid}.DecMode()
+	if err != nil {
+		t.Errorf("DecMode() returned an error %+v", err)
+	}
+
+	testCases := []struct {
+		name     string
+		cborData []byte
+		dm       DecMode
+		wantObj  interface{}
+	}{
+		{
+			name:     "with UTF8RejectInvalid",
+			cborData: hexDecode("6973747265616d696e67"),
+			dm:       dmRejectInvalidUTF8,
+			wantObj:  "streaming",
+		},
+		{
+			name:     "with UTF8DecodeInvalid",
+			cborData: hexDecode("6973747265616d696e67"),
+			dm:       dmDecodeInvalidUTF8,
+			wantObj:  "streaming",
+		},
+		{
+			name:     "indef length with UTF8RejectInvalid",
+			cborData: hexDecode("7f657374726561646d696e67ff"),
+			dm:       dmRejectInvalidUTF8,
+			wantObj:  "streaming",
+		},
+		{
+			name:     "indef length with UTF8DecodeInvalid",
+			cborData: hexDecode("7f657374726561646d696e67ff"),
+			dm:       dmDecodeInvalidUTF8,
+			wantObj:  "streaming",
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// Decode to empty interface
+			var i interface{}
+			err = tc.dm.Unmarshal(tc.cborData, &i)
+			if err != nil {
+				t.Errorf("Unmarshal(0x%x) returned error %q", tc.cborData, err)
+			}
+			if !reflect.DeepEqual(i, tc.wantObj) {
+				t.Errorf("Unmarshal(0x%x) returned %v (%T), want %v (%T)", tc.cborData, i, i, tc.wantObj, tc.wantObj)
+			}
+
+			// Decode to string
+			var v string
+			err = tc.dm.Unmarshal(tc.cborData, &v)
+			if err != nil {
+				t.Errorf("Unmarshal(0x%x) returned error %q", tc.cborData, err)
+			}
+			if !reflect.DeepEqual(v, tc.wantObj) {
+				t.Errorf("Unmarshal(0x%x) returned %v (%T), want %v (%T)", tc.cborData, v, v, tc.wantObj, tc.wantObj)
+			}
+		})
+	}
+}
+
+func TestInvalidUTF8String(t *testing.T) {
+	dmRejectInvalidUTF8, err := DecOptions{UTF8: UTF8RejectInvalid}.DecMode()
+	if err != nil {
+		t.Errorf("DecMode() returned an error %+v", err)
+	}
+	dmDecodeInvalidUTF8, err := DecOptions{UTF8: UTF8DecodeInvalid}.DecMode()
+	if err != nil {
+		t.Errorf("DecMode() returned an error %+v", err)
+	}
+
+	testCases := []struct {
 		name         string
 		cborData     []byte
+		dm           DecMode
+		wantObj      interface{}
 		wantErrorMsg string
 	}{
-		{"definite length text string", hexDecode("61fe"), invalidUTF8ErrorMsg},
-		{"indefinite length text string", hexDecode("7f62e6b061b4ff"), invalidUTF8ErrorMsg},
+		{
+			name:         "with UTF8RejectInvalid",
+			cborData:     hexDecode("61fe"),
+			dm:           dmRejectInvalidUTF8,
+			wantErrorMsg: invalidUTF8ErrorMsg,
+		},
+		{
+			name:     "with UTF8DecodeInvalid",
+			cborData: hexDecode("61fe"),
+			dm:       dmDecodeInvalidUTF8,
+			wantObj:  string([]byte{0xfe}),
+		},
+		{
+			name:         "indef length with UTF8RejectInvalid",
+			cborData:     hexDecode("7f62e6b061b4ff"),
+			dm:           dmRejectInvalidUTF8,
+			wantErrorMsg: invalidUTF8ErrorMsg,
+		},
+		{
+			name:     "indef length with UTF8DecodeInvalid",
+			cborData: hexDecode("7f62e6b061b4ff"),
+			dm:       dmDecodeInvalidUTF8,
+			wantObj:  string([]byte{0xe6, 0xb0, 0xb4}),
+		},
 	}
-	for _, tc := range invalidUTF8TextStringTests {
+
+	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			var i interface{}
-			if err := Unmarshal(tc.cborData, &i); err == nil {
-				t.Errorf("Unmarshal(0x%x) didn't return an error", tc.cborData)
-			} else if err.Error() != tc.wantErrorMsg {
-				t.Errorf("Unmarshal(0x%x) error %q, want %q", tc.cborData, err.Error(), tc.wantErrorMsg)
+			// Decode to empty interface
+			var v interface{}
+			err = tc.dm.Unmarshal(tc.cborData, &v)
+			if tc.wantErrorMsg != "" {
+				if err == nil {
+					t.Errorf("Unmarshal(0x%x) didn't return error", tc.cborData)
+				} else if !strings.Contains(err.Error(), tc.wantErrorMsg) {
+					t.Errorf("Unmarshal(0x%x) error %q, want %q", tc.cborData, err.Error(), tc.wantErrorMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("Unmarshal(0x%x) returned error %q", tc.cborData, err)
+				}
+				if !reflect.DeepEqual(v, tc.wantObj) {
+					t.Errorf("Unmarshal(0x%x) returned %v (%T), want %v (%T)", tc.cborData, v, v, tc.wantObj, tc.wantObj)
+				}
 			}
 
+			// Decode to string
 			var s string
-			if err := Unmarshal(tc.cborData, &s); err == nil {
-				t.Errorf("Unmarshal(0x%x) didn't return an error", tc.cborData)
-			} else if err.Error() != tc.wantErrorMsg {
-				t.Errorf("Unmarshal(0x%x) error %q, want %q", tc.cborData, err.Error(), tc.wantErrorMsg)
+			err = tc.dm.Unmarshal(tc.cborData, &s)
+			if tc.wantErrorMsg != "" {
+				if err == nil {
+					t.Errorf("Unmarshal(0x%x) didn't return error", tc.cborData)
+				} else if !strings.Contains(err.Error(), tc.wantErrorMsg) {
+					t.Errorf("Unmarshal(0x%x) error %q, want %q", tc.cborData, err.Error(), tc.wantErrorMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("Unmarshal(0x%x) returned error %q", tc.cborData, err)
+				}
+				if !reflect.DeepEqual(s, tc.wantObj) {
+					t.Errorf("Unmarshal(0x%x) returned %v (%T), want %v (%T)", tc.cborData, s, s, tc.wantObj, tc.wantObj)
+				}
 			}
 		})
 	}
+
 	// Test decoding of mixed invalid text string and valid text string
+	// with UTF8RejectInvalid option (default)
 	cborData := hexDecode("7f62e6b061b4ff7f657374726561646d696e67ff")
 	dec := NewDecoder(bytes.NewReader(cborData))
 	var s string
@@ -1429,6 +1553,20 @@ func TestInvalidUTF8TextString(t *testing.T) {
 	} else if s != "streaming" {
 		t.Errorf("Decode() returned %q, want %q", s, "streaming")
 	}
+
+	// Test decoding of mixed invalid text string and valid text string
+	// with UTF8DecodeInvalid option
+	dec = dmDecodeInvalidUTF8.NewDecoder(bytes.NewReader(cborData))
+	if err := dec.Decode(&s); err != nil {
+		t.Errorf("Decode() returned error %q", err)
+	} else if s != string([]byte{0xe6, 0xb0, 0xb4}) {
+		t.Errorf("Decode() returned %q, want %q", s, string([]byte{0xe6, 0xb0, 0xb4}))
+	}
+	if err := dec.Decode(&s); err != nil {
+		t.Errorf("Decode() returned error %v", err)
+	} else if s != "streaming" {
+		t.Errorf("Decode() returned %q, want %q", s, "streaming")
+	}
 }
 
 func TestUnmarshalStruct(t *testing.T) {
@@ -3070,15 +3208,16 @@ func TestUnmarshalToNotNilInterface(t *testing.T) {
 
 func TestDecOptions(t *testing.T) {
 	opts1 := DecOptions{
-		TimeTag:           DecTagRequired,
 		DupMapKey:         DupMapKeyEnforcedAPF,
-		IndefLength:       IndefLengthForbidden,
+		TimeTag:           DecTagRequired,
 		MaxNestedLevels:   100,
-		MaxMapPairs:       101,
 		MaxArrayElements:  102,
+		MaxMapPairs:       101,
+		IndefLength:       IndefLengthForbidden,
 		TagsMd:            TagsForbidden,
 		IntDec:            IntDecConvertSigned,
 		ExtraReturnErrors: ExtraDecErrorUnknownField,
+		UTF8:              UTF8DecodeInvalid,
 	}
 	dm, err := opts1.DecMode()
 	if err != nil {
@@ -4585,6 +4724,16 @@ func TestExtraErrorCondUnknowField(t *testing.T) {
 	}
 }
 
+func TestInvalidUTF8Mode(t *testing.T) {
+	wantErrorMsg := "cbor: invalid UTF8 2"
+	_, err := DecOptions{UTF8: 2}.DecMode()
+	if err == nil {
+		t.Errorf("DecMode() didn't return an error")
+	} else if err.Error() != wantErrorMsg {
+		t.Errorf("DecMode() returned error %q, want %q", err.Error(), wantErrorMsg)
+	}
+}
+
 func TestStreamExtraErrorCondUnknowField(t *testing.T) {
 	type s struct {
 		A string
diff --git a/stream_decode_test.go b/stream_decode_test.go
index 49b8fae..7898b9c 100644
--- a/stream_decode_test.go
+++ b/stream_decode_test.go
@@ -624,24 +624,49 @@ func TestStreamDecodeInvalidUTF8String(t *testing.T) {
 
 	expectedType := TextStringType
 
+	dmDecodeInvalidUTF8, err := DecOptions{UTF8: UTF8DecodeInvalid}.DecMode()
+	if err != nil {
+		t.Errorf("DecMode() returned an error %+v", err)
+	}
+
 	data := []byte{0x61, 0xfe}
 
 	t.Parallel()
 
-	decoders := []struct {
-		name string
-		sd   *StreamDecoder
+	testCases := []struct {
+		name         string
+		sd           *StreamDecoder
+		wantObj      string
+		wantErrorMsg string
 	}{
-		{"byte_decoder", NewByteStreamDecoder(data)},
-		{"reader_decoder", NewStreamDecoder(bytes.NewReader(data))},
+		{
+			name:         "byte decoder",
+			sd:           NewByteStreamDecoder(data),
+			wantErrorMsg: invalidUTF8ErrorMsg,
+		},
+		{
+			name:    "byte decoder with UTF8DecodeInvalid",
+			sd:      dmDecodeInvalidUTF8.NewByteStreamDecoder(data),
+			wantObj: string([]byte{0xfe}),
+		},
+		{
+			name:         "reader decoder",
+			sd:           NewStreamDecoder(bytes.NewReader(data)),
+			wantErrorMsg: invalidUTF8ErrorMsg,
+		},
+		{
+			name:    "reader decoder with UTF8DecodeInvalid",
+			sd:      dmDecodeInvalidUTF8.NewStreamDecoder(bytes.NewReader(data)),
+			wantObj: string([]byte{0xfe}),
+		},
 	}
 
-	for _, sd := range decoders {
+	for _, tc := range testCases {
 
-		t.Run(sd.name, func(t *testing.T) {
+		t.Run(tc.name, func(t *testing.T) {
 
 			// NextType() peeks at next CBOR data type (data offset is not moved)
-			nt, err := sd.sd.NextType()
+			nt, err := tc.sd.NextType()
 			if err != nil {
 				t.Errorf("NextType() returned error %v", err)
 			}
@@ -649,22 +674,34 @@ func TestStreamDecodeInvalidUTF8String(t *testing.T) {
 				t.Errorf("NextType() returned %s, want %s", nt, expectedType)
 			}
 
-			// DecodeString() should return error and string is skipped (data offset is moved)
-			_, err = sd.sd.DecodeString()
-			if _, ok := err.(*SemanticError); !ok {
-				t.Errorf("DecodeString() returned wrong type of error %T, want (*SemanticError)", err)
-			} else if err.Error() != invalidUTF8ErrorMsg {
-				t.Errorf("DecodeString() returned error %q, want error %q", err.Error(), invalidUTF8ErrorMsg)
+			// If decode option UTF8 is UTF8DecodeInvalid (default),
+			// DecodeString() returns error and string is skipped (data offset is moved).
+			// Otherwise, DecodeString() returns invalid UTF-8 string.
+			s, err := tc.sd.DecodeString()
+
+			if tc.wantErrorMsg != "" {
+				if err == nil {
+					t.Errorf("DecodeString() didn't return error")
+				} else if !strings.Contains(err.Error(), tc.wantErrorMsg) {
+					t.Errorf("DecodeString() returned error %q, want error %q", err.Error(), tc.wantErrorMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("DecodeString() returned error %q", err)
+				}
+				if s != tc.wantObj {
+					t.Errorf("DecodeString() returned %s, want %s", s, tc.wantObj)
+				}
 			}
 
 			// NextType() should return io.EOF
-			_, err = sd.sd.NextType()
+			_, err = tc.sd.NextType()
 			if err != io.EOF {
 				t.Errorf("NextType() returned error %v, want io.EOF", err)
 			}
 
 			// DecodeString() should return io.EOF
-			_, err = sd.sd.DecodeString()
+			_, err = tc.sd.DecodeString()
 			if err != io.EOF {
 				t.Errorf("DecodeString() returned error %v, want io.EOF", err)
 			}