Rust: New Query rust/cleartext-storage-database #20137
+3,167
−96
There are no files selected for viewing
This file contains hidden or 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 hidden or 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 hidden or 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 hidden or 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,13 @@ | ||
| extensions: | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sinkModel | ||
| data: | ||
| - ["sqlx_core::query::query", "Argument[0]", "sql-injection", "manual"] | ||
| - ["sqlx_core::query_as::query_as", "Argument[0]", "sql-injection", "manual"] | ||
| - ["sqlx_core::query_with::query_with", "Argument[0]", "sql-injection", "manual"] | ||
| - ["sqlx_core::query_as_with::query_as_with", "Argument[0]", "sql-injection", "manual"] | ||
| - ["sqlx_core::query_scalar::query_scalar", "Argument[0]", "sql-injection", "manual"] | ||
| - ["sqlx_core::query_scalar_with::query_scalar_with", "Argument[0]", "sql-injection", "manual"] | ||
| - ["sqlx_core::raw_sql::raw_sql", "Argument[0]", "sql-injection", "manual"] | ||
| - ["<_ as sqlx_core::executor::Executor>::execute", "Argument[0]", "sql-injection", "manual"] |
This file contains hidden or 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,48 @@ | ||
| /** | ||
| * 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. | ||
| * - SQL commands | ||
| * - other database storage operations | ||
| */ | ||
| private class ModelsAsDataSink extends Sink { | ||
| ModelsAsDataSink() { sinkNode(this, ["sql-injection", "database-store"]) } | ||
| } | ||
| } | ||
This file contains hidden or 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,4 @@ | ||
| --- | ||
| category: newQuery | ||
| --- | ||
| * Added a new query, `rust/cleartext-storage-database`, for detecting cases where sensitive information is stored non-encrypted in a database. |
This file contains hidden or 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,53 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p> | ||
| Sensitive information that is stored unencrypted in a database is accessible to an attacker | ||
| who gains access to that database. For example, the information could be accessed by any | ||
| process or user in a rooted device, or exposed through another vulnerability. | ||
| </p> | ||
| </overview> | ||
|
|
||
| <recommendation> | ||
| <p> | ||
| Either encrypt the entire database, or ensure that each piece of sensitive information is | ||
| encrypted before being stored. In general, decrypt sensitive information only at the point | ||
| where it is necessary for it to be used in cleartext. Avoid storing sensitive information | ||
| at all if you do not need to keep it. | ||
| </p> | ||
| </recommendation> | ||
|
|
||
| <example> | ||
| <p> | ||
| The following example stores sensitive information into a database without encryption, using the | ||
| SQLx library: | ||
| </p> | ||
| <sample src="CleartextStorageDatabaseBad.rs"/> | ||
| <p> | ||
| This is insecure because the sensitive data is stored in cleartext, making it accessible to anyone | ||
| with access to the database. | ||
| </p> | ||
| <p> | ||
| To fix this, we can either encrypt the entire database or encrypt just the sensitive data before it | ||
| is stored. Take care to select a secure modern encryption algorithm and put suitable key management | ||
| practices into place. In the following example, we have encrypted the sensitive data using 256-bit | ||
| AES before storing it in the database: | ||
| </p> | ||
| <sample src="CleartextStorageDatabaseGood.rs"/> | ||
| </example> | ||
|
|
||
| <references> | ||
| <li> | ||
| OWASP Top 10:2021: | ||
| <a href="https://owasp.org/Top10/A02_2021-Cryptographic_Failures/">A02:2021 - Cryptographic Failures</a>. | ||
| </li> | ||
| <li> | ||
| OWASP: | ||
| <a href="https://cheatsheetseries.owasp.org/cheatsheets/Key_Management_Cheat_Sheet.html">Key Management Cheat Sheet</a>. | ||
| </li> | ||
| </references> | ||
|
|
||
| </qhelp> |
This file contains hidden or 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,54 @@ | ||
| /** | ||
| * @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 7.5 | ||
| * @precision high | ||
| * @id rust/cleartext-storage-database | ||
| * @tags security | ||
| * external/cwe/cwe-312 | ||
| */ | ||
|
|
||
| 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. 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. |
||
| } | ||
|
|
||
| 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.getNode(), sourceNode, sinkNode, | ||
| "This database operation may read or write unencrypted sensitive data from $@.", sourceNode, | ||
| sourceNode.getNode().toString() | ||
This file contains hidden or 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,6 @@ | ||
| let query = "INSERT INTO PAYMENTDETAILS(ID, CARDNUM) VALUES(?, ?)"; | ||
| let result = sqlx::query(query) | ||
| .bind(id) | ||
| .bind(credit_card_number) // BAD: Cleartext storage of sensitive data in the database | ||
| .execute(pool) | ||
| .await?; |
This file contains hidden or 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,42 @@ | ||
|
|
||
|
|
||
| fn encrypt(text: String, encryption_key: &aes_gcm::Key<Aes256Gcm>) -> String { | ||
| // encrypt text -> ciphertext | ||
| let cipher = Aes256Gcm::new(&encryption_key); | ||
| let nonce = Aes256Gcm::generate_nonce(&mut OsRng); | ||
| let ciphertext = cipher.encrypt(&nonce, text.as_ref()).unwrap(); | ||
|
|
||
| // append (nonce, ciphertext) | ||
| let mut combined = nonce.to_vec(); | ||
| combined.extend(ciphertext); | ||
|
|
||
| // encode to base64 string | ||
| BASE64_STANDARD.encode(combined) | ||
| } | ||
|
|
||
| fn decrypt(data: String, encryption_key: &aes_gcm::Key<Aes256Gcm>) -> String { | ||
|
There was a problem hiding this comment. Is there a good reason to include this function (not called)? |
||
| let cipher = Aes256Gcm::new(&encryption_key); | ||
|
|
||
| // decode base64 string | ||
| let decoded = BASE64_STANDARD.decode(data).unwrap(); | ||
|
|
||
| // split into (nonce, ciphertext) | ||
| let nonce_size = <Aes256Gcm as AeadCore>::NonceSize::to_usize(); | ||
| let (nonce, ciphertext) = decoded.split_at(nonce_size); | ||
|
|
||
| // decrypt ciphertext -> plaintext | ||
| let plaintext = cipher.decrypt(nonce.into(), ciphertext).unwrap(); | ||
| String::from_utf8(plaintext).unwrap() | ||
| } | ||
|
|
||
| ... | ||
|
|
||
| let encryption_key = Aes256Gcm::generate_key(OsRng); | ||
|
|
||
| ... | ||
|
|
||
| let query = "INSERT INTO PAYMENTDETAILS(ID, CARDNUM) VALUES(?, ?)"; | ||
| let result = sqlx::query(query) | ||
| .bind(id) | ||
| .bind(encrypt(credit_card_number, &encryption_key)) // GOOD: Encrypted storage of sensitive data in the database | ||
| .execute(pool) | ||
| .await?; | ||
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.
nit: Oxford comma