Skip to content

Commit 48a52cf

Browse files
author
Sauyon Lee
authored
Merge pull request #437 from sauyon/goproxy
Model elazarl/goproxy
2 parents 93aaa74 + fb84df2 commit 48a52cf

28 files changed

Lines changed: 822 additions & 31 deletions

File tree

‎change-notes/2020-12-18-goproxy.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
lgtm,codescanning
2+
* Added support for the `github.com/elazarl/goproxy` package.
3+
* The query "Incomplete regular expression for hostnames" has been improved to recognize some cases
4+
when the regexp in question is guarding an HTTP error response, which will lead to fewer false
5+
positives.

‎ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,55 @@ predicate isIncompleteHostNameRegexpPattern(string pattern, string hostPart) {
3030
"(([():|?a-z0-9-]+(\\\\)?[.])?" + commonTLD() + ")" + ".*", 1)
3131
}
3232

33+
/** Holds if `b` sets the HTTP status code (represented by a pseudo-header named `status`) */
34+
predicate writesHttpError(ReachableBasicBlock b) {
35+
forex(HTTP::HeaderWrite hw |
36+
hw.getHeaderName() = "status" and hw.asInstruction().getBasicBlock() = b
37+
|
38+
exists(string code | code.matches("4__") or code.matches("5__") |
39+
hw.definesHeader("status", code)
40+
)
41+
)
42+
}
43+
44+
/** Holds if starting from `block` a status code representing an error is certainly set. */
45+
predicate onlyErrors(BasicBlock block) {
46+
writesHttpError(block)
47+
or
48+
forex(ReachableBasicBlock pred | pred = block.getAPredecessor() | onlyErrors(pred))
49+
}
50+
51+
/** Gets a node that refers to a handler that is considered to return an HTTP error. */
52+
DataFlow::Node getASafeHandler() {
53+
exists(Variable v |
54+
v.hasQualifiedName(ElazarlGoproxy::packagePath(), ["AlwaysReject", "RejectConnect"])
55+
|
56+
result = v.getARead()
57+
)
58+
or
59+
exists(Function f |
60+
onlyErrors(f.getFuncDecl().(ControlFlow::Root).getExitNode().getBasicBlock())
61+
|
62+
result = f.getARead()
63+
)
64+
}
65+
66+
/** Holds if `regexp` is used in a check before `handler` is called. */
67+
predicate regexpGuardsHandler(RegexpPattern regexp, HTTP::RequestHandler handler) {
68+
handler.guardedBy(DataFlow::exprNode(regexp.getAUse().asExpr().getParent*()))
69+
}
70+
71+
/** Holds if `regexp` guards an HTTP error write. */
72+
predicate regexpGuardsError(RegexpPattern regexp) {
73+
exists(ControlFlow::ConditionGuardNode cond, RegexpMatchFunction match, DataFlow::CallNode call |
74+
call.getTarget() = match and
75+
match.getRegexp(call) = regexp
76+
|
77+
cond.ensures(match.getResult().getNode(call).getASuccessor*(), true) and
78+
cond.dominates(any(ReachableBasicBlock b | writesHttpError(b)))
79+
)
80+
}
81+
3382
class Config extends DataFlow::Configuration {
3483
Config() { this = "IncompleteHostNameRegexp::Config" }
3584

@@ -47,7 +96,13 @@ class Config extends DataFlow::Configuration {
4796

4897
override predicate isSource(DataFlow::Node source) { isSource(source, _) }
4998

50-
override predicate isSink(DataFlow::Node sink) { sink instanceof RegexpPattern }
99+
override predicate isSink(DataFlow::Node sink) {
100+
sink instanceof RegexpPattern and
101+
forall(HTTP::RequestHandler handler | regexpGuardsHandler(sink, handler) |
102+
not handler = getASafeHandler()
103+
) and
104+
not regexpGuardsError(sink)
105+
}
51106
}
52107

53108
from Config c, DataFlow::PathNode source, DataFlow::PathNode sink, string hostPart

‎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.BeegoOrm
3434
import semmle.go.frameworks.Chi
3535
import semmle.go.frameworks.Couchbase
3636
import semmle.go.frameworks.Echo
37+
import semmle.go.frameworks.ElazarlGoproxy
3738
import semmle.go.frameworks.Email
3839
import semmle.go.frameworks.Encoding
3940
import semmle.go.frameworks.EvanphxJsonPatch

‎ql/src/semmle/go/concepts/HTTP.qll‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,17 @@ module HTTP {
6464
/** Gets the (lower-case) name of a header set by this definition. */
6565
string getHeaderName() { result = this.getName().getStringValue().toLowerCase() }
6666

67+
/** Gets the value of the header set by this definition. */
68+
string getHeaderValue() {
69+
result = this.getValue().getStringValue()
70+
or
71+
result = this.getValue().getIntValue().toString()
72+
}
73+
6774
/** Holds if this header write defines the header `header`. */
6875
predicate definesHeader(string header, string value) {
6976
header = this.getHeaderName() and
70-
value = this.getValue().getStringValue()
77+
value = this.getHeaderValue()
7178
}
7279

7380
/**
@@ -101,6 +108,9 @@ module HTTP {
101108
/** Gets the (lower-case) name of a header set by this definition. */
102109
string getHeaderName() { result = self.getHeaderName() }
103110

111+
/** Gets the value of the header set by this definition. */
112+
string getHeaderValue() { result = self.getHeaderValue() }
113+
104114
/** Holds if this header write defines the header `header`. */
105115
predicate definesHeader(string header, string value) { self.definesHeader(header, value) }
106116

@@ -302,4 +312,33 @@ module HTTP {
302312
/** Gets the response writer that this redirect is sent on, if any. */
303313
ResponseWriter getResponseWriter() { result = self.getResponseWriter() }
304314
}
315+
316+
/** Provides a class for modeling new HTTP handler APIs. */
317+
module RequestHandler {
318+
/**
319+
* An HTTP request handler.
320+
*
321+
* Extend this class to model new APIs. If you want to refine existing API models,
322+
* extend `HTTP::RequestHandler` instead.
323+
*/
324+
abstract class Range extends DataFlow::Node {
325+
/** Gets a node that is used in a check that is tested before this handler is run. */
326+
abstract predicate guardedBy(DataFlow::Node check);
327+
}
328+
}
329+
330+
/**
331+
* An HTTP request handler.
332+
*
333+
* Extend this class to refine existing API models. If you want to model new APIs,
334+
* extend `HTTP::RequestHandler::Range` instead.
335+
*/
336+
class RequestHandler extends DataFlow::Node {
337+
RequestHandler::Range self;
338+
339+
RequestHandler() { this = self }
340+
341+
/** Gets a node that is used in a check that is tested before this handler is run. */
342+
predicate guardedBy(DataFlow::Node check) { self.guardedBy(check) }
343+
}
305344
}

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

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
/**
2+
* Provides classes for working with concepts relating to the [github.com/elazarl/goproxy](https://pkg.go.dev/github.com/elazarl/goproxy) package.
3+
*/
4+
5+
import go
6+
7+
/**
8+
* Provides classes for working with concepts relating to the [github.com/elazarl/goproxy](https://pkg.go.dev/github.com/elazarl/goproxy) package.
9+
*/
10+
module ElazarlGoproxy {
11+
/** Gets the package name. */
12+
bindingset[result]
13+
string packagePath() { result = package("github.com/elazarl/goproxy", "") }
14+
15+
private class NewResponse extends HTTP::HeaderWrite::Range, DataFlow::CallNode {
16+
NewResponse() { this.getTarget().hasQualifiedName(packagePath(), "NewResponse") }
17+
18+
override string getHeaderName() { this.definesHeader(result, _) }
19+
20+
override string getHeaderValue() { this.definesHeader(_, result) }
21+
22+
override DataFlow::Node getName() { none() }
23+
24+
override DataFlow::Node getValue() { result = this.getArgument([1, 2]) }
25+
26+
override predicate definesHeader(string header, string value) {
27+
header = "status" and value = this.getArgument(2).getIntValue().toString()
28+
or
29+
header = "content-type" and value = this.getArgument(1).getStringValue()
30+
}
31+
32+
override HTTP::ResponseWriter getResponseWriter() { none() }
33+
}
34+
35+
/** A body argument to a `NewResponse` call. */
36+
private class NewResponseBody extends HTTP::ResponseBody::Range {
37+
NewResponse call;
38+
39+
NewResponseBody() { this = call.getArgument(3) }
40+
41+
override DataFlow::Node getAContentTypeNode() { result = call.getArgument(1) }
42+
43+
override HTTP::ResponseWriter getResponseWriter() { none() }
44+
}
45+
46+
private class TextResponse extends HTTP::HeaderWrite::Range, DataFlow::CallNode {
47+
TextResponse() { this.getTarget().hasQualifiedName(packagePath(), "TextResponse") }
48+
49+
override string getHeaderName() { this.definesHeader(result, _) }
50+
51+
override string getHeaderValue() { this.definesHeader(_, result) }
52+
53+
override DataFlow::Node getName() { none() }
54+
55+
override DataFlow::Node getValue() { none() }
56+
57+
override predicate definesHeader(string header, string value) {
58+
header = "status" and value = "200"
59+
or
60+
header = "content-type" and value = "text/plain"
61+
}
62+
63+
override HTTP::ResponseWriter getResponseWriter() { none() }
64+
}
65+
66+
/** A body argument to a `TextResponse` call. */
67+
private class TextResponseBody extends HTTP::ResponseBody::Range, TextResponse {
68+
TextResponse call;
69+
70+
TextResponseBody() { this = call.getArgument(2) }
71+
72+
override DataFlow::Node getAContentTypeNode() { result = call.getArgument(1) }
73+
74+
override HTTP::ResponseWriter getResponseWriter() { none() }
75+
}
76+
77+
/** A handler attached to a goproxy proxy type. */
78+
private class ProxyHandler extends HTTP::RequestHandler::Range {
79+
DataFlow::MethodCallNode handlerReg;
80+
81+
ProxyHandler() {
82+
handlerReg
83+
.getTarget()
84+
.hasQualifiedName(packagePath(), "ReqProxyConds", ["Do", "DoFunc", "HandleConnect"]) and
85+
this = handlerReg.getArgument(0)
86+
}
87+
88+
override predicate guardedBy(DataFlow::Node check) {
89+
// note OnResponse is not modeled, as that server responses are not currently considered untrusted input
90+
exists(DataFlow::MethodCallNode onreqcall |
91+
onreqcall.getTarget().hasQualifiedName(packagePath(), "ProxyHttpServer", "OnRequest")
92+
|
93+
handlerReg.getReceiver() = onreqcall.getASuccessor*() and
94+
check = onreqcall.getArgument(0)
95+
)
96+
}
97+
}
98+
99+
private class UserControlledRequestData extends UntrustedFlowSource::Range {
100+
UserControlledRequestData() {
101+
exists(DataFlow::FieldReadNode frn | this = frn |
102+
// liberally consider ProxyCtx.UserData to be untrusted; it's a data field set by a request handler
103+
frn.getField().hasQualifiedName(packagePath(), "ProxyCtx", "UserData")
104+
)
105+
or
106+
exists(DataFlow::MethodCallNode call | this = call |
107+
call.getTarget().hasQualifiedName(packagePath(), "ProxyCtx", "Charset")
108+
)
109+
}
110+
}
111+
112+
private class ProxyLog extends LoggerCall::Range, DataFlow::MethodCallNode {
113+
ProxyLog() { this.getTarget().hasQualifiedName(packagePath(), "ProxyCtx", ["Logf", "Warnf"]) }
114+
115+
override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
116+
}
117+
118+
private class MethodModels extends TaintTracking::FunctionModel, Method {
119+
FunctionInput inp;
120+
FunctionOutput outp;
121+
122+
MethodModels() {
123+
// Methods:
124+
// signature: func CertStorage.Fetch(hostname string, gen func() (*tls.Certificate, error)) (*tls.Certificate, error)
125+
//
126+
// `hostname` excluded because if the cert storage or generator function themselves have not
127+
// been tainted, `hostname` would be unlikely to fetch user-controlled data
128+
this.hasQualifiedName(packagePath(), "CertStorage", "Fetch") and
129+
(inp.isReceiver() or inp.isParameter(1)) and
130+
outp.isResult(0)
131+
}
132+
133+
override predicate hasTaintFlow(FunctionInput i, FunctionOutput o) { i = inp and o = outp }
134+
}
135+
}

‎ql/src/semmle/go/frameworks/stdlib/NetHttp.qll‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -108,17 +108,25 @@ module NetHttp {
108108

109109
override string getHeaderName() { result = "status" }
110110

111-
override predicate definesHeader(string header, string value) {
112-
header = "status" and value = this.getValue().getIntValue().toString()
113-
}
114-
115111
override DataFlow::Node getName() { none() }
116112

117113
override DataFlow::Node getValue() { result = this.getArgument(0) }
118114

119115
override HTTP::ResponseWriter getResponseWriter() { result.getANode() = this.getReceiver() }
120116
}
121117

118+
private class ResponseErrorCall extends HTTP::HeaderWrite::Range, DataFlow::CallNode {
119+
ResponseErrorCall() { this.getTarget().hasQualifiedName("net/http", "Error") }
120+
121+
override string getHeaderName() { result = "status" }
122+
123+
override DataFlow::Node getName() { none() }
124+
125+
override DataFlow::Node getValue() { result = this.getArgument(2) }
126+
127+
override HTTP::ResponseWriter getResponseWriter() { result.getANode() = this.getArgument(0) }
128+
}
129+
122130
private class RequestBody extends HTTP::RequestBody::Range, DataFlow::ExprNode {
123131
RequestBody() {
124132
exists(Function newRequest |
@@ -227,6 +235,20 @@ module NetHttp {
227235
}
228236
}
229237

238+
private class Handler extends HTTP::RequestHandler::Range {
239+
DataFlow::CallNode handlerReg;
240+
241+
Handler() {
242+
exists(Function regFn | regFn = handlerReg.getTarget() |
243+
regFn.hasQualifiedName("net/http", ["Handle", "HandleFunc"]) or
244+
regFn.(Method).hasQualifiedName("net/http", "ServeMux", ["Handle", "HandleFunc"])
245+
) and
246+
this = handlerReg.getArgument(1)
247+
}
248+
249+
override predicate guardedBy(DataFlow::Node check) { check = handlerReg.getArgument(0) }
250+
}
251+
230252
private class FunctionModels extends TaintTracking::FunctionModel {
231253
FunctionInput inp;
232254
FunctionOutput outp;

‎ql/test/library-tests/semmle/go/concepts/HTTP/Handler.expected‎

Whitespace-only changes.

‎ql/test/library-tests/semmle/go/concepts/HTTP/Handler.ql‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import go
2+
import TestUtilities.InlineExpectationsTest
3+
4+
class HttpHandler extends InlineExpectationsTest {
5+
HttpHandler() { this = "httphandler" }
6+
7+
override string getARelevantTag() { result = "handler" }
8+
9+
override predicate hasActualResult(string file, int line, string element, string tag, string value) {
10+
tag = "handler" and
11+
exists(HTTP::RequestHandler h, DataFlow::Node check |
12+
element = h.toString() and value = check.toString()
13+
|
14+
h.hasLocationInfo(file, line, _, _, _) and
15+
h.guardedBy(check)
16+
)
17+
}
18+
}

‎ql/test/library-tests/semmle/go/concepts/HTTP/Header.expected‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1-
| main.go:29:2:29:19 | call to WriteHeader | n/a | 418 | status | 418 |
2-
| main.go:31:2:31:51 | call to Set | "Authorization" | "Basic example:example" | authorization | Basic example:example |
3-
| main.go:32:2:32:26 | call to Add | "Age" | "342232" | age | 342232 |
4-
| main.go:34:2:34:55 | call to Add | server | call to Sprintf | n/a | n/a |
5-
| main.go:35:2:35:45 | call to Set | LOC_HEADER | ...+... | n/a | n/a |
6-
| main.go:36:2:36:5 | head | "Unknown-Header" | slice literal | n/a | n/a |
7-
| main.go:48:2:48:43 | call to Add | "Not-A-Response" | "Header" | not-a-response | Header |
8-
| main.go:49:2:49:42 | call to Set | "Accept" | "nota/response" | accept | nota/response |
9-
| main.go:50:2:50:11 | selection of Header | "Accept-Charset" | slice literal | n/a | n/a |
10-
| main.go:57:2:57:42 | call to Set | "This-Makes" | "No sense" | this-makes | No sense |
1+
| main.go:30:2:30:19 | call to WriteHeader | n/a | 418 | status | 418 |
2+
| main.go:32:2:32:51 | call to Set | "Authorization" | "Basic example:example" | authorization | Basic example:example |
3+
| main.go:33:2:33:26 | call to Add | "Age" | "342232" | age | 342232 |
4+
| main.go:35:2:35:55 | call to Add | server | call to Sprintf | n/a | n/a |
5+
| main.go:36:2:36:45 | call to Set | LOC_HEADER | ...+... | n/a | n/a |
6+
| main.go:37:2:37:5 | head | "Unknown-Header" | slice literal | n/a | n/a |
7+
| main.go:49:2:49:43 | call to Add | "Not-A-Response" | "Header" | not-a-response | Header |
8+
| main.go:50:2:50:42 | call to Set | "Accept" | "nota/response" | accept | nota/response |
9+
| main.go:51:2:51:11 | selection of Header | "Accept-Charset" | slice literal | n/a | n/a |
10+
| main.go:58:2:58:42 | call to Set | "This-Makes" | "No sense" | this-makes | No sense |

‎ql/test/library-tests/semmle/go/concepts/HTTP/RequestBody.expected‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
| main.go:46:57:46:59 | nil |
2-
| main.go:52:13:52:34 | call to NopCloser |
1+
| main.go:47:57:47:59 | nil |
2+
| main.go:53:13:53:34 | call to NopCloser |

0 commit comments

Comments
 (0)