SONARJAVA-6783: Implement rule S9345 Classes with throwing constructors should be protected against Finalizer attacks - #5981
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| if (ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.FINAL) || | ||
| ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.ABSTRACT)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Sealed classes flagged as noncompliant (false positive)
A sealed non-final class cannot be subclassed by an attacker — its permitted subclasses are fixed at compile time — so it is not exploitable by a Finalizer attack, yet the check only exempts final and abstract classes and would report a sealed class with a throwing constructor. Consider also returning early when ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.SEALED) is true (subject to RSPEC scope), to avoid false positives on sealed hierarchies.
Also skip sealed classes, which attackers cannot subclass.:
if (ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.FINAL) ||
ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.ABSTRACT) ||
ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.SEALED)) {
return;
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| for (Tree member : classTree.members()) { | ||
| if (member.is(Kind.CONSTRUCTOR) && isVulnerableConstructor((MethodTree) member)) { | ||
| reportIssue(classTree.simpleName(), "Make this class \"final\" or make the throwing constructors \"private\"."); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static boolean isVulnerableConstructor(MethodTree constructor) { | ||
| if (ModifiersUtils.hasModifier(constructor.modifiers(), Modifier.PRIVATE)) { | ||
| return false; | ||
| } | ||
| return !constructor.throwsClauses().isEmpty() || containsThrowStatement(constructor); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Edge Case: Throwing field/instance initializers not detected
The check only inspects CONSTRUCTOR members and scans each constructor's own block, so a throw that occurs in an instance initializer block or a field initializer (e.g. private final X x = compute(); where compute() throws) is missed even though those run during construction. A class with no explicit constructor but a throwing field/instance initializer has no CONSTRUCTOR member at all and is never flagged, a false negative for the same finalizer-attack vector. Consider also examining instance initializer blocks and field initializers, and accounting for the implicit default constructor.
Was this helpful? React with 👍 / 👎
|
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/FinalizerAttackCheckSample.java")) | ||
| .withCheck(new FinalizerAttackCheck()) |
There was a problem hiding this comment.
We should add a test withoutSemantic
|
❌ Ruling needs updating. A fix PR has been created: #5982 Please review and merge it into your branch. |
| "sqKey": "S9345", | ||
| "scope": "Main", | ||
| "defaultQualityProfiles": [ | ||
| "Sonar way" |
There was a problem hiding this comment.
The rule should have been added to the quality profiles. rule-api needs to be re-run.
Ruling Diff SummaryDetected changes in 4 rule files: 0 issues removed, 56 issues added. S9345 (
|
| { | ||
| "com.macro.mall:mall:mall-admin/src/main/java/com/macro/mall/dto/PmsProductResult.java": [ | ||
| 11 | ||
| ] |
There was a problem hiding this comment.
💡 Quality: Unrelated ruling results (S2160, S6212) bundled into new-rule PR
This PR adds rule S9345, but it also introduces a new S2160 expected-results file and adds line 91 to the S6212 results for the mall project. These rules are unrelated to the Finalizer-attack rule and the ruling projects (mall, guava, etc.) are unaffected by the test-source and check additions here, so these diffs likely reflect stale/regenerated results being swept in rather than an intended change. Bundling unrelated rule-result churn into a feature PR obscures whether it hides a real regression; either split these out into a dedicated ruling-update PR or confirm they are expected and intentional.
Was this helpful? React with 👍 / 👎
|
❌ Ruling needs updating. A fix PR has been created: #5983 Please review and merge it into your branch. |
…rs should be protected against Finalizer attacks Detect non-final, non-abstract classes whose non-private constructors can throw exceptions (via throws clause or throw statements in the body), making them vulnerable to Finalizer attacks through malicious subclasses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🤖 Generated with GitHub Actions
…houtSemantic test - Make inner classes static in FinalizerAttackCheckSample to fix "non-static variable this cannot be referenced from a static context" compilation error caused by FactoryService's static factory method - Add S9345 placeholder to Sonar way quality profile - Add withoutSemantic test since the check only uses syntactic analysis Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d5edb3f to
ea06a0f
Compare
|
❌ Ruling needs updating. A fix PR has been created: #5983 Please review and merge it into your branch. |
…ass as secondary The main issue location is now on the throwing constructor (primary) with the class declaration as a secondary location, instead of the other way around. Each vulnerable constructor gets its own issue. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI failed: CI failures caused by a compilation error in a test sample (referencing 'this' from a static context) and integration test ruling mismatches from the new S9345 rule implementation.OverviewTwo distinct change-related failure patterns were identified across the CI logs: a compilation failure due to an invalid static reference in FailuresCompilation Error in FinalizerAttackCheckSample (confidence: high)
Java Ruling Integration Test Failure / Rule Exception (confidence: high)
Summary
Code Review 👍 Approved with suggestions 0 resolved / 3 findingsImplements rule S9345 to detect classes with throwing constructors vulnerable to Finalizer attacks. Consider addressing false positives on sealed classes, handling throwing initializers, and separating unrelated ruling changes. 💡 Edge Case: Sealed classes flagged as noncompliant (false positive)📄 java-checks/src/main/java/org/sonar/java/checks/FinalizerAttackCheck.java:45-48 A Also skip sealed classes, which attackers cannot subclass.💡 Edge Case: Throwing field/instance initializers not detected📄 java-checks/src/main/java/org/sonar/java/checks/FinalizerAttackCheck.java:49-63 The check only inspects 💡 Quality: Unrelated ruling results (S2160, S6212) bundled into new-rule PR📄 its/ruling/src/test/resources/mall/java-S2160.json:1-4 📄 its/ruling/src/test/resources/mall/java-S6212.json:13 This PR adds rule S9345, but it also introduces a new S2160 expected-results file and adds line 91 to the S6212 results for the 🤖 Prompt for agentsImplementation Status ✅ 1 / 1 issues implemented✅ SONARJAVA-6783 — 1 / 1 objectivesThe PR successfully implements rule S9345 to detect non-final classes with throwing constructors and protect them against Finalizer attacks. ✅ 1 complete
Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
1 participant
Summary
throwsclause or containsthrowstatementsfinalclasses,abstractclasses, classes with onlyprivatethrowing constructors (factory pattern), enums, records, and classes without throwing constructorsTest plan
CheckVerifiercovering noncompliant and compliant patterns🤖 Generated with Claude Code