LaconicNetwork/cosmos-sdk
7
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 2 Wiki Activity

Reject unknown fields in TxDecoder and sign mode handlers (#6883)

Browse Source 
* WIP on unknown field rejection in TxDecoder

* WIP on unknown field rejection in TxDecoder

* WIP

* WIP

* WIP

* WIP

* Fix bugs with RejectUnknownFields

* Fix tests

* Fix bug and update docs

* Lint

* Add tests

* Add unknown field tests

* Lint

* Address review comments
This commit is contained in:
Aaron Craelius
2020-08-03 19:47:25 +00:00
committed by GitHub
parent 57cd7d62b3
commit 6d937443b2
31 changed files with 2598 additions and 968 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codec/unknownproto/benchmarks_test.go
+2 -4
View File
@@ -53,11 +53,10 @@ func benchmarkRejectUnknownFields(b *testing.B, parallel bool) {
b.ReportAllocs()
if !parallel {
ckr := new(unknownproto.Checker)
b.ResetTimer()
for i := 0; i < b.N; i++ {
n1A := new(testdata.Nested1A)
if err := ckr.RejectUnknownFields(n1BBlob, n1A); err == nil {
if err := unknownproto.RejectUnknownFieldsStrict(n1BBlob, n1A); err == nil {
b.Fatal("expected an error")
}
b.SetBytes(int64(len(n1BBlob)))
@@ -66,11 +65,10 @@ func benchmarkRejectUnknownFields(b *testing.B, parallel bool) {
var mu sync.Mutex
b.ResetTimer()
b.RunParallel(func(pb *testing.PB) {
ckr := new(unknownproto.Checker)
for pb.Next() {
// To simulate the conditions of multiple transactions being processed in parallel.
n1A := new(testdata.Nested1A)
if err := ckr.RejectUnknownFields(n1BBlob, n1A); err == nil {
if err := unknownproto.RejectUnknownFieldsStrict(n1BBlob, n1A); err == nil {
b.Fatal("expected an error")
}
mu.Lock()
codec/unknownproto/doc.go
+4 -8
View File
@@ -6,22 +6,18 @@ a) Unknown fields in the stream -- this is indicative of mismatched services, pe
b) Mismatched wire types for a field -- this is indicative of mismatched services
Its API signature is similar to proto.Unmarshal([]byte, proto.Message) as
Its API signature is similar to proto.Unmarshal([]byte, proto.Message) in the strict case
ckr := new(unknownproto.Checker)
if err := ckr.RejectUnknownFields(protoBlob, protoMessage); err != nil {
if err := RejectUnknownFieldsStrict(protoBlob, protoMessage, false); err != nil {
// Handle the error.
}
and ideally should be added before invoking proto.Unmarshal, if you'd like to enforce the features mentioned above.
By default, for security we report every single field that's unknown, whether a non-critical field or not. To customize
this behavior, please create a Checker and set the AllowUnknownNonCriticals to true, for example:
this behavior, please set the boolean parameter allowUnknownNonCriticals to true to RejectUnknownFields:
ckr := &unknownproto.Checker{
AllowUnknownNonCriticals: true,
}
if err := ckr.RejectUnknownFields(protoBlob, protoMessage); err != nil {
if err := RejectUnknownFields(protoBlob, protoMessage, true); err != nil {
// Handle the error.
}
*/
codec/unknownproto/unknown_fields.go
+50 -26
View File
@@ -22,30 +22,37 @@ type descriptorIface interface {
Descriptor() ([]byte, []int)
}
type Checker struct {
// AllowUnknownNonCriticals when set will skip over non-critical fields that are unknown.
AllowUnknownNonCriticals bool
// RejectUnknownFieldsStrict rejects any bytes bz with an error that has unknown fields for the provided proto.Message type.
// This function traverses inside of messages nested via google.protobuf.Any. It does not do any deserialization of the proto.Message.
func RejectUnknownFieldsStrict(bz []byte, msg proto.Message) error {
_, err := RejectUnknownFields(bz, msg, false)
return err
}
func (ckr *Checker) RejectUnknownFields(b []byte, msg proto.Message) error {
if len(b) == 0 {
return nil
// RejectUnknownFields rejects any bytes bz with an error that has unknown fields for the provided proto.Message type with an
// option to allow non-critical fields (specified as those fields with bit 11) to pass through. In either case, the
// hasUnknownNonCriticals will be set to true if non-critical fields were encountered during traversal. This flag can be
// used to treat a message with non-critical field different in different security contexts (such as transaction signing).
// This function traverses inside of messages nested via google.protobuf.Any. It does not do any deserialization of the proto.Message.
func RejectUnknownFields(bz []byte, msg proto.Message, allowUnknownNonCriticals bool) (hasUnknownNonCriticals bool, err error) {
if len(bz) == 0 {
return hasUnknownNonCriticals, nil
}
desc, ok := msg.(descriptorIface)
if !ok {
return fmt.Errorf("%T does not have a Descriptor() method", msg)
return hasUnknownNonCriticals, fmt.Errorf("%T does not have a Descriptor() method", msg)
}
fieldDescProtoFromTagNum, _, err := getDescriptorInfo(desc, msg)
if err != nil {
return err
return hasUnknownNonCriticals, err
}
for len(b) > 0 {
tagNum, wireType, n := protowire.ConsumeField(b)
if n < 0 {
return errors.New("invalid length")
for len(bz) > 0 {
tagNum, wireType, m := protowire.ConsumeTag(bz)
if m < 0 {
return hasUnknownNonCriticals, errors.New("invalid length")
}
fieldDescProto, ok := fieldDescProtoFromTagNum[int32(tagNum)]
@@ -53,7 +60,7 @@ func (ckr *Checker) RejectUnknownFields(b []byte, msg proto.Message) error {
case ok:
// Assert that the wireTypes match.
if !canEncodeType(wireType, fieldDescProto.GetType()) {
return &errMismatchedWireType{
return hasUnknownNonCriticals, &errMismatchedWireType{
Type: reflect.ValueOf(msg).Type().String(),
TagNum: tagNum,
GotWireType: wireType,
@@ -62,9 +69,15 @@ func (ckr *Checker) RejectUnknownFields(b []byte, msg proto.Message) error {
}
default:
if !ckr.AllowUnknownNonCriticals || tagNum&bit11NonCritical == 0 {
isCriticalField := tagNum&bit11NonCritical == 0
if !isCriticalField {
hasUnknownNonCriticals = true
}
if isCriticalField || !allowUnknownNonCriticals {
// The tag is critical, so report it.
return &errUnknownField{
return hasUnknownNonCriticals, &errUnknownField{
Type: reflect.ValueOf(msg).Type().String(),
TagNum: tagNum,
WireType: wireType,
@@ -72,9 +85,11 @@ func (ckr *Checker) RejectUnknownFields(b []byte, msg proto.Message) error {
}
}
// Skip over the 2 bytes that store fieldNumber and wireType bytes.
fieldBytes := b[2:n]
b = b[n:]
// Skip over the bytes that store fieldNumber and wireType bytes.
bz = bz[m:]
n := protowire.ConsumeFieldValue(tagNum, wireType, bz)
fieldBytes := bz[:n]
bz = bz[n:]
// An unknown but non-critical field or just a scalar type (aka *INT and BYTES like).
if fieldDescProto == nil || fieldDescProto.IsScalar() {
@@ -89,22 +104,28 @@ func (ckr *Checker) RejectUnknownFields(b []byte, msg proto.Message) error {
// TYPE_BYTES and TYPE_STRING as per
// https://github.com/gogo/protobuf/blob/5628607bb4c51c3157aacc3a50f0ab707582b805/protoc-gen-gogo/descriptor/descriptor.go#L95-L118
default:
return fmt.Errorf("failed to get typename for message of type %v, can only be TYPE_STRING or TYPE_BYTES", typ)
return hasUnknownNonCriticals, fmt.Errorf("failed to get typename for message of type %v, can only be TYPE_STRING or TYPE_BYTES", typ)
}
continue
}
// Let's recursively traverse and typecheck the field.
// consume length prefix of nested message
_, o := protowire.ConsumeVarint(fieldBytes)
fieldBytes = fieldBytes[o:]
if protoMessageName == ".google.protobuf.Any" {
// Firstly typecheck types.Any to ensure nothing snuck in.
if err := ckr.RejectUnknownFields(fieldBytes, (*types.Any)(nil)); err != nil {
return err
hasUnknownNonCriticalsChild, err := RejectUnknownFields(fieldBytes, (*types.Any)(nil), allowUnknownNonCriticals)
hasUnknownNonCriticals = hasUnknownNonCriticals || hasUnknownNonCriticalsChild
if err != nil {
return hasUnknownNonCriticals, err
}
// And finally we can extract the TypeURL containing the protoMessageName.
any := new(types.Any)
if err := proto.Unmarshal(fieldBytes, any); err != nil {
return err
return hasUnknownNonCriticals, err
}
protoMessageName = any.TypeUrl
fieldBytes = any.Value
@@ -112,14 +133,17 @@ func (ckr *Checker) RejectUnknownFields(b []byte, msg proto.Message) error {
msg, err := protoMessageForTypeName(protoMessageName[1:])
if err != nil {
return err
return hasUnknownNonCriticals, err
}
if err := ckr.RejectUnknownFields(fieldBytes, msg); err != nil {
return err
hasUnknownNonCriticalsChild, err := RejectUnknownFields(fieldBytes, msg, allowUnknownNonCriticals)
hasUnknownNonCriticals = hasUnknownNonCriticals || hasUnknownNonCriticalsChild
if err != nil {
return hasUnknownNonCriticals, err
}
}
return nil
return hasUnknownNonCriticals, nil
}
var protoMessageForTypeNameMu sync.RWMutex
codec/unknownproto/unknown_fields_test.go
+15 -16
View File
@@ -4,6 +4,8 @@ import (
"reflect"
"testing"
"github.com/stretchr/testify/require"
"github.com/gogo/protobuf/proto"
"github.com/cosmos/cosmos-sdk/codec/types"
@@ -17,6 +19,7 @@ func TestRejectUnknownFieldsRepeated(t *testing.T) {
recv proto.Message
wantErr error
allowUnknownNonCriticals bool
hasUnknownNonCriticals bool
}{
{
name: "Unknown field in midst of repeated values",
@@ -172,6 +175,7 @@ func TestRejectUnknownFieldsRepeated(t *testing.T) {
TagNum: 1031,
WireType: 2,
},
hasUnknownNonCriticals: true,
},
{
name: "Unknown field in midst of repeated values, non-critical field ignored",
@@ -213,8 +217,9 @@ func TestRejectUnknownFieldsRepeated(t *testing.T) {
},
},
},
recv: new(testdata.TestVersion1),
wantErr: nil,
recv: new(testdata.TestVersion1),
wantErr: nil,
hasUnknownNonCriticals: true,
},
}
@@ -225,11 +230,9 @@ func TestRejectUnknownFieldsRepeated(t *testing.T) {
if err != nil {
t.Fatal(err)
}
ckr := &Checker{AllowUnknownNonCriticals: tt.allowUnknownNonCriticals}
gotErr := ckr.RejectUnknownFields(protoBlob, tt.recv)
if !reflect.DeepEqual(gotErr, tt.wantErr) {
t.Fatalf("Error mismatch\nGot:\n%v\n\nWant:\n%v", gotErr, tt.wantErr)
}
hasUnknownNonCriticals, gotErr := RejectUnknownFields(protoBlob, tt.recv, tt.allowUnknownNonCriticals)
require.Equal(t, tt.wantErr, gotErr)
require.Equal(t, tt.hasUnknownNonCriticals, hasUnknownNonCriticals)
})
}
}
@@ -263,7 +266,7 @@ func TestRejectUnknownFields_allowUnknownNonCriticals(t *testing.T) {
wantErr: nil,
},
{
name: "Unkown fields that are critical, but with allowUnknownNonCriticals set",
name: "Unknown fields that are critical, but with allowUnknownNonCriticals set",
allowUnknownNonCriticals: true,
in: &testdata.Customer2{
Id: 289,
@@ -285,9 +288,8 @@ func TestRejectUnknownFields_allowUnknownNonCriticals(t *testing.T) {
t.Fatalf("Failed to marshal input: %v", err)
}
ckr := &Checker{AllowUnknownNonCriticals: tt.allowUnknownNonCriticals}
c1 := new(testdata.Customer1)
gotErr := ckr.RejectUnknownFields(blob, c1)
_, gotErr := RejectUnknownFields(blob, c1, tt.allowUnknownNonCriticals)
if !reflect.DeepEqual(gotErr, tt.wantErr) {
t.Fatalf("Error mismatch\nGot:\n%s\n\nWant:\n%s", gotErr, tt.wantErr)
}
@@ -498,8 +500,7 @@ func TestRejectUnknownFieldsNested(t *testing.T) {
if err != nil {
t.Fatal(err)
}
ckr := new(Checker)
gotErr := ckr.RejectUnknownFields(protoBlob, tt.recv)
gotErr := RejectUnknownFieldsStrict(protoBlob, tt.recv)
if !reflect.DeepEqual(gotErr, tt.wantErr) {
t.Fatalf("Error mismatch\nGot:\n%s\n\nWant:\n%s", gotErr, tt.wantErr)
}
@@ -652,8 +653,7 @@ func TestRejectUnknownFieldsFlat(t *testing.T) {
}
c1 := new(testdata.Customer1)
ckr := new(Checker)
gotErr := ckr.RejectUnknownFields(blob, c1)
gotErr := RejectUnknownFieldsStrict(blob, c1)
if !reflect.DeepEqual(gotErr, tt.wantErr) {
t.Fatalf("Error mismatch\nGot:\n%s\n\nWant:\n%s", gotErr, tt.wantErr)
}
@@ -738,8 +738,7 @@ func TestMismatchedTypes_Nested(t *testing.T) {
if err != nil {
t.Fatal(err)
}
ckr := new(Checker)
gotErr := ckr.RejectUnknownFields(protoBlob, tt.recv)
_, gotErr := RejectUnknownFields(protoBlob, tt.recv, false)
if !reflect.DeepEqual(gotErr, tt.wantErr) {
t.Fatalf("Error mismatch\nGot:\n%s\n\nWant:\n%s", gotErr, tt.wantErr)
}