Add static analysis rules for C++ and Rust security checks #128
Conversation
|
Sakshis seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThis pull request introduces new static analysis rules and test configurations across multiple programming languages. The primary focus is on enhancing code security and preventing common programming mistakes. For C++, a new rule warns against using Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
tests/cpp/size-of-this-test.yml (1)
1-7: Consider adding more test cases for better coverage.While the current test cases cover the basic scenario, consider adding these additional cases to improve coverage:
sizeof(this)in different contexts (not just return statements)sizeof(&this)which is also invalidsizeof(**this)which would be invalidid: sizeof-this-cpp valid: - | return sizeof(*this); + - | + size_t size = sizeof(*this); invalid: - | return sizeof(this); + - | + size_t size = sizeof(this); + - | + return sizeof(&this); + - | + return sizeof(**this);rules/cpp/sizeof-this-cpp.yml (1)
19-27: Consider expanding pattern matching beyond return statements.The current rule only matches
sizeof(this)within return statements. This might miss problematic usage in other contexts, such as assignments or function arguments.inside: - stopBy: end - kind: return_statement - inside: - kind: compound_statement - follows: - kind: function_declarator - inside: - kind: function_definition + any: + - kind: return_statement + - kind: assignment_expression + - kind: call_expressiontests/__snapshots__/cbc-padding-oracle-java-snapshot.yml (1)
4-4: Consider adding test cases for secure alternatives.While this snapshot correctly identifies the potentially vulnerable
AES/CBC/PKCS5Paddingconfiguration, consider adding test cases for secure alternatives likeAES/GCM/NoPaddingto demonstrate best practices.tests/__snapshots__/tokio-postgres-empty-password-rust-snapshot.yml (1)
19-21: Fix indentation in error handling block.The error handling block has inconsistent indentation. The error message should be aligned with the
if letstatement.if let Err(e) = connection.await { - tracing::error!("postgres db connection error: {}", e); + tracing::error!("postgres db connection error: {}", e); }tests/rust/tokio-postgres-empty-password-rust-test.yml (1)
50-50: Add newline at end of file.Add a newline character at the end of the file to comply with POSIX standards.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 50-50: no new line character at the end of file
(new-line-at-end-of-file)
tests/rust/tokio-postgres-hardcoded-password-rust-test.yml (2)
2-50: Enhance test coverage with additional cases.Consider adding more test cases to cover:
- Passwords loaded from environment variables (valid)
- Different types of hardcoded passwords (invalid):
- Base64 encoded
- Hex encoded
- String concatenation
🧰 Tools
🪛 yamllint (1.35.1)
[error] 50-50: no new line character at the end of file
(new-line-at-end-of-file)
50-50: Add newline at end of file.Add a newline character at the end of the file to comply with POSIX standards.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 50-50: no new line character at the end of file
(new-line-at-end-of-file)
rules/rust/security/tokio-postgres-hardcoded-password-rust.yml (2)
5-9: Minor grammar improvements needed in the message.Consider these grammatical improvements:
- Change "retrieve them" to "retrieve it" since "secret" is singular
- Add an "or" before "alternatively" for better flow
- recommended to rotate the secret and retrieve them from a secure secret - vault or Hardware Security Module (HSM), alternatively environment + recommended to rotate the secret and retrieve it from a secure secret + vault or Hardware Security Module (HSM), or alternatively environment
16-55: Fix YAML indentation for better maintainability.The indentation is inconsistent throughout the utils section. While this doesn't affect functionality, consistent indentation improves readability and maintainability.
utils: MATCH_FOLLOW_1: follows: - stopBy: end - any: - - kind: let_declaration + stopBy: end + any: + - kind: let_declaration🧰 Tools
🪛 yamllint (1.35.1)
[warning] 19-19: wrong indentation: expected 6 but found 8
(indentation)
[warning] 21-21: wrong indentation: expected 10 but found 12
(indentation)
[error] 54-54: trailing spaces
(trailing-spaces)
rules/rust/security/tokio-postgres-empty-password-rust.yml (1)
1-248: Consider combining empty and hardcoded password rules.Both rules share similar patterns and could be combined into a single rule with multiple conditions. This would:
- Reduce code duplication
- Simplify maintenance
- Potentially improve performance
Here's how you could combine them:
- Create a new rule
tokio-postgres-password-security-rust- Add a condition field to distinguish between empty and hardcoded passwords
- Share the common pattern matching logic
Would you like me to provide a detailed implementation of the combined rule?
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 20-20: wrong indentation: expected 6 but found 8
(indentation)
[warning] 22-22: wrong indentation: expected 10 but found 12
(indentation)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 64-64: trailing spaces
(trailing-spaces)
[warning] 93-93: wrong indentation: expected 22 but found 20
(indentation)
[error] 128-128: trailing spaces
(trailing-spaces)
[warning] 131-131: wrong indentation: expected 26 but found 28
(indentation)
[warning] 150-150: wrong indentation: expected 22 but found 20
(indentation)
[warning] 185-185: wrong indentation: expected 22 but found 20
(indentation)
[warning] 212-212: wrong indentation: expected 18 but found 20
(indentation)
[warning] 214-214: wrong indentation: expected 22 but found 24
(indentation)
[warning] 228-228: wrong indentation: expected 30 but found 28
(indentation)
[warning] 246-246: wrong indentation: expected 22 but found 20
(indentation)
[warning] 248-248: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
rules/cpp/sizeof-this-cpp.yml(1 hunks)rules/rust/security/tokio-postgres-empty-password-rust.yml(1 hunks)rules/rust/security/tokio-postgres-hardcoded-password-rust.yml(1 hunks)tests/__snapshots__/cbc-padding-oracle-java-snapshot.yml(1 hunks)tests/__snapshots__/sizeof-this-cpp-snapshot.yml(1 hunks)tests/__snapshots__/tokio-postgres-empty-password-rust-snapshot.yml(1 hunks)tests/__snapshots__/tokio-postgres-hardcoded-password-rust-snapshot.yml(1 hunks)tests/cpp/size-of-this-test.yml(1 hunks)tests/rust/tokio-postgres-empty-password-rust-test.yml(1 hunks)tests/rust/tokio-postgres-hardcoded-password-rust-test.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/sizeof-this-cpp-snapshot.yml
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/rust/tokio-postgres-empty-password-rust-test.yml
[error] 50-50: no new line character at the end of file
(new-line-at-end-of-file)
tests/rust/tokio-postgres-hardcoded-password-rust-test.yml
[error] 50-50: no new line character at the end of file
(new-line-at-end-of-file)
rules/cpp/sizeof-this-cpp.yml
[warning] 30-30: wrong indentation: expected 2 but found 3
(indentation)
[error] 30-30: trailing spaces
(trailing-spaces)
[warning] 33-33: wrong indentation: expected 9 but found 8
(indentation)
[warning] 37-37: wrong indentation: expected 11 but found 12
(indentation)
[warning] 39-39: wrong indentation: expected 14 but found 15
(indentation)
[warning] 44-44: too many blank lines
(2 > 0) (empty-lines)
rules/rust/security/tokio-postgres-empty-password-rust.yml
[warning] 20-20: wrong indentation: expected 6 but found 8
(indentation)
[warning] 22-22: wrong indentation: expected 10 but found 12
(indentation)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 64-64: trailing spaces
(trailing-spaces)
[warning] 93-93: wrong indentation: expected 22 but found 20
(indentation)
[error] 128-128: trailing spaces
(trailing-spaces)
[warning] 131-131: wrong indentation: expected 26 but found 28
(indentation)
[warning] 150-150: wrong indentation: expected 22 but found 20
(indentation)
[warning] 185-185: wrong indentation: expected 22 but found 20
(indentation)
[warning] 212-212: wrong indentation: expected 18 but found 20
(indentation)
[warning] 214-214: wrong indentation: expected 22 but found 24
(indentation)
[warning] 228-228: wrong indentation: expected 30 but found 28
(indentation)
[warning] 246-246: wrong indentation: expected 22 but found 20
(indentation)
[warning] 248-248: too many blank lines
(1 > 0) (empty-lines)
rules/rust/security/tokio-postgres-hardcoded-password-rust.yml
[warning] 19-19: wrong indentation: expected 6 but found 8
(indentation)
[warning] 21-21: wrong indentation: expected 10 but found 12
(indentation)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 63-63: trailing spaces
(trailing-spaces)
[warning] 91-91: wrong indentation: expected 22 but found 20
(indentation)
[warning] 123-123: too many spaces after colon
(colons)
[error] 125-125: trailing spaces
(trailing-spaces)
[warning] 128-128: wrong indentation: expected 26 but found 28
(indentation)
[warning] 134-134: too many spaces after colon
(colons)
[warning] 146-146: wrong indentation: expected 22 but found 20
(indentation)
[warning] 180-180: wrong indentation: expected 22 but found 20
(indentation)
[warning] 207-207: wrong indentation: expected 18 but found 20
(indentation)
[warning] 209-209: wrong indentation: expected 22 but found 24
(indentation)
[warning] 222-222: wrong indentation: expected 30 but found 28
(indentation)
[warning] 239-239: wrong indentation: expected 22 but found 20
(indentation)
🔇 Additional comments (4)
rules/cpp/sizeof-this-cpp.yml (1)
1-11: LGTM! Well-documented rule with clear security implications.The rule is well-structured with:
- Clear message explaining the issue
- Proper CWE reference (CWE-467)
- Helpful link to SEI CERT C Coding Standard
tests/rust/tokio-postgres-empty-password-rust-test.yml (1)
2-25: Review test case categorization.The test categorization seems counterintuitive. If this rule is meant to detect empty passwords, then the case with an empty password should be in the
invalidsection, not thevalidsection.tests/__snapshots__/tokio-postgres-hardcoded-password-rust-snapshot.yml (1)
4-25: LGTM! Well-structured test case.The implementation includes:
- Proper error handling with context
- Correct async/await usage
- Comprehensive connection management
rules/rust/security/tokio-postgres-hardcoded-password-rust.yml (1)
56-240: Consider additional edge cases in pattern matching.The rule effectively covers basic scenarios but might miss these edge cases:
- String concatenation in password
- Password from environment variables
- Password from configuration files
Let's verify if these patterns are needed by checking the codebase:
✅ Verification successful
Current rule patterns are sufficient for actual usage patterns.
Based on the codebase analysis, the suggested edge cases (string concatenation, environment variables, config files) are not present in actual tokio-postgres password usage patterns. The existing rule effectively covers the observed patterns:
- Direct string literals
- Variable references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for string concatenation in passwords rg -U 'Config::new\(\).*\.password\([^)]*\+[^)]*\)' # Check for environment variables in passwords rg -U 'Config::new\(\).*\.password\(.*env!.*\)' # Check for config file usage rg -U 'Config::new\(\).*\.password\(.*from_file.*\)'Length of output: 158
Script:
#!/bin/bash # Search for any tokio-postgres password configuration rg -U "\.password\(" -A 2 # Search for environment variable usage in Rust rg "std::env::(var|var_os)\(" -A 1 # Search for file reading operations that might be used for configs rg "(fs::read_to_string|File::open)" -A 1Length of output: 18151
🧰 Tools
🪛 yamllint (1.35.1)
[error] 63-63: trailing spaces
(trailing-spaces)
[warning] 91-91: wrong indentation: expected 22 but found 20
(indentation)
[warning] 123-123: too many spaces after colon
(colons)
[error] 125-125: trailing spaces
(trailing-spaces)
[warning] 128-128: wrong indentation: expected 26 but found 28
(indentation)
[warning] 134-134: too many spaces after colon
(colons)
[warning] 146-146: wrong indentation: expected 22 but found 20
(indentation)
[warning] 180-180: wrong indentation: expected 22 but found 20
(indentation)
[warning] 207-207: wrong indentation: expected 18 but found 20
(indentation)
[warning] 209-209: wrong indentation: expected 22 but found 24
(indentation)
[warning] 222-222: wrong indentation: expected 30 but found 28
(indentation)
[warning] 239-239: wrong indentation: expected 22 but found 20
(indentation)
Summary by CodeRabbit
New Features
sizeof(this)usage in C++Security Improvements
Test Cases