Python: Add cookie security-related queries #6360

jorgectf · 2021-07-25T02:10:30Z

This PR adds:

Modeling of:
- Adding cookies in flask.
- Adding cookies in django.
Queries:
- Cookie without (secure|httponly|samesite) flag properly set.
- Cookie constructed from user-supplied input.

… into jorgectf/python/insecure-cookie

…nsecure-cookie

…ure-cookie

RasmusWL

I have not looked at this PR in depth, but just noted that this deals with setting cookies in HTTP responses. Would be great if you could integrate this with the existing HTTP::Server::CookieWrite concept implemented here:

codeql/python/ql/lib/semmle/python/Concepts.qll

Lines 759 to 784 in d97b130

    
               /** 
        
                * 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. 
        
                */ 
        
               class CookieWrite extends DataFlow::Node { 
        
                 CookieWrite::Range range; 
        
                 CookieWrite() { this = range } 
        
                 /** 
        
                  * Gets the argument, if any, specifying the raw cookie header. 
        
                  */ 
        
                 DataFlow::Node getHeaderArg() { result = range.getHeaderArg() } 
        
                 /** 
        
                  * Gets the argument, if any, specifying the cookie name. 
        
                  */ 
        
                 DataFlow::Node getNameArg() { result = range.getNameArg() } 
        
                 /** 
        
                  * Gets the argument, if any, specifying the cookie value. 
        
                  */ 
        
                 DataFlow::Node getValueArg() { result = range.getValueArg() } 
        
               }

You can do this by creating your own experimental concepts that extend the existing concept, for an exmaple, see

codeql/python/ql/lib/semmle/python/Concepts.qll

Lines 75 to 106 in 85f00fd

    
           /** 
        
            * A data flow node that writes data to the file system access. 
        
            * 
        
            * Extend this class to refine existing API models. If you want to model new APIs, 
        
            * extend `FileSystemWriteAccess::Range` instead. 
        
            */ 
        
           class FileSystemWriteAccess extends FileSystemAccess { 
        
             override FileSystemWriteAccess::Range range; 
        
             /** 
        
              * Gets a node that represents data to be written to the file system (possibly with 
        
              * some transformation happening before it is written, like JSON encoding). 
        
              */ 
        
             DataFlow::Node getADataNode() { result = range.getADataNode() } 
        
           } 
        
           /** Provides a class for modeling new file system writes. */ 
        
           module FileSystemWriteAccess { 
        
             /** 
        
              * A data flow node that writes data to the file system access. 
        
              * 
        
              * Extend this class to model new APIs. If you want to refine existing API models, 
        
              * extend `FileSystemWriteAccess` instead. 
        
              */ 
        
             abstract class Range extends FileSystemAccess::Range { 
        
               /** 
        
                * Gets a node that represents data to be written to the file system (possibly with 
        
                * some transformation happening before it is written, like JSON encoding). 
        
                */ 
        
               abstract DataFlow::Node getADataNode(); 
        
             } 
        
           }

I would base my modeling in Django/Flask on the existing modeling (that is, copy-paste it), such that it is easy to just override the existing modeling when promoting this query 👍

jorgectf · 2022-01-20T12:21:00Z

I haven't been able to find any CVE for this specific security issue, so the bounty submission has been closed.

RasmusWL · 2022-01-24T19:07:34Z

Thanks for the update @jorgectf. Will take a look on this PR soon 👍

Matching the Java query https://github.com/github/codeql/blob/7d4767a4f57316429dda8813e6c744ecd4f16cb5/java/ql/src/Security/CWE/CWE-614/InsecureCookie.ql#L7

github-advanced-security

Found 5 vulnerabilities.

python/ql/src/experimental/semmle/python/frameworks/Django.qll

+          private DataFlow::TypeTrackingNode instance(DataFlow::TypeTracker t) {
+            t.start() and
+            result instanceof InstanceSource
+            or
+            exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t))
+          }


python/ql/src/experimental/semmle/python/frameworks/Django.qll

+          }
+
+          /** Gets a reference to an instance of `django.http.response.HttpResponse`. */
+          DataFlow::Node instance() { instance(DataFlow::TypeTracker::end()).flowsTo(result) }


python/ql/src/experimental/semmle/python/CookieHeader.qll

+/**
+ * Gets a header setting a cookie.
+ *
+ * Given the following example:
+ *
+ * ```py
+ * @app.route("/")
+ * def flask_make_response():
+ *    resp = make_response("")
+ *    resp.headers['Set-Cookie'] = "name=value; Secure;"
+ *    return resp
+ * ```
+ *
+ * * `this` would be `resp.headers['Set-Cookie'] = "name=value; Secure;"`.
+ * * `isSecure()` predicate would succeed.
+ * * `isHttpOnly()` predicate would fail.
+ * * `isSameSite()` predicate would fail.
+ * * `getName()` and `getValue()` results would be `"name=value; Secure;"`.
+ */


python/ql/src/experimental/semmle/python/frameworks/Flask.qll

+  /**
+   * Gets a call to `set_cookie()`.
+   *
+   * Given the following example:
+   *
+   * ```py
+   * @app.route("/")
+   * def false():
+   *    resp = make_response()
+   *    resp.set_cookie("name", value="value", secure=True, httponly=True, samesite='Lax')
+   *    return resp
+   * ```
+   *
+   * * `this` would be `resp.set_cookie("name", value="value", secure=False, httponly=False, samesite='None')`.
+   * * `getName()`'s result would be `"name"`.
+   * * `getValue()`'s result would be `"value"`.
+   * * `isSecure()` predicate would succeed.
+   * * `isHttpOnly()` predicate would succeed.
+   * * `isSameSite()` predicate would succeed.
+   */


python/ql/src/experimental/semmle/python/frameworks/Django.qll

+          /**
+           * Gets a call to `set_cookie()`.
+           *
+           * Given the following example:
+           *
+           * ```py
+           * def django_response(request):
+           *    resp = django.http.HttpResponse()
+           *    resp.set_cookie("name", "value", secure=True, httponly=True, samesite='Lax')
+           *    return resp
+           * ```
+           *
+           * * `this` would be `resp.set_cookie("name", "value", secure=False, httponly=False, samesite='None')`.
+           * * `getName()`'s result would be `"name"`.
+           * * `getValue()`'s result would be `"value"`.
+           * * `isSecure()` predicate would succeed.
+           * * `isHttpOnly()` predicate would succeed.
+           * * `isSameSite()` predicate would succeed.
+           */


RasmusWL

I took forever to review this, sorry 😬

For reference when promoting this, JS adopted an approach with multiple queries for each of these flags, so we should consider doing the same

js/samesite-none-cookie (precision medium)
js/client-exposed-cookie (precision high) -- for httpOnly flag
js/clear-text-cookie (precision high) -- for 'secure' flag

See #6855 and #7721

I also think we need to review how bad CookieInjection really is. My thoughts without reading up on this at all is that it should be a @precision medium query -- a user can always set cookies on their own machine, so this is only a problem when a third-party attacker can set a cookie on a victim machine, meaning some cookie writing request without CSRF protection... but my intuition might be wrong 🤷

jorgectf added 4 commits July 25, 2021 03:22

Merge remote-tracking branch 'origin/jorgectf/python/headerInjection'…

0aaa9c1

… into jorgectf/python/insecure-cookie

Add tests and .qlref

8a3e4f1

Add query

c8983be

Write documentation and example

4f68a17

jorgectf requested a review from a team as a code owner July 25, 2021 02:10

github-actions bot added documentation Python labels Jul 25, 2021

jorgectf added 5 commits July 25, 2021 04:35

Merge branch 'jorgectf/python/headerInjection' into jorgectf/python/i…

66fdd53

…nsecure-cookie

Add CookieWrite concept

6504429

Polish CookieWrite

9834659

Add .expected

c8a7f48

Change False and None scopes

54ed25a

jorgectf marked this pull request as draft August 30, 2021 13:46

jorgectf changed the title ~~Python: Add missing 'secure' flag in response cookies query~~ Python: Add cookie-related queries Oct 27, 2021

jorgectf added 6 commits October 27, 2021 19:00

Merge remote-tracking branch 'origin/main' into jorgectf/python/insec…

28ec8c9

…ure-cookie

Broaden scope

48c3c3d

Polish tests

0f2b81e

Polish .ql

5dc1ad6

Update .expected

129edd6

Add cookie injection query missing proper tests

cf9e9f9

jorgectf changed the title ~~Python: Add cookie-related queries~~ Python: Add cookie security-related queries Oct 28, 2021

jorgectf added 7 commits November 5, 2021 20:08

Fix typo

4cb78ac

Improve tests

d7a7946

Add CookieInjection sample and .qhelp

b3258ce

Fix endpoints' naming

cf47e8e

Add CookieInjection.qlref

a420e6e

Add/Update .expected files.

86aac7c

Merge remote-tracking branch 'origin/main' into jorgectf/python/insec…

ed74bd6

…ure-cookie

jorgectf marked this pull request as ready for review November 5, 2021 19:17

Polish documentation.

83e3de1

jorgectf mentioned this pull request Nov 15, 2021

[Python] CWE-614: Cookie security-related queries github/securitylab#479

Closed

1 task

RasmusWL self-assigned this Nov 15, 2021

RasmusWL reviewed Nov 15, 2021

View reviewed changes

jorgectf added 2 commits November 16, 2021 13:54

Make Cookie concept extend HTTP::Server::CookieWrite

e7d649f

Adapt Django and Flask to their main modelings

6ecb6d1

jorgectf requested a review from RasmusWL November 16, 2021 14:00

jorgectf added 2 commits November 16, 2021 19:00

Avoid using Str_ internal class

a4204cc

Avoid using Str_ in CookieHeader

840cded

RasmusWL added 4 commits May 11, 2022 11:13

Merge branch 'main' into jorgectf/python/insecure-cookie

d127d21

Python: Fix Django import

84ad45c

Python: Add security-severity for py/insecure-cookie

a902d3d

Matching the Java query https://github.com/github/codeql/blob/7d4767a4f57316429dda8813e6c744ecd4f16cb5/java/ql/src/Security/CWE/CWE-614/InsecureCookie.ql#L7

Python: Add placeholder precision for py/insecure-cookie

27b99c5

github-advanced-security bot found potential problems May 11, 2022

View reviewed changes

RasmusWL previously approved these changes May 11, 2022

View reviewed changes

RasmusWL dismissed their stale review via 62b3ec1 May 11, 2022 11:16

Python: Fix select for py/cookie-injection

fc8633c

RasmusWL force-pushed the jorgectf/python/insecure-cookie branch from 62b3ec1 to fc8633c Compare May 11, 2022 11:18

RasmusWL previously approved these changes May 11, 2022

View reviewed changes

Python: Fix select of py/insecure-cookie

cff950f

RasmusWL dismissed their stale review via cff950f May 11, 2022 12:06

RasmusWL approved these changes May 11, 2022

View reviewed changes

RasmusWL merged commit 46f309c into github:main May 11, 2022

joefarebrother mentioned this pull request Jun 6, 2024

Python: Model CookieWrites from HeaderWrites #16696

Merged

This was referenced Jul 2, 2024

Python: Promote cookie injection query from experimental #16893

Merged

Python: Promote the insecure cookie query from experimental #16933

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Python: Add cookie security-related queries #6360

Python: Add cookie security-related queries #6360

Uh oh!

jorgectf commented Jul 25, 2021 •

edited

Loading

Uh oh!

RasmusWL left a comment •

edited

Loading

Uh oh!

jorgectf commented Jan 20, 2022

Uh oh!

RasmusWL commented Jan 24, 2022

Uh oh!

github-advanced-security bot left a comment

Uh oh!

Check warning

Check warning

Check warning

Check warning

Check warning

RasmusWL left a comment •

edited

Loading

Uh oh!

Uh oh!

	/**
	* 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.
	*/
	class CookieWrite extends DataFlow::Node {
	CookieWrite::Range range;

	CookieWrite() { this = range }

	/**
	* Gets the argument, if any, specifying the raw cookie header.
	*/
	DataFlow::Node getHeaderArg() { result = range.getHeaderArg() }

	/**
	* Gets the argument, if any, specifying the cookie name.
	*/
	DataFlow::Node getNameArg() { result = range.getNameArg() }

	/**
	* Gets the argument, if any, specifying the cookie value.
	*/
	DataFlow::Node getValueArg() { result = range.getValueArg() }
	}

	/**
	* A data flow node that writes data to the file system access.
	*
	* Extend this class to refine existing API models. If you want to model new APIs,
	* extend `FileSystemWriteAccess::Range` instead.
	*/
	class FileSystemWriteAccess extends FileSystemAccess {
	override FileSystemWriteAccess::Range range;

	/**
	* Gets a node that represents data to be written to the file system (possibly with
	* some transformation happening before it is written, like JSON encoding).
	*/
	DataFlow::Node getADataNode() { result = range.getADataNode() }
	}

	/** Provides a class for modeling new file system writes. */
	module FileSystemWriteAccess {
	/**
	* A data flow node that writes data to the file system access.
	*
	* Extend this class to model new APIs. If you want to refine existing API models,
	* extend `FileSystemWriteAccess` instead.
	*/
	abstract class Range extends FileSystemAccess::Range {
	/**
	* Gets a node that represents data to be written to the file system (possibly with
	* some transformation happening before it is written, like JSON encoding).
	*/
	abstract DataFlow::Node getADataNode();
	}
	}

Python: Add cookie security-related queries #6360

Python: Add cookie security-related queries #6360

Uh oh!

Conversation

jorgectf commented Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RasmusWL left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgectf commented Jan 20, 2022

Uh oh!

RasmusWL commented Jan 24, 2022

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

Check warning

Check warning

Check warning

Check warning

Check warning

RasmusWL left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jorgectf commented Jul 25, 2021 •

edited

Loading

RasmusWL left a comment •

edited

Loading

RasmusWL left a comment •

edited

Loading