Add Security Rules for File Permissions and Hard-Coded Secrets Detection #113
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. |
|
Warning Rate limit exceeded@ESS-ENN has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis pull request introduces new security rules for C, C++, and Python programming languages to detect vulnerabilities related to file permissions and hard-coded secrets. Specifically, it adds rules that identify instances of world-writable files in C and C++ and detects hard-coded secrets in Python applications. Each rule includes specific matching patterns and constraints to ensure proper permission settings and secure handling of sensitive data. Additionally, test cases are provided to validate the functionality of these rules across the respective languages. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 7
🧹 Outside diff range and nitpick comments (8)
rules/python/security/python-webrepl-hardcoded-secret-python.yml (1)
15-58: Consider additional patterns for string detectionThe
match_callpattern might miss some variations of string literals:
- f-strings (e.g., f"password123")
- raw strings (e.g., r"password123")
- triple-quoted strings
Consider extending the pattern to catch these variations:
- has: - kind: string + any: + - kind: string + - kind: formatted_string + - kind: raw_string field: valuetests/c/world-writable-file-c-test.yml (2)
16-26: Consider adding error handling in test_octal_badThe test case correctly validates world-writable permissions, but it would be more robust to verify that the operations fail as expected.
Consider adding error checking:
void test_octal_bad() { mode_t mode = 0666; - chmod("/tmp/foo", mode); + int ret = chmod("/tmp/foo", mode); + assert(ret == -1 && errno == EPERM); int fd = open_log(); - fchmod(fd, mode); + ret = fchmod(fd, mode); + assert(ret == -1 && errno == EPERM); // ... similar checks for other operations }
28-37: Enhance test coverage in test_symbol_direct_badThe test case covers various combinations of world-writable permissions, but could be more comprehensive.
Consider adding tests for:
- Edge cases with mixed permissions
- Directory permissions
- Symbolic link handling
- Error conditions with invalid file descriptors
tests/cpp/world-writable-file-cpp-test.yml (1)
16-37: Enhance test robustness with RAII and C++ featuresThe test cases could benefit from modern C++ practices.
Consider:
- Using
std::filesystemfor file operations- RAII for file handle management
- C++ exceptions for error handling
std::error_codefor error checkinggtestor similar C++ testing framework featurestests/__snapshots__/world-writable-file-c-snapshot.yml (2)
15-64: Enhance snapshot labels for better maintainabilityThe current label structure could be more descriptive and maintainable.
Consider:
- Adding descriptive comments for label purposes
- Grouping related labels together
- Using more semantic style names than "primary" and "secondary"
- Adding validation rules for label ranges
75-103: Consider adding metadata to snapshot labelsThe labels could benefit from additional context.
Consider adding:
- Expected validation results
- Related rule IDs
- Category information (e.g., "permission", "function_call")
- Severity levels for violations
tests/__snapshots__/world-writable-file-cpp-snapshot.yml (1)
65-74: Improve test organization and permission combinationsThe test function
test_symbol_direct_badmixes different permission combinations. Consider:
- Testing each permission combination separately
- Adding comments explaining the permission combinations
- Testing boundary cases
+ // Test world-writable with minimal other permissions + void test_symbol_minimal_bad() { + chmod("/tmp/foo", S_IWOTH); // Only world-writable + } + + // Test world-writable with full permissions + void test_symbol_full_bad() { + chmod("/tmp/foo", S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH); + }rules/cpp/security/world-writable-file-cpp.yml (1)
1-172: Consider unifying C and C++ rules to reduce duplicationThe C++ rule is almost identical to the C rule. Consider:
- Creating a shared base rule file
- Extending the base rule with language-specific patterns
- Using YAML anchors and aliases to reduce duplication
Example structure:
# base-world-writable-file.yml base-rule: &base-rule severity: warning message: >- This call makes a world-writable file... utils: &base-utils match_identifier_with_stringliteral_mode: ... # world-writable-file-c.yml id: world-writable-file-c language: c <<: *base-rule # world-writable-file-cpp.yml id: world-writable-file-cpp language: cpp <<: *base-rule
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
rules/c/security/world-writable-file-c.yml(1 hunks)rules/cpp/security/world-writable-file-cpp.yml(1 hunks)rules/python/security/python-webrepl-hardcoded-secret-python.yml(1 hunks)tests/__snapshots__/python-webrepl-hardcoded-secret-python-snapshot.yml(1 hunks)tests/__snapshots__/world-writable-file-c-snapshot.yml(1 hunks)tests/__snapshots__/world-writable-file-cpp-snapshot.yml(1 hunks)tests/c/world-writable-file-c-test.yml(1 hunks)tests/cpp/world-writable-file-cpp-test.yml(1 hunks)tests/python/python-webrepl-hardcoded-secret-python-test.yml(1 hunks)
🔇 Additional comments (2)
rules/python/security/python-webrepl-hardcoded-secret-python.yml (1)
1-14: LGTM: Well-documented security rule with proper references
The rule is well-documented with:
- Clear description of the security risk
- Proper references to CWE-798 and OWASP guidelines
- Actionable remediation advice
tests/__snapshots__/python-webrepl-hardcoded-secret-python-snapshot.yml (1)
1-160: LGTM: Snapshot correctly captures test cases
The snapshot file correctly captures:
- AST node information
- Source positions
- Node styles
- All test cases from the test file
Note: Remember to update this snapshot when adding the suggested new test cases.
| match_call_with_identifier: | ||
| kind: call | ||
| all: | ||
| - has: | ||
| stopBy: end | ||
| kind: attribute | ||
| field: function | ||
| all: | ||
| - has: | ||
| stopBy: end | ||
| kind: identifier | ||
| field: object | ||
| regex: "^webrepl$" | ||
| - has: | ||
| stopBy: end | ||
| kind: identifier | ||
| field: attribute | ||
| regex: "^start$" | ||
| - has: | ||
| stopBy: end | ||
| kind: argument_list | ||
| field: arguments | ||
| has: | ||
| stopBy: end | ||
| kind: keyword_argument | ||
| all: | ||
| - has: | ||
| kind: identifier | ||
| field: name | ||
| regex: "^password$" | ||
| - has: | ||
| kind: identifier | ||
| field: value | ||
| pattern: $PASS | ||
| inside: | ||
| stopBy: end | ||
| kind: expression_statement | ||
| follows: | ||
| stopBy: end | ||
| kind: expression_statement | ||
| has: | ||
| stopBy: end | ||
| kind: assignment | ||
| all: | ||
| - has: | ||
| kind: identifier | ||
| pattern: $PASS | ||
| - has: | ||
| kind: string | ||
| rule: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance variable tracking for better secret detection
The current pattern for tracking variables (match_call_with_identifier) only catches immediate assignments. Consider these scenarios:
- Variable reassignment
- Multiple variable assignments
- String concatenation
Consider extending the pattern to handle these cases:
- has:
kind: string
+ - has:
+ kind: binary_operator
+ field: operator
+ regex: "^\\+$"Committable suggestion skipped: line range outside the PR's diff.
| id: python-webrepl-hardcoded-secret-python | ||
| valid: | ||
| - | | ||
| webrepl.start(password=os.getenv('PASSWORD')) | ||
| invalid: | ||
| - | | ||
| webrepl.start(password="mypassword") | ||
| - | | ||
| webrepl.start(password="12345") | ||
| - | | ||
| PASSWORD2 = "hardcodedsecret" | ||
| webrepl.start(password=PASSWORD2) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more test cases for comprehensive coverage
Current test cases are good but could be enhanced with:
- f-string usage:
f"pass_{secret}" - String concatenation:
"pass_" + secret - Multiple assignments:
a = b = "secret" - Environment variable edge cases:
os.environ.get('PASSWORD')
Add these test cases:
valid:
- |
webrepl.start(password=os.getenv('PASSWORD'))
+ - |
+ webrepl.start(password=os.environ.get('PASSWORD'))
invalid:
- |
webrepl.start(password="mypassword")
- |
webrepl.start(password="12345")
- |
PASSWORD2 = "hardcodedsecret"
webrepl.start(password=PASSWORD2)
+ - |
+ webrepl.start(password=f"pass_{secret}")
+ - |
+ webrepl.start(password="pass_" + "secret")
+ - |
+ a = b = "secret"
+ webrepl.start(password=a)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id: python-webrepl-hardcoded-secret-python | |
| valid: | |
| - | | |
| webrepl.start(password=os.getenv('PASSWORD')) | |
| invalid: | |
| - | | |
| webrepl.start(password="mypassword") | |
| - | | |
| webrepl.start(password="12345") | |
| - | | |
| PASSWORD2 = "hardcodedsecret" | |
| webrepl.start(password=PASSWORD2) | |
| id: python-webrepl-hardcoded-secret-python | |
| valid: | |
| - | | |
| webrepl.start(password=os.getenv('PASSWORD')) | |
| - | | |
| webrepl.start(password=os.environ.get('PASSWORD')) | |
| invalid: | |
| - | | |
| webrepl.start(password="mypassword") | |
| - | | |
| webrepl.start(password="12345") | |
| - | | |
| PASSWORD2 = "hardcodedsecret" | |
| webrepl.start(password=PASSWORD2) | |
| - | | |
| webrepl.start(password=f"pass_{secret}") | |
| - | | |
| webrepl.start(password="pass_" + "secret") | |
| - | | |
| a = b = "secret" | |
| webrepl.start(password=a) |
| open("log", O_CREAT, mode); | ||
| openat(fd, "log", O_CREAT, mode); | ||
| creat("log", mode); |
There was a problem hiding this comment.
Fix undefined variable usage in test_symbol_direct_good
The mode variable is used in file operations but hasn't been defined earlier in the function.
Add the mode definition before usage:
void test_symbol_direct_good() {
chmod("/tmp/foo", S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH);
int fd = open_log();
fchmod(fd, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH);
int dirfd = open_log_dir();
fchmodat(dirfd, "log", S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH, AT_SYMLINK_NOFOLLOW);
+ mode_t mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH;
open("log", O_CREAT, mode);
openat(fd, "log", O_CREAT, mode);
creat("log", mode);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| open("log", O_CREAT, mode); | |
| openat(fd, "log", O_CREAT, mode); | |
| creat("log", mode); | |
| chmod("/tmp/foo", S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH); | |
| int fd = open_log(); | |
| fchmod(fd, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH); | |
| int dirfd = open_log_dir(); | |
| fchmodat(dirfd, "log", S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH, AT_SYMLINK_NOFOLLOW); | |
| mode_t mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH; | |
| open("log", O_CREAT, mode); | |
| openat(fd, "log", O_CREAT, mode); | |
| creat("log", mode); |
| void test_symbol_direct_good() { | ||
| chmod("/tmp/foo", S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH); | ||
| int fd = open_log(); | ||
| fchmod(fd, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH); | ||
| int dirfd = open_log_dir(); | ||
| fchmodat(dirfd, "log", S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH, AT_SYMLINK_NOFOLLOW); | ||
| open("log", O_CREAT, mode); | ||
| openat(fd, "log", O_CREAT, mode); | ||
| creat("log", mode); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using C++ idioms in test_symbol_direct_good
While the current implementation works, it could better leverage C++ features.
Consider using C++ features:
void test_symbol_direct_good() {
+ std::filesystem::path tmp_path("/tmp/foo");
+ std::filesystem::permissions(tmp_path,
+ std::filesystem::perms::owner_read |
+ std::filesystem::perms::owner_write |
+ std::filesystem::perms::group_read |
+ std::filesystem::perms::group_write |
+ std::filesystem::perms::others_read);
- chmod("/tmp/foo", S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH);
// ... similar changes for other operations
}Committable suggestion skipped: line range outside the PR's diff.
| void test_octal_bad() { | ||
| mode_t mode = 0666; | ||
| chmod("/tmp/foo", mode); | ||
| int fd = open_log(); | ||
| fchmod(fd, mode); | ||
| int dirfd = open_log_dir(); | ||
| fchmodat(dirfd, "log", mode, AT_SYMLINK_NOFOLLOW); | ||
| open("log", O_CREAT, mode); | ||
| openat(fd, "log", O_CREAT, mode); | ||
| creat("log", mode); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add positive test cases and improve test coverage
The test function test_octal_bad only covers negative cases. Consider adding:
- Positive test cases with secure permissions (e.g., 0600, 0640)
- Edge cases (e.g., 0622, 0624)
- Consistent permission patterns across different file operations
+ void test_octal_good() {
+ mode_t secure_mode = 0600; // user read/write only
+ chmod("/tmp/foo", secure_mode);
+ open("log", O_CREAT, secure_mode);
+ }Committable suggestion skipped: line range outside the PR's diff.
| kind: binary_expression | ||
| has: | ||
| kind: identifier | ||
| regex: ^(S_IWOTH|S_IWUSR|S_IGRP|S_IROTH|S_IRUSR|S_IWOTH)$ |
There was a problem hiding this comment.
Remove redundant patterns in regex
The regex patterns contain duplicate entries:
regex: ^(S_IWOTH|S_IWUSR|S_IGRP|S_IROTH|S_IRUSR|S_IWOTH)$
regex: ^(S_IWOTH|S_IWUSR|S_IGRP|S_IRUSR|S_IWOTH)$
S_IWOTH appears twice in both patterns.
- regex: ^(S_IWOTH|S_IWUSR|S_IGRP|S_IROTH|S_IRUSR|S_IWOTH)$
+ regex: ^(S_IWOTH|S_IWUSR|S_IGRP|S_IROTH|S_IRUSR)$
- regex: ^(S_IWOTH|S_IWUSR|S_IGRP|S_IRUSR|S_IWOTH)$
+ regex: ^(S_IWOTH|S_IWUSR|S_IGRP|S_IRUSR)$Also applies to: 95-95
| BINARY: | ||
| regex: ^0[0-7]*[2367]$ |
There was a problem hiding this comment.
Strengthen binary constraint regex pattern
The current regex ^0[0-7]*[2367]$ might miss some world-writable combinations. Consider:
- Adding explicit length validation
- Handling all possible world-writable combinations
- regex: ^0[0-7]*[2367]$
+ regex: ^0[0-7]{0,3}[2367]$📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BINARY: | |
| regex: ^0[0-7]*[2367]$ | |
| BINARY: | |
| regex: ^0[0-7]{0,3}[2367]$ |
Summary by CodeRabbit
New Features
Bug Fixes
Tests