Add C++ security rules for NUL terminators and string_view safety #183
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 adds two new C++ security rules: one to detect missing NUL terminators when using memory copy functions and another to flag unsafe constructions of Changes
Sequence Diagram(s)sequenceDiagram
participant Code
participant ASTParser
participant MemcpyRule
Code->>ASTParser: Parse code with memcpy usage
ASTParser->>MemcpyRule: Analyze memcpy parameters
MemcpyRule-->>ASTParser: Identify missing NUL terminator if present
ASTParser-->>Code: Emit warning for potential out-of-bounds read
sequenceDiagram
participant Code
participant ASTParser
participant StringViewRule
Code->>ASTParser: Parse code with std::string_view usage
ASTParser->>StringViewRule: Analyze construction from temporary std::string
StringViewRule-->>ASTParser: Detect potential use-after-free issue
ASTParser-->>Code: Emit warning for unsafe string_view assignment
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: 0
🧹 Nitpick comments (10)
tests/__snapshots__/missing-nul-cpp-string-memcpy-copy-cpp-snapshot.yml (3)
11-55: Test_001 Labels: Consider streamlining duplicates
The label section for test_001 contains several repeated entries (for example, duplicate entries forchar to[20]and the length initialization). If these entries are auto-generated, consider deduplicating or commenting their purpose to improve maintainability and clarity.
64-120: Test_002 Labels: Review for redundancy
Much like test_001, the labels here are very detailed and include repeated segments. While comprehensive labeling may be useful for snapshot diffing, streamlining these entries may improve readability without losing important information.
129-173: Test_003 Labels: Verify accuracy and reduce overlap
The labels for test_003 are comprehensive but exhibit redundant definitions. Consider whether all duplicated label entries (e.g. forchar to[20]and the length calculation) are necessary. A cleanup here might reduce noise during snapshot comparison.rules/cpp/security/missing-nul-cpp-string-memcpy-cpp.yml (3)
14-65: AST Pattern forstrlen($STR.c_str()): Structural Verification
This section defines an AST pattern targeting calls where the third argument is computed viastrlen($STR.c_str()). The use ofnthChildand nested conditions appears well thought out. For future maintainability, adding inline comments explaining each nested condition could help clarify the matching logic for maintainers. Verify that these AST positions reliably match the target code’s structure.
66-80: Handling of Associated Type Declarations in Pattern Matching
The alternative pattern block (lines 66–80) focuses on validating type declarations for the destination buffer context. This extra check is useful for confirming the usage context of the memory copy call. Consider expanding inline documentation to clarify why these particular patterns (e.g.$TYPE $DEST[$DIM] = $$$;) are checked and whether they cover all expected scenarios.
161-405: Overall Rule Complexity and Maintainability
The rule employs multiple alternative branches with deeply nested conditions to catch a range of misuse patterns. While comprehensive, the complexity could make future maintenance challenging. Consider modularizing the rule (or adding detailed inline comments) to clarify the purpose of each branch. This can aid in both debugging and future extensions of the pattern logic.tests/cpp/string-view-temporary-string-cpp-test.yml (1)
28-33: Invalid Test Case: Concatenation with an Undeclared Identifier
The snippet usingfoo + "foo" + barintroduces an undeclared symbol (bar). This appears to deliberately test the rule’s behavior when the concatenation involves an unexpected or undefined operand. Consider adding a comment in the YAML (or documentation) clarifying that this case is meant to simulate an unsafe pattern with an external or missing variable reference.tests/__snapshots__/string-view-temporary-string-cpp-snapshot.yml (1)
1-311: Comprehensive Snapshot File Coverage
The snapshot file provides detailed label mappings for multiple scenarios of unsafe string concatenations. The various snapshot groups (covering cases likereturns_std_string() + "bar",returns_std_string() + foo, literal-first concatenations, and chained concatenations) are well structured with clearly specified source ranges and label styles.
To enhance maintainability, consider adding a top-level YAML comment summarizing the purpose of the snapshots (e.g., “Expected outcomes for unsafestd::string_viewconstructions from temporarystd::stringobjects”).rules/cpp/security/string-view-temporary-string-cpp.yml (2)
1-13: Metadata and Messaging Accuracy
The rule’s metadata is clear and informative. One nitpick: there is a typo in the security message – “immeadiately” should be corrected to “immediately” for clarity.
16-907: Comprehensive Utility Definitions for Unsafe Constructions
The extensive collection of utility patterns covers a wide range of potential unsafe constructions—from assignments using direct concatenation (+), literal-first operations, to cases involving function returns and substring calls. This comprehensive approach minimizes the risk of missing a dangerous pattern. For long-term maintainability, consider adding inline comments or documentation for each utility block to explain the targeted use-case in brief.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
rules/cpp/security/missing-nul-cpp-string-memcpy-cpp.yml(1 hunks)rules/cpp/security/string-view-temporary-string-cpp.yml(1 hunks)tests/__snapshots__/missing-nul-cpp-string-memcpy-copy-cpp-snapshot.yml(1 hunks)tests/__snapshots__/string-view-temporary-string-cpp-snapshot.yml(1 hunks)tests/cpp/missing-nul-cpp-string-memcpy-cpp-test.yml(1 hunks)tests/cpp/string-view-temporary-string-cpp-test.yml(1 hunks)
🔇 Additional comments (24)
tests/__snapshots__/missing-nul-cpp-string-memcpy-copy-cpp-snapshot.yml (3)
4-10: Test_001 Snapshot: Validate detection of missing NUL terminator
This test case demonstrates a common mistake—usingstrlen(from.c_str())to determine the length, which does not include the terminating NUL. It appears designed to trigger the new rule. Please confirm that this behavior is intentional for snapshot validation.
57-63: Test_002 Snapshot: Validate alternate misuse usingfrom.size()
This test case computes the length usingfrom.size(), which again omits adding an extra byte for the NUL terminator. It correctly represents an unsafe usage scenario. Please double-check that the snapshot outcome is as expected for the rule validation.
122-128: Test_003 Snapshot: Validate misuse withfrom.length()
This snapshot usesfrom.length()to determine the copy length, which, similar to the other cases, does not account for the NUL terminator. Ensure that the inclusion of this variant provides distinct coverage versus thefrom.size()test.tests/cpp/missing-nul-cpp-string-memcpy-cpp-test.yml (5)
3-10: Valid Test Case (test_001): Verify correct length handling
Under the “valid” section, test_001 computes the length usingsize_t len_001 = strlen(from.c_str()+1);This approach subtracts one character from what would normally be returned by
strlen(from.c_str()). Typically, to correctly include the NUL terminator you would expect a calculation likestrlen(from.c_str()) + 1. Please verify if the current formulation is intentional or requires adjustment.
11-18: Valid Test Case (test_002): Correct inclusion of NUL terminator
This test correctly computes the length asfrom.size()+1, which properly includes the terminating NUL. This case appears to be the ideal example for safe usage.
20-27: Invalid Test Case (test_001): Missing NUL terminator
Here the length is computed asstrlen(from.c_str())with no addition of an extra byte. This unsafe usage is expected to be flagged by the rule. The test case aligns with the intended invalid pattern.
28-35: Invalid Test Case (test_002): Omission of extra byte withfrom.size()
The usage offrom.size()without adding 1 results in the NUL terminator being omitted. This invalid scenario should be captured by the new rule as designed.
36-43: Invalid Test Case (test_003): Unsafe use offrom.length()
Usingfrom.length()to determine the number of bytes to copy fails to account for the null terminator and is correctly categorized here as an unsafe pattern.rules/cpp/security/missing-nul-cpp-string-memcpy-cpp.yml (3)
1-13: Rule Header and Metadata: Clear and Informative
The rule’s header clearly defines its purpose with an appropriate severity and message. The reference to [CWE-125] and the link to external guidelines adds useful context.
81-120: AST Pattern for$STR.size()Usage: Consistency Check
This block addresses cases where the length is determined via$STR.size(). It correctly expects the absence of a+1and thus a missing NUL terminator. Please verify that thenthChildchecks and nested conditions mirror those in thestrlenblock, ensuring consistent rule behavior.
121-160: AST Pattern for$STR.length()Usage: Consistency Across Variants
The subsequent alternative captures scenarios using$STR.length(). Its structure should be consistent with the$STR.size()handling so that all unsafe patterns are flagged equivalently. Confirm that no edge cases are missed in these variants.tests/cpp/string-view-temporary-string-cpp-test.yml (6)
1-8: Valid Test Case Definition: Clear and Concise
The valid test snippet correctly assigns the result of string concatenation to a temporary variable of typestd::string, ensuring safety. The YAML structure and indentation are clear.
9-15: Invalid Test Case: Direct Assignment from Temporary
This snippet demonstrates an unsafe use—assigning the result offoo + "bar"(a temporarystd::string) directly to astd::string_view. This is exactly the scenario the new rule intends to catch.
16-21: Invalid Test Case: Reversed Concatenation Order
The test case with"bar" + foocorrectly represents an unsafe assignment where a literal is used on the left side. This helps ensure the rule detects concatenation order issues.
22-27: Invalid Test Case: Chained String Literals and Variables
Here the concatenation involves multiple operands (foo + foo + "bar"). This chained operation, which yields a temporary, is properly modeled as an invalid case for astd::string_viewassignment.
34-40: Invalid Test Case: Unsafe External Function Usage
By concatenating the temporary returned fromreturns_std_string()withfoo, this test case simulates a dangerous scenario originating from an external function’s return value. This is well aligned with the rule’s objectives.
41-47: Invalid Test Case: External Function and Literal Combination
This snippet tests the unsafe construction of astd::string_viewfrom the temporary result ofreturns_std_string()concatenated with a literal. It accurately represents one of the unsafe patterns the rule is targeting.rules/cpp/security/string-view-temporary-string-cpp.yml (7)
14-14: Ast-Grep Essentials Flag Usage
Theast-grep-essentials: trueflag is appropriately set, enabling the tool to use tree-sitter–based matching effectively.
16-39: Utility Pattern forstd::to_string(...)Matching
The first utility pattern captures assignments where astd::string_viewis being set via a call tostd::to_string. The conditions—ensuring the target identifier matchesstring_vieworwstring_viewand that the call expression points tostd::to_string—are correctly structured.
40-62: Utility Pattern for Substring Method Matching
The rule properly defines a pattern to catch assignments that use the.substr(...)method on a string (or wstring). The usage of regex and structure in the nested matching conditions looks correct.
63-82: Utility Pattern for Binary '+' Concatenation
This pattern ensures that assignments involving a binary+expression—with the left-hand value being an identifier matchingwstringorstring—are detected when assigned to astring_view. The approach is logical and concise.
83-106: Utility Pattern for Literal-First Concatenations
The construction matching assignments that start with a literal (i.e."..." + $EXPR) is well defined. It correctly ensures that the string literal and the subsequent identifier conform to the expected types (e.g.stringorwstring).
107-130: Utility Pattern for Instance Initialization via Concatenation
This pattern targets cases where the assignment is part of a variable’s initialization using literal concatenation. The nested conditions that check for corresponding declarations and types provide robust matching.
908-944: Rule Aggregation Block
Therule:section successfully aggregates the previously defined utility patterns and specifies conditions under which the rule should trigger. It distinguishes between different kinds of expressions (e.g. expression statements, call expressions, declarations) and excludes any with errors in the surrounding code. This design is both thorough and effective for detecting unsafestd::string_viewconstructions.
Summary by CodeRabbit