Skip to content

Conversation

@geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Sep 12, 2025

While writing the insecure cookie query (WIP), I've been using the cookie and biscotti crates. I noticed they both have the capability to take encryption keys and this can be modelled.

In addition to modelling those I've added a model for alloc::vec::from_elem and cleaned up the scattered manual models for <core::convert::From>::from.

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

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 adds cryptographic key modeling for the cookie and biscotti crates to detect when hard-coded cryptographic values are passed as keys. The changes include models for key creation functions and test updates to verify the new detection capabilities.

  • Added sink models for cookie::Key::from and biscotti::Key::from to detect hard-coded cryptographic keys
  • Consolidated the scattered manual models for <core::convert::From>::from into a single generic model
  • Added a model for alloc::vec::from_elem to track element propagation

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
rust/ql/test/query-tests/security/CWE-798/test_cookie.rs New test file demonstrating hard-coded cryptographic key usage in cookie and biscotti libraries
rust/ql/test/query-tests/security/CWE-798/options.yml Added cookie and biscotti crate dependencies for testing
rust/ql/test/query-tests/security/CWE-798/HardcodedCryptographicValue.expected Updated test expectations with new detections for cookie/biscotti hard-coded keys
rust/ql/test/query-tests/security/CWE-022/TaintedPath.expected Updated expectations reflecting the consolidated From model changes
rust/ql/test/library-tests/dataflow/strings/inline-taint-flow.expected Updated expectations with new generic From model
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml Added generic <_ as core::convert::From>::from model
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-alloc.model.yml Added alloc::vec::from_elem model and removed specific String From model
rust/ql/lib/codeql/rust/frameworks/stdlib/fs.model.yml Removed PathBuf From model in favor of generic one
rust/ql/lib/codeql/rust/frameworks/cookie.model.yml New model file defining cookie key sink
rust/ql/lib/codeql/rust/frameworks/biscotti.model.yml New model file defining biscotti key sink
rust/ql/lib/change-notes/2025-09-12-cookie.md Added change note for the new cryptography models

@geoffw0
Copy link
Contributor Author

geoffw0 commented Sep 15, 2025

DCA LGTM (small slowdown, believed to be wobble).

Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

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

LGTM! Great to have one single from model :)

@geoffw0
Copy link
Contributor Author

geoffw0 commented Sep 15, 2025

Thanks for reviewing.

@geoffw0 geoffw0 merged commit 00f6448 into github:main Sep 15, 2025
20 checks passed
- ["<alloc::string::String as core::convert::Into>::into", "Argument[self].Element", "ReturnValue.Element", "taint", "manual"]
- ["<alloc::string::String as core::convert::Into>::into", "Argument[self].Reference.Element", "ReturnValue.Element", "taint", "manual"]
# From
- ["<_ as core::convert::From>::from", "Argument[0]", "ReturnValue", "value", "manual"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have expected that this should be a taint model and not a value model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree - here: #20529

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.

3 participants