From 455cf0c502cac449cac0a04c3e9ceee5c2f9375e Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Thu, 27 Aug 2020 17:51:29 +0100 Subject: [PATCH] Add support and tests for protobuf messages with map fields --- ql/src/semmle/go/frameworks/Protobuf.qll | 5 ++ .../frameworks/Protobuf/TaintFlows.expected | 8 +++ .../go/frameworks/Protobuf/protos/query.proto | 2 + .../Protobuf/protos/query/query.pb.go | 68 ++++++++++++------- .../frameworks/Protobuf/testDeprecatedApi.go | 40 +++++++++++ .../go/frameworks/Protobuf/testModernApi.go | 40 +++++++++++ 6 files changed, 138 insertions(+), 25 deletions(-) diff --git a/ql/src/semmle/go/frameworks/Protobuf.qll b/ql/src/semmle/go/frameworks/Protobuf.qll index 279983da75f..fbbce8e4496 100644 --- a/ql/src/semmle/go/frameworks/Protobuf.qll +++ b/ql/src/semmle/go/frameworks/Protobuf.qll @@ -153,6 +153,11 @@ module Protobuf { exists(DataFlow::ReadNode base | succ = DataFlow::getUnderlyingNode(base) | any(DataFlow::Write w).writesField(base, getAMessageField(), pred) ) + or + exists(DataFlow::ReadNode base | succ = DataFlow::getUnderlyingNode(base) | + any(DataFlow::Write w).writesElement(base, _, pred) and + [succ.getType(), succ.getType().getPointerType()] instanceof MessageType + ) } } } diff --git a/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected b/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected index 09cd857e287..136839b7b9f 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected +++ b/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected @@ -8,6 +8,10 @@ | testDeprecatedApi.go:93:25:93:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:97:13:97:31 | selection of Msg | | testDeprecatedApi.go:104:22:104:41 | call to getUntrustedString : string | testDeprecatedApi.go:105:13:105:20 | selection of Id | | testDeprecatedApi.go:112:22:112:41 | call to getUntrustedString : string | testDeprecatedApi.go:117:12:117:21 | serialized | +| testDeprecatedApi.go:133:29:133:48 | call to getUntrustedString : string | testDeprecatedApi.go:137:12:137:21 | serialized | +| testDeprecatedApi.go:143:20:143:39 | call to getUntrustedString : string | testDeprecatedApi.go:148:12:148:21 | serialized | +| testDeprecatedApi.go:152:25:152:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:157:13:157:36 | index expression | +| testDeprecatedApi.go:161:25:161:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:168:13:168:25 | index expression | | testModernApi.go:11:22:11:41 | call to getUntrustedString : string | testModernApi.go:15:12:15:21 | serialized | | testModernApi.go:20:22:20:41 | call to getUntrustedString : string | testModernApi.go:26:12:26:21 | serialized | | testModernApi.go:30:25:30:43 | call to getUntrustedBytes : slice type | testModernApi.go:34:13:34:29 | selection of Description | @@ -21,3 +25,7 @@ | testModernApi.go:131:25:131:43 | call to getUntrustedBytes : slice type | testModernApi.go:135:13:135:29 | selection of Description | | testModernApi.go:142:22:142:41 | call to getUntrustedString : string | testModernApi.go:143:13:143:20 | selection of Id | | testModernApi.go:150:22:150:41 | call to getUntrustedString : string | testModernApi.go:155:12:155:21 | serialized | +| testModernApi.go:190:29:190:48 | call to getUntrustedString : string | testModernApi.go:194:12:194:21 | serialized | +| testModernApi.go:200:20:200:39 | call to getUntrustedString : string | testModernApi.go:205:12:205:21 | serialized | +| testModernApi.go:209:25:209:43 | call to getUntrustedBytes : slice type | testModernApi.go:214:13:214:36 | index expression | +| testModernApi.go:218:25:218:43 | call to getUntrustedBytes : slice type | testModernApi.go:225:13:225:25 | index expression | diff --git a/ql/test/library-tests/semmle/go/frameworks/Protobuf/protos/query.proto b/ql/test/library-tests/semmle/go/frameworks/Protobuf/protos/query.proto index 6e2b38ea866..6a1d9c158b9 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Protobuf/protos/query.proto +++ b/ql/test/library-tests/semmle/go/frameworks/Protobuf/protos/query.proto @@ -16,6 +16,8 @@ message Query { } repeated Alert alerts = 4; + + map keyValuePairs = 5; } message QuerySuite { diff --git a/ql/test/library-tests/semmle/go/frameworks/Protobuf/protos/query/query.pb.go b/ql/test/library-tests/semmle/go/frameworks/Protobuf/protos/query/query.pb.go index c323b98d5ae..ab171353a05 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Protobuf/protos/query/query.pb.go +++ b/ql/test/library-tests/semmle/go/frameworks/Protobuf/protos/query/query.pb.go @@ -1,7 +1,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.24.0-devel -// protoc v3.12.0 +// protoc-gen-go v1.25.0-devel +// protoc v3.12.4 // source: query.proto package query @@ -76,9 +76,10 @@ type Query struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - Description string `protobuf:"bytes,1,opt,name=description,proto3" json:"description,omitempty"` - Id string `protobuf:"bytes,2,opt,name=id,proto3" json:"id,omitempty"` - Alerts []*Query_Alert `protobuf:"bytes,4,rep,name=alerts,proto3" json:"alerts,omitempty"` + Description string `protobuf:"bytes,1,opt,name=description,proto3" json:"description,omitempty"` + Id string `protobuf:"bytes,2,opt,name=id,proto3" json:"id,omitempty"` + Alerts []*Query_Alert `protobuf:"bytes,4,rep,name=alerts,proto3" json:"alerts,omitempty"` + KeyValuePairs map[int32]string `protobuf:"bytes,5,rep,name=keyValuePairs,proto3" json:"keyValuePairs,omitempty" protobuf_key:"varint,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"` } func (x *Query) Reset() { @@ -134,6 +135,13 @@ func (x *Query) GetAlerts() []*Query_Alert { return nil } +func (x *Query) GetKeyValuePairs() map[int32]string { + if x != nil { + return x.KeyValuePairs + } + return nil +} + type QuerySuite struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache @@ -239,23 +247,31 @@ func (x *Query_Alert) GetLoc() int64 { var File_query_proto protoreflect.FileDescriptor var file_query_proto_rawDesc = []byte{ - 0x0a, 0x0b, 0x71, 0x75, 0x65, 0x72, 0x79, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x22, 0xb0, 0x01, + 0x0a, 0x0b, 0x71, 0x75, 0x65, 0x72, 0x79, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x22, 0xb3, 0x02, 0x0a, 0x05, 0x51, 0x75, 0x65, 0x72, 0x79, 0x12, 0x20, 0x0a, 0x0b, 0x64, 0x65, 0x73, 0x63, 0x72, 0x69, 0x70, 0x74, 0x69, 0x6f, 0x6e, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x0b, 0x64, 0x65, 0x73, 0x63, 0x72, 0x69, 0x70, 0x74, 0x69, 0x6f, 0x6e, 0x12, 0x0e, 0x0a, 0x02, 0x69, 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x02, 0x69, 0x64, 0x12, 0x24, 0x0a, 0x06, 0x61, 0x6c, 0x65, 0x72, 0x74, 0x73, 0x18, 0x04, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x0c, 0x2e, 0x51, 0x75, 0x65, 0x72, - 0x79, 0x2e, 0x41, 0x6c, 0x65, 0x72, 0x74, 0x52, 0x06, 0x61, 0x6c, 0x65, 0x72, 0x74, 0x73, 0x1a, - 0x2b, 0x0a, 0x05, 0x41, 0x6c, 0x65, 0x72, 0x74, 0x12, 0x10, 0x0a, 0x03, 0x6d, 0x73, 0x67, 0x18, - 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x03, 0x6d, 0x73, 0x67, 0x12, 0x10, 0x0a, 0x03, 0x6c, 0x6f, - 0x63, 0x18, 0x02, 0x20, 0x01, 0x28, 0x03, 0x52, 0x03, 0x6c, 0x6f, 0x63, 0x22, 0x22, 0x0a, 0x08, - 0x53, 0x65, 0x76, 0x65, 0x72, 0x69, 0x74, 0x79, 0x12, 0x09, 0x0a, 0x05, 0x45, 0x52, 0x52, 0x4f, - 0x52, 0x10, 0x00, 0x12, 0x0b, 0x0a, 0x07, 0x57, 0x41, 0x52, 0x4e, 0x49, 0x4e, 0x47, 0x10, 0x01, - 0x22, 0x2e, 0x0a, 0x0a, 0x51, 0x75, 0x65, 0x72, 0x79, 0x53, 0x75, 0x69, 0x74, 0x65, 0x12, 0x20, - 0x0a, 0x07, 0x71, 0x75, 0x65, 0x72, 0x69, 0x65, 0x73, 0x18, 0x01, 0x20, 0x03, 0x28, 0x0b, 0x32, - 0x06, 0x2e, 0x51, 0x75, 0x65, 0x72, 0x79, 0x52, 0x07, 0x71, 0x75, 0x65, 0x72, 0x69, 0x65, 0x73, - 0x42, 0x0e, 0x5a, 0x0c, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x73, 0x2f, 0x71, 0x75, 0x65, 0x72, 0x79, - 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x79, 0x2e, 0x41, 0x6c, 0x65, 0x72, 0x74, 0x52, 0x06, 0x61, 0x6c, 0x65, 0x72, 0x74, 0x73, 0x12, + 0x3f, 0x0a, 0x0d, 0x6b, 0x65, 0x79, 0x56, 0x61, 0x6c, 0x75, 0x65, 0x50, 0x61, 0x69, 0x72, 0x73, + 0x18, 0x05, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x19, 0x2e, 0x51, 0x75, 0x65, 0x72, 0x79, 0x2e, 0x4b, + 0x65, 0x79, 0x56, 0x61, 0x6c, 0x75, 0x65, 0x50, 0x61, 0x69, 0x72, 0x73, 0x45, 0x6e, 0x74, 0x72, + 0x79, 0x52, 0x0d, 0x6b, 0x65, 0x79, 0x56, 0x61, 0x6c, 0x75, 0x65, 0x50, 0x61, 0x69, 0x72, 0x73, + 0x1a, 0x2b, 0x0a, 0x05, 0x41, 0x6c, 0x65, 0x72, 0x74, 0x12, 0x10, 0x0a, 0x03, 0x6d, 0x73, 0x67, + 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x03, 0x6d, 0x73, 0x67, 0x12, 0x10, 0x0a, 0x03, 0x6c, + 0x6f, 0x63, 0x18, 0x02, 0x20, 0x01, 0x28, 0x03, 0x52, 0x03, 0x6c, 0x6f, 0x63, 0x1a, 0x40, 0x0a, + 0x12, 0x4b, 0x65, 0x79, 0x56, 0x61, 0x6c, 0x75, 0x65, 0x50, 0x61, 0x69, 0x72, 0x73, 0x45, 0x6e, + 0x74, 0x72, 0x79, 0x12, 0x10, 0x0a, 0x03, 0x6b, 0x65, 0x79, 0x18, 0x01, 0x20, 0x01, 0x28, 0x05, + 0x52, 0x03, 0x6b, 0x65, 0x79, 0x12, 0x14, 0x0a, 0x05, 0x76, 0x61, 0x6c, 0x75, 0x65, 0x18, 0x02, + 0x20, 0x01, 0x28, 0x09, 0x52, 0x05, 0x76, 0x61, 0x6c, 0x75, 0x65, 0x3a, 0x02, 0x38, 0x01, 0x22, + 0x22, 0x0a, 0x08, 0x53, 0x65, 0x76, 0x65, 0x72, 0x69, 0x74, 0x79, 0x12, 0x09, 0x0a, 0x05, 0x45, + 0x52, 0x52, 0x4f, 0x52, 0x10, 0x00, 0x12, 0x0b, 0x0a, 0x07, 0x57, 0x41, 0x52, 0x4e, 0x49, 0x4e, + 0x47, 0x10, 0x01, 0x22, 0x2e, 0x0a, 0x0a, 0x51, 0x75, 0x65, 0x72, 0x79, 0x53, 0x75, 0x69, 0x74, + 0x65, 0x12, 0x20, 0x0a, 0x07, 0x71, 0x75, 0x65, 0x72, 0x69, 0x65, 0x73, 0x18, 0x01, 0x20, 0x03, + 0x28, 0x0b, 0x32, 0x06, 0x2e, 0x51, 0x75, 0x65, 0x72, 0x79, 0x52, 0x07, 0x71, 0x75, 0x65, 0x72, + 0x69, 0x65, 0x73, 0x42, 0x0e, 0x5a, 0x0c, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x73, 0x2f, 0x71, 0x75, + 0x65, 0x72, 0x79, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( @@ -271,21 +287,23 @@ func file_query_proto_rawDescGZIP() []byte { } var file_query_proto_enumTypes = make([]protoimpl.EnumInfo, 1) -var file_query_proto_msgTypes = make([]protoimpl.MessageInfo, 3) +var file_query_proto_msgTypes = make([]protoimpl.MessageInfo, 4) var file_query_proto_goTypes = []interface{}{ (Query_Severity)(0), // 0: Query.Severity (*Query)(nil), // 1: Query (*QuerySuite)(nil), // 2: QuerySuite (*Query_Alert)(nil), // 3: Query.Alert + nil, // 4: Query.KeyValuePairsEntry } var file_query_proto_depIdxs = []int32{ 3, // 0: Query.alerts:type_name -> Query.Alert - 1, // 1: QuerySuite.queries:type_name -> Query - 2, // [2:2] is the sub-list for method output_type - 2, // [2:2] is the sub-list for method input_type - 2, // [2:2] is the sub-list for extension type_name - 2, // [2:2] is the sub-list for extension extendee - 0, // [0:2] is the sub-list for field type_name + 4, // 1: Query.keyValuePairs:type_name -> Query.KeyValuePairsEntry + 1, // 2: QuerySuite.queries:type_name -> Query + 3, // [3:3] is the sub-list for method output_type + 3, // [3:3] is the sub-list for method input_type + 3, // [3:3] is the sub-list for extension type_name + 3, // [3:3] is the sub-list for extension extendee + 0, // [0:3] is the sub-list for field type_name } func init() { file_query_proto_init() } @@ -337,7 +355,7 @@ func file_query_proto_init() { GoPackagePath: reflect.TypeOf(x{}).PkgPath(), RawDescriptor: file_query_proto_rawDesc, NumEnums: 1, - NumMessages: 3, + NumMessages: 4, NumExtensions: 0, NumServices: 0, }, diff --git a/ql/test/library-tests/semmle/go/frameworks/Protobuf/testDeprecatedApi.go b/ql/test/library-tests/semmle/go/frameworks/Protobuf/testDeprecatedApi.go index 27d608f5982..be1e0aa77e3 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Protobuf/testDeprecatedApi.go +++ b/ql/test/library-tests/semmle/go/frameworks/Protobuf/testDeprecatedApi.go @@ -127,3 +127,43 @@ func testSubmessageAliasFalseNegative() { sinkBytes(serialized) // BAD (but not noticed by our current implementation) } + +func testTaintedMapFieldWrite() { + query := &query.Query{} + query.KeyValuePairs[123] = getUntrustedString() + + serialized, _ := proto.Marshal(query) + + sinkBytes(serialized) // BAD +} + +func testTaintedMapWriteWholeMap() { + query := &query.Query{} + taintedMap := map[int32]string{} + taintedMap[123] = getUntrustedString() + query.KeyValuePairs = taintedMap + + serialized, _ := proto.Marshal(query) + + sinkBytes(serialized) // BAD +} + +func testTaintedMapFieldRead() { + untrustedSerialized := getUntrustedBytes() + query := &query.Query{} + + proto.Unmarshal(untrustedSerialized, query) + + sinkString(query.KeyValuePairs[123]) // BAD +} + +func testTaintedMapFieldReadViaAlias() { + untrustedSerialized := getUntrustedBytes() + query := &query.Query{} + + proto.Unmarshal(untrustedSerialized, query) + + alias := &query.KeyValuePairs + + sinkString((*alias)[123]) // BAD +} diff --git a/ql/test/library-tests/semmle/go/frameworks/Protobuf/testModernApi.go b/ql/test/library-tests/semmle/go/frameworks/Protobuf/testModernApi.go index 29af1a35804..070e6deeebd 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Protobuf/testModernApi.go +++ b/ql/test/library-tests/semmle/go/frameworks/Protobuf/testModernApi.go @@ -184,3 +184,43 @@ func testMarshalStateFalseNegative() { sinkBytes(serialized.Buf) // BAD (but not noticed by our current implementation) } + +func testTaintedMapFieldWriteModern() { + query := &query.Query{} + query.KeyValuePairs[123] = getUntrustedString() + + serialized, _ := proto.Marshal(query) + + sinkBytes(serialized) // BAD +} + +func testTaintedMapWriteWholeMapModern() { + query := &query.Query{} + taintedMap := map[int32]string{} + taintedMap[123] = getUntrustedString() + query.KeyValuePairs = taintedMap + + serialized, _ := proto.Marshal(query) + + sinkBytes(serialized) // BAD +} + +func testTaintedMapFieldReadModern() { + untrustedSerialized := getUntrustedBytes() + query := &query.Query{} + + proto.Unmarshal(untrustedSerialized, query) + + sinkString(query.KeyValuePairs[123]) // BAD +} + +func testTaintedMapFieldReadViaAliasModern() { + untrustedSerialized := getUntrustedBytes() + query := &query.Query{} + + proto.Unmarshal(untrustedSerialized, query) + + alias := &query.KeyValuePairs + + sinkString((*alias)[123]) // BAD +}