Add security rules for memcpy in C/C++ and new test cases #49
Conversation
WalkthroughTwo new rules have been added to enhance security in C and C++ code regarding the use of the Changes
Possibly related PRs
Suggested reviewers
🪧 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: 6
🧹 Outside diff range and nitpick comments (8)
tests/c/missing-nul-cpp-string-memcpy-copy-c-test.yml (3)
2-6: Consider adding more edge cases to valid tests.The current valid test cases correctly verify the inclusion of NUL terminator using both
strlen()andsize(). However, consider adding these additional test cases for better coverage:
- Empty string:
""- String literal source
- Very long string (near size_t max)
Add these test cases:
valid: - | memcpy(to, from.c_str(), strlen(from.c_str()) + 1); - | memcpy(to, from.c_str(), from.size() + 1); + - | + memcpy(to, "", 1); // Empty string + - | + memcpy(to, "literal", 8); // String literal + - | + memcpy(to, very_long.c_str(), very_long.size() + 1); // Long string
7-19: Consider adding more security-critical invalid cases.The current invalid test cases effectively catch missing NUL terminators and undefined lengths. However, consider adding these security-critical scenarios:
- Partial string copy (e.g.,
memcpy(to, from.c_str(), n)where n < length)- Zero length copy
- Integer overflow cases (e.g.,
size_t(-1))Add these test cases:
invalid: - | memcpy(to, from.c_str(), from.size()); - | memcpy(to, from.c_str(), strlen(from.c_str())); - | memcpy(to, from.c_str(), from.length()); - | memcpy(to, from.c_str(), len_001); - | memcpy(to, from.c_str(), len_002); - | memcpy(to, from.c_str(), len_003); + - | + memcpy(to, from.c_str(), 5); // Partial copy without NUL + - | + memcpy(to, from.c_str(), 0); // Zero length + - | + memcpy(to, from.c_str(), SIZE_MAX); // Overflow risk
1-19: Consider adding a comment about modern C++ alternatives.While these tests are valuable for legacy code and C/C++ interop scenarios, consider adding a comment in the test file recommending modern C++ alternatives like
std::string::copy()orstd::copy()for new code. This helps developers make better choices when they reference these tests.Add this comment at the top of the file:
+# Note: For modern C++ code, prefer std::string methods or std::copy over memcpy +# These tests are primarily for C/C++ interoperability scenarios id: missing-nul-cpp-string-memcpy-copy-ctests/cpp/missing-nul-cpp-string-memcpy-copy-cpp-test.yml (3)
2-6: Consider adding more comprehensive valid test cases.The current valid cases correctly demonstrate including the NUL terminator using both
strlen()andsize(). Consider adding these additional scenarios to strengthen the test suite:
- Explicit size calculation with bounds checking
- Safe practices with destination buffer size verification
Add these additional valid test cases:
valid: - | memcpy(to, from.c_str(), strlen(from.c_str()) + 1); - | memcpy(to, from.c_str(), from.size() + 1); + - | + size_t size = from.size() + 1; + if (size <= to_size) { + memcpy(to, from.c_str(), size); + } + - | + const size_t src_size = from.size() + 1; + assert(to_size >= src_size); + memcpy(to, from.c_str(), src_size);
7-19: Enhance invalid test cases with more subtle scenarios.The current invalid cases effectively cover basic mistakes and undefined variables. Consider adding these subtle scenarios that could slip through code review:
Add these additional invalid test cases:
invalid: # existing cases... + - | + memcpy(to, from.c_str(), from.size() + offset); // offset might be 0 + - | + size_t len = from.size(); + memcpy(to, from.c_str(), len); // indirect size without NUL + - | + memcpy(to, from.c_str(), from.size() + (condition ? 0 : 1)); // conditional NULThese additions would help catch more subtle bugs where the NUL terminator might be missing in less obvious ways.
1-19: Consider adding documentation about security implications.While the test cases effectively validate the rule, adding comments that explain the security implications would be valuable:
- Document the potential for buffer overflows
- Explain the risks of out-of-bounds reads
- Reference any relevant security standards or CVEs
This documentation would help developers understand why these checks are crucial.
Add a documentation section at the top of the file:
+# Security Test Cases for memcpy with C++ Strings +# +# These tests validate proper handling of NUL terminators in string copies to prevent: +# - Buffer overflows from missing string terminators +# - Out-of-bounds reads leading to information disclosure +# - Memory corruption in downstream string operations +# +# Related CWEs: CWE-119, CWE-120, CWE-126 + id: missing-nul-cpp-string-memcpy-copy-cpp valid: ...rules/c/security/missing-nul-cpp-string-memcpy-copy-c.yml (1)
1-12: Consider enhancing the security context in messages.While the current message and note are informative, consider:
- Adding severity impact to the message (e.g., "...can lead to buffer overflows and potential remote code execution")
- Including additional relevant CWE references:
- CWE-119 (Improper Restriction of Operations within the Bounds of a Memory Buffer)
- CWE-788 (Access of Memory Location After End of Buffer)
rules/cpp/security/missing-nul-cpp-string-memcpy-copy-cpp.yml (1)
159-163: Add pattern descriptions in comments.The rule definition would benefit from comments explaining what each pattern catches, making it easier for future maintainers to understand the rule's coverage.
rule: any: + # Catches direct strlen() usage with memcpy - matches: match_identifier + # Catches string size/length field usage - matches: match_identifier_from_size + # Catches specific length variable patterns - matches: match_identifier_len
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- rules/c/security/missing-nul-cpp-string-memcpy-copy-c.yml (1 hunks)
- rules/cpp/security/missing-nul-cpp-string-memcpy-copy-cpp.yml (1 hunks)
- tests/snapshots/missing-nul-cpp-string-memcpy-copy-c-snapshot.yml (1 hunks)
- tests/snapshots/missing-nul-cpp-string-memcpy-copy-cpp-snapshot.yml (1 hunks)
- tests/c/missing-nul-cpp-string-memcpy-copy-c-test.yml (1 hunks)
- tests/cpp/missing-nul-cpp-string-memcpy-copy-cpp-test.yml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/missing-nul-cpp-string-memcpy-copy-cpp-snapshot.yml
🔇 Additional comments (11)
tests/c/missing-nul-cpp-string-memcpy-copy-c-test.yml (1)
1-1: LGTM! Clear and descriptive test case identifier.The test case ID accurately reflects its purpose of checking for missing NUL terminators in C++ string memcpy operations.
tests/cpp/missing-nul-cpp-string-memcpy-copy-cpp-test.yml (1)
1-1: LGTM! Clear and descriptive test identifier.The identifier accurately reflects the security check being tested.
rules/c/security/missing-nul-cpp-string-memcpy-copy-c.yml (2)
1-163: Verify test coverage for the security rule.Please ensure comprehensive test cases exist for:
- True positives: Various unsafe memcpy patterns
- True negatives: Safe implementations that shouldn't trigger
- Edge cases: Custom string classes, different string types
Let's check for existing test files:
#!/bin/bash # Search for corresponding test files fd -g "*missing-nul-cpp-string-memcpy*test*" # Check for example code in test files rg -l "memcpy.*c_str" test/Would you like help creating comprehensive test cases for this rule?
13-158: Consider additional string handling scenarios.The current patterns might miss or incorrectly flag:
- std::string_view conversions to c_str()
- Custom string classes with similar methods
- Safe implementations using sizeof() with character arrays
Let's check for potential false positives:
rules/cpp/security/missing-nul-cpp-string-memcpy-copy-cpp.yml (3)
121-158:⚠️ Potential issueReview the specific length variable pattern.
The pattern at line 157 uses very specific variable names (
len_001$|len_002$|len_003$). This seems overly restrictive and might miss valid cases.#!/bin/bash # Check for length variable naming patterns in the codebase rg -g '*.{cpp,h,hpp}' '\blen_\d+\b' -A 2Consider using a more generic pattern:
- regex: "^len_001$|len_002$|len_003$" + regex: "^len_\d+$"
1-163: Verify rule coverage with common string handling patterns.Please ensure the rule catches these common patterns:
- Direct string usage:
memcpy(dst, str.c_str(), str.length())- Temporary strings:
memcpy(dst, std::string("test").c_str(), 4)- String view usage:
memcpy(dst, sv.data(), sv.length())#!/bin/bash # Search for various memcpy patterns with strings in the codebase echo "Checking direct string usage..." ast-grep --pattern 'memcpy($DST, $STR.c_str(), $STR.length())' echo "Checking temporary string usage..." ast-grep --pattern 'memcpy($DST, std::string($STR).c_str(), $LEN)' echo "Checking string_view usage..." ast-grep --pattern 'memcpy($DST, $STR.data(), $STR.length())'
14-67: 🛠️ Refactor suggestionEnhance match_identifier pattern to handle string::length().
The current pattern only catches
strlen()calls. Consider adding a pattern to catchstd::string::length()usage as well, as it's a common C++ idiom.tests/__snapshots__/missing-nul-cpp-string-memcpy-copy-c-snapshot.yml (4)
1-2: LGTM: Clear snapshot identifier.The snapshot file is properly identified for the C-specific rule testing memcpy operations with C++ strings.
1-316: Verify test coverage for secure patterns.While these test cases appear to demonstrate unsafe patterns, we should ensure there are corresponding positive test cases that show the secure way to handle string copying (i.e., including space for the NUL terminator).
Let's check for positive test cases:
#!/bin/bash # Search for test cases that might include NUL terminator handling echo "Searching for positive test cases..." rg -A 5 "memcpy.*\+ 1\)" tests/
113-206: Verify the length variables' initialization.The test cases using
len_001andlen_002need verification of how these variables are initialized. If they don't include space for the NUL terminator, they could lead to the same security issues.Let's check the variable definitions:
#!/bin/bash # Search for len_001 and len_002 variable definitions echo "Searching for length variable definitions..." rg "len_00[12] = " -A 2
3-57: Potential security risk: Length calculation doesn't account for NUL terminator.The test case
memcpy(to, from.c_str(), from.length())demonstrates a common security vulnerability. Usingfrom.length()excludes the NUL terminator, which could lead to:
- Buffer overflow if the destination buffer size matches the string length
- String truncation if the code assumes NUL termination
Let's confirm if this is intended as a negative test case:
✅ Verification successful
The previous command failed, so let's try a more targeted search to understand if this is a test case demonstrating a security issue or if we need to flag it as a real concern.
The output shows this is part of security rules and has corresponding test files. Let's examine the rule definition and test case to confirm if this is intentionally demonstrating a vulnerability pattern.
Security rule verification confirmed: NUL terminator omission is a valid concern
This is a snapshot of a security rule test case that correctly identifies a potential buffer overflow vulnerability. The rule definition and test file confirm that:
- The pattern
memcpy(to, from.c_str(), from.length())is explicitly listed as an invalid case- Valid cases require adding 1 to the length to include the NUL terminator
- The rule is designed to detect and prevent out-of-bounds reads (CWE-125)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the corresponding test file and rule definition echo "Searching for related test and rule files..." fd -e yml -e yaml . | rg -l "missing-nul-cpp-string-memcpy-copy"Length of output: 291
Script:
#!/bin/bash # Let's try a different approach to find the test case and rule definition # First, let's check if this is a snapshot file and find its corresponding test rg -l "memcpy.*from\.c_str.*from\.length" --type yaml # Also search for any rule definitions related to this pattern rg -l "missing-nul|cpp.*string.*memcpy" --type yamlLength of output: 737
Script:
#!/bin/bash # Let's check the rule definition to confirm this is indeed a security rule cat rules/cpp/security/missing-nul-cpp-string-memcpy-copy-cpp.yml # Also check the test file to see if this is marked as a negative test case cat tests/cpp/missing-nul-cpp-string-memcpy-copy-cpp-test.ymlLength of output: 5550
Summary by CodeRabbit
New Features
memcpyfunction, accounting for NUL terminators to prevent out-of-bounds reads.Tests
memcpyin both C and C++ contexts, ensuring that the NUL terminator is included in memory operations.Documentation