Rust: New query rust/insecure-cookie

[geoffw0]

New Rust query rust/insecure-cookie, identifying when a cookie is created without the Secure attribute (without which a client might leak information from the cookie via an HTTP connection). This is similar to the java/insecure-cookie and cs/web/cookie-secure-not-set queries.

I found 62 results on the MRVA top 1000. They appear to be mostly correct results (one case of roughly secure(!debug_build) which the query flags but might be considered safe; one where secure(false) is used only when clearing a cookie, which should be safe and may be justified).

[Copilot]

Pull Request Overview

This PR introduces a new Rust security query rust/insecure-cookie that identifies when cookies are created without the Secure attribute set to true. The query helps detect potential security vulnerabilities where cookies might be transmitted over insecure HTTP connections instead of HTTPS only.

• Implements comprehensive data flow analysis to track cookie creation and configuration
• Supports both cookie and biscotti crate libraries with extensive test coverage
• Includes proper handling of the partitioned attribute which implies secure behavior

Reviewed Changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
rust/ql/src/queries/security/CWE-614/InsecureCookie.ql	Main query implementation with dual data flow configurations
rust/ql/lib/codeql/rust/security/InsecureCookieExtensions.qll	Core logic for cookie security analysis including attribute tracking
rust/ql/lib/codeql/rust/frameworks/cookie.model.yml	Models-as-data definitions for the cookie crate
rust/ql/lib/codeql/rust/frameworks/biscotti.model.yml	Models-as-data definitions for the biscotti crate
rust/ql/test/query-tests/security/CWE-614/main.rs	Comprehensive test cases covering various cookie creation patterns
rust/ql/src/queries/security/CWE-614/InsecureCookie.qhelp	Documentation and examples for the query

[github-actions]

QHelp previews:

rust/ql/src/queries/security/CWE-614/InsecureCookie.qhelp

'Secure' attribute is not set to true

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.

Recommendation

Always set the cookie 'Secure' attribute so that the browser only sends the cookie over HTTPS.

Example

The following example creates a cookie using the cookie crate without the 'Secure' attribute:

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());

In the fixed example, we either call secure(true) on the CookieBuilder or set_secure(true) on the Cookie itself:

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

References

• MDN Web Docs: Using HTTP cookies.
• OWASP Cheat Sheet Series: Session Management Cheat Sheet - Transport Layer Security.
• MDN Web Docs: Set-Cookie header - Secure.
• Common Weakness Enumeration: CWE-319.
• Common Weakness Enumeration: CWE-614.

[geoffw0]

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.

[geoffw0]

DCA run complete and LGTM.

[paldepind]

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()
    )
  }

[geoffw0]

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

[geoffw0]

I've pushed the split and my best effort to clarify all this logic. There are a couple of gotchas:

• its a slight abuse of OptionalBarrier, because in practice these nodes can be interpreted as barriers or (less commonly I think) as sources. I have explained that edge case now.
• we can't put the barrier on the FlowSummaryNode because that node is inside the summary, and there's only one summary shared by all the calls to the summarized function. So we have to find another node to place the barrier at, and that turns out to be a bit fiddly.

We can do a quick 1:1 if you'd like me to talk you through this some more?

[subatoi]

Docs look good 👍

[paldepind]

Thanks for doing this! Looks good to me.