Model and test UnmarshalOptions.Unmarshal

Support for UnmarshalOptions.UnmarshalState is dropped for now as too hard to model.

Author: smowton

ql/src/semmle/go/frameworks/Protobuf.qll

@@ -77,13 +77,13 @@ module Protobuf {
     }
   }
 
-  /** The `Unmarshal` or `UnmarshalState` functions in the protobuf packages. */
+  /** The `Unmarshal` function in the protobuf packages. */
   class UnmarshalFunction extends TaintTracking::FunctionModel, UnmarshalingFunction::Range {
-    string name;
-
     UnmarshalFunction() {
-      name = ["Unmarshal", "UnmarshalState"] and
-      this.hasQualifiedName(protobufPackages(), name)
+      this.hasQualifiedName(protobufPackages(), "Unmarshal") or
+      this
+          .(Method)
+          .hasQualifiedName("google.golang.org/protobuf/proto", "UnmarshalOptions", "Unmarshal")
     }
 
     override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {

ql/test/library-tests/semmle/go/frameworks/Protobuf/FunctionModel.expected

@@ -38,6 +38,7 @@
 | testModernApi.go:112:38:112:42 | alert | testModernApi.go:112:17:112:43 | call to append |
 | testModernApi.go:115:33:115:37 | query | testModernApi.go:115:2:115:38 | ... := ...[0] |
 | testModernApi.go:123:18:123:36 | untrustedSerialized | testModernApi.go:122:2:122:6 | definition of query |
-| testModernApi.go:143:33:143:37 | query | testModernApi.go:143:2:143:38 | ... := ...[0] |
-| testModernApi.go:154:33:154:37 | query | testModernApi.go:154:2:154:38 | ... := ...[0] |
-| testModernApi.go:168:12:168:16 | query | testModernApi.go:168:12:168:31 | call to ProtoReflect |
+| testModernApi.go:133:20:133:38 | untrustedSerialized | testModernApi.go:132:2:132:6 | definition of query |
+| testModernApi.go:153:33:153:37 | query | testModernApi.go:153:2:153:38 | ... := ...[0] |
+| testModernApi.go:164:33:164:37 | query | testModernApi.go:164:2:164:38 | ... := ...[0] |
+| testModernApi.go:178:12:178:16 | query | testModernApi.go:178:12:178:31 | call to ProtoReflect |

ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected

@@ -18,5 +18,6 @@
 | testModernApi.go:98:14:98:33 | call to getUntrustedString : string | testModernApi.go:105:12:105:21 | serialized |
 | testModernApi.go:113:24:113:43 | call to getUntrustedString : string | testModernApi.go:117:12:117:21 | serialized |
 | testModernApi.go:121:25:121:43 | call to getUntrustedBytes : slice type | testModernApi.go:125:13:125:31 | selection of Msg |
-| testModernApi.go:132:22:132:41 | call to getUntrustedString : string | testModernApi.go:133:13:133:20 | selection of Id |
-| testModernApi.go:140:22:140:41 | call to getUntrustedString : string | testModernApi.go:145:12:145:21 | serialized |
+| 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 |

ql/test/library-tests/semmle/go/frameworks/Protobuf/testModernApi.go

@@ -125,6 +125,16 @@ func testUnmarshalTaintedSubmessageModern() {
 	sinkString(query.Alerts[0].Msg) // BAD
 }
 
+func testUnmarshalOptions() {
+	options := proto.UnmarshalOptions{}
+
+	untrustedSerialized := getUntrustedBytes()
+	query := &query.Query{}
+	options.Unmarshal(untrustedSerialized, query)
+
+	sinkString(query.Description) // BAD
+}
+
 // This test should be ok, but is flagged because writing taint to a field of a Message
 // taints the entire Message structure in our current implementation.
 func testFieldConflationFalsePositiveModern() {