Skip to content

Commit c11d63e

Browse files
smowtonsmowton
authored
Merge pull request github#11015 from smowton/smowton/fix/go-cleartext-logging-exclude-protobuf-getters
Go: exclude protobuf read steps from cleartext-logging query
2 parents c161bb5 + 3573e21 commit c11d63e

File tree

17 files changed

+1559
-6
lines changed

17 files changed

+1559
-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: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,12 @@ module CleartextLogging {
4848
write.writesField(trg.(DataFlow::PostUpdateNode).getPreUpdateNode(), _, src)
4949
)
5050
or
51-
// taint steps that do not include flow through fields
52-
TaintTracking::localTaintStep(src, trg) and not TaintTracking::fieldReadStep(src, trg)
51+
// taint steps that do not include flow through fields. Field reads would produce FPs due to
52+
// the additional taint step above that taints whole structs from individual field writes.
53+
TaintTracking::localTaintStep(src, trg) and
54+
not TaintTracking::fieldReadStep(src, trg) and
55+
// Also exclude protobuf field fetches, since they amount to single field reads.
56+
not any(Protobuf::GetMethod gm).taintStep(src, trg)
5357
}
5458
}
5559
}

‎go/ql/src/change-notes/2022-10-28-protobuf-cleartext-logging.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Query `go/clear-text-logging` now excludes `GetX` methods of protobuf `Message` structs, except where taint is specifically known to belong to the right field. This is to avoid FPs where taint is written to one field and then spuriously read from another.

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,16 @@ 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:12:2:12:6 | implicit dereference [Description] : string | protobuf.go:11:2:11:6 | definition of query [pointer, Description] : string |
30+
| protobuf.go:12:2:12:6 | query [pointer, Description] : string | protobuf.go:12:2:12:6 | implicit dereference [Description] : string |
31+
| protobuf.go:12:22:12:29 | password : string | protobuf.go:12:2:12:6 | implicit dereference [Description] : string |
32+
| protobuf.go:14:14:14:18 | query [pointer, Description] : string | protobuf.go:14:14:14:35 | call to GetDescription |
33+
| 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 |
34+
| 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 |
35+
| 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 |
36+
| 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 |
2737
| util.go:16:9:16:18 | selection of password : string | passwords.go:28:14:28:28 | call to getPassword |
2838
nodes
2939
| klog.go:20:30:20:37 | selection of Header : Header | semmle.label | selection of Header : Header |
@@ -77,8 +87,19 @@ nodes
7787
| passwords.go:126:14:126:21 | selection of x | semmle.label | selection of x |
7888
| passwords.go:127:14:127:19 | config [y] : string | semmle.label | config [y] : string |
7989
| passwords.go:127:14:127:21 | selection of y | semmle.label | selection of y |
90+
| protobuf.go:11:2:11:6 | definition of query [pointer, Description] : string | semmle.label | definition of query [pointer, Description] : string |
91+
| protobuf.go:12:2:12:6 | implicit dereference [Description] : string | semmle.label | implicit dereference [Description] : string |
92+
| protobuf.go:12:2:12:6 | query [pointer, Description] : string | semmle.label | query [pointer, Description] : string |
93+
| protobuf.go:12:22:12:29 | password : string | semmle.label | password : string |
94+
| protobuf.go:14:14:14:18 | query [pointer, Description] : string | semmle.label | query [pointer, Description] : string |
95+
| protobuf.go:14:14:14:35 | call to GetDescription | semmle.label | call to GetDescription |
96+
| protos/query/query.pb.go:117:7:117:7 | definition of x [pointer, Description] : string | semmle.label | definition of x [pointer, Description] : string |
97+
| protos/query/query.pb.go:119:10:119:10 | implicit dereference [Description] : string | semmle.label | implicit dereference [Description] : string |
98+
| protos/query/query.pb.go:119:10:119:10 | x [pointer, Description] : string | semmle.label | x [pointer, Description] : string |
99+
| protos/query/query.pb.go:119:10:119:22 | selection of Description : string | semmle.label | selection of Description : string |
80100
| util.go:16:9:16:18 | selection of password : string | semmle.label | selection of password : string |
81101
subpaths
102+
| 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 |
82103
#select
83104
| 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 |
84105
| 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 +132,4 @@ subpaths
111132
| 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 |
112133
| 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 |
113134
| 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 |
135+
| 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 |

‎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)