-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: New query rust/insecure-cookie #20503
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
Open
geoffw0
wants to merge
18
commits into
github:main
Choose a base branch
from
geoffw0:cookie
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,847
−2
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
513ae2a
Rust: Add tests for insecure cookies.
geoffw0 7e75c1d
Rust: Add very basic query prototype.
geoffw0 d52b668
Rust: Add security-severity tag.
geoffw0 eadf922
Rust: Use models-as-data, add source/sink/flow models.
geoffw0 257a1b0
Rust: Refactor sources, sinks into an extensions source file.
geoffw0 2654aff
Rust: Account for the 'secure' and 'partitioned' attributes.
geoffw0 a3ed83b
Rust: Make state transition / barrier nodes more reliable.
geoffw0 94afc82
Rust: Fix an issue with the local flow.
geoffw0 bd07350
Rust: Add qhelp and examples.
geoffw0 4662e42
Rust: Add examples as tests (and fix them).
geoffw0 ae90253
Rust: Add the new query to suite lists.
geoffw0 3de1911
Rust: Change note.
geoffw0 cc9c414
Apply suggestions from code review
geoffw0 5b4632b
Apply suggestions from code review
geoffw0 43ac75e
Rust: Address another tiny suggestion from review.
geoffw0 6362884
Rust: Autoformat.
geoffw0 86c8c3c
Rust: Fix warning by making the query a path-problem.
geoffw0 266624d
Rust: The test needs to have Source tags now.
geoffw0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,32 @@ | ||
| # Models for the `biscotti` crate. | ||
| extensions: | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sourceModel | ||
| data: | ||
| - ["<biscotti::response_cookie::ResponseCookie>::new", "ReturnValue", "cookie-create", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie as core::convert::From>::from", "ReturnValue", "cookie-create", "manual"] | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sinkModel | ||
| data: | ||
| - ["<biscotti::response_cookies::ResponseCookies>::insert", "Argument[0]", "cookie-use", "manual"] | ||
| - ["<biscotti::crypto::master::Key>::from", "Argument[0]", "credentials-key", "manual"] | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: summaryModel | ||
| data: | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_secure", "Argument[self].OptionalBarrier[cookie-secure-arg0]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_partitioned", "Argument[self].OptionalBarrier[cookie-partitioned-arg0]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_name", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_value", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_http_only", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_same_site", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_max_age", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_path", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::unset_path", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_domain", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::unset_domain", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::set_expires", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::unset_expires", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<biscotti::response_cookie::ResponseCookie>::make_permanent", "Argument[self]", "ReturnValue", "taint", "manual"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,40 @@ | ||
| # Models for the `cookie` crate. | ||
| extensions: | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sourceModel | ||
| data: | ||
| - ["<cookie::Cookie>::build", "ReturnValue", "cookie-create", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::new", "ReturnValue", "cookie-create", "manual"] | ||
| - ["<cookie::Cookie>::new", "ReturnValue", "cookie-create", "manual"] | ||
| - ["<cookie::Cookie>::named", "ReturnValue", "cookie-create", "manual"] | ||
| - ["<cookie::Cookie as core::convert::From>::from", "ReturnValue", "cookie-create", "manual"] | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sinkModel | ||
| data: | ||
| - ["<cookie::builder::CookieBuilder>::build", "Argument[self]", "cookie-use", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::finish", "Argument[self]", "cookie-use", "manual"] | ||
| - ["<cookie::jar::CookieJar>::add", "Argument[0]", "cookie-use", "manual"] | ||
| - ["<cookie::jar::CookieJar>::add_original", "Argument[0]", "cookie-use", "manual"] | ||
| - ["<cookie::secure::signed::SignedJar>::add", "Argument[0]", "cookie-use", "manual"] | ||
| - ["<cookie::secure::signed::SignedJar>::add_original", "Argument[0]", "cookie-use", "manual"] | ||
| - ["<cookie::secure::private::PrivateJar>::add", "Argument[0]", "cookie-use", "manual"] | ||
| - ["<cookie::secure::private::PrivateJar>::add_original", "Argument[0]", "cookie-use", "manual"] | ||
| - ["<cookie::secure::key::Key>::from", "Argument[0].Reference", "credentials-key", "manual"] | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: summaryModel | ||
| data: | ||
| - ["<cookie::builder::CookieBuilder>::secure", "Argument[self].OptionalBarrier[cookie-secure-arg0]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::partitioned", "Argument[self].OptionalBarrier[cookie-partitioned-arg0]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::expires", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::max_age", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::domain", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::path", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::http_only", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::same_site", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::permanent", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::builder::CookieBuilder>::removal", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::Cookie>::set_secure", "Argument[self].OptionalBarrier[cookie-secure-arg0]", "ReturnValue", "taint", "manual"] | ||
| - ["<cookie::Cookie>::set_partitioned", "Argument[self].OptionalBarrier[cookie-partitioned-arg0]", "ReturnValue", "taint", "manual"] |
93 changes: 93 additions & 0 deletions
93
rust/ql/lib/codeql/rust/security/InsecureCookieExtensions.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| /** | ||
| * Provides classes and predicates for reasoning about insecure cookie | ||
| * vulnerabilities. | ||
| */ | ||
|
|
||
| import rust | ||
| private import codeql.rust.dataflow.DataFlow | ||
| private import codeql.rust.dataflow.FlowSource | ||
| private import codeql.rust.dataflow.FlowSink | ||
| private import codeql.rust.Concepts | ||
| private import codeql.rust.dataflow.internal.DataFlowImpl as DataFlowImpl | ||
| private import codeql.rust.dataflow.internal.Node | ||
| private import codeql.rust.controlflow.BasicBlocks | ||
|
|
||
| /** | ||
| * Provides default sources, sinks and barriers for detecting insecure | ||
| * cookie vulnerabilities, as well as extension points for adding your own. | ||
| */ | ||
| module InsecureCookie { | ||
| /** | ||
| * A data flow source for insecure cookie vulnerabilities. | ||
| */ | ||
| abstract class Source extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A data flow sink for insecure cookie vulnerabilities. | ||
| */ | ||
| abstract class Sink extends QuerySink::Range { | ||
| override string getSinkType() { result = "InsecureCookie" } | ||
| } | ||
|
|
||
| /** | ||
| * A barrier for insecure cookie vulnerabilities. | ||
| */ | ||
| abstract class Barrier extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A source for insecure cookie vulnerabilities from model data. | ||
| */ | ||
| private class ModelsAsDataSource extends Source { | ||
| ModelsAsDataSource() { sourceNode(this, "cookie-create") } | ||
| } | ||
|
|
||
| /** | ||
| * A sink for insecure cookie vulnerabilities from model data. | ||
| */ | ||
| private class ModelsAsDataSink extends Sink { | ||
| ModelsAsDataSink() { sinkNode(this, "cookie-use") } | ||
| } | ||
|
|
||
| /** | ||
| * Holds if cookie attribute `attrib` (`secure` or `partitioned`) is set to `value` (`true` or `false`) at `node`. | ||
| * A value that cannot be determined is treated as `false`. | ||
| * | ||
| * This references models-as-data optional barrier nodes, for example `OptionalBarrier[cookie-secure-arg0]`. | ||
| */ | ||
| predicate cookieSetNode(DataFlow::Node node, string attrib, boolean value) { | ||
| exists( | ||
| FlowSummaryNode summaryNode, string barrierName, CallExprBase ce, int arg, | ||
| DataFlow::Node argNode | ||
| | | ||
| // decode a `cookie-`... optional barrier | ||
| DataFlowImpl::optionalBarrier(summaryNode, barrierName) and | ||
| attrib = barrierName.regexpCapture("cookie-(secure|partitioned)-arg([0-9]+)", 1) and | ||
| arg = barrierName.regexpCapture("cookie-(secure|partitioned)-arg([0-9]+)", 2).toInt() and | ||
| // find a call and arg referenced by this optional barrier | ||
| ce.getStaticTarget() = summaryNode.getSummarizedCallable() and | ||
| ce.getArg(arg) = argNode.asExpr().getExpr() and | ||
| // check if the argument is always `true` | ||
| ( | ||
| if | ||
| forex(DataFlow::Node argSourceNode, BooleanLiteralExpr argSourceValue | | ||
| DataFlow::localFlow(argSourceNode, argNode) and | ||
| argSourceValue = argSourceNode.asExpr().getExpr() | ||
| | | ||
| argSourceValue.getTextValue() = "true" | ||
| ) | ||
| then value = true // `true` flows to here | ||
| else value = false // `false`, unknown, or multiple values | ||
| ) and | ||
| // and find the node where this happens | ||
| ( | ||
| node.asExpr().getExpr() = ce.(MethodCallExpr).getReceiver() // e.g. `a` in `a.set_secure(true)` | ||
| or | ||
| exists(BasicBlock bb, int i | | ||
| // associated SSA node | ||
| node.(SsaNode).asDefinition().definesAt(_, bb, i) and | ||
| ce.(MethodCallExpr).getReceiver() = bb.getNode(i).getAstNode() | ||
| ) | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: newQuery | ||
| --- | ||
| * Added a new query, `rust/insecure-cookie`, to detect cookies created without the 'Secure' attribute. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
|
|
||
| <p>Failing to set the 'Secure' attribute on a cookie allows it to be transmitted over an unencrypted (HTTP) connection. If an attacker can observe a user's network traffic (for example over an insecure Wi‑Fi network), they can access sensitive information in the cookie and potentially use it to impersonate the user.</p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
|
|
||
| <p>Always set the cookie 'Secure' attribute so that the browser only sends the cookie over HTTPS.</p> | ||
|
|
||
| </recommendation> | ||
| <example> | ||
|
|
||
| <p>The following example creates a cookie using the <code>cookie</code> crate without the 'Secure' attribute:</p> | ||
|
|
||
| <sample src="InsecureCookieBad.rs" /> | ||
|
|
||
| <p>In the fixed example, we either call <code>secure(true)</code> on the <code>CookieBuilder</code> or <code>set_secure(true)</code> on the <code>Cookie</code> itself:</p> | ||
|
|
||
| <sample src="InsecureCookieGood.rs" /> | ||
|
|
||
| </example> | ||
| <references> | ||
|
|
||
| <li>MDN Web Docs: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/Cookies">Using HTTP cookies</a>.</li> | ||
| <li>OWASP Cheat Sheet Series: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#transport-layer-security">Session Management Cheat Sheet - Transport Layer Security</a>.</li> | ||
| <li>MDN Web Docs: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Set-Cookie#secure">Set-Cookie header - Secure</a>.</li> | ||
|
|
||
| </references> | ||
| </qhelp> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| /** | ||
| * @name 'Secure' attribute is not set to true | ||
| * @description Omitting the 'Secure' attribute allows data to be transmitted insecurely | ||
| * using HTTP. Always set 'Secure' to 'true' to ensure that HTTPS | ||
| * is used at all times. | ||
| * @kind path-problem | ||
| * @problem.severity error | ||
| * @security-severity 7.5 | ||
| * @precision high | ||
| * @id rust/insecure-cookie | ||
| * @tags security | ||
| * external/cwe/cwe-319 | ||
| * external/cwe/cwe-614 | ||
| */ | ||
|
|
||
| import rust | ||
| import codeql.rust.dataflow.DataFlow | ||
| import codeql.rust.dataflow.TaintTracking | ||
| import codeql.rust.security.InsecureCookieExtensions | ||
|
|
||
| /** | ||
| * A data flow configuration for tracking values representing cookies without the | ||
| * 'secure' attribute set. This is the primary data flow configuration for this | ||
| * query. | ||
| */ | ||
| module InsecureCookieConfig implements DataFlow::ConfigSig { | ||
| import InsecureCookie | ||
|
|
||
| predicate isSource(DataFlow::Node node) { | ||
| // creation of a cookie or cookie configuration with default, insecure settings | ||
| node instanceof Source | ||
| or | ||
| // setting the 'secure' attribute to false (or an unknown value) | ||
| cookieSetNode(node, "secure", false) | ||
| } | ||
|
|
||
| predicate isSink(DataFlow::Node node) { | ||
| // use of a cookie or cookie configuration | ||
| node instanceof Sink | ||
| } | ||
|
|
||
| predicate isBarrier(DataFlow::Node node) { | ||
| // setting the 'secure' attribute to true | ||
| cookieSetNode(node, "secure", true) | ||
| or | ||
| node instanceof Barrier | ||
| } | ||
|
|
||
| predicate observeDiffInformedIncrementalMode() { any() } | ||
| } | ||
|
|
||
| /** | ||
| * A data flow configuration for tracking values representing cookies with the | ||
| * 'partitioned' attribute set. This is a secondary data flow configuration used | ||
| * to filter out unwanted results. | ||
| */ | ||
| module PartitionedCookieConfig implements DataFlow::ConfigSig { | ||
| import InsecureCookie | ||
|
|
||
| predicate isSource(DataFlow::Node node) { | ||
| // setting the 'partitioned' attribute to true | ||
| cookieSetNode(node, "partitioned", true) | ||
| } | ||
|
|
||
| predicate isSink(DataFlow::Node node) { | ||
| // use of a cookie or cookie configuration | ||
| node instanceof Sink | ||
| } | ||
|
|
||
| predicate isBarrier(DataFlow::Node node) { | ||
| // setting the 'partitioned' attribute to false (or an unknown value) | ||
| cookieSetNode(node, "partitioned", false) | ||
| or | ||
| node instanceof Barrier | ||
| } | ||
|
|
||
| predicate observeDiffInformedIncrementalMode() { any() } | ||
| } | ||
|
|
||
| module InsecureCookieFlow = TaintTracking::Global<InsecureCookieConfig>; | ||
|
|
||
| module PartitionedCookieFlow = TaintTracking::Global<PartitionedCookieConfig>; | ||
|
|
||
| import InsecureCookieFlow::PathGraph | ||
|
|
||
| from InsecureCookieFlow::PathNode sourceNode, InsecureCookieFlow::PathNode sinkNode | ||
| where | ||
| InsecureCookieFlow::flowPath(sourceNode, sinkNode) and | ||
| not PartitionedCookieFlow::flow(_, sinkNode.getNode()) | ||
| select sinkNode.getNode(), sourceNode, sinkNode, "Cookie attribute 'Secure' is not set to true." |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| use cookie::Cookie; | ||
|
|
||
| // BAD: creating a cookie without specifying the `secure` attribute | ||
| let cookie = Cookie::build(("session", "abcd1234")).build(); | ||
| let mut jar = cookie::CookieJar::new(); | ||
| jar.add(cookie.clone()); |
11 changes: 11 additions & 0 deletions
11
rust/ql/src/queries/security/CWE-614/InsecureCookieGood.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| use cookie::Cookie; | ||
|
|
||
| // GOOD: set the `CookieBuilder` 'Secure' attribute so that the cookie is only sent over HTTPS | ||
| let secure_cookie = Cookie::build(("session", "abcd1234")).secure(true).build(); | ||
| let mut jar = cookie::CookieJar::new(); | ||
| jar.add(secure_cookie.clone()); | ||
|
|
||
| // GOOD: alternatively, set the 'Secure' attribute on an existing `Cookie` | ||
| let mut secure_cookie2 = Cookie::new("session", "abcd1234"); | ||
| secure_cookie2.set_secure(true); | ||
| jar.add(secure_cookie2); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
These nodes (which function as barriers) are essentially duplicated at the corresponding SSA dataflow nodes, this shouldn't be necessary but it currently is necessary for the state-setting calls (e.g. test line 61) to work properly.