Python: Promote the insecure cookie query from experimental

[joefarebrother]

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, or SameSite attributes.

[github-actions]

QHelp previews:

python/ql/src/Security/CWE-614/InsecureCookie.qhelp

Failure to use secure cookies

Cookies without the Secure flag set may be transmittd using HTTP instead of HTTPS, which leaves it vulnerable to being read by a third party.

Cookies without the HttpOnly flag set are accessible to JavaScript running in the same origin. In case of a Cross-Site Scripting (XSS) vulnerability, the cookie can be stolen by a malicious script.

Cookies with the SameSite attribute set to 'None' will be sent with cross-origin requests, which can be controlled by third-party JavaScript code and allow for Cross-Site Request Forgery (CSRF) attacks.

Recommendation

Always set secure to True or add "; Secure;" to the cookie's raw value.

Always set httponly to True or add "; HttpOnly;" to the cookie's raw value.

Always set samesite to Lax or Strict, or add "; SameSite=Lax;", or "; Samesite=Strict;" to the cookie's raw header value.

Example

In 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

• Detectify: Cookie lack Secure flag.
• PortSwigger: TLS cookie without secure flag set.
• Common Weakness Enumeration: CWE-614.
• Common Weakness Enumeration: CWE-1004.
• Common Weakness Enumeration: CWE-1275.

[yoff]

NIce work, I like the more careful semantics regarding known values.
A few suggestions around coding style.

[yoff]

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).

[joefarebrother]

Seems like it would make sense. Should it be defined in the Concepts library?

[yoff]

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.

[yoff]

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).

[yoff]

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...

[yoff]

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.)

[yoff]

LGTM