diff --git a/ql/src/semmle/go/frameworks/Protobuf.qll b/ql/src/semmle/go/frameworks/Protobuf.qll index 8b040ef5317..9c17c331b78 100644 --- a/ql/src/semmle/go/frameworks/Protobuf.qll +++ b/ql/src/semmle/go/frameworks/Protobuf.qll @@ -1,16 +1,21 @@ -/** Provides models of commonly used functions and types in the `google.golang.org/protobuf/proto` package. */ +/** Provides models of commonly used functions and types in the protobuf packages. */ import go -/** Provides models of commonly used functions and types in the `google.golang.org/protobuf/proto` package. */ +/** Provides models of commonly used functions and types in the protobuf packages. */ module Protobuf { - /** The `Marshal`, `MarshalAppend`, or `MarshalState` functions in the `google.golang.org/protobuf/proto` package. */ + pragma[inline] + string protobufPackages() { + result in ["github.com/golang/protobuf/proto", "google.golang.org/protobuf/proto"] + } + + /** The `Marshal`, `MarshalAppend`, or `MarshalState` functions in the protobuf packages. */ private class MarshalFunction extends TaintTracking::FunctionModel, MarshalingFunction::Range { string name; MarshalFunction() { name = ["Marshal", "MarshalAppend", "MarshalState"] and - this.hasQualifiedName("github.com/golang/protobuf/proto", name) + this.hasQualifiedName(protobufPackages(), name) } override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) { @@ -30,13 +35,13 @@ module Protobuf { override string getFormat() { result = "protobuf" } } - /** The `Unmarshal` or `UnmarshalState` functions in the `google.golang.org/protobuf/proto` package. */ + /** The `Unmarshal` or `UnmarshalState` functions in the protobuf packages. */ class UnmarshalFunction extends TaintTracking::FunctionModel, UnmarshalingFunction::Range { string name; UnmarshalFunction() { name = ["Unmarshal", "UnmarshalState"] and - this.hasQualifiedName("github.com/golang/protobuf/proto", name) + this.hasQualifiedName(protobufPackages(), name) } override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) { @@ -50,9 +55,9 @@ module Protobuf { override string getFormat() { result = "protobuf" } } - /** The `Merge` function in the `google.golang.org/protobuf/proto` package. */ + /** The `Merge` function in the protobuf packages. */ private class MergeFunction extends TaintTracking::FunctionModel { - MergeFunction() { this.hasQualifiedName("github.com/golang/protobuf/proto", "Merge") } + MergeFunction() { this.hasQualifiedName(protobufPackages(), "Merge") } override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { inp.isParameter(1) and outp.isParameter(0) @@ -66,9 +71,9 @@ module Protobuf { } } - /** The `Clone` method of a protobuf `Message` type. */ + /** The `Clone` function in the protobuf packages. */ private class MessageCloneFunction extends TaintTracking::FunctionModel { - MessageCloneFunction() { this.hasQualifiedName("github.com/golang/protobuf/proto", "Clone") } + MessageCloneFunction() { this.hasQualifiedName(protobufPackages(), "Clone") } override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { inp.isParameter(0) and outp.isResult() diff --git a/ql/test/library-tests/semmle/go/frameworks/Protobuf/FunctionModel.expected b/ql/test/library-tests/semmle/go/frameworks/Protobuf/FunctionModel.expected index 8316180cba7..26ee02a37c7 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Protobuf/FunctionModel.expected +++ b/ql/test/library-tests/semmle/go/frameworks/Protobuf/FunctionModel.expected @@ -10,3 +10,11 @@ | testDeprecatedApi.go:53:13:53:17 | query | testDeprecatedApi.go:53:13:53:34 | call to GetDescription | | testDeprecatedApi.go:61:22:61:27 | query1 | testDeprecatedApi.go:60:2:60:7 | definition of query2 | | testDeprecatedApi.go:63:33:63:38 | query2 | testDeprecatedApi.go:63:2:63:39 | ... := ...[0] | +| testModernApi.go:24:33:24:37 | query | testModernApi.go:24:2:24:38 | ... := ...[0] | +| testModernApi.go:33:28:33:32 | query | testModernApi.go:33:16:33:33 | call to Clone | +| testModernApi.go:35:33:35:42 | queryClone | testModernApi.go:35:2:35:43 | ... := ...[0] | +| testModernApi.go:43:18:43:36 | untrustedSerialized | testModernApi.go:42:2:42:6 | definition of query | +| testModernApi.go:51:18:51:36 | untrustedSerialized | testModernApi.go:50:2:50:6 | definition of query | +| testModernApi.go:53:13:53:17 | query | testModernApi.go:53:13:53:34 | call to GetDescription | +| testModernApi.go:61:22:61:27 | query1 | testModernApi.go:60:2:60:7 | definition of query2 | +| testModernApi.go:63:33:63:38 | query2 | testModernApi.go:63:2:63:39 | ... := ...[0] | 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 829d2e315b3..713a89f53d3 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected +++ b/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected @@ -3,3 +3,8 @@ | testDeprecatedApi.go:41:25:41:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:45:13:45:29 | selection of Description | | testDeprecatedApi.go:49:25:49:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:53:13:53:34 | call to GetDescription | | testDeprecatedApi.go:58:23:58:42 | call to getUntrustedString : string | testDeprecatedApi.go:65:12:65:21 | serialized | +| testModernApi.go:22:22:22:41 | call to getUntrustedString : string | testModernApi.go:26:12:26:21 | serialized | +| testModernApi.go:31:22:31:41 | call to getUntrustedString : string | testModernApi.go:37:12:37:21 | serialized | +| testModernApi.go:41:25:41:43 | call to getUntrustedBytes : slice type | testModernApi.go:45:13:45:29 | selection of Description | +| testModernApi.go:49:25:49:43 | call to getUntrustedBytes : slice type | testModernApi.go:53:13:53:34 | call to GetDescription | +| testModernApi.go:58:23:58:42 | call to getUntrustedString : string | testModernApi.go:65:12:65:21 | serialized | diff --git a/ql/test/library-tests/semmle/go/frameworks/Protobuf/testModernApi.go b/ql/test/library-tests/semmle/go/frameworks/Protobuf/testModernApi.go new file mode 100644 index 00000000000..d2c0525c5df --- /dev/null +++ b/ql/test/library-tests/semmle/go/frameworks/Protobuf/testModernApi.go @@ -0,0 +1,66 @@ +package main + +import ( + "codeql-go-tests/protobuf/protos/query" + "google.golang.org/protobuf/proto" +) + +func getUntrustedString() string { + return "trouble" +} + +func getUntrustedBytes() []byte { + return []byte{} +} + +func sinkString(_ string) {} + +func sinkBytes(_ []byte) {} + +func testMarshal() { + query := &query.Query{} + query.Description = getUntrustedString() + + serialized, _ := proto.Marshal(query) + + sinkBytes(serialized) +} + +func testCloneThenMarshal() { + query := &query.Query{} + query.Description = getUntrustedString() + + queryClone := proto.Clone(query) + + serialized, _ := proto.Marshal(queryClone) + + sinkBytes(serialized) +} + +func testUnmarshalFieldAccess() { + untrustedSerialized := getUntrustedBytes() + query := &query.Query{} + proto.Unmarshal(untrustedSerialized, query) + + sinkString(query.Description) +} + +func testUnmarshalGetter() { + untrustedSerialized := getUntrustedBytes() + query := &query.Query{} + proto.Unmarshal(untrustedSerialized, query) + + sinkString(query.GetDescription()) +} + +func testMergeThenMarshal() { + query1 := &query.Query{} + query1.Description = getUntrustedString() + + query2 := &query.Query{} + proto.Merge(query2, query1) + + serialized, _ := proto.Marshal(query2) + + sinkBytes(serialized) +}