Add AST rules for detecting world-writable file creation in C/C++ #191
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 security rules for detecting instances of world-writable file creation in C and C++ code. The rules, defined in YAML files, use AST analysis to check for unsafe file permission settings and reference CWE-732. Additionally, corresponding snapshot and test files have been added to validate both correct and incorrect permission configurations through functions testing octal and symbolic modes. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer Code
participant AST as AST Parser
participant Rule as Security Rule Checker
participant Test as Test Runner
Dev->>AST: Submit C/C++ source code
AST->>Rule: Analyze file creation calls (chmod, open, etc.)
Rule-->>AST: Return warning if world-writable permission detected
AST->>Test: Log results for snapshot validation
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
tests/__snapshots__/world-writable-file-c-snapshot.yml (1)
15-51: Label Metadata Validation.
The labels accurately capture different segments (such as the mode literal, variable assignment, and function calls) and their character ranges. Please verify that these numeric ranges (e.g., start and end positions) match the expected AST output from your parser.tests/__snapshots__/world-writable-file-cpp-snapshot.yml (2)
15-51: Label Metadata Consistency for Octal Block.
The labels for thetest_octal_bad()snapshot are well defined, with proper identification of the mode variable and function call positions. Double-check that these positions remain accurate with any future AST refinements.
64-79: Label Definitions for Symbolic Mode Block.
The label definitions for the symbolic mode test consistently mark the key expressions and their ranges. Ensure that the AST matching produces similar ranges during test execution for reliable detections.rules/c/security/world-writable-file-c.yml (5)
13-32: Utility Definitions and AST Helpers.
Theutilssection defines helper mappings—follows_umaskandAND_2_EQUALS_2_&_S_IXXXX—to assist with the AST matching. The regex in the latter is complex; please verify it covers all intended insecure mode patterns.
43-116: Rule Block for chmod/fchmod/creat Calls.
This section meticulously matches calls tochmod,fchmod, andcreat, ensuring the mode argument meets insecure specifications and is not corrected by a preceding umask operation.
Consider adding inline comments to break down the nested matching logic for better readability and maintainability.
118-182: Rule Block for fchmodat Calls.
The rules captured here correctly addressfchmodatby ensuring that the mode argument is flagged if it matches the insecure pattern. Additional inline documentation within this complex nested structure might help future maintainers.
184-255: Rule Block for open Calls.
This section checks calls toopenwith insecure mode settings. The nested AST conditions are consistent with the previous blocks; however, consider adding clarifying comments to explain the overall logic.
257-328: Rule Block for openat Calls.
The matching conditions foropenatmimic those foropenand ensure that insecure mode values are accurately detected. Due to the heavy nesting, a few inline comments summarizing each step would improve clarity and future maintenance.rules/cpp/security/world-writable-file-cpp.yml (1)
32-43: Utility:AND_2_EQUALS_2_&_S_IXXXXComplexity
This utility uses a complex regex pattern to match mode values either as a number literal or as part of a binary expression. The name isn’t very self-explanatory. It would be beneficial to add inline comments or documentation explaining the intent and structure of the regex alternatives for future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
rules/c/security/world-writable-file-c.yml(1 hunks)rules/cpp/security/world-writable-file-cpp.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)
🔇 Additional comments (15)
tests/__snapshots__/world-writable-file-c-snapshot.yml (1)
1-14: Comprehensive Snapshot for C World-Writable Detection.
The snapshot fortest_octal_bad()clearly demonstrates the insecure use of mode0666through various file operations (e.g.,chmod,fchmod,fchmodat,open, andopenat). This provides a robust test case for the security rule.tests/__snapshots__/world-writable-file-cpp-snapshot.yml (2)
1-14: Snapshot for Insecure Octal Mode in C++ Test.
The first snapshot block representingtest_octal_bad()correctly reproduces an insecure scenario by using octal mode0666across the file operations in C++ code.
52-63: Snapshot for Insecure Symbolic Mode in C++ Test.
The snapshot fortest_symbol_direct_bad()effectively captures a scenario using symbolic constants that include insecure permission flags (e.g.,S_IWOTH).tests/cpp/world-writable-file-cpp-test.yml (2)
14-26: Validation of Insecure Octal Usage in Invalid Section.
Thetest_octal_bad()function in the invalid section is correctly implemented by explicitly declaringmode_t mode = 0666and reusing it across various file operations to simulate an insecure condition.
27-37: Validation of Insecure Symbolic Mode in Invalid Section.
Thetest_symbol_direct_bad()function properly demonstrates the risky behavior by combining file permission constants that includeS_IWOTH.tests/c/world-writable-file-c-test.yml (2)
14-26: Validation of Insecure Octal Mode in Invalid Test.
Thetest_octal_bad()function is correctly defined to simulate a misconfiguration usingmode_t mode = 0666, making it a suitable test case for triggering the rule.
27-37: Validation of Insecure Symbolic Mode in Invalid Test.
Thetest_symbol_direct_bad()function clearly demonstrates insecure symbolic permission settings by includingS_IWOTH, which is useful for rule verification.rules/c/security/world-writable-file-c.yml (1)
1-12: Metadata and General Configuration.
The file metadata (ID, language, severity, message, and note) is well articulated. The message explains the risk of world-writable file creation and includes a reference (CWE-732 with a link) for further context.rules/cpp/security/world-writable-file-cpp.yml (7)
1-10: Metadata and Documentation Consistency
The metadata fields (id,language,severity,message, andnote) are clearly defined. The description in themessageaccurately describes the potential security issue, and thenoteprovides a CWE reference with a helpful link.
11-11: Flag for Essential Rule Identification
Settingast-grep-essentials: trueproperly tags this rule for integration with the ast-grep essential ruleset.
13-31: Utility:follows_umaskStructure
Thefollows_umaskutility is well-structured to detect an expression statement that callsumask. The use ofnthChildand the regex (^umask$) ensures accurate matching of the intended identifier.
44-118: Rule Branch forchmod,fchmod, andcreat
This branch meticulously checks calls tochmod,fchmod, orcreatby scrutinizing the$MODEargument through nested AST patterns. The conditions such as checking declarations, assignment expressions, and comma expressions are comprehensive.
It is recommended to validate these patterns against a suite of test cases to ensure all edge cases are covered.
119-184: Rule Branch forfchmodat
Thefchmodatbranch mirrors the logical structure of the previous rule branch while targetingfchmodatcalls. It carefully encapsulates the conditions to verify the mode argument, ensuring consistency in approach.
Ensure this branch is validated with diverse code scenarios to confirm accurate AST pattern matching.
185-257: Rule Branch foropen
This segment targets theopenfunction and employs nested checks consistent with the other branches. The approach to match the$MODEargument—including handling for declarations, assignment expressions, and comma expressions—is thorough.
Testing with various call signatures is advised to guarantee the rule’s robustness.
258-329: Rule Branch foropenat
The implementation for detectingopenatcalls with inappropriate$MODEsettings aligns with the established patterns in the rule. The attentive use of AST nodes and nested conditions should effectively capture world-writable file creation issues.
As with the other branches, comprehensive testing is recommended to ensure this branch behaves as intended across different code patterns.
Summary by CodeRabbit
New Features
Tests