diff --git a/python/ql/src/experimental/Security/CWE-614/CookieInjection.ql b/python/ql/src/experimental/Security/CWE-614/CookieInjection.ql new file mode 100644 index 00000000000..e97ff962919 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-614/CookieInjection.ql @@ -0,0 +1,28 @@ +/** + * @name Failure to use secure cookies + * @description Insecure cookies may be sent in cleartext, which makes them vulnerable to + * interception. + * @kind problem + * @problem.severity error + * @id py/insecure-cookie + * @tags security + * external/cwe/cwe-614 + */ + +// determine precision above +import python +import semmle.python.dataflow.new.DataFlow +import experimental.semmle.python.Concepts +import experimental.semmle.python.CookieHeader +import experimental.semmle.python.security.injection.CookieInjection + +from + CookieInjectionFlowConfig config, DataFlow::PathNode source, DataFlow::PathNode sink, + string insecure +where + config.hasFlowPath(source, sink) and + if exists(sink.getNode().(CookieSink)) + then insecure = "and it's " + sink.getNode().(CookieSink).getFlag() + " flag is not properly set" + else insecure = "" +select sink.getNode(), "Cookie is constructed from a", source.getNode(), "user-supplied input", + insecure diff --git a/python/ql/src/experimental/semmle/python/Concepts.qll b/python/ql/src/experimental/semmle/python/Concepts.qll index b6a15cb025b..64aa755d9cf 100644 --- a/python/ql/src/experimental/semmle/python/Concepts.qll +++ b/python/ql/src/experimental/semmle/python/Concepts.qll @@ -322,6 +322,16 @@ class Cookie extends DataFlow::Node { * Holds if the cookie is SameSite */ predicate isSameSite() { range.isSameSite() } + + /** + * Gets the argument containing the header name. + */ + DataFlow::Node getName() { result = range.getName() } + + /** + * Gets the argument containing the header value. + */ + DataFlow::Node getValue() { result = range.getValue() } } /** Provides a class for modeling new cookie writes on HTTP responses. */ @@ -347,5 +357,15 @@ module Cookie { * Holds if the cookie is SameSite. */ abstract predicate isSameSite(); + + /** + * Gets the argument containing the header name. + */ + abstract DataFlow::Node getName(); + + /** + * Gets the argument containing the header value. + */ + abstract DataFlow::Node getValue(); } } diff --git a/python/ql/src/experimental/semmle/python/CookieHeader.qll b/python/ql/src/experimental/semmle/python/CookieHeader.qll index 610fa311310..c7779aadd80 100644 --- a/python/ql/src/experimental/semmle/python/CookieHeader.qll +++ b/python/ql/src/experimental/semmle/python/CookieHeader.qll @@ -9,18 +9,28 @@ import experimental.semmle.python.Concepts class CookieHeader extends HeaderDeclaration, Cookie::Range { CookieHeader() { - this instanceof HeaderDeclaration and this.getNameArg().asExpr().(Str_).getS() = "Set-Cookie" + this instanceof HeaderDeclaration and + this.(HeaderDeclaration).getNameArg().asExpr().(Str_).getS() = "Set-Cookie" } override predicate isSecure() { - this.getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *Secure;.*") + this.(HeaderDeclaration).getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *Secure;.*") } override predicate isHttpOnly() { - this.getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *HttpOnly;.*") + this.(HeaderDeclaration).getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *HttpOnly;.*") } override predicate isSameSite() { - this.getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *SameSite=(Strict|Lax);.*") + this.(HeaderDeclaration) + .getValueArg() + .asExpr() + .(Str_) + .getS() + .regexpMatch(".*; *SameSite=(Strict|Lax);.*") } + + override DataFlow::Node getName() { result = this.(HeaderDeclaration).getValueArg() } + + override DataFlow::Node getValue() { result = this.(HeaderDeclaration).getValueArg() } } diff --git a/python/ql/src/experimental/semmle/python/frameworks/Django.qll b/python/ql/src/experimental/semmle/python/frameworks/Django.qll index fb6762d3dc3..ba99ddaa800 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Django.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Django.qll @@ -90,6 +90,10 @@ private module PrivateDjango { class DjangoSetCookieCall extends DataFlow::CallCfgNode, Cookie::Range { DjangoSetCookieCall() { this = baseClassRef().getMember("set_cookie").getACall() } + override DataFlow::Node getName() { result = this.getArg(0) } + + override DataFlow::Node getValue() { result = this.getArgByName("value") } + override predicate isSecure() { DataFlow::exprNode(any(True t)) .(DataFlow::LocalSourceNode) diff --git a/python/ql/src/experimental/semmle/python/frameworks/Flask.qll b/python/ql/src/experimental/semmle/python/frameworks/Flask.qll index 614e0738bcc..b93e6713846 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Flask.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Flask.qll @@ -91,6 +91,10 @@ module ExperimentalFlask { .getACall() } + override DataFlow::Node getName() { result = this.getArg(0) } + + override DataFlow::Node getValue() { result = this.getArgByName("value") } + override predicate isSecure() { DataFlow::exprNode(any(True t)) .(DataFlow::LocalSourceNode) diff --git a/python/ql/src/experimental/semmle/python/security/injection/CookieInjection.qll b/python/ql/src/experimental/semmle/python/security/injection/CookieInjection.qll new file mode 100644 index 00000000000..41f7c2af7d3 --- /dev/null +++ b/python/ql/src/experimental/semmle/python/security/injection/CookieInjection.qll @@ -0,0 +1,40 @@ +import python +import experimental.semmle.python.Concepts +import semmle.python.dataflow.new.DataFlow +import semmle.python.dataflow.new.TaintTracking +import semmle.python.dataflow.new.RemoteFlowSources + +class CookieSink extends DataFlow::Node { + string flag; + + CookieSink() { + exists(Cookie cookie | + this in [cookie.getName(), cookie.getValue()] and + ( + not cookie.isSecure() and + flag = "secure" + or + not cookie.isHttpOnly() and + flag = "httponly" + or + not cookie.isSameSite() and + flag = "samesite" + ) + ) + } + + string getFlag() { result = flag } +} + +/** + * A taint-tracking configuration for detecting Cookie injections. + */ +class CookieInjectionFlowConfig extends TaintTracking::Configuration { + CookieInjectionFlowConfig() { this = "CookieInjectionFlowConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { + exists(Cookie c | sink in [c.getName(), c.getValue()]) + } +} diff --git a/python/ql/test/experimental/query-tests/Security/CWE-614/flask_bad.py b/python/ql/test/experimental/query-tests/Security/CWE-614/flask_bad.py index f32d28a6f65..fc0177e3012 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-614/flask_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-614/flask_bad.py @@ -6,7 +6,7 @@ app = Flask(__name__) @app.route("/false") def false(): resp = make_response() - resp.set_cookie("name", value="value", secure=False, + resp.set_cookie(request.args["name"], value=request.args["value"], secure=False, httponly=False, samesite='None') return resp