-
Notifications
You must be signed in to change notification settings - Fork 5
Add cryptography-random-usage opengrep rule for Chromium #864
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
base: main
Are you sure you want to change the base?
Changes from all commits
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,99 @@ | ||
| // Test cases for cryptography-random-usage rule | ||
| #include <cstdlib> | ||
| #include <random> | ||
| #include "base/rand_util.h" | ||
| #include "crypto/random.h" | ||
| #include "third_party/boringssl/src/include/openssl/rand.h" | ||
| #include "third_party/boringssl/src/include/openssl/evp.h" | ||
|
|
||
| class CryptoUsageExamples { | ||
| public: | ||
| void BadRandomUsage() { | ||
| // SHOULD TRIGGER: Weak C-style random functions (insecure) | ||
| // ruleid: chromium-cryptography-random-usage | ||
| int weak_random1 = rand(); | ||
| // ruleid: chromium-cryptography-random-usage | ||
| srand(time(nullptr)); | ||
|
Contributor
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. reported by reviewdog 🐶
Contributor
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. reported by reviewdog 🐶 |
||
| // ruleid: chromium-cryptography-random-usage | ||
| int weak_random2 = random(); | ||
|
Contributor
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. reported by reviewdog 🐶
Contributor
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. reported by reviewdog 🐶 |
||
| // ruleid: chromium-cryptography-random-usage | ||
| srandom(12345); | ||
|
Contributor
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. reported by reviewdog 🐶
Contributor
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. reported by reviewdog 🐶 |
||
|
|
||
| // SHOULD TRIGGER: C++ std random functions (potentially weak) | ||
| // ruleid: chromium-cryptography-random-usage | ||
| int weak_random3 = std::rand(); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| // ruleid: chromium-cryptography-random-usage | ||
| std::srand(42); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| } | ||
|
|
||
| void ChromiumRandomUsage() { | ||
| // SHOULD TRIGGER: Chromium random functions (need security review) | ||
| // ruleid: chromium-cryptography-random-usage | ||
| int random_int = base::RandInt(1, 100); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| // ruleid: chromium-cryptography-random-usage | ||
| uint64_t random_uint64 = base::RandUint64(); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
|
|
||
| uint8_t buffer[32]; | ||
| // ruleid: chromium-cryptography-random-usage | ||
| base::RandGenerator(sizeof(buffer)); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| } | ||
|
|
||
| void CryptographicUsage() { | ||
| // SHOULD TRIGGER: Cryptographic random functions (need security review) | ||
| uint8_t crypto_buffer[32]; | ||
| // ruleid: chromium-cryptography-random-usage | ||
| crypto::RandBytes(crypto_buffer, sizeof(crypto_buffer)); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
|
|
||
| // SHOULD TRIGGER: OpenSSL random functions (need security review) | ||
| uint8_t ssl_buffer[16]; | ||
| // ruleid: chromium-cryptography-random-usage | ||
| RAND_bytes(ssl_buffer, sizeof(ssl_buffer)); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
|
|
||
| // SHOULD TRIGGER: Encryption/Decryption operations (need security review) | ||
| // ruleid: chromium-cryptography-random-usage | ||
| EVP_CIPHER_CTX* ctx = EVP_CIPHER_CTX_new(); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| // ruleid: chromium-cryptography-random-usage | ||
| EVP_EncryptInit(ctx, EVP_aes_256_gcm(), key_, iv_); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| // ruleid: chromium-cryptography-random-usage | ||
| EVP_DecryptInit(ctx, EVP_aes_256_gcm(), key_, iv_); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| } | ||
|
|
||
| void BadStdRandomUsage() { | ||
| // SHOULD TRIGGER: std::random engines/generators are banned per Chromium style guide | ||
| // Use base::RandomBitGenerator instead | ||
| // ruleid: chromium-cryptography-random-usage | ||
| std::random_device rd; | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| // ruleid: chromium-cryptography-random-usage | ||
| std::mt19937 gen(rd()); | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| std::uniform_int_distribution<> dis(1, 6); | ||
| int dice_roll = dis(gen); | ||
|
|
||
| // ruleid: chromium-cryptography-random-usage | ||
| std::default_random_engine engine; | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| } | ||
|
|
||
| void AcceptableUsage() { | ||
| // SHOULD NOT TRIGGER: Correct usage with base::RandomBitGenerator | ||
| base::RandomBitGenerator rng; | ||
| std::uniform_int_distribution<> dis(1, 6); | ||
| int dice_roll = dis(rng); | ||
|
|
||
| // SHOULD NOT TRIGGER: Hash functions (different security concern) | ||
| std::hash<std::string> hasher; | ||
| size_t hash = hasher("some string"); | ||
|
|
||
| // SHOULD NOT TRIGGER: Time-based operations | ||
| auto now = std::chrono::steady_clock::now(); | ||
| auto timestamp = now.time_since_epoch().count(); | ||
| } | ||
|
|
||
| void TestHelperUsage() { | ||
| // This would be excluded by test path filter | ||
| // ruleid: chromium-cryptography-random-usage | ||
| int test_random = rand(); // OK in tests | ||
|
Contributor
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. reported by reviewdog 🐶 |
||
| } | ||
|
|
||
| private: | ||
| uint8_t key_[32] = {}; | ||
| uint8_t iv_[16] = {}; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| rules: | ||
| - id: chromium-cryptography-random-usage | ||
| metadata: | ||
| author: Andrea Brancaleoni <[email protected]> | ||
| references: | ||
| - https://github.com/brave/brave-browser/wiki/Security-reviews | ||
| - https://chromium.googlesource.com/chromium/src/+/main/docs/security/web-platform-security-guidelines.md | ||
| - https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++-features.md#engines-and-generators-from-random_banned | ||
| source: https://github.com/brave/security-action/blob/main/assets/opengrep_rules/client/cryptography-random-usage.yaml | ||
| assignees: | | ||
| thypon | ||
| cdesouza-chromium | ||
| fmarier | ||
| category: security | ||
| languages: [cpp, c] | ||
| message: | | ||
| Usage of cryptographic functions or random number generation requires | ||
| security review according to Brave security guidelines. Use crypto::RandBytes() | ||
| for cryptographic purposes and base::RandomBitGenerator for non-cryptographic | ||
| randomness. Do not use engines/generators from <random> - use base::RandomBitGenerator | ||
| instead. Weak randomness can lead to serious security vulnerabilities. | ||
| severity: WARNING | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: rand() | ||
| - pattern: srand($SEED) | ||
| - pattern: random() | ||
| - pattern: srandom($SEED) | ||
| - pattern: std::rand() | ||
| - pattern: std::srand($SEED) | ||
| - pattern-regex: std::random_device\s+\w+ | ||
| - pattern-regex: std::mt19937\s+\w+ | ||
| - pattern-regex: std::mt19937_64\s+\w+ | ||
| - pattern-regex: std::minstd_rand\s+\w+ | ||
| - pattern-regex: std::minstd_rand0\s+\w+ | ||
| - pattern-regex: std::ranlux24\s+\w+ | ||
| - pattern-regex: std::ranlux48\s+\w+ | ||
| - pattern-regex: std::knuth_b\s+\w+ | ||
| - pattern-regex: std::default_random_engine\s+\w+ | ||
| - pattern: base::RandInt($MIN, $MAX) | ||
| - pattern: base::RandUint64() | ||
| - pattern: base::RandGenerator($SIZE) | ||
| - pattern: crypto::RandBytes($BUFFER, $SIZE) | ||
|
Collaborator
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. there is also RandBytesAsVector and RandBytesAsArray
Collaborator
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. I think there's also an existing presubmit for this |
||
| - pattern: RAND_bytes($BUFFER, $SIZE) | ||
| - pattern: EVP_CIPHER_CTX_new() | ||
| - pattern: EVP_EncryptInit($CTX, ...) | ||
| - pattern: EVP_DecryptInit($CTX, ...) | ||
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.
reported by reviewdog 🐶
[opengrep] Usage of cryptographic functions or random number generation requires
security review according to Brave security guidelines. Use crypto::RandBytes()
for cryptographic purposes and base::RandomBitGenerator for non-cryptographic
randomness. Do not use engines/generators from - use base::RandomBitGenerator
instead. Weak randomness can lead to serious security vulnerabilities.
Source: https://github.com/brave/security-action/blob/main/assets/opengrep_rules/client/cryptography-random-usage.yaml
Cc @thypon @cdesouza-chromium @fmarier