Add security rules for C++ and Go to prevent use-after-free and interface binding risks #130
Conversation
WalkthroughThis pull request introduces two new security-focused rules for C++ and Go programming languages. The first rule in Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Rule as Security Rule
participant Code as Source Code
Dev->>Code: Writes code with potential vulnerability
Code->>Rule: Rule checks code pattern
alt Vulnerable Pattern Detected
Rule-->>Dev: Warns about potential security risk
else Safe Pattern
Rule->>Code: No warning issued
end
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: 1
🧹 Nitpick comments (8)
rules/cpp/return-c-str-c.yml (1)
16-22: Consider adding pattern for string_view to prevent similar issues.The patterns comprehensively cover std::string variants, but might want to consider adding patterns for std::string_view to prevent similar lifetime issues with .data().
Add these patterns:
any: - pattern: return basic_string<$TYPE>($$$).$METHOD(); - pattern: return std::basic_string<$TYPE>($$$).$METHOD(); - pattern: return string($$$).$METHOD(); - pattern: return std::string($$$).$METHOD(); - pattern: return wstring($$$).$METHOD(); - pattern: return std::wstring($$$).$METHOD(); + - pattern: return string_view($$$).$METHOD(); + - pattern: return std::string_view($$$).$METHOD();tests/cpp/return-c-str-cpp-test.yml (1)
8-24: Enhance test coverage with additional edge cases.While the current test cases cover the basic scenarios well, consider adding these additional test cases:
Add these test cases:
invalid: - | char *return_namespace_directly() { return std::string("foo").c_str(); } + - | + const char *return_const_char() { + return std::string("foo").c_str(); + } + - | + const char *return_from_temporary() { + return (std::string("foo") + "bar").c_str(); + }tests/go/avoid-bind-to-all-interfaces-go-test.yml (2)
2-4: Consider expanding valid test cases for better coverage.The valid test cases could be enhanced to include more secure binding scenarios:
- Localhost (127.0.0.1)
- IPv6 addresses (::1)
- Named interfaces (localhost)
valid: - | l, err := net.Listen("tcp", "192.168.1.101:2000") + - | + l, err := net.Listen("tcp", "127.0.0.1:2000") + - | + l, err := net.Listen("tcp", "[::1]:2000") + - | + l, err := net.Listen("tcp", "localhost:2000")
5-9: Consider additional invalid test cases for comprehensive coverage.The invalid test cases could include more variations of problematic bindings:
- IPv6 all interfaces (::)
- Alternative notations for 0.0.0.0
invalid: - | l, err := net.Listen("tcp", "0.0.0.0:2000") - | l, err := net.Listen("tcp", ":2000") + - | + l, err := net.Listen("tcp", "[::]:2000") + - | + l, err := net.Listen("tcp", "0:2000")rules/go/security/avoid-bind-to-all-interfaces-go.yml (4)
4-8: Enhance the message with more actionable guidance.The current message could be more specific about recommended secure alternatives.
message: >- "Detected a network listener listening on 0.0.0.0 or an empty string. This could unexpectedly expose the server publicly as it binds to all - available interfaces. Instead, specify another IP address that is not - 0.0.0.0 nor the empty string." + available interfaces. Use '127.0.0.1' or 'localhost' for local-only access, + or specify a specific interface IP address for controlled network access."
9-12: Add more specific security references.Consider adding more relevant CWE references for network exposure:
- CWE-1327: Binding to an Unrestricted IP Address
- CWE-668: Exposure of Resource to Wrong Sphere
note: >- [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor + [CWE-1327] Binding to an Unrestricted IP Address + [CWE-668] Exposure of Resource to Wrong Sphere [REFERENCES] - https://owasp.org/Top10/A01_2021-Broken_Access_Control + - https://cwe.mitre.org/data/definitions/1327.html
19-21: Consider expanding pattern matching for comprehensive coverage.The rule could be enhanced to catch more network binding scenarios:
any: - pattern: tls.Listen($NETWORK, $IP $$$) - pattern: net.Listen($NETWORK, $IP $$$) + - pattern: http.ListenAndServe($IP $$$) + - pattern: http.ListenAndServeTLS($IP $$$) + - pattern: grpc.NewServer().Serve(net.Listen($NETWORK, $IP $$$))
26-29: Strengthen regex patterns for IP matching.The current regex patterns could be enhanced to catch more edge cases:
- kind: interpreted_string_literal - regex: ^"0.0.0.0:.*"$|^":.*"$|^'0.0.0.0:.*'$|^':.*'$ + regex: ^"(0\.0\.0\.0|0|::|):.*"$|^":.*"$|^'(0\.0\.0\.0|0|::|):.*'$|^':.*'$ - kind: raw_string_literal - regex: ^`0.0.0.0:.*`$|^`:.*`$ + regex: ^`(0\.0\.0\.0|0|::|):.*`$|^`:.*`$
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
rules/cpp/return-c-str-c.yml(1 hunks)rules/go/security/avoid-bind-to-all-interfaces-go.yml(1 hunks)tests/__snapshots__/avoid-bind-to-all-interfaces-go-snapshot.yml(1 hunks)tests/__snapshots__/return-c-str-cpp-snapshot.yml(1 hunks)tests/__snapshots__/sizeof-this-cpp-snapshot.yml(1 hunks)tests/cpp/return-c-str-cpp-test.yml(1 hunks)tests/go/avoid-bind-to-all-interfaces-go-test.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/snapshots/avoid-bind-to-all-interfaces-go-snapshot.yml
- tests/snapshots/return-c-str-cpp-snapshot.yml
🔇 Additional comments (2)
rules/cpp/return-c-str-c.yml (1)
4-12: LGTM! Well-documented security rule with proper references.The warning message is clear and the documentation includes relevant CWE and SEI CERT references. Good practice linking to both C and C++ specific guidelines.
tests/__snapshots__/sizeof-this-cpp-snapshot.yml (1)
2-13: LGTM! Well-structured snapshot with proper label positioning.The snapshot correctly captures the sizeof(this) expression with appropriate primary and secondary labels for syntax highlighting.
Summary by CodeRabbit
New Features
Tests