Add YAML-based AST security rules and tests for C#, Java, Ruby #186
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 for C#, Java, and Ruby by adding YAML configuration files that use AST pattern matching to detect insecure practices. The rules flag improper usage of methods that expose sensitive data, such as Changes
Sequence Diagram(s)sequenceDiagram
participant CSharp as C# Code
participant Analyzer as AST Analyzer
participant Rule as Stacktrace Disclosure Rule
CSharp->>Analyzer: Submit source code
Analyzer->>Rule: Match UseDeveloperExceptionPage usage
Rule-->>Analyzer: Validate environment context
Analyzer->>CSharp: Emit warning if misused
sequenceDiagram
participant Java as Java Code
participant Analyzer as AST Analyzer
participant Rule as Weak SSL Context Rule
Java->>Analyzer: Submit source code
Analyzer->>Rule: Identify SSLContext.getInstance call
Rule-->>Analyzer: Check protocol arguments
Analyzer->>Java: Flag insecure protocol usage
sequenceDiagram
participant Ruby as Ruby Code
participant Analyzer as AST Analyzer
participant Rule as RSA Passphrase Rule
Ruby->>Analyzer: Submit source code
Analyzer->>Rule: Detect hardcoded passphrase in RSA calls
Rule-->>Analyzer: Validate secret handling
Analyzer->>Ruby: Emit warning for hardcoding issues
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
tests/java/weak-ssl-context-java-test.yml (1)
1-9: Clear Separation of Valid Scenarios
The valid section clearly differentiates secure SSL context instantiation examples by demonstrating protocols "TLSv1.2", "TLSv1.3", and a dynamic use viagetSslContext(). This explicit separation helps ensure that only properly secured contexts are accepted.rules/java/security/weak-ssl-context-java.yml (2)
16-35: Remove Unnecessary Commented Code
The commented-out rule configuration (lines 16–35) appears redundant given the active rule block below. Removing it (or moving it to a developer note) could help keep the file concise and easier to maintain.
36-80: AST Rule Configuration Complexity
The AST rule block is quite intricate and appears intended to flag insecure invocations ofSSLContext.getInstance—by allowing only "TLSv1.2" and "TLSv1.3". Please verify that the use ofnthChild: 3for the argument list aligns with your AST’s node structure. Adding inline comments to explain the nested conditional logic (especially in lines 57–72) would benefit future maintainers in understanding the rationale behind these constraints.tests/__snapshots__/weak-ssl-context-java-snapshot.yml (1)
1-158: Comprehensive Snapshot Documentation
This snapshot file meticulously captures various insecure SSLContext instantiations (using "SSL", "SSLv3", "TLS", "TLSv1", and "TLSv1.1") along with detailed labeling of each component (primary and secondary). Such granularity is highly useful for validating the AST rule’s performance. Please confirm that the repeated label entries (e.g. duplicate protocol labels) are intentional and are produced consistently by the AST tool; if not, consider streamlining these to ease future maintenance.tests/ruby/hardcoded-secret-rsa-passphrase-ruby-test.yml (1)
15-31: Insecure Examples Clearly Illustrate Hardcoded Secrets.
The “invalid” section intentionally includes hardcoded values (e.g.$pass = 'super secret',@pass1 = 'my secure pass phrase goes here', and the literal"secret"in the RSA instantiation) to trigger the detection rule. Consider adding inline comments or annotations within this block to document each vulnerability for clarity during testing.tests/__snapshots__/hardcoded-secret-rsa-passphrase-ruby-snapshot.yml (1)
18-42: Detailed Label Definitions for AST Matching.
The labels provide a granular breakdown of the code segments and their offsets. Please verify that thestartandendpositions accurately reflect the intended AST node boundaries. Additionally, if possible, consider consolidating overly granular labels to reduce redundancy.rules/ruby/security/hardcoded-secret-rsa-passphrase-ruby.yml (4)
1-15: Metadata and Rule Configuration Look Correct.
The rule metadata is properly set up with an appropriate severity level and clear messaging. One minor nitpick: update “an hardcoded” to “a hardcoded” for grammatical accuracy.
44-74: Refine Regex in Call Chain Matching.
This segment for detectingOpenSSL::PKey::RSA.new(...).to_pem(..., '...')is well structured. However, the regex pattern^to_pem|export$(line 57) could be more precise if rewritten as^(to_pem|export)$to avoid any unintended matches.
106-137: Consistent Matching for Extended Method Chains.
TheOpenSSL::PKey::RSA.new(...).to_pem(..., '...')_with_instancesegment mirrors earlier patterns with an additional instance check. As with the previous block, consider updating the regex (line 117) to^(to_pem|export)$for clarity and accuracy.
171-204: Thorough Matching for$OPENSSL.to_pemUsage.
The segment captures method calls on$OPENSSLwith a hardcoded secret ($ASSIGN) and validates corresponding assignments within the class. Similar to earlier notes, consider refining the regex for method identifiers for enhanced precision.tests/__snapshots__/stacktrace-disclosure-csharp-snapshot.yml (1)
6-6: Trailing Whitespace Issue
YAMLlint has flagged trailing spaces on several lines (lines 6, 10, 12, 20, 22, 25, 30, 32, 35, and 37). Please remove these extraneous spaces to ensure a clean and consistent YAML format.Also applies to: 10-10, 12-12, 20-20, 22-22, 25-25, 30-30, 32-32, 35-35, 37-37
tests/csharp/stacktrace-disclosure-csharp-test.yml (1)
6-6: Trailing Whitespace Issue
YAMLlint reports trailing spaces on several lines (lines 6, 10, 12, 20, 22, 25, 30, 32, 35, and 37). Please remove these trailing spaces to maintain consistency and prevent potential formatting issues.Also applies to: 10-10, 12-12, 20-20, 22-22, 25-25, 30-30, 32-32, 35-35, 37-37
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: trailing spaces
(trailing-spaces)
rules/csharp/security/stacktrace-disclosure-csharp.yml (1)
6-6: Trailing Whitespace Issue
YAMLlint has identified trailing spaces on multiple lines (lines 6, 10, 12, 20, 22, 25, 30, 32, 35, and 37). Removing these will help ensure the YAML remains clean and compliant with formatting standards.Also applies to: 10-10, 12-12, 20-20, 22-22, 25-25, 30-30, 32-32, 35-35, 37-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
rules/csharp/security/stacktrace-disclosure-csharp.yml(1 hunks)rules/java/security/weak-ssl-context-java.yml(1 hunks)rules/ruby/security/hardcoded-secret-rsa-passphrase-ruby.yml(1 hunks)tests/__snapshots__/hardcoded-secret-rsa-passphrase-ruby-snapshot.yml(1 hunks)tests/__snapshots__/stacktrace-disclosure-csharp-snapshot.yml(1 hunks)tests/__snapshots__/weak-ssl-context-java-snapshot.yml(1 hunks)tests/csharp/stacktrace-disclosure-csharp-test.yml(1 hunks)tests/java/weak-ssl-context-java-test.yml(1 hunks)tests/ruby/hardcoded-secret-rsa-passphrase-ruby-test.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/csharp/stacktrace-disclosure-csharp-test.yml
[error] 6-6: trailing spaces
(trailing-spaces)
[error] 10-10: trailing spaces
(trailing-spaces)
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
🔇 Additional comments (11)
tests/java/weak-ssl-context-java-test.yml (1)
9-19: Comprehensive Insecure Protocol Testing
The invalid section effectively enumerates deprecated or insecure protocols—"SSL", "TLS", "TLSv1", "SSLv3", and "TLSv1.1"—thus covering all the anticipated insecure instantiation scenarios for SSLContext. This should provide robust testing of the rule’s negative matches.rules/java/security/weak-ssl-context-java.yml (1)
1-15: Metadata and Documentation Clarity
The metadata (id, language, severity, message, note) is clear and informative. Including references to [CWE-326] and RFC links strengthens the guidance provided by the rule message.tests/ruby/hardcoded-secret-rsa-passphrase-ruby-test.yml (1)
1-14: Valid Examples Adequately Demonstrate Secure Practices.
The examples in the “valid” section correctly use environment variables (e.g.ENV['SECRET_PASSPHRASE']andENV['SECURE_KEY']) for RSA key operations rather than hardcoding secrets. This aligns well with the security objectives.tests/__snapshots__/hardcoded-secret-rsa-passphrase-ruby-snapshot.yml (1)
1-17: Snapshot Captures Insecure Pattern Effectively.
The snapshot block reproduces the insecure code (with hardcoded passphrases and key values) as expected. This provides a good reference for what the rule should flag.rules/ruby/security/hardcoded-secret-rsa-passphrase-ruby.yml (4)
17-43: AST Matching for RSA Instantiation is Thorough.
The first utils block targetingOpenSSL::PKey::RSA.new(..., '...')leverages detailed AST conditions. The use ofnthChild: 2to capture the hardcoded string is appropriate. No changes are needed here.
75-105: Instance-Based Detection Block is Appropriately Detailed.
The block forOpenSSL::PKey::RSA.new(..., '...')_with_instancecorrectly extends the matching by considering an instance context (including checking for an assignment of$SECRET). This helps pinpoint hardcoded secrets embedded in class structures.
138-170: Detection of$OPENSSL.exportCalls is Clear and Structured.
This block accurately targets calls to$OPENSSL.export(...,'...')and also includes an inner check for assignment patterns. The logic is sound and aligns with the detection strategy.
205-235: Aggregated Rule Matches are Comprehensive.
The finalmatch_callandrulesections effectively aggregate all individual patterns into a single rule. This ensures that various insecure patterns are detected. The design is robust and the use of the “any” matcher makes the rule flexible.tests/__snapshots__/stacktrace-disclosure-csharp-snapshot.yml (1)
1-43: Snapshot Content Verification
The snapshot file clearly lists multiple usage scenarios forapp.UseDeveloperExceptionPage(), covering diverse conditional invocations (e.g. checking for non-development environments, specific days, and even within a method). This comprehensive coverage will help ensure that the new AST-based rule is thoroughly validated.tests/csharp/stacktrace-disclosure-csharp-test.yml (1)
1-39: Test Scenarios Are Well-Covered
The test file defines both valid and invalid blocks appropriately. The valid case properly usesapp.UseExceptionHandler("/Error")whenenv.IsDevelopment()is true, while the invalid cases cover multiple misconfigurations whereapp.UseDeveloperExceptionPage()is used inappropriately. This robust set of scenarios should effectively validate the new security rule.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: trailing spaces
(trailing-spaces)
[error] 10-10: trailing spaces
(trailing-spaces)
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
rules/csharp/security/stacktrace-disclosure-csharp.yml (1)
1-46: Security Rule Configuration Looks Solid
The rule is well structured and documented. It clearly defines the severity, language, message, and even provides helpful references (CWE-209 and OWASP guidelines). The AST pattern matching under theutilssection is logically grouped and should effectively detect invocations ofUseDeveloperExceptionPageoutside the permitted contexts.
Summary by CodeRabbit
New Features
Tests