Skip to content
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

Rust Semgrep Rules #3151

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions rust/lang/security/audit/Insufficient-protected-credentials.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
use postgres::{Connection, TlsMode};

//This works but does it really cover the rule? We need to rethink this one
//ruleid: insufficient-protected-credentials
fn main() {
//The following code reads a password and uses the password to connect to a database.
let password = requests::get("http://example.org/getPassword").unwrap();
let url = format!(
"postgresql://postgres:{}@localhost:5433:2994/food",
password
);
let conn = Connection::connect(url, TlsMode::None).unwrap();
}
20 changes: 20 additions & 0 deletions rust/lang/security/audit/Insufficient-protected-credentials.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
rules:
- id: insufficient-protected-credentials
message: |
This is a Insufficiently Protected Credentials weakness: https://cwe.mitre.org/data/definitions/522.html
Consider using an appropriate security mechanism to protect the credentials (e.g. keeping secrets in environment variables)
Comment on lines +3 to +5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
message: |
This is a Insufficiently Protected Credentials weakness: https://cwe.mitre.org/data/definitions/522.html
Consider using an appropriate security mechanism to protect the credentials (e.g. keeping secrets in environment variables)
message: >-
This is an Insufficiently Protected Credentials weakness: https://cwe.mitre.org/data/definitions/522.html
Consider using an appropriate security mechanism to protect the credentials (e.g. keeping secrets in environment variables).

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the >- operator for messages. This together with some other guidelines regarding metadata are available in the docs.

metadata:
cwe: 'CWE-522: Insufficiently Protected Credentials'
owasp: 'A2: Broken Authentication'
source-rule-url: https://r2c.dev/blog/2020/hardcoded-secrets-unverified-tokens-and-other-common-jwt-mistakes/
pattern-either:
- patterns:
Comment on lines +10 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

some unnecessary operators here

- pattern: |
fn $FUNC(...) {
requests::$METHOD(...);
...
Connection::connect(...);
}

languages: [rust]
severity: WARNING
5 changes: 5 additions & 0 deletions rust/lang/security/audit/command-injection-process-builder.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
//ruleid: dangerous-spawn-process
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add True Negative tests, so tests indicated with //ok: <ruleid>

fn main() {
let a = requests::get("http://example.org/get").unwrap();
Command::new("ls").arg(a);
}
26 changes: 26 additions & 0 deletions rust/lang/security/audit/command-injection-process-builder.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
rules:
- id: dangerous-spawn-process
message: |
Found dynamic content when spawning a process. This is dangerous if external
data can reach this function call because it allows a malicious actor to
execute commands. Ensure no external data reaches here.
metadata:
cwe: "CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')"
owasp: 'A1: Injection'
asvs:
section: 'V5: Validation, Sanitization and Encoding Verification Requirements'
control_id: 5.3.8 OS Command Injection
control_url: https://github.com/OWASP/ASVS/blob/master/4.0/en/0x13-V5-Validation-Sanitization-Encoding.md#v53-output-encoding-and-injection-prevention-requirements
version: '4'
languages: [rust]
severity: WARNING
patterns:
- pattern-either:
- patterns:
Comment on lines +17 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary operators

- pattern: |
fn $FUNC(...) {
...
Command::new(...);
...
}

Comment on lines +20 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- pattern: |
fn $FUNC(...) {
...
Command::new(...);
...
}
- pattern: |
Command::new(...)
- pattern-not: |
Command::new("...")

Some suggestions:

  1. By removing the function declaration around the actual vulnerable line of code we can reduce the amount of lines being reported or marked. Instead of the entire function declaration, Semgrep will now report the exact line of the Command::new. You will need to update the test syntax accordingly! Additionally, this would also capture calls that do not end in a semicolon.

For example, a construction like this might be missed by your pattern (I didn't actually run it):

let output = if cfg!(target_os = "windows") {
    Command::new(cmd)
            .args(["/C", "echo hello"])
            .output()
            .expect("failed to execute process")
} else {
    Command::new("sh")
            .arg("-c")
            .arg("echo hello")
            .output()
            .expect("failed to execute process")
};

You should look up some uses of the functions and add them to the testcode with appropriate test syntax to verify this!

  1. I have added a pattern-not to exclude very obvious False Positives, like a hardcoded String. This will reduce the noise for this rule.

17 changes: 17 additions & 0 deletions rust/lang/security/audit/crypto/deprecated-ssl.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
fn set_min_version_ssl3() {
let mut acceptor = SslAcceptor::mozilla_intermediate(SslMethod::tls()).unwrap();
//ruleid: ssl-v3-is-insecure
acceptor.set_min_proto_version(Some(SslVersion::SSL3));
}

fn set_min_version_tls1() {
let mut acceptor = SslAcceptor::mozilla_intermediate(SslMethod::tls()).unwrap();
//ruleid: tls-v1-is-insecure
acceptor.set_min_proto_version(Some(SslVersion::TLS1));
}

fn set_min_version_tls1_1() {
let mut acceptor = SslAcceptor::mozilla_intermediate(SslMethod::tls()).unwrap();
//ruleid: tls-v1_1-is-insecure
acceptor.set_min_proto_version(Some(SslVersion::TLS1_1));
}
50 changes: 50 additions & 0 deletions rust/lang/security/audit/crypto/deprecated-ssl.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
rules:
- id: ssl-v3-is-insecure
message: |
SSLv3 is insecure because it has known vulnerabilities.
Instead, use 'SslVersion::TLS1_2 or SslVersion::TLS1_3'.
metadata:
cwe: 'CWE-327: Use of a Broken or Risky Cryptographic Algorithm'
owasp: 'A9: Using Components with Known Vulnerabilities'
references:
- https://golang.org/doc/go1.14#crypto/tls
- https://www.us-cert.gov/ncas/alerts/TA14-290A
languages: [rust]
severity: ERROR
fix-regex:
regex: SslVersion::SSL3
replacement: SslVersion::TLS1_3
Comment on lines +14 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are fixes, it is possible to add fix tests.

Create a new file called deprecated-ssl.fixed.rs with the expected outcome of running the autofix on the deprecated-ssl.rs file.

More info here: https://semgrep.dev/docs/writing-rules/testing-rules/#testing-autofix

fix-regex works but using regular fix will be more powerful, especially when we develop AST-based autofix for Rust. (https://semgrep.dev/blog/2022/autofixing-code-with-semgrep/)

If you use fix your rules will likely be more future-proof. Your rule would like something like this:

patterns:
  - pattern: |
        $VAR.set_min_proto_version(Some($TLS))
  - metavariable-regex:
        metavariable: $TLS
        regex: SslVersion::(SSL3|TLSv1|TLSv1_1)
  - focus-metavariable: $TLS
fix: |
  SslVersion::TLS1_3

This would allow you to combine all three rules into just one rule! You can even use the metavariable in the message if you want to still keep three separate messages:

message: >-
    $TLS is insecure ...

pattern: $VAR.set_min_proto_version(Some(SslVersion::SSL3))

- id: tls-v1-is-insecure
message: |
TLSv1 is insecure because it has known vulnerabilities.
Instead, use 'SslVersion::TLS1_2 or SslVersion::TLS1_3'.
metadata:
cwe: 'CWE-327: Use of a Broken or Risky Cryptographic Algorithm'
owasp: 'A9: Using Components with Known Vulnerabilities'
references:
- https://www.us-cert.gov/ncas/alerts/TA14-290A
languages: [rust]
severity: ERROR
fix-regex:
regex: SslVersion::TLSv1
replacement: SslVersion::TLS1_3
pattern: $VAR.set_min_proto_version(Some(SslVersion::TLS1))


- id: tls-v1_1-is-insecure
message: |
TLSv1_1 is insecure because it has known vulnerabilities.
Instead, use 'SslVersion::TLS1_2 or SslVersion::TLS1_3'.
metadata:
cwe: 'CWE-327: Use of a Broken or Risky Cryptographic Algorithm'
owasp: 'A9: Using Components with Known Vulnerabilities'
references:
- https://www.us-cert.gov/ncas/alerts/TA14-290A
languages: [rust]
severity: ERROR
fix-regex:
regex: SslVersion::TLSv1_1
replacement: SslVersion::TLS1_3
pattern: $VAR.set_min_proto_version(Some(SslVersion::TLS1_1))
15 changes: 15 additions & 0 deletions rust/lang/security/audit/crypto/math-random.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
fn random() {
//ruleid: math-random-used
let x = rand::random();
}
fn thread_rng() {
//ruleid: math-random-used
let x = rand::thread_rng();
}

fn thread_rng_1() {
//todoruleid: math-random-used
use rand::thread_rng;
//todoruleid: math-random-used
let _ = thread_rng();
Comment on lines +13 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

Semgrep does some symbolic propagation and should be aware enough for this case to be captured with your current version of the rule:

https://semgrep.dev/playground/s/N0oy

}
30 changes: 30 additions & 0 deletions rust/lang/security/audit/crypto/math-random.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
rules:
- id: math-random-used
metadata:
cwe: 'CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)'
owasp: 'A3: Sensitive Data Exposure'
references:
- https://cheatsheetseries.owasp.org/cheatsheets/Cryptographic_Storage_Cheat_Sheet.html#secure-random-number-generation
message: |
The product uses a Pseudo-Random Number Generator (PRNG) in a security context,
but the PRNG's algorithm is not cryptographically strong.
languages: [rust]
severity: WARNING

pattern-either:
- pattern: rand::thread_rng()
- pattern: rand::random()
# issues with pattern inside
# - pattern: |
# use rand::thread_rng;
# ...

# - pattern-not-inside: |
# use rand::CryptoRng;
# ...
# - pattern-not-inside: |
# use rand_core::StdRng;
# ...
# - pattern-not-inside: |
# use rand_core::OsRng;
# ...
Comment on lines +17 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem here is that you have nested them under the pattern-either, this operator works like an OR operator.

Your pattern-inside is written correctly and works as expected.
https://semgrep.dev/playground/s/kJPP

Please add testcases for these pattern-not-inside cases to your testcode.

33 changes: 33 additions & 0 deletions rust/lang/security/audit/crypto/reversible-hash.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
use openssl::hash::MessageDigest;
use openssl::pkey::PKey;
use openssl::rsa::Rsa;
use openssl::sign::Signer;

// Use of unsecure sha1 as MessageDigest
fn reversible_hash_1() {
//ruleid: reversible-hash
let msg_digest = MessageDigest::from_name("sha1").unwrap();

let keypair = PKey::from_rsa(Rsa::generate(2048).unwrap()).unwrap();
let signer = Signer::new(msg_digest, &keypair).unwrap();
let signature = signer.sign_to_vec().unwrap();

println!("signature = {:?}", signature);
}

// Use of unsecure sha1 as MessageDigest
fn reversible_hash_2() {
//ruleid: reversible-hash
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add negative test cases for approved hash algortihms not flagged by this rule.

let msg_digest = MessageDigest::sha1();

let keypair = PKey::from_rsa(Rsa::generate(2048).unwrap()).unwrap();
let signer = Signer::new(msg_digest, &keypair).unwrap();
let signature = signer.sign_to_vec().unwrap();

println!("signature = {:?}", signature);
}

fn main() {
reversible_hash_1();
reversible_hash_2();
}
21 changes: 21 additions & 0 deletions rust/lang/security/audit/crypto/reversible-hash.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
rules:
- id: reversible-hash
message: |
The product uses a hashing algorithm that produces a hash value that can be used
to determine the original input, or to find an input that can produce the same hash,
more efficiently than brute force techniques. This weakness is especially dangerous
when the hash is used in security algorithms that require the one-way property to
hold. For example, if an authentication system takes an incoming password and
generates a hash, then compares the hash to another hash that it has stored in its
authentication database, then the ability to create a collision could allow an attacker
to provide an alternate password that produces the same target hash, bypassing authentication.
Comment on lines +9 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be a recommended solution?

languages: [rust]
severity: WARNING
metadata:
owasp: 'A9: Using Components with Known Vulnerabilities'
cwe: 'CWE-328: Reversible One-Way Hash'
pattern-either:
- pattern: |
MessageDigest::from_name("sha1")
- pattern: |
MessageDigest::sha1()
32 changes: 32 additions & 0 deletions rust/lang/security/audit/crypto/tls-insecure-cipher.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
fn tls_insecure_cipher() {
//ruleid: tls-with-insecure-cipher
acceptor
.set_ciphersuites(
"TLS_AES_256_GCM_SHA384:TLS_AES_128_GCM_SHA256:TLS_CHACHA20_POLY1305_SHA256",
)
.unwrap();
}

fn tls_insecure_cipher_rsa_rc4_sha() {
//ruleid: tls-with-insecure-cipher
acceptor
.set_ciphersuites(
"TLS_RSA_WITH_RC4_128_SHA:TLS_AES_128_GCM_SHA256:TLS_CHACHA20_POLY1305_SHA256",
)
.unwrap();
}

fn tls_insecure_cipher_rsa_acs_128() {
//ruleid: tls-with-insecure-cipher
acceptor
.set_ciphersuites(
"TLS_RSA_WITH_AES_128_CBC_SHA256:TLS_AES_128_GCM_SHA256:TLS_CHACHA20_POLY1305_SHA256",
)
.unwrap();
}

fn tls_insecure_cipher_all() {
//ruleid: tls-with-insecure-cipher
acceptor.set_ciphersuites(
"TLS_RSA_WITH_RC4_128_SHA:TLS_RSA_WITH_AES_128_CBC_SHA256:TLS_RSA_WITH_AES_128_CBC_SHA256:TLS_ECDHE_ECDSA_WITH_RC4_128_SHA:TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256:TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256").unwrap();
}
14 changes: 14 additions & 0 deletions rust/lang/security/audit/crypto/tls-insecure-cipher.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
rules:
- id: tls-with-insecure-cipher
message: |
Detected an insecure CipherSuite via the 'tls' module. This suite is considered weak.
Use the function 'acceptor.set_ciphersuites()' to get a list of good cipher suites.
metadata:
cwe: 'CWE-327: Use of a Broken or Risky Cryptographic Algorithm'
owasp: 'A9: Using Components with Known Vulnerabilities'
languages: [rust]
severity: WARNING
pattern-either:
- pattern: |
$VAR.set_ciphersuites("=~/(TLS_RSA_WITH_RC4_128_SHA|TLS_RSA_WITH_AES_128_CBC_SHA256|TLS_RSA_WITH_AES_128_CBC_SHA256|TLS_ECDHE_ECDSA_WITH_RC4_128_SHA|TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256|TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256|TLS_AES_256_GCM_SHA384)/")
Comment on lines +11 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pattern-either:
- pattern: |
$VAR.set_ciphersuites("=~/(TLS_RSA_WITH_RC4_128_SHA|TLS_RSA_WITH_AES_128_CBC_SHA256|TLS_RSA_WITH_AES_128_CBC_SHA256|TLS_ECDHE_ECDSA_WITH_RC4_128_SHA|TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256|TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256|TLS_AES_256_GCM_SHA384)/")
patterns:
- pattern: |
$VAR.set_ciphersuites($CIPHER)
- metavariable-regex:
metavariable: $CIPHER
regex: (TLS_RSA_WITH_RC4_128_SHA|TLS_RSA_WITH_AES_128_CBC_SHA256|TLS_RSA_WITH_AES_128_CBC_SHA256|TLS_ECDHE_ECDSA_WITH_RC4_128_SHA|TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256|TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256|TLS_AES_256_GCM_SHA384)
fix: |
TLS_AES_128_GCM_SHA256

Feels like a rule that could easily have a fix. Mind adding one, as well as a fixtest? I've made a suggestion that might help you in the right direction.

Copy link
Author

Choose a reason for hiding this comment

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

I was reading through the docs and i couldnt find anything about fix tests? i was also looking for examples in the other rules.

Copy link
Author

Choose a reason for hiding this comment

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

I saw the higher comment now


12 changes: 12 additions & 0 deletions rust/lang/security/audit/crypto/use-of-des.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Simplest to just block the import of `des` as its interface has changed quite a bit from 0.0.1 to current
pub mod a {
#[allow(unused_imports)]
//ruleid: use-of-DES
use des;
}

pub mod b {
#[allow(unused_imports)]
//ruleid: use-of-DES
use des::Des;
}
15 changes: 15 additions & 0 deletions rust/lang/security/audit/crypto/use-of-des.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
rules:
- id: use-of-DES
message: |
Detected DES cipher algorithm which is insecure. The algorithm is
considered weak and has been deprecated. Use AES instead.
languages: [rust]
severity: WARNING
metadata:
owasp: 'A9: Using Components with Known Vulnerabilities'
cwe: 'CWE-327: Use of a Broken or Risky Cryptographic Algorithm'
references:
- https://docs.rs/des/latest/des/
pattern-either:
- pattern: use des;
- pattern: use des::Des;
Comment on lines +13 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pattern-either:
- pattern: use des;
- pattern: use des::Des;
pattern-either:
- pattern: use des;
- pattern: use des::Des;
fix: |
use aes::Aes128;

Would probably be easy to add a fix here. Can you add a fix test as well?

67 changes: 67 additions & 0 deletions rust/lang/security/audit/crypto/use-of-md2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
pub mod a {
use md2::{Digest, Md2};

pub fn ex() {
let a: [u8; 16] = [0; 16];
//ruleid: use-of-md2
let mut mymd2 = Md2::new();
mymd2.update(a);
println!("{:#?}", mymd2.finalize());
}
}

pub mod b {
use md2::Digest;

pub fn ex() {
let a: [u8; 16] = [0; 16];
//ruleid: use-of-md2
let mut mymd2 = md2::Md2::new();
mymd2.update(a);
println!("{:#?}", mymd2.finalize());
}
}

pub mod c {
use md2::{Digest, Md2};

pub fn ex() {
let a: [u8; 16] = [0; 16];
//ruleid: use-of-md2
let mymd2 = Md2::new_with_prefix(a);
println!("{:#?}", mymd2.finalize());
}
}

pub mod d {
use md2::Digest;

pub fn ex() {
let a: [u8; 16] = [0; 16];
//ruleid: use-of-md2
let mymd2 = md2::Md2::new_with_prefix(a);
println!("{:#?}", mymd2.finalize());
}
}

pub mod e {
use md2::Digest;

pub fn ex() {
let a: [u8; 16] = [0; 16];
//ruleid: use-of-md2
let mymd2 = md2::Md2::digest(a);
println!("{:#?}", mymd2);
}
}

pub mod f {
use md2::{Digest, Md2};

pub fn ex() {
let a: [u8; 16] = [0; 16];
//ruleid: use-of-md2
let mymd2 = Md2::digest(a);
println!("{:#?}", mymd2);
}
}
Loading
Loading