Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Commit 1821cca

Browse files
max-schaefermax-schaefer
authored
Merge pull request #285 from smowton/protobufs
Protobuf modelling
2 parents 25e4245 + cfba089 commit 1821cca

File tree

21 files changed

+2204
-21
lines changed

21 files changed

+2204
-21
lines changed

‎ql/src/go.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import semmle.go.frameworks.HTTP
3434
import semmle.go.frameworks.Macaron
3535
import semmle.go.frameworks.Mux
3636
import semmle.go.frameworks.NoSQL
37+
import semmle.go.frameworks.Protobuf
3738
import semmle.go.frameworks.SQL
3839
import semmle.go.frameworks.Stdlib
3940
import semmle.go.frameworks.SystemCommandExecutors

‎ql/src/semmle/go/controlflow/ControlFlowGraph.qll

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,13 @@ module ControlFlow {
165165
self.getRhs() = rhs.asInstruction()
166166
)
167167
}
168+
169+
/**
170+
* Holds if this node sets any field or element of `base` to `rhs`.
171+
*/
172+
predicate writesComponent(DataFlow::Node base, DataFlow::Node rhs) {
173+
writesElement(base, _, rhs) or writesField(base, _, rhs)
174+
}
168175
}
169176

170177
/**

‎ql/src/semmle/go/controlflow/IR.qll

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -253,23 +253,32 @@ module IR {
253253
)
254254
}
255255

256+
/**
257+
* An IR instruction that reads a component from a composite object.
258+
*
259+
* This is either a field of a struct, or an element of an array, map, slice or string.
260+
*/
261+
class ComponentReadInstruction extends ReadInstruction, EvalInstruction {
262+
ComponentReadInstruction() {
263+
e instanceof IndexExpr
264+
or
265+
e.(SelectorExpr).getBase() instanceof ValueExpr and
266+
not e.(SelectorExpr).getSelector() = any(Method method).getAReference()
267+
}
268+
269+
/** Gets the instruction computing the base value on which the field or element is read. */
270+
Instruction getBase() { result = selectorBase(e) }
271+
}
272+
256273
/**
257274
* An IR instruction that reads the value of a field.
258275
*
259276
* On snapshots with incomplete type information, method expressions may sometimes be
260277
* misclassified as field reads.
261278
*/
262-
class FieldReadInstruction extends ReadInstruction, EvalInstruction {
279+
class FieldReadInstruction extends ComponentReadInstruction {
263280
override SelectorExpr e;
264281

265-
FieldReadInstruction() {
266-
e.getBase() instanceof ValueExpr and
267-
not e.getSelector() = any(Method method).getAReference()
268-
}
269-
270-
/** Gets the instruction computing the base value on which the field is read. */
271-
Instruction getBase() { result = selectorBase(e) }
272-
273282
/** Gets the field being read. */
274283
Field getField() { e.getSelector() = result.getAReference() }
275284

@@ -299,12 +308,9 @@ module IR {
299308
/**
300309
* An IR instruction that reads an element of an array, slice, map or string.
301310
*/
302-
class ElementReadInstruction extends ReadInstruction, EvalInstruction {
311+
class ElementReadInstruction extends ComponentReadInstruction {
303312
override IndexExpr e;
304313

305-
/** Gets the instruction computing the base value on which the element is looked up. */
306-
Instruction getBase() { result = selectorBase(e) }
307-
308314
/** Gets the instruction computing the index of the element being looked up. */
309315
Instruction getIndex() { result = evalExprInstruction(e.getIndex()) }
310316

‎ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -591,13 +591,20 @@ class ReadNode extends InstructionNode {
591591
}
592592

593593
/**
594-
* A data-flow node that reads an element of an array, map, slice or string.
594+
* A data-flow node that reads the value of a field from a struct, or an element from an array, slice, map or string.
595595
*/
596-
class ElementReadNode extends ReadNode {
597-
override IR::ElementReadInstruction insn;
596+
class ComponentReadNode extends ReadNode {
597+
override IR::ComponentReadInstruction insn;
598598

599-
/** Gets the data-flow node representing the base from which the element is read. */
599+
/** Gets the data-flow node representing the base from which the field or element is read. */
600600
Node getBase() { result = instructionNode(insn.getBase()) }
601+
}
602+
603+
/**
604+
* A data-flow node that reads an element of an array, map, slice or string.
605+
*/
606+
class ElementReadNode extends ComponentReadNode {
607+
override IR::ElementReadInstruction insn;
601608

602609
/** Gets the data-flow node representing the index of the element being read. */
603610
Node getIndex() { result = instructionNode(insn.getIndex()) }
@@ -744,12 +751,9 @@ class AddressOperationNode extends UnaryOperationNode, ExprNode {
744751
/**
745752
* A data-flow node that reads the value of a field.
746753
*/
747-
class FieldReadNode extends ReadNode {
754+
class FieldReadNode extends ComponentReadNode {
748755
override IR::FieldReadInstruction insn;
749756

750-
/** Gets the base node from which the field is read. */
751-
Node getBase() { result = instructionNode(insn.getBase()) }
752-
753757
/** Gets the field this node reads. */
754758
Field getField() { result = insn.getField() }
755759

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

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
/** Provides models of commonly used functions and types in the protobuf packages. */
2+
3+
import go
4+
5+
/** Provides models of commonly used functions and types in the protobuf packages. */
6+
module Protobuf {
7+
/** Gets the name of the modern protobuf top-level implementation package. */
8+
string modernProtobufPackage() { result = "google.golang.org/protobuf/proto" }
9+
10+
/** Gets the name of the modern protobuf implementation's `protoiface` subpackage. */
11+
string protobufIfacePackage() { result = "google.golang.org/protobuf/runtime/protoiface" }
12+
13+
/** Gets the name of the modern protobuf implementation's `protoreflect` subpackage. */
14+
string protobufReflectPackage() { result = "google.golang.org/protobuf/reflect/protoreflect" }
15+
16+
/** Gets the name of a top-level protobuf implementation package. */
17+
string protobufPackages() {
18+
result in ["github.com/golang/protobuf/proto", modernProtobufPackage()]
19+
}
20+
21+
/** The `Marshal` and `MarshalAppend` functions in the protobuf packages. */
22+
private class MarshalFunction extends TaintTracking::FunctionModel, MarshalingFunction::Range {
23+
string name;
24+
25+
MarshalFunction() {
26+
name = ["Marshal", "MarshalAppend"] and
27+
(
28+
this.hasQualifiedName(protobufPackages(), name) or
29+
this.(Method).hasQualifiedName(modernProtobufPackage(), "MarshalOptions", name)
30+
)
31+
}
32+
33+
override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {
34+
inp = getAnInput() and outp = getOutput()
35+
}
36+
37+
override DataFlow::FunctionInput getAnInput() {
38+
if name = "MarshalAppend" then result.isParameter(1) else result.isParameter(0)
39+
}
40+
41+
override DataFlow::FunctionOutput getOutput() {
42+
name = "MarshalAppend" and result.isParameter(0)
43+
or
44+
result.isResult(0)
45+
}
46+
47+
override string getFormat() { result = "protobuf" }
48+
}
49+
50+
private Field inputMessageField() {
51+
result.hasQualifiedName(protobufIfacePackage(), "MarshalInput", "Message")
52+
}
53+
54+
private Method marshalStateMethod() {
55+
result.hasQualifiedName(protobufIfacePackage(), "MarshalOptions", "MarshalState")
56+
}
57+
58+
/**
59+
* Additional taint-flow step modelling flow from `MarshalInput.Message` to `MarshalOutput`,
60+
* mediated by a `MarshalOptions.MarshalState` call.
61+
*
62+
* Note we can taint the whole `MarshalOutput` as it only has one field (`Buf`), and taint-
63+
* tracking always considers a field of a tainted struct to itself be tainted.
64+
*/
65+
private class MarshalStateStep extends TaintTracking::AdditionalTaintStep {
66+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
67+
exists(DataFlow::PostUpdateNode marshalInput, DataFlow::CallNode marshalStateCall |
68+
marshalStateCall = marshalStateMethod().getACall() and
69+
// pred -> marshalInput.Message
70+
any(DataFlow::Write w)
71+
.writesField(marshalInput.getPreUpdateNode(), inputMessageField(), pred) and
72+
// marshalInput -> marshalStateCall
73+
marshalStateCall.getArgument(0) = globalValueNumber(marshalInput).getANode() and
74+
// marshalStateCall -> succ
75+
marshalStateCall.getResult() = succ
76+
)
77+
}
78+
}
79+
80+
/** The `Unmarshal` function in the protobuf packages. */
81+
class UnmarshalFunction extends TaintTracking::FunctionModel, UnmarshalingFunction::Range {
82+
UnmarshalFunction() {
83+
this.hasQualifiedName(protobufPackages(), "Unmarshal") or
84+
this.(Method).hasQualifiedName(modernProtobufPackage(), "UnmarshalOptions", "Unmarshal")
85+
}
86+
87+
override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {
88+
inp = getAnInput() and outp = getOutput()
89+
}
90+
91+
override DataFlow::FunctionInput getAnInput() { result.isParameter(0) }
92+
93+
override DataFlow::FunctionOutput getOutput() { result.isParameter(1) }
94+
95+
override string getFormat() { result = "protobuf" }
96+
}
97+
98+
/** The `Merge` function in the protobuf packages. */
99+
private class MergeFunction extends TaintTracking::FunctionModel {
100+
MergeFunction() { this.hasQualifiedName(protobufPackages(), "Merge") }
101+
102+
override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
103+
inp.isParameter(1) and outp.isParameter(0)
104+
}
105+
}
106+
107+
/** A protobuf `Message` type. */
108+
class MessageType extends Type {
109+
MessageType() { this.implements(protobufReflectPackage(), "ProtoMessage") }
110+
}
111+
112+
/** The `Clone` function in the protobuf packages. */
113+
private class MessageCloneFunction extends TaintTracking::FunctionModel {
114+
MessageCloneFunction() { this.hasQualifiedName(protobufPackages(), "Clone") }
115+
116+
override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
117+
inp.isParameter(0) and outp.isResult()
118+
}
119+
}
120+
121+
/** A `Get` method of a protobuf `Message` type. */
122+
private class GetMethod extends DataFlow::FunctionModel, Method {
123+
GetMethod() {
124+
exists(string name | name.matches("Get%") | this = any(MessageType msg).getMethod(name))
125+
}
126+
127+
override predicate hasDataFlow(FunctionInput inp, FunctionOutput outp) {
128+
inp.isReceiver() and outp.isResult()
129+
}
130+
}
131+
132+
/** A `ProtoReflect` method of a protobuf `Message` type. */
133+
private class ProtoReflectMethod extends DataFlow::FunctionModel, Method {
134+
ProtoReflectMethod() { this = any(MessageType msg).getMethod("ProtoReflect") }
135+
136+
override predicate hasDataFlow(FunctionInput inp, FunctionOutput outp) {
137+
inp.isReceiver() and outp.isResult()
138+
}
139+
}
140+
141+
/**
142+
* Gets the base of `node`, looking through any dereference node found.
143+
*/
144+
private DataFlow::Node getBaseLookingThroughDerefs(DataFlow::ComponentReadNode node) {
145+
result = node.getBase().(DataFlow::PointerDereferenceNode).getOperand()
146+
or
147+
result = node.getBase() and not node.getBase() instanceof DataFlow::PointerDereferenceNode
148+
}
149+
150+
/**
151+
* Gets the data-flow node representing the bottom of a stack of zero or more `ComponentReadNode`s
152+
* perhaps with interleaved dereferences.
153+
*
154+
* For example, in the expression a.b[c].d[e], this would return the dataflow node for the read from `a`.
155+
*/
156+
private DataFlow::Node getUnderlyingNode(DataFlow::ReadNode read) {
157+
(result = read or result = getBaseLookingThroughDerefs+(read)) and
158+
not result instanceof DataFlow::ComponentReadNode
159+
}
160+
161+
/**
162+
* Additional taint step tainting a Message when taint is written to any of its fields and/or elements.
163+
*/
164+
private class WriteMessageFieldStep extends TaintTracking::AdditionalTaintStep {
165+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
166+
[succ.getType(), succ.getType().getPointerType()] instanceof MessageType and
167+
exists(DataFlow::ReadNode base |
168+
succ.(DataFlow::PostUpdateNode).getPreUpdateNode() = getUnderlyingNode(base)
169+
|
170+
any(DataFlow::Write w).writesComponent(base, pred)
171+
)
172+
}
173+
}
174+
}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
| testDeprecatedApi.go:22:22:22:41 | call to getUntrustedString : string | testDeprecatedApi.go:26:12:26:21 | serialized |
2+
| testDeprecatedApi.go:31:22:31:41 | call to getUntrustedString : string | testDeprecatedApi.go:37:12:37:21 | serialized |
3+
| testDeprecatedApi.go:41:25:41:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:45:13:45:29 | selection of Description |
4+
| testDeprecatedApi.go:49:25:49:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:53:13:53:34 | call to GetDescription |
5+
| testDeprecatedApi.go:58:23:58:42 | call to getUntrustedString : string | testDeprecatedApi.go:65:12:65:21 | serialized |
6+
| testDeprecatedApi.go:70:14:70:33 | call to getUntrustedString : string | testDeprecatedApi.go:77:12:77:21 | serialized |
7+
| testDeprecatedApi.go:85:24:85:43 | call to getUntrustedString : string | testDeprecatedApi.go:89:12:89:21 | serialized |
8+
| testDeprecatedApi.go:93:25:93:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:97:13:97:31 | selection of Msg |
9+
| testDeprecatedApi.go:104:22:104:41 | call to getUntrustedString : string | testDeprecatedApi.go:105:13:105:20 | selection of Id |
10+
| testDeprecatedApi.go:112:22:112:41 | call to getUntrustedString : string | testDeprecatedApi.go:117:12:117:21 | serialized |
11+
| testDeprecatedApi.go:133:29:133:48 | call to getUntrustedString : string | testDeprecatedApi.go:137:12:137:21 | serialized |
12+
| testDeprecatedApi.go:143:20:143:39 | call to getUntrustedString : string | testDeprecatedApi.go:148:12:148:21 | serialized |
13+
| testDeprecatedApi.go:152:25:152:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:157:13:157:36 | index expression |
14+
| testDeprecatedApi.go:161:25:161:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:168:13:168:25 | index expression |
15+
| testModernApi.go:11:22:11:41 | call to getUntrustedString : string | testModernApi.go:15:12:15:21 | serialized |
16+
| testModernApi.go:20:22:20:41 | call to getUntrustedString : string | testModernApi.go:26:12:26:21 | serialized |
17+
| testModernApi.go:30:25:30:43 | call to getUntrustedBytes : slice type | testModernApi.go:34:13:34:29 | selection of Description |
18+
| testModernApi.go:38:25:38:43 | call to getUntrustedBytes : slice type | testModernApi.go:42:13:42:34 | call to GetDescription |
19+
| testModernApi.go:47:23:47:42 | call to getUntrustedString : string | testModernApi.go:54:12:54:21 | serialized |
20+
| testModernApi.go:59:22:59:41 | call to getUntrustedString : string | testModernApi.go:64:12:64:21 | serialized |
21+
| testModernApi.go:71:22:71:41 | call to getUntrustedString : string | testModernApi.go:77:12:77:21 | serialized |
22+
| testModernApi.go:98:14:98:33 | call to getUntrustedString : string | testModernApi.go:105:12:105:21 | serialized |
23+
| testModernApi.go:113:24:113:43 | call to getUntrustedString : string | testModernApi.go:117:12:117:21 | serialized |
24+
| testModernApi.go:121:25:121:43 | call to getUntrustedBytes : slice type | testModernApi.go:125:13:125:31 | selection of Msg |
25+
| testModernApi.go:131:25:131:43 | call to getUntrustedBytes : slice type | testModernApi.go:135:13:135:29 | selection of Description |
26+
| testModernApi.go:142:22:142:41 | call to getUntrustedString : string | testModernApi.go:143:13:143:20 | selection of Id |
27+
| testModernApi.go:150:22:150:41 | call to getUntrustedString : string | testModernApi.go:155:12:155:21 | serialized |
28+
| testModernApi.go:190:29:190:48 | call to getUntrustedString : string | testModernApi.go:194:12:194:21 | serialized |
29+
| testModernApi.go:200:20:200:39 | call to getUntrustedString : string | testModernApi.go:205:12:205:21 | serialized |
30+
| testModernApi.go:209:25:209:43 | call to getUntrustedBytes : slice type | testModernApi.go:214:13:214:36 | index expression |
31+
| testModernApi.go:218:25:218:43 | call to getUntrustedBytes : slice type | testModernApi.go:225:13:225:25 | index expression |

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import go
2+
3+
class UntrustedFunction extends Function {
4+
UntrustedFunction() { this.getName() = ["getUntrustedString", "getUntrustedBytes"] }
5+
}
6+
7+
class UntrustedSource extends DataFlow::Node, UntrustedFlowSource::Range {
8+
UntrustedSource() { this = any(UntrustedFunction f).getACall() }
9+
}
10+
11+
class SinkFunction extends Function {
12+
SinkFunction() { this.getName() = ["sinkString", "sinkBytes"] }
13+
}
14+
15+
class TestConfig extends TaintTracking::Configuration {
16+
TestConfig() { this = "testconfig" }
17+
18+
override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource }
19+
20+
override predicate isSink(DataFlow::Node sink) {
21+
sink = any(SinkFunction f).getACall().getAnArgument()
22+
}
23+
}
24+
25+
from TaintTracking::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
26+
where config.hasFlowPath(source, sink)
27+
select source, sink

‎ql/test/library-tests/semmle/go/frameworks/Protobuf/go.mod

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
module codeql-go-tests/protobuf
2+
3+
go 1.14
4+
5+
require (
6+
github.com/golang/protobuf v1.4.2
7+
google.golang.org/protobuf v1.23.0
8+
)
9+

‎ql/test/library-tests/semmle/go/frameworks/Protobuf/protos/query.proto

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
syntax = "proto3";
2+
option go_package = "protos/query";
3+
4+
message Query {
5+
string description = 1;
6+
string id = 2;
7+
8+
enum Severity {
9+
ERROR = 0;
10+
WARNING = 1;
11+
}
12+
13+
message Alert {
14+
string msg = 1;
15+
int64 loc = 2;
16+
}
17+
18+
repeated Alert alerts = 4;
19+
20+
map<int32, string> keyValuePairs = 5;
21+
}
22+
23+
message QuerySuite {
24+
repeated Query queries = 1;
25+
}

0 commit comments

Comments
 (0)