Skip to content
Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
natemueller authored and facebook-github-bot committed Feb 25, 2019
1 parent 41af43a commit c461c1b
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 3 deletions.
22 changes: 22 additions & 0 deletions thrift/lib/go/thrift/binary_protocol_test.go
Expand Up @@ -21,9 +21,31 @@
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")
}
}
6 changes: 3 additions & 3 deletions thrift/lib/go/thrift/protocol.go
Expand Up @@ -22,6 +22,7 @@ package thrift

import (
"errors"
"fmt"
)

type ProtocolID int16
Expand Down Expand Up @@ -127,8 +128,6 @@ func Skip(self Protocol, fieldType Type, maxDepth int) (err error) {
}

switch fieldType {
case STOP:
return
case BOOL:
_, err = self.ReadBool()
return
Expand Down Expand Up @@ -206,6 +205,7 @@ func Skip(self Protocol, fieldType Type, maxDepth int) (err error) {
}
}
return self.ReadListEnd()
default:
return fmt.Errorf("unable to skip over unknown type id %d", fieldType)
}
return nil
}

0 comments on commit c461c1b

Please sign in to comment.