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

Commit 0f637c5

Browse files
authored
Merge pull request #379 from smowton/model-revel
Model Revel
2 parents 7ddb289 + 0bf8064 commit 0f637c5

File tree

24 files changed

+1497
-5
lines changed

24 files changed

+1497
-5
lines changed

.github/workflows/codeqltest.yml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ jobs:
2020
echo "Done"
2121
cd $HOME
2222
echo "Downloading CodeQL CLI..."
23-
curl https://github.com/github/codeql-cli-binaries/releases/download/v2.3.0/codeql.zip -L -o codeql.zip
23+
curl https://github.com/github/codeql-cli-binaries/releases/download/v2.3.1/codeql.zip -L -o codeql.zip
2424
echo "Done"
2525
echo "Unpacking CodeQL CLI..."
2626
unzip -q codeql.zip
@@ -65,7 +65,7 @@ jobs:
6565
echo "Done"
6666
cd $HOME
6767
echo "Downloading CodeQL CLI..."
68-
curl https://github.com/github/codeql-cli-binaries/releases/download/v2.2.5/codeql.zip -L -o codeql.zip
68+
curl https://github.com/github/codeql-cli-binaries/releases/download/v2.3.1/codeql.zip -L -o codeql.zip
6969
echo "Done"
7070
echo "Unpacking CodeQL CLI..."
7171
unzip -q codeql.zip
@@ -98,7 +98,7 @@ jobs:
9898
echo "Done"
9999
cd "$HOME"
100100
echo "Downloading CodeQL CLI..."
101-
Invoke-WebRequest -Uri https://github.com/github/codeql-cli-binaries/releases/download/v2.2.5/codeql.zip -OutFile codeql.zip
101+
Invoke-WebRequest -Uri https://github.com/github/codeql-cli-binaries/releases/download/v2.3.1/codeql.zip -OutFile codeql.zip
102102
echo "Done"
103103
echo "Unpacking CodeQL CLI..."
104104
Expand-Archive codeql.zip -DestinationPath $HOME

change-notes/2020-10-19-revel.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* Added basic support for the Revel web framework.

ql/src/go.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import semmle.go.frameworks.Macaron
4040
import semmle.go.frameworks.Mux
4141
import semmle.go.frameworks.NoSQL
4242
import semmle.go.frameworks.Protobuf
43+
import semmle.go.frameworks.Revel
4344
import semmle.go.frameworks.Spew
4445
import semmle.go.frameworks.SQL
4546
import semmle.go.frameworks.Stdlib

ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,10 @@ predicate defaultAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink) {
177177
localAdditionalTaintStep(src, sink)
178178
}
179179

180+
abstract class DefaultTaintSanitizer extends DataFlow::Node { }
181+
180182
/**
181183
* Holds if `node` should be a sanitizer in all global taint flow configurations
182184
* but not in local taint.
183185
*/
184-
predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
186+
predicate defaultTaintSanitizer(DataFlow::Node node) { node instanceof DefaultTaintSanitizer }

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

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
/**
2+
* Provides classes for working with untrusted flow sources from the `github.com/revel/revel` package.
3+
*/
4+
5+
import go
6+
private import semmle.go.security.OpenUrlRedirectCustomizations
7+
8+
module Revel {
9+
/** Gets the package name. */
10+
bindingset[result]
11+
string packagePath() { result = package(["github.com/revel", "github.com/robfig"], "revel") }
12+
13+
private class ControllerParams extends UntrustedFlowSource::Range, DataFlow::FieldReadNode {
14+
ControllerParams() {
15+
exists(Field f |
16+
this.readsField(_, f) and
17+
f.hasQualifiedName(packagePath(), "Controller", "Params")
18+
)
19+
}
20+
}
21+
22+
private class ParamsFixedSanitizer extends TaintTracking::DefaultTaintSanitizer,
23+
DataFlow::FieldReadNode {
24+
ParamsFixedSanitizer() {
25+
exists(Field f |
26+
this.readsField(_, f) and
27+
f.hasQualifiedName(packagePath(), "Params", "Fixed")
28+
)
29+
}
30+
}
31+
32+
private class ParamsBind extends TaintTracking::FunctionModel, Method {
33+
ParamsBind() { this.hasQualifiedName(packagePath(), "Params", ["Bind", "BindJSON"]) }
34+
35+
override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
36+
inp.isReceiver() and outp.isParameter(0)
37+
}
38+
}
39+
40+
private class RouteMatchParams extends UntrustedFlowSource::Range, DataFlow::FieldReadNode {
41+
RouteMatchParams() {
42+
exists(Field f |
43+
this.readsField(_, f) and
44+
f.hasQualifiedName(packagePath(), "RouteMatch", "Params")
45+
)
46+
}
47+
}
48+
49+
/** An access to an HTTP request field whose value may be controlled by an untrusted user. */
50+
private class UserControlledRequestField extends UntrustedFlowSource::Range,
51+
DataFlow::FieldReadNode {
52+
UserControlledRequestField() {
53+
exists(string fieldName |
54+
this.getField().hasQualifiedName(packagePath(), "Request", fieldName)
55+
|
56+
fieldName in ["Header", "ContentType", "AcceptLanguages", "Locale", "URL", "Form",
57+
"MultipartForm"]
58+
)
59+
}
60+
}
61+
62+
private class UserControlledRequestMethod extends UntrustedFlowSource::Range,
63+
DataFlow::MethodCallNode {
64+
UserControlledRequestMethod() {
65+
this
66+
.getTarget()
67+
.hasQualifiedName(packagePath(), "Request",
68+
["FormValue", "PostFormValue", "GetQuery", "GetForm", "GetMultipartForm", "GetBody",
69+
"Cookie", "GetHttpHeader", "GetRequestURI", "MultipartReader", "Referer",
70+
"UserAgent"])
71+
}
72+
}
73+
74+
private class ServerCookieGetValue extends TaintTracking::FunctionModel, Method {
75+
ServerCookieGetValue() { this.hasQualifiedName(packagePath(), "ServerCookie", "GetValue") }
76+
77+
override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
78+
inp.isReceiver() and outp.isResult()
79+
}
80+
}
81+
82+
private class ServerMultipartFormGetFiles extends TaintTracking::FunctionModel, Method {
83+
ServerMultipartFormGetFiles() {
84+
this.hasQualifiedName(packagePath(), "ServerMultipartForm", ["GetFiles", "GetValues"])
85+
}
86+
87+
override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
88+
inp.isReceiver() and outp.isResult()
89+
}
90+
}
91+
92+
private string contentTypeFromFilename(DataFlow::Node filename) {
93+
if filename.getStringValue().toLowerCase().matches(["%.htm", "%.html"])
94+
then result = "text/html"
95+
else result = "application/octet-stream"
96+
// Actually Revel can figure out a variety of other content-types, but none of our analyses care to
97+
// distinguish ones other than text/html.
98+
}
99+
100+
/**
101+
* `revel.Controller` methods which set the response content-type to and designate a result in one operation.
102+
*
103+
* Note these don't actually generate the response, they return a struct which is then returned by the controller
104+
* method, but it is very likely if a string is being rendered that it will end up sent to the user.
105+
*
106+
* The `Render` and `RenderTemplate` methods are excluded for now because both execute HTML templates, and deciding
107+
* whether a particular value is exposed unescaped or not requires parsing the template.
108+
*
109+
* The `RenderError` method can actually return HTML content, but again only via an HTML template if one exists;
110+
* we assume it falls back to return plain text as this implies there is probably not an injection opportunity
111+
* but there is an information leakage issue.
112+
*
113+
* The `RenderBinary` method can also return a variety of content-types based on the file extension passed.
114+
* We look particularly for html file extensions, since these are the only ones we currently have special rules
115+
* for (in particular, detecting XSS vulnerabilities).
116+
*/
117+
private class ControllerRenderMethods extends HTTP::ResponseBody::Range {
118+
string contentType;
119+
120+
ControllerRenderMethods() {
121+
exists(Method m, string methodName, DataFlow::CallNode methodCall |
122+
m.hasQualifiedName(packagePath(), "Controller", methodName) and
123+
methodCall = m.getACall()
124+
|
125+
exists(int exposedArgument |
126+
this = methodCall.getArgument(exposedArgument) and
127+
(
128+
methodName = "RenderBinary" and
129+
contentType = contentTypeFromFilename(methodCall.getArgument(1)) and
130+
exposedArgument = 0
131+
or
132+
methodName = "RenderError" and contentType = "text/plain" and exposedArgument = 0
133+
or
134+
methodName = "RenderHTML" and contentType = "text/html" and exposedArgument = 0
135+
or
136+
methodName = "RenderJSON" and contentType = "application/json" and exposedArgument = 0
137+
or
138+
methodName = "RenderJSONP" and
139+
contentType = "application/javascript" and
140+
exposedArgument = 1
141+
or
142+
methodName = "RenderXML" and contentType = "text/xml" and exposedArgument = 0
143+
)
144+
)
145+
or
146+
methodName = "RenderText" and
147+
contentType = "text/plain" and
148+
this = methodCall.getAnArgument()
149+
)
150+
}
151+
152+
override HTTP::ResponseWriter getResponseWriter() { none() }
153+
154+
override string getAContentType() { result = contentType }
155+
}
156+
157+
/**
158+
* The `revel.Controller.RenderFileName` method, which instructs Revel to open a file and return its contents.
159+
* We extend FileSystemAccess rather than HTTP::ResponseBody as this will usually mean exposing a user-controlled
160+
* file rather than the actual contents being user-controlled.
161+
*/
162+
private class RenderFileNameCall extends FileSystemAccess::Range, DataFlow::CallNode {
163+
RenderFileNameCall() {
164+
this =
165+
any(Method m | m.hasQualifiedName(packagePath(), "Controller", "RenderFileName")).getACall()
166+
}
167+
168+
override DataFlow::Node getAPathArgument() { result = getArgument(0) }
169+
}
170+
171+
/**
172+
* The `revel.Controller.Redirect` method.
173+
*
174+
* It is currently assumed that a tainted `value` in `Redirect(url, value)`, which calls `Sprintf(url, value)`
175+
* internally, cannot lead to an open redirect vulnerability.
176+
*/
177+
private class ControllerRedirectMethod extends HTTP::Redirect::Range, DataFlow::CallNode {
178+
ControllerRedirectMethod() {
179+
exists(Method m | m.hasQualifiedName(packagePath(), "Controller", "Redirect") |
180+
this = m.getACall()
181+
)
182+
}
183+
184+
override DataFlow::Node getUrl() { result = this.getArgument(0) }
185+
186+
override HTTP::ResponseWriter getResponseWriter() { none() }
187+
}
188+
189+
/**
190+
* The getter and setter methods of `revel.RevelHeader`.
191+
*
192+
* Note we currently don't implement `HeaderWrite` and related concepts, as they are currently only used
193+
* to track content-type, and directly setting headers does not seem to be the usual way to set the response
194+
* content-type for this framework. If and when the `HeaderWrite` concept has a more abstract idea of the
195+
* relationship between header-writes and HTTP responses than looking for a particular `http.ResponseWriter`
196+
* instance connecting the two, then we may implement it here for completeness.
197+
*/
198+
private class RevelHeaderMethods extends TaintTracking::FunctionModel {
199+
FunctionInput input;
200+
FunctionOutput output;
201+
string name;
202+
203+
RevelHeaderMethods() {
204+
this.(Method).hasQualifiedName(packagePath(), "RevelHeader", name) and
205+
(
206+
name = ["Add", "Set"] and input.isParameter([0, 1]) and output.isReceiver()
207+
or
208+
name = ["Get", "GetAll"] and input.isReceiver() and output.isResult()
209+
or
210+
name = "SetCookie" and input.isParameter(0) and output.isReceiver()
211+
)
212+
}
213+
214+
override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
215+
inp = input and outp = output
216+
}
217+
}
218+
}

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,4 +271,28 @@ module WebSocketReader {
271271

272272
override FunctionOutput getAnOutput() { result.isResult(1) }
273273
}
274+
275+
/**
276+
* The `ServerWebSocket.MessageReceive` method of the `github.com/revel/revel` package.
277+
*/
278+
private class RevelServerWebSocketMessageReceive extends Range, Method {
279+
RevelServerWebSocketMessageReceive() {
280+
// func MessageReceive(v interface{}) error
281+
this.hasQualifiedName(Revel::packagePath(), "ServerWebSocket", "MessageReceive")
282+
}
283+
284+
override FunctionOutput getAnOutput() { result.isParameter(0) }
285+
}
286+
287+
/**
288+
* The `ServerWebSocket.MessageReceiveJSON` method of the `github.com/revel/revel` package.
289+
*/
290+
private class RevelServerWebSocketMessageReceiveJSON extends Range, Method {
291+
RevelServerWebSocketMessageReceiveJSON() {
292+
// func MessageReceiveJSON(v interface{}) error
293+
this.hasQualifiedName(Revel::packagePath(), "ServerWebSocket", "MessageReceiveJSON")
294+
}
295+
296+
override FunctionOutput getAnOutput() { result.isParameter(0) }
297+
}
274298
}

ql/src/semmle/go/security/OpenUrlRedirect.qll

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@ module OpenUrlRedirect {
3333
// taint steps that do not include flow through fields
3434
TaintTracking::localTaintStep(pred, succ) and not TaintTracking::fieldReadStep(pred, succ)
3535
or
36+
// explicit extra taint steps for this query
37+
any(AdditionalStep s).hasTaintStep(pred, succ)
38+
or
3639
// propagate to a URL when its host is assigned to
3740
exists(Write w, Field f, SsaWithFields v | f.hasQualifiedName("net/url", "URL", "Host") |
3841
w.writesField(v.getAUse(), f, pred) and succ = v.getAUse()

ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,14 @@ module OpenUrlRedirect {
3434
*/
3535
abstract class BarrierGuard extends DataFlow::BarrierGuard { }
3636

37+
/**
38+
* An additional taint propagation step specific to this query.
39+
*/
40+
bindingset[this]
41+
abstract class AdditionalStep extends string {
42+
abstract predicate hasTaintStep(DataFlow::Node pred, DataFlow::Node succ);
43+
}
44+
3745
/**
3846
* A source of third-party user input, considered as a flow source for URL redirects.
3947
*/
@@ -120,3 +128,20 @@ private class UnsafeFieldReadSanitizer extends SafeUrlFlow::SanitizerEdge {
120128
)
121129
}
122130
}
131+
132+
/**
133+
* Reinstate the usual field propagation rules for fields, which the OpenURLRedirect
134+
* query usually excludes, for fields of `Params` other than `Params.Fixed`.
135+
*/
136+
private class PropagateParamsFields extends OpenUrlRedirect::AdditionalStep {
137+
PropagateParamsFields() { this = "PropagateParamsFields" }
138+
139+
override predicate hasTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
140+
exists(Field f, string field |
141+
f.hasQualifiedName(Revel::packagePath(), "Params", field) and
142+
field != "Fixed"
143+
|
144+
succ.(Read).readsField(pred, f)
145+
)
146+
}
147+
}

ql/src/semmle/go/security/ReflectedXssCustomizations.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ module ReflectedXss {
5656
private predicate nonHtmlContentType(HTTP::ResponseBody body) {
5757
not htmlTypeSpecified(body) and
5858
(
59+
exists(body.getAContentType())
60+
or
5961
exists(body.getAContentTypeNode())
6062
or
6163
exists(DataFlow::CallNode call | call.getTarget().hasQualifiedName("fmt", "Fprintf") |
Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
11
import go
2-
import semmle.go.frameworks.Gin
32

43
select any(UntrustedFlowSource src)

0 commit comments

Comments
 (0)