Rust: Query for cleartext logging of sensitive information #18582
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| extensions: | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sinkModel | ||
| data: | ||
| - ["repo:https://github.com/rust-lang/log:log", "crate::__private_api::log", "Argument[0]", "log-injection", "manual"] # args | ||
| - ["repo:https://github.com/rust-lang/log:log", "crate::__private_api::log", "Argument[2]", "log-injection", "manual"] # target | ||
| - ["repo:https://github.com/rust-lang/log:log", "crate::__private_api::log", "Argument[3]", "log-injection", "manual"] # key value | ||
| - ["lang:std", "crate::io::stdio::_print", "Argument[0]", "log-injection", "manual"] | ||
| - ["lang:std", "crate::io::stdio::_eprint", "Argument[0]", "log-injection", "manual"] | ||
| - ["lang:std", "<crate::io::stdio::StdoutLock as crate::io::Write>::write", "Argument[0]", "log-injection", "manual"] | ||
| - ["lang:std", "<crate::io::stdio::StdoutLock as crate::io::Write>::write_all", "Argument[0]", "log-injection", "manual"] | ||
| - ["lang:std", "<crate::io::stdio::StderrLock as crate::io::Write>::write", "Argument[0]", "log-injection", "manual"] | ||
| - ["lang:std", "<crate::io::stdio::StderrLock as crate::io::Write>::write_all", "Argument[0]", "log-injection", "manual"] | ||
| - ["lang:core", "crate::panicking::panic_fmt", "Argument[0]", "log-injection", "manual"] | ||
| - ["lang:core", "crate::panicking::assert_failed", "Argument[3].Variant[crate::option::Option::Some(0)]", "log-injection", "manual"] | ||
| - ["lang:core", "<crate::option::Option>::expect", "Argument[0]", "log-injection", "manual"] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /** | ||
| * Provides classes and predicates for reasoning about cleartext logging | ||
| * of sensitive information vulnerabilities. | ||
| */ | ||
|
|
||
| import rust | ||
| private import codeql.rust.dataflow.DataFlow | ||
| private import codeql.rust.dataflow.internal.DataFlowImpl | ||
| private import codeql.rust.security.SensitiveData | ||
|
|
||
| /** | ||
| * Provides default sources, sinks and barriers for detecting cleartext logging | ||
| * vulnerabilities, as well as extension points for adding your own. | ||
| */ | ||
| module CleartextLogging { | ||
| /** | ||
| * A data flow source for cleartext logging vulnerabilities. | ||
| */ | ||
| abstract class Source extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A data flow sink for cleartext logging vulnerabilities. | ||
| */ | ||
| abstract class Sink extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A barrier for cleartext logging vulnerabilities. | ||
| */ | ||
| abstract class Barrier extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * Sensitive data, considered as a flow source. | ||
| */ | ||
| private class SensitiveDataAsSource extends Source instanceof SensitiveData { } | ||
|
|
||
| /** A sink for logging from model data. */ | ||
| private class ModelsAsDataSinks extends Sink { | ||
| ModelsAsDataSinks() { exists(string s | sinkNode(this, s) and s.matches("log-injection%")) } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p> | ||
| 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. | ||
| </p> | ||
| </overview> | ||
|
|
||
| <recommendation> | ||
| <p> | ||
| Do not log sensitive data. If it is necessary to log sensitive data, encrypt it before logging. | ||
| </p> | ||
| </recommendation> | ||
|
|
||
| <example> | ||
| <p> | ||
| The following example code logs user credentials (in this case, their password) in plaintext: | ||
| </p> | ||
| <sample src="CleartextLoggingBad.rs"/> | ||
| <p> | ||
| Instead, you should encrypt the credentials, or better still, omit them entirely: | ||
| </p> | ||
| <sample src="CleartextLoggingGood.rs"/> | ||
| </example> | ||
|
|
||
| <references> | ||
|
|
||
| <li>M. Dowd, J. McDonald and J. Schuhm, <i>The Art of Software Security Assessment</i>, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.</li> | ||
| <li>M. Howard and D. LeBlanc, <i>Writing Secure Code</i>, 2nd Edition, Chapter 9 - 'Protecting Secret Data', p. 299. Microsoft, 2002.</li> | ||
| <li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html#data-to-exclude">Logging Cheat Sheet - Data to exclude</a>.</li> | ||
|
|
||
| </references> | ||
| </qhelp> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /** | ||
| * @name Cleartext logging of sensitive information | ||
| * @description Logging sensitive information in plaintext can | ||
| * expose it to an attacker. | ||
| * @kind path-problem | ||
| * @problem.severity error | ||
| * @security-severity 7.5 | ||
| * @precision high | ||
| * @id rust/cleartext-logging | ||
| * @tags security | ||
| * external/cwe/cwe-312 | ||
| * external/cwe/cwe-359 | ||
| * external/cwe/cwe-532 | ||
| */ | ||
|
|
||
| import rust | ||
| import codeql.rust.security.CleartextLoggingExtensions | ||
| import codeql.rust.dataflow.DataFlow | ||
| import codeql.rust.dataflow.TaintTracking | ||
| import codeql.rust.dataflow.internal.DataFlowImpl | ||
|
|
||
| /** | ||
| * A taint-tracking configuration for cleartext logging vulnerabilities. | ||
| */ | ||
| module CleartextLoggingConfig implements DataFlow::ConfigSig { | ||
| import CleartextLogging | ||
|
|
||
| predicate isSource(DataFlow::Node source) { source instanceof Source } | ||
|
|
||
| predicate isSink(DataFlow::Node sink) { sink instanceof Sink } | ||
|
|
||
| predicate isBarrier(DataFlow::Node barrier) { barrier instanceof Barrier } | ||
|
|
||
| predicate isBarrierIn(DataFlow::Node node) { | ||
| // make sources barriers so that we only report the closest instance | ||
| isSource(node) | ||
| } | ||
|
|
||
| predicate isAdditionalFlowStep(Node node1, Node node2) { | ||
| // flow from `a` to `&a` | ||
| node2.asExpr().getExpr().(RefExpr).getExpr() = node1.asExpr().getExpr() | ||
| } | ||
|
|
||
| predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) { | ||
| // flow out from tuple content at sinks. | ||
| isSink(node) and | ||
| c.getAReadContent() instanceof TuplePositionContent | ||
|
There was a problem hiding this comment. I couldn't get tuple content to work in the models-as-data sink itself, so this might have to do for now. There was a problem hiding this comment. The There was a problem hiding this comment. I tried the |
||
| } | ||
| } | ||
|
|
||
| module CleartextLoggingFlow = TaintTracking::Global<CleartextLoggingConfig>; | ||
|
|
||
| import CleartextLoggingFlow::PathGraph | ||
|
|
||
| from CleartextLoggingFlow::PathNode source, CleartextLoggingFlow::PathNode sink | ||
| where CleartextLoggingFlow::flowPath(source, sink) | ||
| select sink.getNode(), source, sink, "This operation writes $@ to a log file.", source, | ||
| source.getNode().toString() | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| let password = "P@ssw0rd"; | ||
| info!("User password changed to {password}"); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| let password = "P@ssw0rd"; | ||
| info!("User password changed"); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | | ||
|
There was a problem hiding this comment. Looks like there's an error on line 90. Maybe it's the There was a problem hiding this comment. Ah, I had assumed this must be an extractor bug, but I never actually ran There was a problem hiding this comment. 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 There was a problem hiding this comment. I see. Maybe it's because the key-value syntax requires the There was a problem hiding this comment. 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). There was a problem hiding this comment. Yes, I think you're right :) |
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We possibly want something like this as a general flow step. It matters for basically all the test cases that mention
&passwordin the source I think. @paldepind what do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that adding this as an additional flow step shouldn't be necessary.
I actually would've thought that this should work. If
passwordis tainted then&passwordshould 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please do look into this, it looks like it should work to me as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've written up an issue for this as I think it's worth investigating and there are definitely some improvements we can make.