From 48c3c3d8a8df6cf3d0a65d00fe400aae1537314a Mon Sep 17 00:00:00 2001 From: jorgectf Date: Wed, 27 Oct 2021 21:00:50 +0200 Subject: [PATCH] Broaden scope --- .../Security/CWE-614/InsecureCookie.ql | 25 +++++------- .../experimental/semmle/python/Concepts.qll | 39 +++++++++---------- .../semmle/python/CookieHeader.qll | 26 +++++++++++++ .../semmle/python/frameworks/Django.qll | 19 ++++++--- .../semmle/python/frameworks/Flask.qll | 20 +++++++--- 5 files changed, 82 insertions(+), 47 deletions(-) create mode 100644 python/ql/src/experimental/semmle/python/CookieHeader.qll diff --git a/python/ql/src/experimental/Security/CWE-614/InsecureCookie.ql b/python/ql/src/experimental/Security/CWE-614/InsecureCookie.ql index 02b1280abab..bf0ff22d45e 100644 --- a/python/ql/src/experimental/Security/CWE-614/InsecureCookie.ql +++ b/python/ql/src/experimental/Security/CWE-614/InsecureCookie.ql @@ -12,23 +12,16 @@ // determine precision above import python import semmle.python.dataflow.new.DataFlow -import semmle.python.Concepts import experimental.semmle.python.Concepts -from Expr cookieExpr +from Cookie cookie, string alert where - exists(HeaderDeclaration headerWrite, StrConst headerName, StrConst headerValue | - headerName.getText() = "Set-Cookie" and - DataFlow::exprNode(headerName).(DataFlow::LocalSourceNode).flowsTo(headerWrite.getNameArg()) and - not headerValue.getText().regexpMatch(".*; *Secure;.*") and - DataFlow::exprNode(headerValue).(DataFlow::LocalSourceNode).flowsTo(headerWrite.getValueArg()) and - cookieExpr = headerWrite.asExpr() - ) + cookie.isSecure() and + alert = "secure" or - exists(ExperimentalHTTP::CookieWrite cookieWrite, False f, None n | - [DataFlow::exprNode(f), DataFlow::exprNode(n)] - .(DataFlow::LocalSourceNode) - .flowsTo(cookieWrite.(DataFlow::CallCfgNode).getArgByName("secure")) and - cookieExpr = cookieWrite.asExpr() - ) -select cookieExpr, "Cookie is added to response without the 'secure' flag being set." + not cookie.isHttpOnly() and + alert = "httponly" + or + cookie.isSameSite() and + alert = "samesite" +select cookie, "Cookie is added without the ", alert, " flag properly set." diff --git a/python/ql/src/experimental/semmle/python/Concepts.qll b/python/ql/src/experimental/semmle/python/Concepts.qll index 9da35b854a5..b6a15cb025b 100644 --- a/python/ql/src/experimental/semmle/python/Concepts.qll +++ b/python/ql/src/experimental/semmle/python/Concepts.qll @@ -301,54 +301,51 @@ class HeaderDeclaration extends DataFlow::Node { * A data-flow node that sets a cookie in an HTTP response. * * Extend this class to refine existing API models. If you want to model new APIs, - * extend `HTTP::CookieWrite::Range` instead. + * extend `Cookie::Range` instead. */ -class CookieWrite extends DataFlow::Node { - CookieWrite::Range range; +class Cookie extends DataFlow::Node { + Cookie::Range range; - CookieWrite() { this = range } + Cookie() { this = range } /** - * Gets the argument, if any, specifying the raw cookie header. + * Holds if this cookie is secure. */ - DataFlow::Node getHeaderArg() { result = range.getHeaderArg() } + predicate isSecure() { range.isSecure() } /** - * Gets the argument, if any, specifying the cookie name. + * Holds if this cookie is HttpOnly. */ - DataFlow::Node getNameArg() { result = range.getNameArg() } + predicate isHttpOnly() { range.isHttpOnly() } /** - * Gets the argument, if any, specifying the cookie value. + * Holds if the cookie is SameSite */ - DataFlow::Node getValueArg() { result = range.getValueArg() } + predicate isSameSite() { range.isSameSite() } } /** Provides a class for modeling new cookie writes on HTTP responses. */ -module CookieWrite { +module Cookie { /** * A data-flow node that sets a cookie in an HTTP response. * - * Note: we don't require that this redirect must be sent to a client (a kind of - * "if a tree falls in a forest and nobody hears it" situation). - * * Extend this class to model new APIs. If you want to refine existing API models, - * extend `HttpResponse` instead. + * extend `Cookie` instead. */ abstract class Range extends DataFlow::Node { /** - * Gets the argument, if any, specifying the raw cookie header. + * Holds if this cookie is secure. */ - abstract DataFlow::Node getHeaderArg(); + abstract predicate isSecure(); /** - * Gets the argument, if any, specifying the cookie name. + * Holds if this cookie is HttpOnly. */ - abstract DataFlow::Node getNameArg(); + abstract predicate isHttpOnly(); /** - * Gets the argument, if any, specifying the cookie value. + * Holds if the cookie is SameSite. */ - abstract DataFlow::Node getValueArg(); + abstract predicate isSameSite(); } } diff --git a/python/ql/src/experimental/semmle/python/CookieHeader.qll b/python/ql/src/experimental/semmle/python/CookieHeader.qll new file mode 100644 index 00000000000..610fa311310 --- /dev/null +++ b/python/ql/src/experimental/semmle/python/CookieHeader.qll @@ -0,0 +1,26 @@ +/** + * Temporary: provides a class to extend current cookies to header declarations + */ + +import python +import semmle.python.dataflow.new.DataFlow +import semmle.python.dataflow.new.TaintTracking +import experimental.semmle.python.Concepts + +class CookieHeader extends HeaderDeclaration, Cookie::Range { + CookieHeader() { + this instanceof HeaderDeclaration and this.getNameArg().asExpr().(Str_).getS() = "Set-Cookie" + } + + override predicate isSecure() { + this.getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *Secure;.*") + } + + override predicate isHttpOnly() { + this.getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *HttpOnly;.*") + } + + override predicate isSameSite() { + this.getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *SameSite=(Strict|Lax);.*") + } +} diff --git a/python/ql/src/experimental/semmle/python/frameworks/Django.qll b/python/ql/src/experimental/semmle/python/frameworks/Django.qll index 1a3c5c1cf10..fb6762d3dc3 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Django.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Django.qll @@ -87,15 +87,24 @@ private module PrivateDjango { override DataFlow::Node getValueArg() { result = headerInput } } - class DjangoSetCookieCall extends DataFlow::CallCfgNode, - ExperimentalHTTP::CookieWrite::Range { + class DjangoSetCookieCall extends DataFlow::CallCfgNode, Cookie::Range { DjangoSetCookieCall() { this = baseClassRef().getMember("set_cookie").getACall() } - override DataFlow::Node getHeaderArg() { none() } + override predicate isSecure() { + DataFlow::exprNode(any(True t)) + .(DataFlow::LocalSourceNode) + .flowsTo(this.getArgByName("secure")) + } - override DataFlow::Node getNameArg() { result = this.getArg(0) } + override predicate isHttpOnly() { + DataFlow::exprNode(any(True t)) + .(DataFlow::LocalSourceNode) + .flowsTo(this.getArgByName("httponly")) + } - override DataFlow::Node getValueArg() { result = this.getArg(1) } + override predicate isSameSite() { + this.getArgByName("samesite").asExpr().(Str_).getS() in ["Strict", "Lax"] + } } } } diff --git a/python/ql/src/experimental/semmle/python/frameworks/Flask.qll b/python/ql/src/experimental/semmle/python/frameworks/Flask.qll index 2a2cc68fe84..614e0738bcc 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Flask.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Flask.qll @@ -82,8 +82,8 @@ module ExperimentalFlask { override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() } } - class DjangoSetCookieCall extends DataFlow::CallCfgNode, ExperimentalHTTP::CookieWrite::Range { - DjangoSetCookieCall() { + class FlaskSetCookieCall extends DataFlow::CallCfgNode, Cookie::Range { + FlaskSetCookieCall() { this = [Flask::Response::classRef(), flaskMakeResponse()] .getReturn() @@ -91,10 +91,20 @@ module ExperimentalFlask { .getACall() } - override DataFlow::Node getHeaderArg() { none() } + override predicate isSecure() { + DataFlow::exprNode(any(True t)) + .(DataFlow::LocalSourceNode) + .flowsTo(this.getArgByName("secure")) + } - override DataFlow::Node getNameArg() { result = this.getArg(0) } + override predicate isHttpOnly() { + DataFlow::exprNode(any(True t)) + .(DataFlow::LocalSourceNode) + .flowsTo(this.getArgByName("httponly")) + } - override DataFlow::Node getValueArg() { result = this.getArg(1) } + override predicate isSameSite() { + this.getArgByName("samesite").asExpr().(Str_).getS() in ["Strict", "Lax"] + } } }