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

Commit 9aceae8

Browse files
smowtonsmowton
committed
Revel: add support and tests for Render and Redirect sinks.
1 parent b2b8f10 commit 9aceae8

File tree

17 files changed

+430
-35
lines changed

17 files changed

+430
-35
lines changed

‎.github/workflows/codeqltest.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -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.0/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.0/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/semmle/go/frameworks/Revel.qll‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44

55
import go
6+
private import semmle.go.security.OpenUrlRedirectCustomizations
67

78
module Revel {
89
/** Gets the package name. */
@@ -28,6 +29,23 @@ module Revel {
2829
}
2930
}
3031

32+
/**
33+
* Reinstate the usual field propagation rules for fields, which the OpenURLRedirect
34+
* query usually excludes, for fields of `Params` other than `Params.Fixed`.
35+
*/
36+
private class PropagateParamsFields extends OpenUrlRedirect::AdditionalStep {
37+
PropagateParamsFields() { this = "PropagateParamsFields" }
38+
39+
override predicate hasTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
40+
exists(Field f, string field |
41+
f.hasQualifiedName(packagePath(), "Params", field) and
42+
field != "Fixed"
43+
|
44+
succ.(Read).readsField(pred, f)
45+
)
46+
}
47+
}
48+
3149
private class ParamsBind extends TaintTracking::FunctionModel, Method {
3250
ParamsBind() { this.hasQualifiedName(packagePath(), "Params", ["Bind", "BindJSON"]) }
3351

@@ -94,4 +112,101 @@ module Revel {
94112
inp.isReceiver() and outp.isResult(0)
95113
}
96114
}
115+
116+
private string contentTypeFromFilename(DataFlow::Node filename) {
117+
if filename.getStringValue().toLowerCase().matches(["%.htm", "%.html"])
118+
then result = "text/html"
119+
else result = "application/octet-stream"
120+
// Actually Revel can figure out a variety of other content-types, but none of our analyses care to
121+
// distinguish ones other than text/html.
122+
}
123+
124+
/**
125+
* `revel.Controller` methods which set the response content-type to and designate a result in one operation.
126+
*
127+
* Note these don't actually generate the response, they return a struct which is then returned by the controller
128+
* method, but it is very likely if a string is being rendered that it will end up sent to the user.
129+
*
130+
* The `Render` and `RenderTemplate` methods are excluded for now because both execute HTML templates, and deciding
131+
* whether a particular value is exposed unescaped or not requires parsing the template.
132+
*
133+
* The `RenderError` method can actually return HTML content, but again only via an HTML template if one exists;
134+
* we assume it falls back to return plain text as this implies there is probably not an injection opportunity
135+
* but there is an information leakage issue.
136+
*
137+
* The `RenderBinary` method can also return a variety of content-types based on the file extension passed.
138+
* We look particularly for html file extensions, since these are the only ones we currently have special rules
139+
* for (in particular, detecting XSS vulnerabilities).
140+
*/
141+
private class ControllerRenderMethods extends HTTP::ResponseBody::Range {
142+
string contentType;
143+
144+
ControllerRenderMethods() {
145+
exists(Method m, string methodName, DataFlow::CallNode methodCall |
146+
m.hasQualifiedName(packagePath(), "Controller", methodName) and
147+
methodCall = m.getACall()
148+
|
149+
exists(int exposedArgument |
150+
this = methodCall.getArgument(exposedArgument) and
151+
(
152+
methodName = "RenderBinary" and
153+
contentType = contentTypeFromFilename(methodCall.getArgument(1)) and
154+
exposedArgument = 0
155+
or
156+
methodName = "RenderError" and contentType = "text/plain" and exposedArgument = 0
157+
or
158+
methodName = "RenderHTML" and contentType = "text/html" and exposedArgument = 0
159+
or
160+
methodName = "RenderJSON" and contentType = "application/json" and exposedArgument = 0
161+
or
162+
methodName = "RenderJSONP" and
163+
contentType = "application/javascript" and
164+
exposedArgument = 1
165+
or
166+
methodName = "RenderXML" and contentType = "text/xml" and exposedArgument = 0
167+
)
168+
)
169+
or
170+
methodName = "RenderText" and
171+
contentType = "text/plain" and
172+
this = methodCall.getAnArgument()
173+
)
174+
}
175+
176+
override HTTP::ResponseWriter getResponseWriter() { none() }
177+
178+
override string getAContentType() { result = contentType }
179+
}
180+
181+
/**
182+
* The `revel.Controller.RenderFileName` method, which instructs Revel to open a file and return its contents.
183+
* We extend FileSystemAccess rather than HTTP::ResponseBody as this will usually mean exposing a user-controlled
184+
* file rather than the actual contents being user-controlled.
185+
*/
186+
private class IoUtilFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
187+
IoUtilFileSystemAccess() {
188+
this =
189+
any(Method m | m.hasQualifiedName(packagePath(), "Controller", "RenderFileName")).getACall()
190+
}
191+
192+
override DataFlow::Node getAPathArgument() { result = getArgument(0) }
193+
}
194+
195+
/**
196+
* The `revel.Controller.Redirect` method.
197+
*
198+
* For now I assume that in the context `Redirect(url, value)`, where Revel will `Sprintf(url, value)` internally,
199+
* it is very likely `url` imposes some mandatory prefix, so `value` isn't truly an open redirect opportunity.
200+
*/
201+
private class ControllerRedirectMethod extends HTTP::Redirect::Range, DataFlow::CallNode {
202+
ControllerRedirectMethod() {
203+
exists(Method m | m.hasQualifiedName(packagePath(), "Controller", "Redirect") |
204+
this = m.getACall()
205+
)
206+
}
207+
208+
override DataFlow::Node getUrl() { result = this.getArgument(0) }
209+
210+
override HTTP::ResponseWriter getResponseWriter() { none() }
211+
}
97212
}

‎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: 8 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
*/

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

‎ql/test/library-tests/semmle/go/frameworks/Revel/EndToEnd.go‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
package main
2+
3+
import (
4+
"bytes"
5+
"errors"
6+
staticControllers "github.com/revel/modules/static/app/controllers"
7+
"github.com/revel/revel"
8+
"os"
9+
"time"
10+
)
11+
12+
// Use typical inheritence pattern, per github.com/revel/examples/booking:
13+
14+
// Doesn't really matter which controller is used here since it'll be stubbed--
15+
// the important thing is we're inheriting a controller from a Revel module, which
16+
// itself implements revel.Controller.
17+
type MyApplication struct {
18+
staticControllers.Static
19+
}
20+
21+
// ...then it's common for files to inherit the whole-appliction controller.
22+
type MyRoute struct {
23+
MyApplication
24+
}
25+
26+
// Implement some request handlers on that Controller exhibiting some common problems:
27+
28+
func (c MyRoute) Handler1() revel.Result {
29+
// GOOD: the Render function is likely to properly escape the user-controlled parameter.
30+
return c.Render("someviewparam", c.Params.Form.Get("someField"))
31+
}
32+
33+
func (c MyRoute) Handler2() revel.Result {
34+
// BAD: the RenderBinary function copies an `io.Reader` to the user's browser.
35+
buf := &bytes.Buffer{}
36+
buf.WriteString(c.Params.Form.Get("someField"))
37+
return c.RenderBinary(buf, "index.html", revel.Inline, time.Now())
38+
}
39+
40+
func (c MyRoute) Handler3() revel.Result {
41+
// GOOD: the RenderBinary function copies an `io.Reader` to the user's browser, but the filename
42+
// means it will be given a safe content-type.
43+
buf := &bytes.Buffer{}
44+
buf.WriteString(c.Params.Form.Get("someField"))
45+
return c.RenderBinary(buf, "index.txt", revel.Inline, time.Now())
46+
}
47+
48+
func (c MyRoute) Handler4() revel.Result {
49+
// GOOD: the RenderError function either uses an HTML template with probable escaping,
50+
// or it uses content-type text/plain.
51+
err := errors.New(c.Params.Form.Get("someField"))
52+
return c.RenderError(err)
53+
}
54+
55+
func (c MyRoute) Handler5() revel.Result {
56+
// BAD: returning an arbitrary file (but this is detected at the os.Open call, not
57+
// due to modelling Revel)
58+
f, _ := os.Open(c.Params.Form.Get("someField"))
59+
return c.RenderFile(f, revel.Inline)
60+
}
61+
62+
func (c MyRoute) Handler6() revel.Result {
63+
// BAD: returning an arbitrary file (detected as a user-controlled file-op, not XSS)
64+
return c.RenderFileName(c.Params.Form.Get("someField"), revel.Inline)
65+
}
66+
67+
func (c MyRoute) Handler7() revel.Result {
68+
// BAD: straightforward XSS
69+
return c.RenderHTML(c.Params.Form.Get("someField"))
70+
}
71+
72+
func (c MyRoute) Handler8() revel.Result {
73+
// GOOD: uses JSON content-type
74+
return c.RenderJSON(c.Params.Form.Get("someField"))
75+
}
76+
77+
func (c MyRoute) Handler9() revel.Result {
78+
// GOOD: uses Javascript content-type
79+
return c.RenderJSONP("callback", c.Params.Form.Get("someField"))
80+
}
81+
82+
func (c MyRoute) Handler10() revel.Result {
83+
// GOOD: uses text content-type
84+
return c.RenderText(c.Params.Form.Get("someField"))
85+
}
86+
87+
func (c MyRoute) Handler11() revel.Result {
88+
// GOOD: uses xml content-type
89+
return c.RenderXML(c.Params.Form.Get("someField"))
90+
}
91+
92+
func (c MyRoute) Handler12() revel.Result {
93+
// BAD: open redirect
94+
return c.Redirect(c.Params.Form.Get("someField"))
95+
}

‎ql/test/library-tests/semmle/go/frameworks/Revel/OpenRedirect.expected‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
edges
2+
| EndToEnd.go:94:20:94:27 | implicit dereference : Params | EndToEnd.go:94:20:94:27 | implicit dereference : Params |
3+
| EndToEnd.go:94:20:94:27 | implicit dereference : Params | EndToEnd.go:94:20:94:27 | selection of Params : pointer type |
4+
| EndToEnd.go:94:20:94:27 | implicit dereference : Params | EndToEnd.go:94:20:94:49 | call to Get |
5+
| EndToEnd.go:94:20:94:27 | selection of Params : pointer type | EndToEnd.go:94:20:94:27 | implicit dereference : Params |
6+
| EndToEnd.go:94:20:94:27 | selection of Params : pointer type | EndToEnd.go:94:20:94:27 | selection of Params : pointer type |
7+
| EndToEnd.go:94:20:94:27 | selection of Params : pointer type | EndToEnd.go:94:20:94:49 | call to Get |
8+
nodes
9+
| EndToEnd.go:94:20:94:27 | implicit dereference : Params | semmle.label | implicit dereference : Params |
10+
| EndToEnd.go:94:20:94:27 | selection of Params : pointer type | semmle.label | selection of Params : pointer type |
11+
| EndToEnd.go:94:20:94:49 | call to Get | semmle.label | call to Get |
12+
#select
13+
| EndToEnd.go:94:20:94:49 | call to Get | EndToEnd.go:94:20:94:27 | selection of Params : pointer type | EndToEnd.go:94:20:94:49 | call to Get | Untrusted URL redirection due to $@. | EndToEnd.go:94:20:94:27 | selection of Params | user-provided value |

‎ql/test/library-tests/semmle/go/frameworks/Revel/OpenRedirect.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-601/OpenUrlRedirect.ql

‎ql/test/library-tests/semmle/go/frameworks/Revel/ReflectedXss.expected‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
edges
2+
| EndToEnd.go:36:18:36:25 | implicit dereference : Params | EndToEnd.go:36:18:36:25 | implicit dereference : Params |
3+
| EndToEnd.go:36:18:36:25 | implicit dereference : Params | EndToEnd.go:36:18:36:25 | selection of Params : pointer type |
4+
| EndToEnd.go:36:18:36:25 | implicit dereference : Params | EndToEnd.go:37:24:37:26 | buf |
5+
| EndToEnd.go:36:18:36:25 | selection of Params : pointer type | EndToEnd.go:36:18:36:25 | implicit dereference : Params |
6+
| EndToEnd.go:36:18:36:25 | selection of Params : pointer type | EndToEnd.go:36:18:36:25 | selection of Params : pointer type |
7+
| EndToEnd.go:36:18:36:25 | selection of Params : pointer type | EndToEnd.go:37:24:37:26 | buf |
8+
| EndToEnd.go:69:22:69:29 | implicit dereference : Params | EndToEnd.go:69:22:69:29 | implicit dereference : Params |
9+
| EndToEnd.go:69:22:69:29 | implicit dereference : Params | EndToEnd.go:69:22:69:29 | selection of Params : pointer type |
10+
| EndToEnd.go:69:22:69:29 | implicit dereference : Params | EndToEnd.go:69:22:69:51 | call to Get |
11+
| EndToEnd.go:69:22:69:29 | selection of Params : pointer type | EndToEnd.go:69:22:69:29 | implicit dereference : Params |
12+
| EndToEnd.go:69:22:69:29 | selection of Params : pointer type | EndToEnd.go:69:22:69:29 | selection of Params : pointer type |
13+
| EndToEnd.go:69:22:69:29 | selection of Params : pointer type | EndToEnd.go:69:22:69:51 | call to Get |
14+
nodes
15+
| EndToEnd.go:36:18:36:25 | implicit dereference : Params | semmle.label | implicit dereference : Params |
16+
| EndToEnd.go:36:18:36:25 | selection of Params : pointer type | semmle.label | selection of Params : pointer type |
17+
| EndToEnd.go:37:24:37:26 | buf | semmle.label | buf |
18+
| EndToEnd.go:69:22:69:29 | implicit dereference : Params | semmle.label | implicit dereference : Params |
19+
| EndToEnd.go:69:22:69:29 | selection of Params : pointer type | semmle.label | selection of Params : pointer type |
20+
| EndToEnd.go:69:22:69:51 | call to Get | semmle.label | call to Get |
21+
#select
22+
| EndToEnd.go:37:24:37:26 | buf | EndToEnd.go:36:18:36:25 | selection of Params : pointer type | EndToEnd.go:37:24:37:26 | buf | Cross-site scripting vulnerability due to $@. | EndToEnd.go:36:18:36:25 | selection of Params | user-provided value |
23+
| EndToEnd.go:69:22:69:51 | call to Get | EndToEnd.go:69:22:69:29 | selection of Params : pointer type | EndToEnd.go:69:22:69:51 | call to Get | Cross-site scripting vulnerability due to $@. | EndToEnd.go:69:22:69:29 | selection of Params | user-provided value |

0 commit comments

Comments
 (0)