Rust: Query for cleartext logging of sensitive information by geoffw0 · Pull Request #18582 · github/codeql

geoffw0 · 2025-01-23T18:04:50Z

New query for cleartext logging of sensitive information in Rust. There are plans to add more sinks in future, but what's here should give us fairly good coverage.

Copilot

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

rust/ql/src/queries/security/CWE-312/CleartextLoggingBad.rs

geoffw0 · 2025-01-23T18:15:21Z

rust/ql/src/queries/security/CWE-312/CleartextLogging.ql

+
+  predicate isAdditionalFlowStep(Node node1, Node node2) {
+    // flow from `a` to `&a`
+    node2.asExpr().getExpr().(RefExpr).getExpr() = node1.asExpr().getExpr()


We possibly want something like this as a general flow step. It matters for basically all the test cases that mention &password in the source I think. @paldepind what do you think?

Agree that adding this as an additional flow step shouldn't be necessary.

I actually would've thought that this should work. If password is tainted then &password should have taint stored in the reference and this implicit read step at the sink should ensure that it is read. I can look into it and see why it doesn't work.

Yes, please do look into this, it looks like it should work to me as well.

I've written up an issue for this as I think it's worth investigating and there are definitely some improvements we can make.

geoffw0 · 2025-01-23T18:15:47Z

rust/ql/src/queries/security/CWE-312/CleartextLogging.ql

+  predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
+    // flow out from tuple content at sinks.
+    isSink(node) and
+    c.getAReadContent() instanceof TuplePositionContent


I couldn't get tuple content to work in the models-as-data sink itself, so this might have to do for now.

The Tuple[i] notation should work for summaries (there's a test here). But I'm not sure if it's also expected to work for sinks.

I tried the Tuple[i] notation and couldn't get it to work. I'm not sure I could've messed up the syntax, it's pretty simple. So best guess, it doesn't work on sinks at the moment???

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

github-actions · 2025-01-23T18:50:10Z

QHelp previews:

rust/ql/src/queries/security/CWE-312/CleartextLogging.qhelp

Cleartext logging of sensitive information

Sensitive user data and system information that is logged could be exposed to an attacker when it is displayed. Also, external processes often store the standard output and standard error streams of an application, which will include logged sensitive information.

Recommendation

Do not log sensitive data. If it is necessary to log sensitive data, encrypt it before logging.

Example

The following example code logs user credentials (in this case, their password) in plaintext:

let password = "P@ssw0rd";
info!("User password changed to {password}");

Instead, you should encrypt the credentials, or better still, omit them entirely:

let password = "P@ssw0rd";
info!("User password changed");

References

M. Dowd, J. McDonald and J. Schuhm, The Art of Software Security Assessment, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.
M. Howard and D. LeBlanc, Writing Secure Code, 2nd Edition, Chapter 9 - 'Protecting Secret Data', p. 299. Microsoft, 2002.
OWASP: Logging Cheat Sheet - Data to exclude.
Common Weakness Enumeration: CWE-312.
Common Weakness Enumeration: CWE-359.
Common Weakness Enumeration: CWE-532.

…issues).

rust/ql/test/query-tests/security/CWE-312/test_logging.rs

paldepind · 2025-01-24T12:07:49Z

rust/ql/src/queries/security/CWE-312/CleartextLogging.ql

+  predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
+    // flow out from tuple content at sinks.
+    isSink(node) and
+    c.getAReadContent() instanceof TuplePositionContent


The Tuple[i] notation should work for summaries (there's a test here). But I'm not sure if it's also expected to work for sinks.

paldepind · 2025-01-24T12:14:19Z

rust/ql/src/queries/security/CWE-312/CleartextLogging.ql

+
+  predicate isAdditionalFlowStep(Node node1, Node node2) {
+    // flow from `a` to `&a`
+    node2.asExpr().getExpr().(RefExpr).getExpr() = node1.asExpr().getExpr()


Agree that adding this as an additional flow step shouldn't be necessary.

I actually would've thought that this should work. If password is tainted then &password should have taint stored in the reference and this implicit read step at the sink should ensure that it is read. I can look into it and see why it doesn't work.

paldepind · 2025-01-24T12:28:30Z

rust/ql/test/query-tests/security/CWE-312/CONSISTENCY/ExtractionConsistency.expected

@@ -0,0 +1,3 @@
+extractionWarning
+| test_logging.rs:90:12:90:30 | expected R_PAREN |
+| test_logging.rs:90:12:90:30 | macro expansion failed: the macro '$crate::__private_api::format_args' expands to ERROR but a Expr was expected |


Looks like there's an error on line 90. Maybe it's the :?? I'm not familiar with the error! macro.

Ah, I had assumed this must be an extractor bug, but I never actually ran rustc on this code...

I've made the test runnable, and ran it. The syntax on line 90 is accepted by the compiler and produces the expected output (well, I think SimpleLogger is discarding the keys / values / structured part, presumably a more sophisticated logger would use it).

I see. Maybe it's because the key-value syntax requires the kv feature to be turned on, but the extractor always uses the default features.

Sounds like there's nothing wrong with the test then (it does turn the kv feature on). This probably explains why I couldn't get the results on lines 77 and 78 (which use the kv feature).

Yes, I think you're right :)

mchammer01

LGTM from a Docs perspective ✨
Added two minor suggestions for your consideration.

rust/ql/src/queries/security/CWE-312/CleartextLogging.qhelp

geoffw0 · 2025-01-24T17:04:08Z

DCA shows 19 query results:

6 false positives on diem: num_accounts, requested_accounts, total_requested_accounts are not actually account numbers.
1 false positive for rust: unaccounted is not an account number either.
the other 12 results look like a mix of TP and plausible TP results to me. 🎉

I'll address the clear FPs in a follow-up, since they're all shortcomings in the (shared) sensitive data heuristics.

…ay).

geoffw0 added 12 commits January 23, 2025 12:24

Rust: Add a test for the log crate + placeholder query.

bb3be2f

Rust: Add test cases for various std:: bits.

173cfd5

Rust: Implement the query.

4297d05

Rust: Add some sinks.

1d2950c

Rust: Model StdoutLock, StderrLock methods and String.as_bytes.

484331c

Rust: Model assert_failed.

2bbf493

Rust: Add taint sinks for target and key-value arguments.

6444494

Rust: Allow implicit taint reads from tuple contents at sinks.

78c58aa

Rust: Allow flow through reference taking (&).

59c3ac6

Rust: Add the sinks to metrics.

e708167

Rust: Add .qhelp and examples.

ccc1243

Rust: Clean up the query message.

4214c83

geoffw0 added no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code labels Jan 23, 2025

Copilot AI review requested due to automatic review settings January 23, 2025 18:04

github-actions bot added the documentation label Jan 23, 2025

Copilot AI reviewed Jan 23, 2025

View reviewed changes

rust/ql/src/queries/security/CWE-312/CleartextLoggingBad.rs Outdated Show resolved Hide resolved

Rust: Simplify QL slightly.

613a165

geoffw0 commented Jan 23, 2025

View reviewed changes

geoffw0 and others added 5 commits January 23, 2025 18:18

Update rust/ql/src/queries/security/CWE-312/CleartextLoggingBad.rs

5570523

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Rust: Minor fixes.

d27a71e

Rust: Add missing file.

951d1fc

Rust: Another .qhelp fix.

44b9a11

Rust: Accept changes to integration test results.

f5459d7

geoffw0 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jan 23, 2025

geoffw0 added 2 commits January 23, 2025 19:04

Merge remote-tracking branch 'upstream/main' into logging

814118d

Rust: Fix some more tests (MaD ID changes and extraction consistency …

037d496

…issues).

paldepind self-assigned this Jan 24, 2025

paldepind requested changes Jan 24, 2025

View reviewed changes

hubwriter added ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. and removed ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. labels Jan 24, 2025

mchammer01 self-requested a review January 24, 2025 16:07

mchammer01 previously approved these changes Jan 24, 2025

View reviewed changes

rust/ql/src/queries/security/CWE-312/CleartextLogging.qhelp Outdated Show resolved Hide resolved

rust/ql/src/queries/security/CWE-312/CleartextLogging.qhelp Outdated Show resolved Hide resolved

geoffw0 dismissed mchammer01’s stale review via ed5f19b January 24, 2025 16:34

Rust: Make the test runnable.

117db8a

geoffw0 force-pushed the logging branch from ed5f19b to 117db8a Compare January 24, 2025 17:24

Rust: Re-apply suggested changes (I accidentally force-pushed them aw…

0a3d44c

…ay).

paldepind approved these changes Jan 27, 2025

View reviewed changes

geoffw0 merged commit f32fd38 into github:main Jan 27, 2025
18 checks passed

geoffw0 deleted the logging branch September 26, 2025 13:02

Conversation

geoffw0 commented Jan 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffw0 Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cleartext logging of sensitive information

Recommendation

Example

References

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffw0 Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

geoffw0 commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

geoffw0 Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 23, 2025 •

edited

Loading

geoffw0 Jan 27, 2025 •

edited

Loading

geoffw0 commented Jan 24, 2025 •

edited

Loading