-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python: Promote the insecure cookie query from experimental #16933
Python: Promote the insecure cookie query from experimental #16933
Conversation
QHelp previews: python/ql/src/Security/CWE-614/InsecureCookie.qhelpFailure to use secure cookiesCookies without the Cookies without the Cookies with the RecommendationAlways set Always set Always set ExampleIn the following examples, the cases marked GOOD show secure cookie attributes being set; whereas in the cases marked BAD they are not set. from flask import Flask, request, make_response, Response
@app.route("/good1")
def good1():
resp = make_response()
resp.set_cookie("name", value="value", secure=True, httponly=True, samesite='Strict') # GOOD: Attributes are securely set
return resp
@app.route("/good2")
def good2():
resp = make_response()
resp.headers['Set-Cookie'] = "name=value; Secure; HttpOnly; SameSite=Strict" # GOOD: Attributes are securely set
return resp
@app.route("/bad1")
resp = make_response()
resp.set_cookie("name", value="value", samesite='None') # BAD: the SameSite attribute is set to 'None' and the 'Secure' and 'HttpOnly' attributes are set to False by default.
return resp References
|
4322665
to
1a3ba3c
Compare
5bbf6f9
to
4427181
Compare
01016d9
to
db27fd9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIce work, I like the more careful semantics regarding known values.
A few suggestions around coding style.
python/ql/src/change-notes/2024-07-23-insecure-cookie-promotion.md
Outdated
Show resolved
Hide resolved
override predicate hasSecureFlag(boolean b) { | ||
super.hasSecureFlag(b) | ||
or | ||
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("secure") | | ||
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and | ||
b = bool.booleanValue() | ||
) | ||
or | ||
not exists(this.getArgByName("secure")) and | ||
b = false | ||
} | ||
|
||
override predicate hasHttpOnlyFlag(boolean b) { | ||
super.hasHttpOnlyFlag(b) | ||
or | ||
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("httponly") | | ||
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and | ||
b = bool.booleanValue() | ||
) | ||
or | ||
not exists(this.getArgByName("httponly")) and | ||
b = false | ||
} | ||
|
||
override predicate hasSameSiteAttribute(Http::Server::CookieWrite::SameSiteValue v) { | ||
super.hasSameSiteAttribute(v) | ||
or | ||
exists(DataFlow::Node arg, StringLiteral str | arg = this.getArgByName("samesite") | | ||
DataFlow::localFlow(DataFlow::exprNode(str), arg) and | ||
( | ||
str.getText().toLowerCase() = "strict" and | ||
v instanceof Http::Server::CookieWrite::SameSiteStrict | ||
or | ||
str.getText().toLowerCase() = "lax" and | ||
v instanceof Http::Server::CookieWrite::SameSiteLax | ||
or | ||
str.getText().toLowerCase() = "none" and | ||
v instanceof Http::Server::CookieWrite::SameSiteNone | ||
) | ||
) | ||
or | ||
not exists(this.getArgByName("samesite")) and | ||
v instanceof Http::Server::CookieWrite::SameSiteLax // Lax is the default | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code seems to be copied across a bunch of frameworks. Would it make sense to create a SetCookieCall
abstraction so the frameworks just have to point out where the calls are? It seems all calls to set_cookie
(in any framework) are likely to follow this convention (and if not, it can be overridden by the framework).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it would make sense. Should it be defined in the Concepts
library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since it comes from the protocol (something like here) it should probably not be in a specific framework, so if it could be in Concepts
, that would be nice.
Co-authored-by: yoff <lerchedahl@gmail.com>
For the failing tests, it looks reasonable to just update the inline expectations. The content type will be set by the renderer in the unspecified cases, so is not statically known (at least not by our analysis). |
Co-authored-by: yoff <lerchedahl@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I have one suggestion for a doc link, but otherwise I think I cannot ask for more here :-)
We should kick off a DCA run, but I do not really expect any of this to blow up...
Co-authored-by: yoff <lerchedahl@gmail.com>
DCA run looks good. The new results look fine to me, but please confirm that you would expect them too :-) (The ones that are not unclassified; the unclassified ones are just our tests, it seems.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Part of https://github.com/github/codeql-python-team/issues/792 promoting #6360;
Depends on #16696
Promotes the insecure cookie query from experimental, finding instances of cookies set without secure values for the
Secure
,HttpOnly
, orSameSite
attributes.