Skip to content

Commit d9955ce

Browse files
authored
Merge pull request #20503 from geoffw0/cookie
Rust: New query rust/insecure-cookie
2 parents fa8cbee + 77e7898 commit d9955ce

File tree

19 files changed

+1868
-2
lines changed

19 files changed

+1868
-2
lines changed

rust/ql/integration-tests/query-suite/rust-code-scanning.qls.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ ql/rust/ql/src/queries/security/CWE-312/CleartextStorageDatabase.ql
1717
ql/rust/ql/src/queries/security/CWE-319/UseOfHttp.ql
1818
ql/rust/ql/src/queries/security/CWE-327/BrokenCryptoAlgorithm.ql
1919
ql/rust/ql/src/queries/security/CWE-328/WeakSensitiveDataHashing.ql
20+
ql/rust/ql/src/queries/security/CWE-614/InsecureCookie.ql
2021
ql/rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSize.ql
2122
ql/rust/ql/src/queries/security/CWE-798/HardcodedCryptographicValue.ql
2223
ql/rust/ql/src/queries/security/CWE-825/AccessInvalidPointer.ql

rust/ql/integration-tests/query-suite/rust-security-and-quality.qls.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ ql/rust/ql/src/queries/security/CWE-312/CleartextStorageDatabase.ql
1818
ql/rust/ql/src/queries/security/CWE-319/UseOfHttp.ql
1919
ql/rust/ql/src/queries/security/CWE-327/BrokenCryptoAlgorithm.ql
2020
ql/rust/ql/src/queries/security/CWE-328/WeakSensitiveDataHashing.ql
21+
ql/rust/ql/src/queries/security/CWE-614/InsecureCookie.ql
2122
ql/rust/ql/src/queries/security/CWE-696/BadCtorInitialization.ql
2223
ql/rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSize.ql
2324
ql/rust/ql/src/queries/security/CWE-798/HardcodedCryptographicValue.ql

rust/ql/integration-tests/query-suite/rust-security-extended.qls.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ ql/rust/ql/src/queries/security/CWE-312/CleartextStorageDatabase.ql
1818
ql/rust/ql/src/queries/security/CWE-319/UseOfHttp.ql
1919
ql/rust/ql/src/queries/security/CWE-327/BrokenCryptoAlgorithm.ql
2020
ql/rust/ql/src/queries/security/CWE-328/WeakSensitiveDataHashing.ql
21+
ql/rust/ql/src/queries/security/CWE-614/InsecureCookie.ql
2122
ql/rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSize.ql
2223
ql/rust/ql/src/queries/security/CWE-798/HardcodedCryptographicValue.ql
2324
ql/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,32 @@
11
# Models for the `biscotti` crate.
22
extensions:
3+
- addsTo:
4+
pack: codeql/rust-all
5+
extensible: sourceModel
6+
data:
7+
- ["<biscotti::response_cookie::ResponseCookie>::new", "ReturnValue", "cookie-create", "manual"]
8+
- ["<biscotti::response_cookie::ResponseCookie as core::convert::From>::from", "ReturnValue", "cookie-create", "manual"]
39
- addsTo:
410
pack: codeql/rust-all
511
extensible: sinkModel
612
data:
13+
- ["<biscotti::response_cookies::ResponseCookies>::insert", "Argument[0]", "cookie-use", "manual"]
714
- ["<biscotti::crypto::master::Key>::from", "Argument[0]", "credentials-key", "manual"]
15+
- addsTo:
16+
pack: codeql/rust-all
17+
extensible: summaryModel
18+
data:
19+
- ["<biscotti::response_cookie::ResponseCookie>::set_secure", "Argument[self].OptionalBarrier[cookie-secure-arg0]", "ReturnValue", "taint", "manual"]
20+
- ["<biscotti::response_cookie::ResponseCookie>::set_partitioned", "Argument[self].OptionalBarrier[cookie-partitioned-arg0]", "ReturnValue", "taint", "manual"]
21+
- ["<biscotti::response_cookie::ResponseCookie>::set_name", "Argument[self]", "ReturnValue", "taint", "manual"]
22+
- ["<biscotti::response_cookie::ResponseCookie>::set_value", "Argument[self]", "ReturnValue", "taint", "manual"]
23+
- ["<biscotti::response_cookie::ResponseCookie>::set_http_only", "Argument[self]", "ReturnValue", "taint", "manual"]
24+
- ["<biscotti::response_cookie::ResponseCookie>::set_same_site", "Argument[self]", "ReturnValue", "taint", "manual"]
25+
- ["<biscotti::response_cookie::ResponseCookie>::set_max_age", "Argument[self]", "ReturnValue", "taint", "manual"]
26+
- ["<biscotti::response_cookie::ResponseCookie>::set_path", "Argument[self]", "ReturnValue", "taint", "manual"]
27+
- ["<biscotti::response_cookie::ResponseCookie>::unset_path", "Argument[self]", "ReturnValue", "taint", "manual"]
28+
- ["<biscotti::response_cookie::ResponseCookie>::set_domain", "Argument[self]", "ReturnValue", "taint", "manual"]
29+
- ["<biscotti::response_cookie::ResponseCookie>::unset_domain", "Argument[self]", "ReturnValue", "taint", "manual"]
30+
- ["<biscotti::response_cookie::ResponseCookie>::set_expires", "Argument[self]", "ReturnValue", "taint", "manual"]
31+
- ["<biscotti::response_cookie::ResponseCookie>::unset_expires", "Argument[self]", "ReturnValue", "taint", "manual"]
32+
- ["<biscotti::response_cookie::ResponseCookie>::make_permanent", "Argument[self]", "ReturnValue", "taint", "manual"]
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,40 @@
11
# Models for the `cookie` crate.
22
extensions:
3+
- addsTo:
4+
pack: codeql/rust-all
5+
extensible: sourceModel
6+
data:
7+
- ["<cookie::Cookie>::build", "ReturnValue", "cookie-create", "manual"]
8+
- ["<cookie::builder::CookieBuilder>::new", "ReturnValue", "cookie-create", "manual"]
9+
- ["<cookie::Cookie>::new", "ReturnValue", "cookie-create", "manual"]
10+
- ["<cookie::Cookie>::named", "ReturnValue", "cookie-create", "manual"]
11+
- ["<cookie::Cookie as core::convert::From>::from", "ReturnValue", "cookie-create", "manual"]
312
- addsTo:
413
pack: codeql/rust-all
514
extensible: sinkModel
615
data:
16+
- ["<cookie::builder::CookieBuilder>::build", "Argument[self]", "cookie-use", "manual"]
17+
- ["<cookie::builder::CookieBuilder>::finish", "Argument[self]", "cookie-use", "manual"]
18+
- ["<cookie::jar::CookieJar>::add", "Argument[0]", "cookie-use", "manual"]
19+
- ["<cookie::jar::CookieJar>::add_original", "Argument[0]", "cookie-use", "manual"]
20+
- ["<cookie::secure::signed::SignedJar>::add", "Argument[0]", "cookie-use", "manual"]
21+
- ["<cookie::secure::signed::SignedJar>::add_original", "Argument[0]", "cookie-use", "manual"]
22+
- ["<cookie::secure::private::PrivateJar>::add", "Argument[0]", "cookie-use", "manual"]
23+
- ["<cookie::secure::private::PrivateJar>::add_original", "Argument[0]", "cookie-use", "manual"]
724
- ["<cookie::secure::key::Key>::from", "Argument[0].Reference", "credentials-key", "manual"]
25+
- addsTo:
26+
pack: codeql/rust-all
27+
extensible: summaryModel
28+
data:
29+
- ["<cookie::builder::CookieBuilder>::secure", "Argument[self].OptionalBarrier[cookie-secure-arg0]", "ReturnValue", "taint", "manual"]
30+
- ["<cookie::builder::CookieBuilder>::partitioned", "Argument[self].OptionalBarrier[cookie-partitioned-arg0]", "ReturnValue", "taint", "manual"]
31+
- ["<cookie::builder::CookieBuilder>::expires", "Argument[self]", "ReturnValue", "taint", "manual"]
32+
- ["<cookie::builder::CookieBuilder>::max_age", "Argument[self]", "ReturnValue", "taint", "manual"]
33+
- ["<cookie::builder::CookieBuilder>::domain", "Argument[self]", "ReturnValue", "taint", "manual"]
34+
- ["<cookie::builder::CookieBuilder>::path", "Argument[self]", "ReturnValue", "taint", "manual"]
35+
- ["<cookie::builder::CookieBuilder>::http_only", "Argument[self]", "ReturnValue", "taint", "manual"]
36+
- ["<cookie::builder::CookieBuilder>::same_site", "Argument[self]", "ReturnValue", "taint", "manual"]
37+
- ["<cookie::builder::CookieBuilder>::permanent", "Argument[self]", "ReturnValue", "taint", "manual"]
38+
- ["<cookie::builder::CookieBuilder>::removal", "Argument[self]", "ReturnValue", "taint", "manual"]
39+
- ["<cookie::Cookie>::set_secure", "Argument[self].OptionalBarrier[cookie-secure-arg0]", "ReturnValue", "taint", "manual"]
40+
- ["<cookie::Cookie>::set_partitioned", "Argument[self].OptionalBarrier[cookie-partitioned-arg0]", "ReturnValue", "taint", "manual"]
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
/**
2+
* Provides classes and predicates for reasoning about insecure cookie
3+
* vulnerabilities.
4+
*/
5+
6+
import rust
7+
private import codeql.rust.dataflow.DataFlow
8+
private import codeql.rust.dataflow.FlowSource
9+
private import codeql.rust.dataflow.FlowSink
10+
private import codeql.rust.Concepts
11+
private import codeql.rust.dataflow.internal.DataFlowImpl as DataFlowImpl
12+
private import codeql.rust.dataflow.internal.Node
13+
private import codeql.rust.controlflow.BasicBlocks
14+
15+
/**
16+
* Provides default sources, sinks and barriers for detecting insecure
17+
* cookie vulnerabilities, as well as extension points for adding your own.
18+
*/
19+
module InsecureCookie {
20+
/**
21+
* A data flow source for insecure cookie vulnerabilities.
22+
*/
23+
abstract class Source extends DataFlow::Node { }
24+
25+
/**
26+
* A data flow sink for insecure cookie vulnerabilities.
27+
*/
28+
abstract class Sink extends QuerySink::Range {
29+
override string getSinkType() { result = "InsecureCookie" }
30+
}
31+
32+
/**
33+
* A barrier for insecure cookie vulnerabilities.
34+
*/
35+
abstract class Barrier extends DataFlow::Node { }
36+
37+
/**
38+
* A source for insecure cookie vulnerabilities from model data.
39+
*/
40+
private class ModelsAsDataSource extends Source {
41+
ModelsAsDataSource() { sourceNode(this, "cookie-create") }
42+
}
43+
44+
/**
45+
* A sink for insecure cookie vulnerabilities from model data.
46+
*/
47+
private class ModelsAsDataSink extends Sink {
48+
ModelsAsDataSink() { sinkNode(this, "cookie-use") }
49+
}
50+
51+
/**
52+
* Holds if a models-as-data optional barrier for cookies is specified for `summaryNode`,
53+
* with arguments `attrib` (`secure` or `partitioned`) and `arg` (argument index). For example,
54+
* if a summary has input:
55+
* ```
56+
* [..., Argument[self].OptionalBarrier[cookie-secure-arg0], ...]
57+
* ```
58+
* then `attrib` is `secure` and `arg` is `0`.
59+
*
60+
* The meaning here is that a call to the function summarized by `summaryNode` would set
61+
* the cookie attribute `attrib` to the value of argument `arg`. This may in practice be
62+
* interpretted as a barrier to flow (when the cookie is made secure) or as a source (when
63+
* the cookie is made insecure).
64+
*/
65+
private predicate cookieOptionalBarrier(FlowSummaryNode summaryNode, string attrib, int arg) {
66+
exists(string barrierName |
67+
DataFlowImpl::optionalBarrier(summaryNode, barrierName) and
68+
attrib = barrierName.regexpCapture("cookie-(secure|partitioned)-arg([0-9]+)", 1) and
69+
arg = barrierName.regexpCapture("cookie-(secure|partitioned)-arg([0-9]+)", 2).toInt()
70+
)
71+
}
72+
73+
/**
74+
* Holds if cookie attribute `attrib` (`secure` or `partitioned`) is set to `value`
75+
* (`true` or `false`) at `node`. For example, the call:
76+
* ```
77+
* cookie.secure(true)
78+
* ```
79+
* sets the `"secure"` attribute to `true`. A value that cannot be determined is treated
80+
* as `false`.
81+
*/
82+
predicate cookieSetNode(DataFlow::Node node, string attrib, boolean value) {
83+
exists(FlowSummaryNode summaryNode, CallExprBase ce, int arg, DataFlow::Node argNode |
84+
// decode the models-as-data `OptionalBarrier`
85+
cookieOptionalBarrier(summaryNode, attrib, arg) and
86+
// find a call and arg referenced by this optional barrier
87+
ce.getStaticTarget() = summaryNode.getSummarizedCallable() and
88+
ce.getArg(arg) = argNode.asExpr().getExpr() and
89+
// check if the argument is always `true`
90+
(
91+
if
92+
forex(DataFlow::Node argSourceNode, BooleanLiteralExpr argSourceValue |
93+
DataFlow::localFlow(argSourceNode, argNode) and
94+
argSourceValue = argSourceNode.asExpr().getExpr()
95+
|
96+
argSourceValue.getTextValue() = "true"
97+
)
98+
then value = true // `true` flows to here
99+
else value = false // `false`, unknown, or multiple values
100+
) and
101+
// and find the node where this happens (we can't just use the flow summary node, since its
102+
// shared across all calls to the modeled function, we need a node specific to this call)
103+
(
104+
node.asExpr().getExpr() = ce.(MethodCallExpr).getReceiver() // e.g. `a` in `a.set_secure(true)`
105+
or
106+
exists(BasicBlock bb, int i |
107+
// associated SSA node
108+
node.(SsaNode).asDefinition().definesAt(_, bb, i) and
109+
ce.(MethodCallExpr).getReceiver() = bb.getNode(i).getAstNode()
110+
)
111+
)
112+
)
113+
}
114+
}
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `rust/insecure-cookie`, to detect cookies created without the 'Secure' attribute.
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
7+
<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, they can access sensitive information in the cookie and potentially use it to impersonate the user.</p>
8+
9+
</overview>
10+
<recommendation>
11+
12+
<p>Always set the cookie 'Secure' attribute so that the browser only sends the cookie over HTTPS.</p>
13+
14+
</recommendation>
15+
<example>
16+
17+
<p>The following example creates a cookie using the <a href="https://crates.io/crates/cookie">cookie</a> crate without the 'Secure' attribute:</p>
18+
19+
<sample src="InsecureCookieBad.rs" />
20+
21+
<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>
22+
23+
<sample src="InsecureCookieGood.rs" />
24+
25+
</example>
26+
<references>
27+
28+
<li>MDN Web Docs: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/Cookies">Using HTTP cookies</a>.</li>
29+
<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>
30+
<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>
31+
32+
</references>
33+
</qhelp>
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/**
2+
* @name 'Secure' attribute is not set to true
3+
* @description Omitting the 'Secure' attribute allows data to be transmitted insecurely
4+
* using HTTP. Always set 'Secure' to 'true' to ensure that HTTPS
5+
* is used at all times.
6+
* @kind path-problem
7+
* @problem.severity error
8+
* @security-severity 7.5
9+
* @precision high
10+
* @id rust/insecure-cookie
11+
* @tags security
12+
* external/cwe/cwe-319
13+
* external/cwe/cwe-614
14+
*/
15+
16+
import rust
17+
import codeql.rust.dataflow.DataFlow
18+
import codeql.rust.dataflow.TaintTracking
19+
import codeql.rust.security.InsecureCookieExtensions
20+
21+
/**
22+
* A data flow configuration for tracking values representing cookies without the
23+
* 'secure' attribute set. This is the primary data flow configuration for this
24+
* query.
25+
*/
26+
module InsecureCookieConfig implements DataFlow::ConfigSig {
27+
import InsecureCookie
28+
29+
predicate isSource(DataFlow::Node node) {
30+
// creation of a cookie or cookie configuration with default, insecure settings
31+
node instanceof Source
32+
or
33+
// setting the 'secure' attribute to false (or an unknown value)
34+
cookieSetNode(node, "secure", false)
35+
}
36+
37+
predicate isSink(DataFlow::Node node) {
38+
// use of a cookie or cookie configuration
39+
node instanceof Sink
40+
}
41+
42+
predicate isBarrier(DataFlow::Node node) {
43+
// setting the 'secure' attribute to true
44+
cookieSetNode(node, "secure", true)
45+
or
46+
node instanceof Barrier
47+
}
48+
49+
predicate observeDiffInformedIncrementalMode() { any() }
50+
}
51+
52+
/**
53+
* A data flow configuration for tracking values representing cookies with the
54+
* 'partitioned' attribute set. This is a secondary data flow configuration used
55+
* to filter out unwanted results.
56+
*/
57+
module PartitionedCookieConfig implements DataFlow::ConfigSig {
58+
import InsecureCookie
59+
60+
predicate isSource(DataFlow::Node node) {
61+
// setting the 'partitioned' attribute to true
62+
cookieSetNode(node, "partitioned", true)
63+
}
64+
65+
predicate isSink(DataFlow::Node node) {
66+
// use of a cookie or cookie configuration
67+
node instanceof Sink
68+
}
69+
70+
predicate isBarrier(DataFlow::Node node) {
71+
// setting the 'partitioned' attribute to false (or an unknown value)
72+
cookieSetNode(node, "partitioned", false)
73+
or
74+
node instanceof Barrier
75+
}
76+
77+
predicate observeDiffInformedIncrementalMode() { any() }
78+
}
79+
80+
module InsecureCookieFlow = TaintTracking::Global<InsecureCookieConfig>;
81+
82+
module PartitionedCookieFlow = TaintTracking::Global<PartitionedCookieConfig>;
83+
84+
import InsecureCookieFlow::PathGraph
85+
86+
from InsecureCookieFlow::PathNode sourceNode, InsecureCookieFlow::PathNode sinkNode
87+
where
88+
InsecureCookieFlow::flowPath(sourceNode, sinkNode) and
89+
not PartitionedCookieFlow::flow(_, sinkNode.getNode())
90+
select sinkNode.getNode(), sourceNode, sinkNode, "Cookie attribute 'Secure' is not set to true."
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
use cookie::Cookie;
2+
3+
// BAD: creating a cookie without specifying the `secure` attribute
4+
let cookie = Cookie::build(("session", "abcd1234")).build();
5+
let mut jar = cookie::CookieJar::new();
6+
jar.add(cookie.clone());

0 commit comments

Comments
 (0)