Rust: New query rust/insecure-cookie #20503
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"] |
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,92 @@ | ||
| /** | ||
| * 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 | ||
|
Contributor
There was a problem hiding this comment. I don't understand everything in this predicate (yet). But I'm wondering if some of this could be made easier to read by splitting it into several predicates? For instance, maybe extract the barrier parsing with something like this: /** Holds if `summaryNode` has a cookie barrier for `attrib` and `arg`. */
private predicate cookieBarrier(FlowSummaryNode summaryNode, string attrib, int arg) {
exists(string barrierName |
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()
)
}
Contributor
Author
There was a problem hiding this comment. Yep, its a complicated predicate and could be clearer. I'll add an example to the QLDoc and probably split it up as you suggest...
Contributor
Author
There was a problem hiding this comment. I've pushed the split and my best effort to clarify all this logic. There are a couple of gotchas:
We can do a quick 1:1 if you'd like me to talk you through this some more? |
||
| // 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() | ||
| ) | ||
|
Contributor
Author
There was a problem hiding this comment. 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. |
||
| ) | ||
| ) | ||
| } | ||
| } | ||
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> | ||
|
geoffw0 marked this conversation as resolved.
Outdated
|
||
|
|
||
| </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> | ||
|
geoffw0 marked this conversation as resolved.
Outdated
|
||
|
|
||
| <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 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 configurationn for this | ||
|
geoffw0 marked this conversation as resolved.
Outdated
|
||
| * 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()); |
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.
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.
Uh oh!
There was an error while loading. Please reload this page.