diff --git a/ql/src/semmle/go/frameworks/Revel.qll b/ql/src/semmle/go/frameworks/Revel.qll index 4982b0314f8..878d5ff46f8 100644 --- a/ql/src/semmle/go/frameworks/Revel.qll +++ b/ql/src/semmle/go/frameworks/Revel.qll @@ -70,7 +70,8 @@ module Revel { exists(string fieldName | this.getField().hasQualifiedName(packagePath(), "Request", fieldName) | - fieldName in ["In", "Header", "URL", "Form", "MultipartForm"] + fieldName in ["Header", "ContentType", "AcceptLanguages", "Locale", "URL", "Form", + "MultipartForm"] ) } } @@ -81,13 +82,23 @@ module Revel { this .getTarget() .hasQualifiedName(packagePath(), "Request", - ["FormValue", "PostFormValue", "GetQuery", "GetForm", "GetMultipartForm", "GetBody"]) + ["FormValue", "PostFormValue", "GetQuery", "GetForm", "GetMultipartForm", "GetBody", + "Cookie", "GetHttpHeader", "GetRequestURI", "MultipartReader", "Referer", + "UserAgent"]) + } + } + + private class ServerCookieGetValue extends TaintTracking::FunctionModel, Method { + ServerCookieGetValue() { this.hasQualifiedName(packagePath(), "ServerCookie", "GetValue") } + + override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { + inp.isReceiver() and outp.isResult() } } private class ServerMultipartFormGetFiles extends TaintTracking::FunctionModel, Method { ServerMultipartFormGetFiles() { - this.hasQualifiedName(packagePath(), "ServerMultipartForm", "GetFiles") + this.hasQualifiedName(packagePath(), "ServerMultipartForm", ["GetFiles", "GetValues"]) } override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { @@ -95,24 +106,6 @@ module Revel { } } - private class ServerMultipartFormGetValues extends TaintTracking::FunctionModel, Method { - ServerMultipartFormGetValues() { - this.hasQualifiedName(packagePath(), "ServerMultipartForm", "GetValues") - } - - override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { - inp.isReceiver() and outp.isResult() - } - } - - private class ServerRequestGet extends TaintTracking::FunctionModel, Method { - ServerRequestGet() { this.hasQualifiedName(packagePath(), "ServerRequest", "Get") } - - override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { - inp.isReceiver() and outp.isResult(0) - } - } - private string contentTypeFromFilename(DataFlow::Node filename) { if filename.getStringValue().toLowerCase().matches(["%.htm", "%.html"]) then result = "text/html" @@ -183,8 +176,8 @@ module Revel { * We extend FileSystemAccess rather than HTTP::ResponseBody as this will usually mean exposing a user-controlled * file rather than the actual contents being user-controlled. */ - private class IoUtilFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode { - IoUtilFileSystemAccess() { + private class RenderFileNameCall extends FileSystemAccess::Range, DataFlow::CallNode { + RenderFileNameCall() { this = any(Method m | m.hasQualifiedName(packagePath(), "Controller", "RenderFileName")).getACall() } @@ -195,8 +188,8 @@ module Revel { /** * The `revel.Controller.Redirect` method. * - * For now I assume that in the context `Redirect(url, value)`, where Revel will `Sprintf(url, value)` internally, - * it is very likely `url` imposes some mandatory prefix, so `value` isn't truly an open redirect opportunity. + * It is currently assumed that a tainted `value` in `Redirect(url, value)`, which calls `Sprintf(url, value)` + * internally, cannot lead to an open redirect vulnerability. */ private class ControllerRedirectMethod extends HTTP::Redirect::Range, DataFlow::CallNode { ControllerRedirectMethod() { diff --git a/ql/test/library-tests/semmle/go/frameworks/Revel/Revel.go b/ql/test/library-tests/semmle/go/frameworks/Revel/Revel.go index b067d0238e5..ae59e7d1f51 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Revel/Revel.go +++ b/ql/test/library-tests/semmle/go/frameworks/Revel/Revel.go @@ -78,9 +78,12 @@ func (c myAppController) accessingParamsJSONIsUnsafe() { } func accessingRequestDirectlyIsUnsafe(c *revel.Controller) { - useURLValues(c.Request.GetQuery()) // NOT OK - useURLValues(c.Request.Form) // NOT OK - useURLValues(c.Request.MultipartForm.Value) // NOT OK + useURLValues(c.Request.GetQuery()) // NOT OK + useURLValues(c.Request.Form) // NOT OK + useURLValues(c.Request.MultipartForm.Value) // NOT OK + useString(c.Request.ContentType) // NOT OK + useString(c.Request.AcceptLanguages[0].Language) // NOT OK + useString(c.Request.Locale) // NOT OK form, _ := c.Request.GetForm() // NOT OK useURLValues(form) @@ -95,12 +98,26 @@ func accessingRequestDirectlyIsUnsafe(c *revel.Controller) { json, _ := ioutil.ReadAll(c.Request.GetBody()) // NOT OK useJSON(json) + + cookie, _ := c.Request.Cookie("abc") + useString(cookie.GetValue()) // NOT OK + + useString(c.Request.GetHttpHeader("headername")) // NOT OK + + useString(c.Request.GetRequestURI()) // NOT OK + + reader, _ := c.Request.MultipartReader() + part, _ := reader.NextPart() + partbody := make([]byte, 100) + part.Read(partbody) + useString(string(partbody)) // NOT OK + + useString(c.Request.Referer()) // NOT OK + + useString(c.Request.UserAgent()) // NOT OK } func accessingServerRequest(c *revel.Controller) { - query, _ := c.Request.In.Get(revel.HTTP_QUERY) // NOT OK - useURLValues(query.(url.Values)) - var message string c.Request.WebSocket.MessageReceive(&message) // NOT OK useString(message) diff --git a/ql/test/library-tests/semmle/go/frameworks/Revel/TaintFlows.expected b/ql/test/library-tests/semmle/go/frameworks/Revel/TaintFlows.expected index 1b58fbfb755..51be68608b2 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Revel/TaintFlows.expected +++ b/ql/test/library-tests/semmle/go/frameworks/Revel/TaintFlows.expected @@ -21,14 +21,21 @@ | Revel.go:82:15:82:28 | selection of Form : Values | Revel.go:33:12:33:23 | call to Get | 82 | | Revel.go:83:15:83:37 | selection of MultipartForm : pointer type | Revel.go:32:12:32:22 | index expression | 83 | | Revel.go:83:15:83:37 | selection of MultipartForm : pointer type | Revel.go:33:12:33:23 | call to Get | 83 | -| Revel.go:85:13:85:31 | call to GetForm : tuple type | Revel.go:32:12:32:22 | index expression | 85 | -| Revel.go:85:13:85:31 | call to GetForm : tuple type | Revel.go:33:12:33:23 | call to Get | 85 | -| Revel.go:88:13:88:40 | call to GetMultipartForm : tuple type | Revel.go:32:12:32:22 | index expression | 88 | -| Revel.go:88:13:88:40 | call to GetMultipartForm : tuple type | Revel.go:33:12:33:23 | call to Get | 88 | -| Revel.go:91:13:91:40 | call to GetMultipartForm : tuple type | Revel.go:92:11:92:35 | index expression | 91 | -| Revel.go:94:11:94:33 | selection of MultipartForm : pointer type | Revel.go:94:11:94:48 | index expression | 94 | -| Revel.go:96:28:96:46 | call to GetBody : Reader | Revel.go:97:10:97:13 | json | 96 | -| Revel.go:101:14:101:25 | selection of In : ServerRequest | Revel.go:32:12:32:22 | index expression | 101 | -| Revel.go:101:14:101:25 | selection of In : ServerRequest | Revel.go:33:12:33:23 | call to Get | 101 | -| Revel.go:105:37:105:44 | &... : pointer type | Revel.go:106:12:106:18 | message | 105 | -| Revel.go:109:41:109:42 | &... : pointer type | Revel.go:110:12:110:12 | p | 109 | +| Revel.go:84:12:84:32 | selection of ContentType | Revel.go:84:12:84:32 | selection of ContentType | 84 | +| Revel.go:85:12:85:36 | selection of AcceptLanguages : AcceptLanguages | Revel.go:85:12:85:48 | selection of Language | 85 | +| Revel.go:86:12:86:27 | selection of Locale | Revel.go:86:12:86:27 | selection of Locale | 86 | +| Revel.go:88:13:88:31 | call to GetForm : tuple type | Revel.go:32:12:32:22 | index expression | 88 | +| Revel.go:88:13:88:31 | call to GetForm : tuple type | Revel.go:33:12:33:23 | call to Get | 88 | +| Revel.go:91:13:91:40 | call to GetMultipartForm : tuple type | Revel.go:32:12:32:22 | index expression | 91 | +| Revel.go:91:13:91:40 | call to GetMultipartForm : tuple type | Revel.go:33:12:33:23 | call to Get | 91 | +| Revel.go:94:13:94:40 | call to GetMultipartForm : tuple type | Revel.go:95:11:95:35 | index expression | 94 | +| Revel.go:97:11:97:33 | selection of MultipartForm : pointer type | Revel.go:97:11:97:48 | index expression | 97 | +| Revel.go:99:28:99:46 | call to GetBody : Reader | Revel.go:100:10:100:13 | json | 99 | +| Revel.go:102:15:102:37 | call to Cookie : tuple type | Revel.go:103:12:103:28 | call to GetValue | 102 | +| Revel.go:105:12:105:48 | call to GetHttpHeader | Revel.go:105:12:105:48 | call to GetHttpHeader | 105 | +| Revel.go:107:12:107:36 | call to GetRequestURI | Revel.go:107:12:107:36 | call to GetRequestURI | 107 | +| Revel.go:109:15:109:41 | call to MultipartReader : tuple type | Revel.go:113:12:113:27 | type conversion | 109 | +| Revel.go:115:12:115:30 | call to Referer | Revel.go:115:12:115:30 | call to Referer | 115 | +| Revel.go:117:12:117:32 | call to UserAgent | Revel.go:117:12:117:32 | call to UserAgent | 117 | +| Revel.go:122:37:122:44 | &... : pointer type | Revel.go:123:12:123:18 | message | 122 | +| Revel.go:126:41:126:42 | &... : pointer type | Revel.go:127:12:127:12 | p | 126 |