Add security static analysis rules for C, Java, and Swift #129
Conversation
WalkthroughThis pull request introduces three new security-focused static analysis rules across different programming languages: C, Java, and Swift. The rules aim to address specific security and code quality concerns. In C, a rule warns about incorrect usage of Changes
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 (
|
|
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. |
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
🧹 Nitpick comments (12)
tests/swift/insecure-biometrics-swift-test.yml (3)
2-4: Enhance test coverage with more valid cases.Consider adding more valid test cases to demonstrate proper biometric authentication implementations using Keychain Services. This will help developers understand the correct approach.
valid: - | abc.anyFunc() + - | + abc.accessControl(withBiometrics: .biometryAny) + - | + abc.accessControl(withBiometrics: .biometryCurrentSet)
5-7: Add more invalid test cases for comprehensive detection.Include additional patterns of insecure biometric implementations to improve rule coverage.
invalid: - | abc.evaluatePolicy() + - | + abc.evaluatePolicy(LAPolicy.deviceOwnerAuthenticationWithBiometrics) + - | + abc.evaluatePolicy(.deviceOwnerAuthenticationWithBiometrics)🧰 Tools
🪛 yamllint (1.35.1)
[error] 7-7: no new line character at the end of file
(new-line-at-end-of-file)
7-7: Add newline at end of file.Add a newline character at the end of the file to comply with YAML formatting standards.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 7-7: no new line character at the end of file
(new-line-at-end-of-file)
rules/swift/security/insecure-biometrics-swift.yml (2)
4-10: Fix message indentation.The message contains inconsistent indentation which might affect readability.
message: >- The application was observed to leverage biometrics via Local - Authentication, which returns a simple boolean result for authentication. - This design is subject to bypass with runtime tampering tools such as - Frida, Substrate, and others. Although this is limited to rooted - (jailbroken) devices, consider implementing biometric authentication the - reliable way - via Keychain Services. + Authentication, which returns a simple boolean result for authentication. + This design is subject to bypass with runtime tampering tools such as + Frida, Substrate, and others. Although this is limited to rooted + (jailbroken) devices, consider implementing biometric authentication the + reliable way - via Keychain Services.
11-15: Enhance documentation with code examples.The note section provides good references but could be more helpful with concrete code examples showing both incorrect and correct implementations.
note: >- [CWE-305] Authentication Bypass by Primary Weakness [REFERENCES] - https://mobile-security.gitbook.io/mobile-security-testing-guide/ios-testing-guide/0x06f-testing-local-authentication - https://shirazkhan030.medium.com/biometric-authentication-in-ios-6c53c54f17df + [EXAMPLES] + Incorrect: + let context = LAContext() + context.evaluatePolicy(.deviceOwnerAuthenticationWithBiometrics) { success, error in } + + Correct: + let access = SecAccessControlCreateWithFlags( + kCFAllocatorDefault, + kSecAttrAccessibleWhenPasscodeSetThisDeviceOnly, + .biometryAny, + nil + )rules/java/security/drivermanager-hardcoded-secret-java.yml (2)
51-76: Remove trailing whitespace and consider additional scenarios.The pattern should also consider:
- Setter methods like
setPassword()that might be called after construction- Other common data source implementations like
BasicDataSourceAlso, fix the trailing whitespace on lines 52, 64, and 72.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 52-52: trailing spaces
(trailing-spaces)
[error] 64-64: trailing spaces
(trailing-spaces)
[error] 72-72: trailing spaces
(trailing-spaces)
84-89: Strengthen the constraints for better detection.Consider adding constraints to detect:
- Common default passwords
- Short passwords
- Simple patterns (e.g., "123456", "password")
Also, remove the excessive blank lines at the end of the file.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 89-89: too many blank lines
(2 > 0) (empty-lines)
tests/c/sizeof-this-c-test.yml (1)
2-4: Enhance valid test cases with more context and variations.The valid test case should include:
- Complete class/struct context
- Different valid patterns (e.g., sizeof(*this), sizeof(Foo))
- Template class scenarios
valid: - | - sizeof(*this); + struct Foo { + uint64_t a; + uint8_t b; + size_t get_size() const { + return sizeof(*this); // Valid: Gets size of the object + } + size_t get_type_size() const { + return sizeof(Foo); // Valid: Alternative approach + } + }; + + template<typename T> + struct Bar { + T value; + size_t get_size() const { + return sizeof(*this); // Valid: Works with templates + } + };tests/__snapshots__/sizeof-this-cpp-snapshot.yml (1)
6-13: Enhance label descriptions for better clarity.The labels could be more descriptive to better explain the issue to developers.
labels: - source: sizeof(this) style: primary + message: "Using sizeof on 'this' pointer returns pointer size (typically 4 or 8 bytes)" start: 7 end: 19 - source: this style: secondary + message: "Consider using sizeof(*this) to get the actual object size" start: 14 end: 18rules/c/security/sizeof-this-c.yml (3)
4-10: Enhance error message with more specific guidance.The error message could be more helpful by providing explicit examples of correct usage.
message: >- Do not use `sizeof(this)` to get the number of bytes of the object in - memory. It returns the size of the pointer, not the size of the object. + memory. It returns the size of the pointer (typically 4 or 8 bytes), not + the size of the object. Use `sizeof(*this)` or `sizeof(TypeName)` instead. note: >- [CWE-467]: Use of sizeof() on a Pointer Type [REFERENCES] - https://wiki.sei.cmu.edu/confluence/display/c/ARR01-C.+Do+not+apply+the+sizeof+operator+to+a+pointer+when+taking+the+size+of+an+array + - https://cwe.mitre.org/data/definitions/467.html
45-45: Fix YAML formatting issues.Several formatting issues were reported by yamllint:
- Trailing spaces
- Incorrect indentation
- Missing newline at end of file
nthChild: 1 - + has: - has: + has: pattern: $THIS - + has: - + constraints: THIS: - regex: ^this$ + regex: ^this$ +Also applies to: 67-67, 75-75, 76-76, 79-79, 87-87, 90-90
🧰 Tools
🪛 yamllint (1.35.1)
[error] 45-45: trailing spaces
(trailing-spaces)
12-87: Consider adding pattern for member function context.The rule could be improved by adding a pattern to specifically match member function contexts where
thisis valid.Would you like me to provide an implementation for this enhancement?
🧰 Tools
🪛 yamllint (1.35.1)
[error] 45-45: trailing spaces
(trailing-spaces)
[warning] 67-67: wrong indentation: expected 18 but found 20
(indentation)
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
[warning] 85-85: too many spaces after colon
(colons)
[error] 87-87: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
rules/c/security/sizeof-this-c.yml(1 hunks)rules/java/security/drivermanager-hardcoded-secret-java.yml(1 hunks)rules/swift/security/insecure-biometrics-swift.yml(1 hunks)tests/__snapshots__/drivermanager-hardcoded-secret-java-snapshot.yml(1 hunks)tests/__snapshots__/insecure-biometrics-swift-snapshot.yml(1 hunks)tests/__snapshots__/sizeof-this-c-snapshot.yml(1 hunks)tests/__snapshots__/sizeof-this-cpp-snapshot.yml(1 hunks)tests/c/sizeof-this-c-test.yml(1 hunks)tests/java/drivermanager-hardcoded-secret-java-test.yml(1 hunks)tests/swift/insecure-biometrics-swift-test.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/snapshots/sizeof-this-c-snapshot.yml
- tests/snapshots/insecure-biometrics-swift-snapshot.yml
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/swift/insecure-biometrics-swift-test.yml
[error] 7-7: no new line character at the end of file
(new-line-at-end-of-file)
rules/c/security/sizeof-this-c.yml
[error] 45-45: trailing spaces
(trailing-spaces)
[warning] 67-67: wrong indentation: expected 18 but found 20
(indentation)
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
[warning] 85-85: too many spaces after colon
(colons)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 90-90: no new line character at the end of file
(new-line-at-end-of-file)
rules/java/security/drivermanager-hardcoded-secret-java.yml
[warning] 17-17: wrong indentation: expected 4 but found 3
(indentation)
[warning] 19-19: wrong indentation: expected 5 but found 6
(indentation)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 52-52: trailing spaces
(trailing-spaces)
[error] 64-64: trailing spaces
(trailing-spaces)
[error] 72-72: trailing spaces
(trailing-spaces)
[warning] 89-89: too many blank lines
(2 > 0) (empty-lines)
🔇 Additional comments (1)
tests/__snapshots__/drivermanager-hardcoded-secret-java-snapshot.yml (1)
1-45: LGTM! Consider adding snapshots for recommended test cases.The snapshot definitions are well-structured and correctly highlight the relevant parts of the code. Update them when adding the suggested test cases.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rules/swift/security/insecure-biometrics-swift.yml (1)
17-43: 🛠️ Refactor suggestionEnhance pattern matching for comprehensive detection.
The current patterns might miss common variations of insecure biometric implementations.
rule: any: - kind: navigation_expression - pattern: $X.evaluatePolicy + patterns: + - pattern: $X.evaluatePolicy + - pattern: $X.evaluatePolicy($POLICY) + - pattern: $X.evaluatePolicy($POLICY, localizedReason: $REASON) + - pattern: LAContext().evaluatePolicy + - pattern: LAContext().canEvaluatePolicy🧰 Tools
🪛 yamllint (1.35.1)
[warning] 19-19: wrong indentation: expected 4 but found 3
(indentation)
[warning] 19-19: too many spaces after hyphen
(hyphens)
[warning] 22-22: wrong indentation: expected 8 but found 9
(indentation)
[warning] 23-23: wrong indentation: expected 11 but found 12
(indentation)
[warning] 26-26: wrong indentation: expected 14 but found 15
(indentation)
[warning] 28-28: too many spaces after hyphen
(hyphens)
[warning] 30-30: wrong indentation: expected 8 but found 9
(indentation)
[error] 32-32: trailing spaces
(trailing-spaces)
[warning] 33-33: wrong indentation: expected 11 but found 12
(indentation)
[warning] 36-36: wrong indentation: expected 8 but found 9
(indentation)
[warning] 37-37: wrong indentation: expected 11 but found 12
(indentation)
[warning] 40-40: wrong indentation: expected 14 but found 15
(indentation)
[warning] 42-42: too many spaces after hyphen
(hyphens)
[error] 43-43: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (3)
rules/swift/security/insecure-biometrics-swift.yml (3)
1-16: Consider elevating the severity level and adding official documentation.Given that biometric authentication bypass can lead to unauthorized access, consider changing the severity from "info" to "warning" or "error". Additionally, enhance the references section with Apple's official documentation:
- https://developer.apple.com/documentation/localauthentication
- https://developer.apple.com/documentation/security/keychain_services
-severity: info +severity: warning
17-47: Fix YAML formatting issues.The YAML structure has inconsistent indentation and trailing spaces that should be fixed for better maintainability.
rule: any: - - kind: navigation_expression + - kind: navigation_expression pattern: $X.evaluatePolicy not: - has: - stopBy: end + has: + stopBy: end # ... continue fixing indentation for the rest of the file🧰 Tools
🪛 yamllint (1.35.1)
[warning] 19-19: wrong indentation: expected 4 but found 3
(indentation)
[warning] 19-19: too many spaces after hyphen
(hyphens)
[warning] 22-22: wrong indentation: expected 8 but found 9
(indentation)
[warning] 23-23: wrong indentation: expected 11 but found 12
(indentation)
[warning] 26-26: wrong indentation: expected 14 but found 15
(indentation)
[warning] 28-28: too many spaces after hyphen
(hyphens)
[warning] 30-30: wrong indentation: expected 8 but found 9
(indentation)
[error] 32-32: trailing spaces
(trailing-spaces)
[warning] 33-33: wrong indentation: expected 11 but found 12
(indentation)
[warning] 36-36: wrong indentation: expected 8 but found 9
(indentation)
[warning] 37-37: wrong indentation: expected 11 but found 12
(indentation)
[warning] 40-40: wrong indentation: expected 14 but found 15
(indentation)
[warning] 42-42: too many spaces after hyphen
(hyphens)
[error] 43-43: trailing spaces
(trailing-spaces)
[warning] 45-45: wrong indentation: expected 4 but found 3
(indentation)
[warning] 46-46: wrong indentation: expected 5 but found 6
(indentation)
17-47: Simplify rule structure for better maintainability.The current rule structure has redundant patterns and complex nesting. Consider simplifying it while maintaining the same functionality.
rule: any: - - kind: navigation_expression - pattern: $X.evaluatePolicy - not: - has: - stopBy: end - kind: tuple_expression - has: - nthChild: 2 - - kind: navigation_expression - has: - kind: navigation_suffix - regex: \.evaluatePolicy$ - - pattern: '.evaluatePolicy' + - pattern: $X.evaluatePolicy($ARGS) + where: + $X: + matches: LAContext + $ARGS: + not: + includes: reply + - pattern: $X.canEvaluatePolicy($ARGS) + where: + $X: + matches: LAContext🧰 Tools
🪛 yamllint (1.35.1)
[warning] 19-19: wrong indentation: expected 4 but found 3
(indentation)
[warning] 19-19: too many spaces after hyphen
(hyphens)
[warning] 22-22: wrong indentation: expected 8 but found 9
(indentation)
[warning] 23-23: wrong indentation: expected 11 but found 12
(indentation)
[warning] 26-26: wrong indentation: expected 14 but found 15
(indentation)
[warning] 28-28: too many spaces after hyphen
(hyphens)
[warning] 30-30: wrong indentation: expected 8 but found 9
(indentation)
[error] 32-32: trailing spaces
(trailing-spaces)
[warning] 33-33: wrong indentation: expected 11 but found 12
(indentation)
[warning] 36-36: wrong indentation: expected 8 but found 9
(indentation)
[warning] 37-37: wrong indentation: expected 11 but found 12
(indentation)
[warning] 40-40: wrong indentation: expected 14 but found 15
(indentation)
[warning] 42-42: too many spaces after hyphen
(hyphens)
[error] 43-43: trailing spaces
(trailing-spaces)
[warning] 45-45: wrong indentation: expected 4 but found 3
(indentation)
[warning] 46-46: wrong indentation: expected 5 but found 6
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rules/swift/security/insecure-biometrics-swift.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
rules/swift/security/insecure-biometrics-swift.yml
[warning] 19-19: wrong indentation: expected 4 but found 3
(indentation)
[warning] 19-19: too many spaces after hyphen
(hyphens)
[warning] 22-22: wrong indentation: expected 8 but found 9
(indentation)
[warning] 23-23: wrong indentation: expected 11 but found 12
(indentation)
[warning] 26-26: wrong indentation: expected 14 but found 15
(indentation)
[warning] 28-28: too many spaces after hyphen
(hyphens)
[warning] 30-30: wrong indentation: expected 8 but found 9
(indentation)
[error] 32-32: trailing spaces
(trailing-spaces)
[warning] 33-33: wrong indentation: expected 11 but found 12
(indentation)
[warning] 36-36: wrong indentation: expected 8 but found 9
(indentation)
[warning] 37-37: wrong indentation: expected 11 but found 12
(indentation)
[warning] 40-40: wrong indentation: expected 14 but found 15
(indentation)
[warning] 42-42: too many spaces after hyphen
(hyphens)
[error] 43-43: trailing spaces
(trailing-spaces)
[warning] 45-45: wrong indentation: expected 4 but found 3
(indentation)
[warning] 46-46: wrong indentation: expected 5 but found 6
(indentation)
🔇 Additional comments (1)
rules/swift/security/insecure-biometrics-swift.yml (1)
1-47: Verify rule effectiveness with test cases.To ensure the rule catches all insecure biometric implementations while avoiding false positives, we should verify it against various test cases.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 19-19: wrong indentation: expected 4 but found 3
(indentation)
[warning] 19-19: too many spaces after hyphen
(hyphens)
[warning] 22-22: wrong indentation: expected 8 but found 9
(indentation)
[warning] 23-23: wrong indentation: expected 11 but found 12
(indentation)
[warning] 26-26: wrong indentation: expected 14 but found 15
(indentation)
[warning] 28-28: too many spaces after hyphen
(hyphens)
[warning] 30-30: wrong indentation: expected 8 but found 9
(indentation)
[error] 32-32: trailing spaces
(trailing-spaces)
[warning] 33-33: wrong indentation: expected 11 but found 12
(indentation)
[warning] 36-36: wrong indentation: expected 8 but found 9
(indentation)
[warning] 37-37: wrong indentation: expected 11 but found 12
(indentation)
[warning] 40-40: wrong indentation: expected 14 but found 15
(indentation)
[warning] 42-42: too many spaces after hyphen
(hyphens)
[error] 43-43: trailing spaces
(trailing-spaces)
[warning] 45-45: wrong indentation: expected 4 but found 3
(indentation)
[warning] 46-46: wrong indentation: expected 5 but found 6
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
rules/c/security/sizeof-this-c.yml (5)
1-11: Consider elevating severity and enhancing the message.The rule metadata is well-structured, but consider these improvements:
- Elevate severity to
errorsincesizeof(this)misuse can lead to buffer overflows- Add the correct usage pattern
sizeof(*this)to the message to make it more actionableid: sizeof-this-c language: c -severity: warning +severity: error message: >- Do not use `sizeof(this)` to get the number of bytes of the object in - memory. It returns the size of the pointer, not the size of the object. + memory. It returns the size of the pointer, not the size of the object. + Use `sizeof(*this)` instead to get the actual object size.
65-68: Remove commented out code.Remove these commented lines as they add noise to the rule definition. If they represent a future enhancement, create an issue to track it instead.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 68-68: trailing spaces
(trailing-spaces)
12-128: Consider simplifying the AST patterns.The rule pattern has multiple branches that look similar, particularly around parameter list matching. Consider consolidating these patterns to reduce duplication and improve maintainability.
For example, the patterns at lines 44-64 and 70-98 could potentially be unified since they both match parameter declarations with similar constraints.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 68-68: trailing spaces
(trailing-spaces)
[warning] 90-90: wrong indentation: expected 18 but found 20
(indentation)
[error] 98-98: trailing spaces
(trailing-spaces)
[error] 102-102: trailing spaces
(trailing-spaces)
[warning] 108-108: too many spaces after colon
(colons)
[error] 124-124: trailing spaces
(trailing-spaces)
129-131: Add word boundaries to the regex pattern.To prevent false positives where "this" appears as part of another identifier, consider adding word boundaries to the regex:
constraints: THIS: - regex: ^this$ + regex: \bthis\b
43-43: Fix YAML formatting issues.Several formatting issues were detected by yamllint:
- Remove trailing spaces (lines 43, 68, 98, 102, 124)
- Fix indentation at line 90 (expected 18 spaces, found 20)
- Fix extra space after colon at line 108
Also applies to: 68-68, 98-98, 102-102, 108-108, 124-124
🧰 Tools
🪛 yamllint (1.35.1)
[error] 43-43: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rules/c/security/sizeof-this-c.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
rules/c/security/sizeof-this-c.yml
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 68-68: trailing spaces
(trailing-spaces)
[warning] 90-90: wrong indentation: expected 18 but found 20
(indentation)
[error] 98-98: trailing spaces
(trailing-spaces)
[error] 102-102: trailing spaces
(trailing-spaces)
[warning] 108-108: too many spaces after colon
(colons)
[error] 124-124: trailing spaces
(trailing-spaces)
Summary by CodeRabbit
Release Notes
New Security Rules
sizeof(this)in C programming.Test Cases
Snapshots
These updates enhance code security by detecting potential vulnerabilities in C, Java, and Swift programming practices.