CVE-2019-3564: Improper Handling of Exceptional Conditions (CWE-755)
value: Go Facebook Thrift servers would not error upon receiving messages with containers of fields of unknown type. As a result, malicious clients could send short messages which would take a long time for the server to parse, potentially leading to denial of service. This issue affects Facebook Thrift prior to v2019.03.04.00.
CWE description: The product does not handle or incorrectly handles an exceptional condition.
CVSS v3 base score: 7.5
Published: 2019-05-06T16:29Z
Repository: https://github.com/facebook/fbthrift
Language: Go
Files changed: thrift/lib/go/thrift/binary_protocol_test.go, thrift/lib/go/thrift/protocol.go
Commit date: 2019-02-25 08:43:08 -0800
Commit message: Properly error when asked to skip an unknown field type

Summary:
We weren't returning an error when asked to skip over a field with and
unknown type.  In this particular test case the code attempts to skip
over a map with a large number of fields of unknown type and the ~3B
noop calls take almost 30s.  A misbehaving client could DoS a server
by sending short messages that take a long time to parse.  There may
have been other failure modes as well.

The test covers the binary protocol because that is where the issue
was found.  However, the issue is common to all protocols.

This fixes CVE-2019-3552.

Reviewed By: spalamarchuk

Differential Revision: D14088980

fbshipit-source-id: 8a9d63260db717347217a8d2ac883c4ce331d964

Diff: diff --git a/thrift/lib/go/thrift/binary_protocol_test.go b/thrift/lib/go/thrift/binary_protocol_test.go
index d4dfea7f7a..806094ea49 100644
--- a/thrift/lib/go/thrift/binary_protocol_test.go
+++ b/thrift/lib/go/thrift/binary_protocol_test.go
@@ -19,11 +19,33 @@
  * under the License.
  */
 package thrift
 
 import (
+	"strings"
 	"testing"
+	"time"
 )
 
 func TestReadWriteBinaryProtocol(t *testing.T) {
 	ReadWriteProtocolTest(t, NewBinaryProtocolFactoryDefault())
 }
+
+func TestSkipUnknownTypeBinaryProtocol(t *testing.T) {
+	var m MyTestStruct
+	d := NewDeserializer()
+	f := NewBinaryProtocolFactoryDefault()
+	d.Protocol = f.GetProtocol(d.Transport)
+	// skip over a map with invalid key/value type and 1.7B entries
+	data := []byte("\n\x10\rO\t6\x03\n\n\n\x10\r\n\tslice\x00")
+	start := time.Now()
+	err := d.Read(&m, data)
+	if err == nil {
+		t.Fatalf("Parsed invalid message correctly")
+	} else if !strings.Contains(err.Error(), "unknown type") {
+		t.Fatalf("Failed for reason besides unknown type")
+	}
+
+	if time.Now().Sub(start).Seconds() > 5 {
+		t.Fatalf("It should not take seconds to parse a small message")
+	}
+}
diff --git a/thrift/lib/go/thrift/protocol.go b/thrift/lib/go/thrift/protocol.go
index b641767519..3a1e2547e1 100644
--- a/thrift/lib/go/thrift/protocol.go
+++ b/thrift/lib/go/thrift/protocol.go
@@ -20,10 +20,11 @@
  */
 package thrift
 
 import (
 	"errors"
+	"fmt"
 )
 
 type ProtocolID int16
 
 const (
@@ -125,12 +126,10 @@ func Skip(self Protocol, fieldType Type, maxDepth int) (err error) {
 	if maxDepth <= 0 {
 		return NewProtocolExceptionWithType(DEPTH_LIMIT, errors.New("Depth limit exceeded"))
 	}
 
 	switch fieldType {
-	case STOP:
-		return
 	case BOOL:
 		_, err = self.ReadBool()
 		return
 	case BYTE:
 		_, err = self.ReadByte()
@@ -204,8 +203,9 @@ func Skip(self Protocol, fieldType Type, maxDepth int) (err error) {
 			if err != nil {
 				return err
 			}
 		}
 		return self.ReadListEnd()
+	default:
+		return fmt.Errorf("unable to skip over unknown type id %d", fieldType)
 	}
-	return nil
 }

Vulnerable code: 	case STOP:
		return
	return nil
Fixed code: 	"strings"
	"time"

func TestSkipUnknownTypeBinaryProtocol(t *testing.T) {
	var m MyTestStruct
	d := NewDeserializer()
	f := NewBinaryProtocolFactoryDefault()
	d.Protocol = f.GetProtocol(d.Transport)
	// skip over a map with invalid key/value type and 1.7B entries
	data := []byte("\n\x10\rO\t6\x03\n\n\n\x10\r\n\tslice\x00")
	start := time.Now()
	err := d.Read(&m, data)
	if err == nil {
		t.Fatalf("Parsed invalid message correctly")
	} else if !strings.Contains(err.Error(), "unknown type") {
		t.Fatalf("Failed for reason besides unknown type")
	}

	if time.Now().Sub(start).Seconds() > 5 {
		t.Fatalf("It should not take seconds to parse a small message")
	}
}
	"fmt"
	default:
		return fmt.Errorf("unable to skip over unknown type id %d", fieldType)