Add support and tests for protobuf messages with map fields

2026-01-30 14:52:57 +01:00 · 2020-08-27 17:51:29 +01:00
parent b2d4e2692f
commit 455cf0c502
6 changed files with 138 additions and 25 deletions
--- 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
+      )
    }
  }
 }
--- 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 |
--- 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<int32, string> keyValuePairs = 5;
 }

 message QuerySuite {
--- 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,
 		},
--- 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
+}
--- 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
+}