Skip to content

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Sep 22, 2025

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

@geoffw0 geoffw0 requested a review from a team as a code owner September 22, 2025 15:15
@Copilot Copilot AI review requested due to automatic review settings September 22, 2025 15:15
@geoffw0 geoffw0 added the Rust Pull requests that update Rust code label Sep 22, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

github-actions bot commented Sep 22, 2025

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 (for example over an insecure Wi‑Fi network), 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

// associated SSA node
node.(SsaNode).asDefinition().definesAt(_, bb, i) and
ce.(MethodCallExpr).getReceiver() = bb.getNode(i).getAstNode()
)
Copy link
Contributor Author

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.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Sep 23, 2025

DCA run complete and LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant