-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: New Query rust/cleartext-storage-database #20137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
9972aaf
897822d
5c64d4e
a3110a9
e585e67
215fe7d
b6e60e4
42ced8a
f1cb1a3
989b48d
e368ee4
a86479e
836f797
b60faad
def655f
eab7481
c8e9ed3
83ec1d0
e991aa3
1965fdb
3382d06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
extensions: | ||
- addsTo: | ||
pack: codeql/rust-all | ||
extensible: sinkModel | ||
data: | ||
- ["sqlx_core::query::query", "Argument[0]", "database-store", "manual"] | ||
- ["sqlx_core::query_as::query_as", "Argument[0]", "database-store", "manual"] | ||
- ["sqlx_core::query_with::query_with", "Argument[0]", "database-store", "manual"] | ||
- ["sqlx_core::query_as_with::query_as_with", "Argument[0]", "database-store", "manual"] | ||
- ["sqlx_core::query_scalar::query_scalar", "Argument[0]", "database-store", "manual"] | ||
- ["sqlx_core::query_scalar_with::query_scalar_with", "Argument[0]", "database-store", "manual"] | ||
- ["sqlx_core::raw_sql::raw_sql", "Argument[0]", "database-store", "manual"] | ||
- ["<_ as sqlx_core::executor::Executor>::execute", "Argument[0]", "database-store", "manual"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/** | ||
* Provides classes and predicates for reasoning about cleartext storage | ||
* of sensitive information in a database. | ||
*/ | ||
|
||
import rust | ||
private import codeql.rust.dataflow.DataFlow | ||
private import codeql.rust.dataflow.internal.DataFlowImpl | ||
private import codeql.rust.security.SensitiveData | ||
private import codeql.rust.Concepts | ||
|
||
/** | ||
* Provides default sources, sinks and barriers for detecting cleartext storage | ||
* of sensitive information in a database, as well as extension points for | ||
* adding your own. | ||
*/ | ||
module CleartextStorageDatabase { | ||
/** | ||
* A data flow source for cleartext storage vulnerabilities. | ||
*/ | ||
abstract class Source extends DataFlow::Node { } | ||
|
||
/** | ||
* A data flow sink for cleartext storage vulnerabilities. | ||
*/ | ||
abstract class Sink extends QuerySink::Range { | ||
override string getSinkType() { result = "CleartextStorageDatabase" } | ||
} | ||
|
||
/** | ||
* A barrier for cleartext storage vulnerabilities. | ||
*/ | ||
abstract class Barrier extends DataFlow::Node { } | ||
|
||
/** | ||
* Sensitive data, considered as a flow source. | ||
*/ | ||
private class SensitiveDataAsSource extends Source instanceof SensitiveData { } | ||
|
||
/** | ||
* A sink for cleartext storage vulnerabilities from model data. | ||
*/ | ||
private class ModelsAsDataSink extends Sink { | ||
ModelsAsDataSink() { exists(string s | sinkNode(this, s) and s.matches("database-store")) } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
/** | ||
* @name Cleartext storage of sensitive information in a database | ||
* @description Storing sensitive information in a non-encrypted | ||
* database can expose it to an attacker. | ||
* @kind path-problem | ||
* @problem.severity warning | ||
* @security-severity TODO | ||
* @precision high | ||
* @id rust/cleartext-storage-database | ||
* @tags security | ||
* external/cwe/cwe-312 | ||
*/ | ||
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Show resolved
Hide resolved
|
||
|
||
import rust | ||
import codeql.rust.dataflow.DataFlow | ||
import codeql.rust.dataflow.TaintTracking | ||
import codeql.rust.security.CleartextStorageDatabaseExtensions | ||
|
||
/** | ||
* A taint configuration from sensitive information to expressions that are | ||
* stored in a database. | ||
*/ | ||
module CleartextStorageDatabaseConfig implements DataFlow::ConfigSig { | ||
import CleartextStorageDatabase | ||
|
||
predicate isSource(DataFlow::Node node) { node instanceof Source } | ||
|
||
predicate isSink(DataFlow::Node node) { node 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(DataFlow::Node node1, DataFlow::Node node2) { | ||
// flow from `a` to `&a` | ||
node2.asExpr().getExpr().(RefExpr).getExpr() = node1.asExpr().getExpr() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this step needed? It will already give rise to a store step, and usually it is not a good idea to have store steps as taint steps as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a workaround for dataflow failures involving implicit dereferences, which are very common for this query. One of the other queries has something similar. I hope to remove them eventually, as I expect it is bad for performance. |
||
} | ||
|
||
predicate observeDiffInformedIncrementalMode() { any() } | ||
} | ||
|
||
module CleartextStorageDatabaseFlow = TaintTracking::Global<CleartextStorageDatabaseConfig>; | ||
|
||
import CleartextStorageDatabaseFlow::PathGraph | ||
|
||
from | ||
CleartextStorageDatabaseFlow::PathNode sourceNode, CleartextStorageDatabaseFlow::PathNode sinkNode | ||
where CleartextStorageDatabaseFlow::flowPath(sourceNode, sinkNode) | ||
select sinkNode, sourceNode, sinkNode, | ||
"This operation stores '" + sinkNode.toString() + | ||
"' in a database. It may contain unencrypted sensitive data from $@.", sourceNode, | ||
sourceNode.getNode().toString() | ||
|
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.
nit: Oxford comma
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 thought no comma here was the standard. Happy to be corrected (in which case there are at least 10 cases of this to update).