Add security rules for system() usage in C/C++ and JWT expiry in C# #152
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 and corresponding test cases for multiple programming languages. Two rules warn against the use of the system function in C and C++ by matching specific AST patterns and recommending the use of more restrictive APIs. An additional rule in C# checks JWT token validation parameters for missing expiry checks. Snapshot files and unit tests for C, C++, C#, and Java are added or updated to verify both valid and invalid cases for these new rules. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Analyzer as Code Analyzer
participant RuleEngine as Rule Engine
participant Reporter as Reporter
Dev->>Analyzer: Submit source code with system() call
Analyzer->>RuleEngine: Apply AST patterns for system detection
RuleEngine-->>Analyzer: Return warning if pattern matches
Analyzer->>Reporter: Report security warning
sequenceDiagram
participant Dev as Developer
participant Analyzer as Code Analyzer
participant JWTRule as JWT Validation Rule
participant Reporter as Reporter
Dev->>Analyzer: Submit JWT TokenValidationParameters configuration
Analyzer->>JWTRule: Check properties (ValidateLifetime, RequireExpirationTime)
JWTRule-->>Analyzer: Return warning if expiry validation is disabled
Analyzer->>Reporter: Report token configuration issue
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: 0
🧹 Nitpick comments (7)
tests/csharp/jwt-tokenvalidationparameters-no-expiry-validation-csharp-test.yml (2)
1-5: Valid Configuration Block:
The valid snippet correctly enforces token expiration by setting both ValidateLifetime and RequireExpirationTime to true. One minor note: line 5 omits a semicolon ("parameters.RequireExpirationTime = true") while line 4 uses one. If this snippet represents executable C# code, adding the semicolon might help maintain consistency.
35-42: Duplicate Invalid Configuration Notice:
This snippet appears similar in spirit to previous invalid cases. Please review if the redundancy is intentional or if consolidating similar scenarios would streamline the test cases.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 37-37: trailing spaces
(trailing-spaces)
tests/__snapshots__/jwt-tokenvalidationparameters-no-expiry-validation-csharp-snapshot.yml (2)
43-47: Indentation Issue:
Static analysis detected a wrong indentation on line 46 (expected 4 spaces but found 6). Please adjust this line's indentation to match YAML formatting standards.
90-90: Trailing Whitespace Detected:
Trailing whitespace is present on lines 90, 107, and 135. Please remove these extra spaces to comply with YAML linting guidelines.Also applies to: 107-107, 135-135
rules/csharp/security/jwt-tokenvalidationparameters-no-expiry-validation-csharp.yml (3)
4-9: Rule Message Clarity:
The message clearly explains the risk of improperly validated JWT token lifetimes. Please double-check that placeholders ($LIFETIME, $FALSE) are replaced as intended during rule evaluation.
45-47: Indentation Warning in Pattern Definition:
Static analysis reports a wrong indentation on line 46; expected 4 spaces but found 6. Adjusting this indentation will improve readability and maintain YAML consistency. Consider this diff:- kind: boolean_literal + kind: boolean_literal🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 46-46: wrong indentation: expected 4 but found 6
(indentation)
89-90: Trailing Whitespace Issues:
Trailing spaces are detected on lines 90, 107, and 135 in the utility patterns section. Please remove these to adhere to YAML linting rules.Also applies to: 105-107, 133-135
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 90-90: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
rules/c/security/dont-call-system-c.yml(1 hunks)rules/cpp/dont-call-system-cpp.yml(1 hunks)rules/csharp/security/jwt-tokenvalidationparameters-no-expiry-validation-csharp.yml(1 hunks)tests/__snapshots__/dont-call-system-c-snapshot.yml(1 hunks)tests/__snapshots__/dont-call-system-cpp-snapshot.yml(1 hunks)tests/__snapshots__/jwt-tokenvalidationparameters-no-expiry-validation-csharp-snapshot.yml(1 hunks)tests/cpp/dont-call-system-cpp-test.yml(1 hunks)tests/csharp/jwt-tokenvalidationparameters-no-expiry-validation-csharp-test.yml(1 hunks)tests/java/dont-call-system-c-test.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/csharp/jwt-tokenvalidationparameters-no-expiry-validation-csharp-test.yml
[error] 37-37: trailing spaces
(trailing-spaces)
rules/csharp/security/jwt-tokenvalidationparameters-no-expiry-validation-csharp.yml
[warning] 46-46: wrong indentation: expected 4 but found 6
(indentation)
[error] 90-90: trailing spaces
(trailing-spaces)
[error] 107-107: trailing spaces
(trailing-spaces)
[error] 135-135: trailing spaces
(trailing-spaces)
🔇 Additional comments (28)
tests/java/dont-call-system-c-test.yml (2)
1-7: Solid valid test case implementation.
The valid test (test_003) correctly demonstrates a safe code path by using storer->store_binary(Clocks->system()) instead of directly invoking system(). This clearly aligns with the intended secure usage pattern.
8-34: Clear demonstration of prohibited system() usage.
Both invalid cases (test_002 and test_001) effectively capture the misuse of system(). In test_001, the inclusion of bounds checking and error handling (for snprintf failures and system() return value) clearly illustrates the risks and proper detection of errors.tests/cpp/dont-call-system-cpp-test.yml (2)
1-7: Valid C++ test case is well structured.
The valid test (test_003) mirrors the Java test case by safely routing execution through storer->store_binary(Clocks->system()), which is in line with the safe-pattern prescription.
8-34: Invalid test cases accurately flag risky patterns.
The two invalid cases in this file, one with a direct system() call (test_002) and one with proper error handling (test_001), provide a clear illustration of unsafe usage. They will serve well to verify that the new rule catches all instances of the forbidden call.tests/__snapshots__/dont-call-system-c-snapshot.yml (2)
1-28: Snapshot tests correctly mirror unsafe scenarios.
The snapshot block defining test_002 and test_001 successfully reproduces the invalid usage cases. The inclusion of error checking in test_001 ensures that the snapshot captures all aspects of the expected behavior in failure cases.
29-45: Detailed label metadata enhances traceability.
The labels provided (with primary and secondary styles and explicit start–end positions) add valuable metadata that can assist downstream tools in pinpointing the exact code fragments triggering the rule.tests/__snapshots__/dont-call-system-cpp-snapshot.yml (2)
1-28: Snapshot tests for C++ invalid cases are well defined.
Just like the C snapshot, these tests encapsulate both direct and conditionally handled system() calls. They offer a robust reference point for the rule's expected behavior in a C++ context.
29-45: Precision in label annotations is commendable.
The snapshot’s label details accurately identify the source components (system(cmdbuf), system, and the argument cmdbuf) with precise positions, reinforcing the integrity of the testing framework.rules/c/security/dont-call-system-c.yml (5)
1-13: Comprehensive rule metadata and documentation.
The header section properly defines the rule’s ID, target language, severity level, and includes a clear message along with references (CWE-78 and OWASP guidelines). This sets strong expectations for the rule’s purpose and usage.
14-15: Configuration flag is properly set.
Enabling ast-grep-essentials ensures that the necessary pattern-matching capabilities are active, which is essential for accurately parsing the AST.
16-31: Utility pattern for if-statement context is well crafted.
The definition of PATTERN_SYSTEM_INSIDE_IF_STATEMENT appropriately targets calls to system() within an if-statement. It checks for the presence of the identifier "system" and its associated argument list, ensuring that contextual misuse is caught.
32-47: Robust utility pattern for general system() invocation.
The PATTERN_SYSTEM is defined to capture system() calls in several syntactic contexts (expression statements, return statements, or field declarations). This broad approach is effective for flagging varied code patterns that use system() unsafely.
48-61: Logical rule configuration aligns well with security best practices.
The rule section correctly combines both utility patterns, while the exclusion conditions (notably filtering out contexts with errors) ensure that false positives are minimized. This careful design supports secure coding standards.rules/cpp/dont-call-system-cpp.yml (6)
1-7: Basic Rule Definition is Clear and Concise
The configuration for the rule’s ID, language, severity, and message is well defined. The message clearly warns against the use of the system call and suggests a safer alternative.
8-13: Informative Note Block with CWE and References
The note block effectively highlights the security concern (CWE-78) and provides a useful reference link for further context related to OS command injection. This additional metadata aids in understanding the rationale behind the rule.
14-14: AST-Grep Essentials Flag Set Appropriately
Enabling the AST grep essentials by setting "ast-grep-essentials" to true aligns with the toolchain integration and ensures that the appropriate analysis is performed.
16-31: Robust Pattern for System Call within If Statements
The "PATTERN_SYSTEM_INSIDE_IF_STATEMENT" block is well structured. It specifically captures a call expression with an identifier exactly matching "system", checks for an accompanying argument list, and ensures the call is nested inside an if_statement via a parenthesized expression. This precision should minimize false positives in contexts where "system" is used conditionally.
32-47: Comprehensive Pattern for General System Calls
The "PATTERN_SYSTEM" block is designed to capture system calls across various code contexts (expression statements, return statements, and field declarations). This broad matching ensures that instances of unsafe system usage are identified regardless of context. Consider adding inline comments or documentation in the YAML if further clarification on each context is desired for future maintainers.
48-61: Well-Defined Rule Logic with False Positive Filtering
The rule block leverages "any" to match calls identified by either of the two patterns and includes a "not" clause to filter out cases where the matched code has an ERROR kind. This approach minimizes false positives and provides a robust safeguard against incorrectly flagging benign expressions.tests/csharp/jwt-tokenvalidationparameters-no-expiry-validation-csharp-test.yml (3)
6-18: Combined Invalid Configuration Block:
This block first assigns options.TokenValidationParameters with a full initialization and then creates a separate TokenValidationParameters instance with its properties set. Please verify that combining these two assignments in one test case accurately tests the intended invalid scenario. If the behaviors differ, it might be beneficial to split them into distinct test cases.
19-26: Direct Assignment Invalid Test:
This snippet directly initializes a TokenValidationParameters instance with ValidateLifetime and RequireExpirationTime set to false. The configuration is clear and effective for testing an invalid JWT token configuration.
27-34: Partial Validation Invalid Test:
This configuration sets ValidateLifetime to true but RequireExpirationTime to false. It effectively tests a case where token lifetime is only partially validated.tests/__snapshots__/jwt-tokenvalidationparameters-no-expiry-validation-csharp-snapshot.yml (2)
1-10: Snapshot Initialization:
The snapshot correctly captures a configuration of TokenValidationParameters with all key validations disabled. The structure is clear and reflects the intended test setup.
78-84: Valid Snapshot Entry:
This snapshot entry for options.TokenValidationParameters with ValidateLifetime set to true and other properties disabled is clear and correctly formatted.rules/csharp/security/jwt-tokenvalidationparameters-no-expiry-validation-csharp.yml (4)
1-3: Rule Metadata:
The rule metadata (id, severity, language) is defined correctly and aligns with the PR objectives.
10-15: Reference Note:
The note section delivers useful security references (CWE, OWASP, Microsoft documentation), which enhances the rule's credibility.
21-44: Utility Patterns Definition:
The utility patterns for detecting problematic configurations are well-structured. Ensure that any nested structure adheres consistently to the YAML formatting guidelines.
137-147: Rule Definition:
The final rule definition combines multiple match patterns effectively and correctly excludes nodes with errors. The overall logic is solid and meets the security intent.
Summary by CodeRabbit