Skip to content

Commit 0c6c135

Browse files
smowtonsmowton
committed
Go: exclude protobuf read steps from cleartext-logging query
This query already treats structs differently to usual: it includes field -> whole struct taint steps, but explicitly excludes struct -> field steps. This means that a logging framework sinking an entire struct with a tainted field yields an alert, but we don't get FPs caused by writing field `x` but then reading field `y`. However, protobuf messages have a special treatment, with taint usually associated with the whole struct and getter methods propagating that taint out. Suppressing these getter method steps specifically for the cleartext-logging query mirrors its treatment of structs in general and avoids this sort of field-mismatch FP. On the downside we will miss same-field propagation like `m.field = password; Log(m.GetField())` if we don't have source code for the implementation of `m`. However this is hopefully unusual since the typical use of protobufs is to serialize and deserialize, rather than using the struct as a general-purpose datastructure.
1 parent 85790fc commit 0c6c135

File tree

16 files changed

+1570
-6
lines changed

16 files changed

+1570
-6
lines changed

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,21 +123,21 @@ module Protobuf {
123123
}
124124

125125
/** A `Get` method of a protobuf `Message` type. */
126-
private class GetMethod extends DataFlow::FunctionModel, Method {
126+
class GetMethod extends TaintTracking::FunctionModel, Method {
127127
GetMethod() {
128128
exists(string name | name.matches("Get%") | this = any(MessageType msg).getMethod(name))
129129
}
130130

131-
override predicate hasDataFlow(FunctionInput inp, FunctionOutput outp) {
131+
override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
132132
inp.isReceiver() and outp.isResult()
133133
}
134134
}
135135

136136
/** A `ProtoReflect` method of a protobuf `Message` type. */
137-
private class ProtoReflectMethod extends DataFlow::FunctionModel, Method {
137+
private class ProtoReflectMethod extends TaintTracking::FunctionModel, Method {
138138
ProtoReflectMethod() { this = any(MessageType msg).getMethod("ProtoReflect") }
139139

140-
override predicate hasDataFlow(FunctionInput inp, FunctionOutput outp) {
140+
override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
141141
inp.isReceiver() and outp.isResult()
142142
}
143143
}

‎go/ql/lib/semmle/go/security/CleartextLogging.qll

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
*/
99

1010
import go
11+
private import semmle.go.frameworks.Protobuf
1112

1213
/**
1314
* Provides a data-flow tracking configuration for reasoning about
@@ -48,8 +49,12 @@ module CleartextLogging {
4849
write.writesField(trg.(DataFlow::PostUpdateNode).getPreUpdateNode(), _, src)
4950
)
5051
or
51-
// taint steps that do not include flow through fields
52-
TaintTracking::localTaintStep(src, trg) and not TaintTracking::fieldReadStep(src, trg)
52+
// taint steps that do not include flow through fields. Field reads would produce FPs due to
53+
// the additional taint step above that taints whole structs from individual field writes.
54+
TaintTracking::localTaintStep(src, trg) and
55+
not TaintTracking::fieldReadStep(src, trg) and
56+
// Also exclude protobuf field fetches, since they amount to single field reads.
57+
not any(Protobuf::GetMethod gm).taintStep(src, trg)
5358
}
5459
}
5560
}

‎go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,25 @@ edges
2424
| passwords.go:122:13:122:25 | call to getPassword : string | passwords.go:125:14:125:19 | config |
2525
| passwords.go:126:14:126:19 | config [x] : string | passwords.go:126:14:126:21 | selection of x |
2626
| passwords.go:127:14:127:19 | config [y] : string | passwords.go:127:14:127:21 | selection of y |
27+
| protobuf.go:11:2:11:6 | definition of query [pointer, Description] : string | protobuf.go:12:2:12:6 | query [pointer, Description] : string |
28+
| protobuf.go:11:2:11:6 | definition of query [pointer, Description] : string | protobuf.go:14:14:14:18 | query [pointer, Description] : string |
29+
| protobuf.go:11:2:11:6 | definition of query [pointer] : Query | protobuf.go:12:2:12:6 | query [pointer] : Query |
30+
| protobuf.go:12:2:12:6 | implicit dereference : Query | protobuf.go:11:2:11:6 | definition of query [pointer] : Query |
31+
| protobuf.go:12:2:12:6 | implicit dereference : Query | protobuf.go:12:2:12:6 | implicit dereference : Query |
32+
| protobuf.go:12:2:12:6 | implicit dereference : Query | protobuf.go:14:14:14:35 | call to GetDescription |
33+
| protobuf.go:12:2:12:6 | implicit dereference : Query | protobuf.go:15:14:15:26 | call to GetId |
34+
| protobuf.go:12:2:12:6 | implicit dereference [Description] : string | protobuf.go:11:2:11:6 | definition of query [pointer, Description] : string |
35+
| protobuf.go:12:2:12:6 | query [pointer, Description] : string | protobuf.go:12:2:12:6 | implicit dereference [Description] : string |
36+
| protobuf.go:12:2:12:6 | query [pointer] : Query | protobuf.go:12:2:12:6 | implicit dereference : Query |
37+
| protobuf.go:12:22:12:29 | password : string | protobuf.go:12:2:12:6 | implicit dereference : Query |
38+
| protobuf.go:12:22:12:29 | password : string | protobuf.go:12:2:12:6 | implicit dereference [Description] : string |
39+
| protobuf.go:12:22:12:29 | password : string | protobuf.go:14:14:14:35 | call to GetDescription |
40+
| protobuf.go:12:22:12:29 | password : string | protobuf.go:15:14:15:26 | call to GetId |
41+
| protobuf.go:14:14:14:18 | query [pointer, Description] : string | protobuf.go:14:14:14:35 | call to GetDescription |
42+
| protobuf.go:14:14:14:18 | query [pointer, Description] : string | protos/query/query.pb.go:117:7:117:7 | definition of x [pointer, Description] : string |
43+
| protos/query/query.pb.go:117:7:117:7 | definition of x [pointer, Description] : string | protos/query/query.pb.go:119:10:119:10 | x [pointer, Description] : string |
44+
| protos/query/query.pb.go:119:10:119:10 | implicit dereference [Description] : string | protos/query/query.pb.go:119:10:119:22 | selection of Description : string |
45+
| protos/query/query.pb.go:119:10:119:10 | x [pointer, Description] : string | protos/query/query.pb.go:119:10:119:10 | implicit dereference [Description] : string |
2746
| util.go:16:9:16:18 | selection of password : string | passwords.go:28:14:28:28 | call to getPassword |
2847
nodes
2948
| klog.go:20:30:20:37 | selection of Header : Header | semmle.label | selection of Header : Header |
@@ -77,8 +96,23 @@ nodes
7796
| passwords.go:126:14:126:21 | selection of x | semmle.label | selection of x |
7897
| passwords.go:127:14:127:19 | config [y] : string | semmle.label | config [y] : string |
7998
| passwords.go:127:14:127:21 | selection of y | semmle.label | selection of y |
99+
| protobuf.go:11:2:11:6 | definition of query [pointer, Description] : string | semmle.label | definition of query [pointer, Description] : string |
100+
| protobuf.go:11:2:11:6 | definition of query [pointer] : Query | semmle.label | definition of query [pointer] : Query |
101+
| protobuf.go:12:2:12:6 | implicit dereference : Query | semmle.label | implicit dereference : Query |
102+
| protobuf.go:12:2:12:6 | implicit dereference [Description] : string | semmle.label | implicit dereference [Description] : string |
103+
| protobuf.go:12:2:12:6 | query [pointer, Description] : string | semmle.label | query [pointer, Description] : string |
104+
| protobuf.go:12:2:12:6 | query [pointer] : Query | semmle.label | query [pointer] : Query |
105+
| protobuf.go:12:22:12:29 | password : string | semmle.label | password : string |
106+
| protobuf.go:14:14:14:18 | query [pointer, Description] : string | semmle.label | query [pointer, Description] : string |
107+
| protobuf.go:14:14:14:35 | call to GetDescription | semmle.label | call to GetDescription |
108+
| protobuf.go:15:14:15:26 | call to GetId | semmle.label | call to GetId |
109+
| protos/query/query.pb.go:117:7:117:7 | definition of x [pointer, Description] : string | semmle.label | definition of x [pointer, Description] : string |
110+
| protos/query/query.pb.go:119:10:119:10 | implicit dereference [Description] : string | semmle.label | implicit dereference [Description] : string |
111+
| protos/query/query.pb.go:119:10:119:10 | x [pointer, Description] : string | semmle.label | x [pointer, Description] : string |
112+
| protos/query/query.pb.go:119:10:119:22 | selection of Description : string | semmle.label | selection of Description : string |
80113
| util.go:16:9:16:18 | selection of password : string | semmle.label | selection of password : string |
81114
subpaths
115+
| protobuf.go:14:14:14:18 | query [pointer, Description] : string | protos/query/query.pb.go:117:7:117:7 | definition of x [pointer, Description] : string | protos/query/query.pb.go:119:10:119:22 | selection of Description : string | protobuf.go:14:14:14:35 | call to GetDescription : string |
82116
#select
83117
| klog.go:22:15:22:20 | header | klog.go:20:30:20:37 | selection of Header : Header | klog.go:22:15:22:20 | header | $@ flows to a logging call. | klog.go:20:30:20:37 | selection of Header | Sensitive data returned by HTTP request headers |
84118
| klog.go:28:13:28:41 | call to Get | klog.go:28:13:28:20 | selection of Header : Header | klog.go:28:13:28:41 | call to Get | $@ flows to a logging call. | klog.go:28:13:28:20 | selection of Header | Sensitive data returned by HTTP request headers |
@@ -111,3 +145,5 @@ subpaths
111145
| passwords.go:125:14:125:19 | config | passwords.go:122:13:122:25 | call to getPassword : string | passwords.go:125:14:125:19 | config | $@ flows to a logging call. | passwords.go:122:13:122:25 | call to getPassword | Sensitive data returned by a call to getPassword |
112146
| passwords.go:126:14:126:21 | selection of x | passwords.go:121:13:121:20 | password : string | passwords.go:126:14:126:21 | selection of x | $@ flows to a logging call. | passwords.go:121:13:121:20 | password | Sensitive data returned by an access to password |
113147
| passwords.go:127:14:127:21 | selection of y | passwords.go:122:13:122:25 | call to getPassword : string | passwords.go:127:14:127:21 | selection of y | $@ flows to a logging call. | passwords.go:122:13:122:25 | call to getPassword | Sensitive data returned by a call to getPassword |
148+
| protobuf.go:14:14:14:35 | call to GetDescription | protobuf.go:12:22:12:29 | password : string | protobuf.go:14:14:14:35 | call to GetDescription | $@ flows to a logging call. | protobuf.go:12:22:12:29 | password | Sensitive data returned by an access to password |
149+
| protobuf.go:15:14:15:26 | call to GetId | protobuf.go:12:22:12:29 | password : string | protobuf.go:15:14:15:26 | call to GetId | $@ flows to a logging call. | protobuf.go:12:22:12:29 | password | Sensitive data returned by an access to password |

‎go/ql/test/query-tests/Security/CWE-312/go.mod

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,6 @@ require (
66
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b
77
github.com/sirupsen/logrus v1.5.0
88
k8s.io/klog v1.0.0
9+
github.com/golang/protobuf v1.4.2
10+
google.golang.org/protobuf v1.23.0
911
)

‎go/ql/test/query-tests/Security/CWE-312/protobuf.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package main
2+
3+
import (
4+
"log"
5+
"main/protos/query"
6+
)
7+
8+
func testProtobuf() {
9+
password := "P@ssw0rd"
10+
11+
query := &query.Query{}
12+
query.Description = password
13+
14+
log.Println(query.GetDescription()) // NOT OK
15+
log.Println(query.GetId()) // OK
16+
}

‎go/ql/test/query-tests/Security/CWE-312/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)