re-use vulnerability type/detector instances to avoid unnecessary mem usage #247
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| return new Attack( | ||
| operation, | ||
| new Vulnerabilities.StoredSSRFVulnerability(), | ||
| Vulnerabilities.STORED_SSRF, |
There was a problem hiding this comment.
Replacing a per-call Vulnerability object with a shared Vulnerabilities.STORED_SSRF instance may introduce unsafe shared mutable state access across threads.
Details
✨ AI Reasoning
1) The method creates an Attack object and previously instantiated a fresh Vulnerabilities.StoredSSRFVulnerability per call; the change replaces that with a shared Vulnerabilities.STORED_SSRF instance.
2) Reusing a shared vulnerability instance can introduce shared mutable state if the Vulnerability object is not immutable, risking data races or accidental cross-request mutation in multi-threaded environments.
3) This harms maintainability and could cause concurrency bugs if the shared instance is mutated elsewhere.
4) Changing from per-call allocation to a shared instance is a localized change and its thread-safety implications are directly introduced by this PR.
5) It's not known here whether the instance is immutable, so the conservative action is to flag the potential issue.
6) The fix may be to ensure the shared object is immutable or to synchronize access, which is reasonable within the PR scope.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| // scan | ||
| Vulnerabilities.Vulnerability vulnerability = new Vulnerabilities.ShellInjectionVulnerability(); | ||
| Scanner.scanForGivenVulnerability(vulnerability, "runtime.Exec", new String[]{commandStr}); | ||
| Scanner.scanForGivenVulnerability(Vulnerabilities.SHELL_INJECTION, "runtime.Exec", new String[]{commandStr}); |
There was a problem hiding this comment.
Replaced per-call new Vulnerabilities.ShellInjectionVulnerability() with shared Vulnerabilities.SHELL_INJECTION, which can introduce thread-safety/data-sharing issues if the vulnerability object is mutable.
Details
✨ AI Reasoning
1) The change replaces creating a new Vulnerabilities.ShellInjectionVulnerability() with reusing a shared instance Vulnerabilities.SHELL_INJECTION when calling Scanner.scanForGivenVulnerability.
2) If the Vulnerability instances are mutable or Scanner/consumers modify instance state, reusing a single static instance can introduce data races or incorrect cross-request/scan contamination in multithreaded use.
3) This harms maintainability and can cause subtle concurrency bugs if the object was previously isolated per-call. Therefore this change potentially introduces a thread-safety regression.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| // scan | ||
| Vulnerabilities.Vulnerability vulnerability = new Vulnerabilities.SQLInjectionVulnerability(); | ||
| Scanner.scanForGivenVulnerability(vulnerability, operation, new String[]{sql, dialect}); | ||
| Scanner.scanForGivenVulnerability(Vulnerabilities.SQL_INJECTION, operation, new String[]{sql, dialect}); |
There was a problem hiding this comment.
Reusing a shared Vulnerabilities.SQL_INJECTION instance may introduce thread-safety issues if the vulnerability object is mutable or mutated during scanning.
Details
✨ AI Reasoning
1) The code replaces creating a new Vulnerabilities.SQLInjectionVulnerability() per call with reusing a shared instance Vulnerabilities.SQL_INJECTION when calling Scanner.scanForGivenVulnerability.
2) Reusing a shared/static vulnerability instance can introduce data races or inconsistent behavior if that instance is mutable or if Scanner or other code mutates it during scanning, harming thread-safety in a multi-threaded environment.
3) This is a change introduced by the diff (previously per-call instantiation avoided shared mutable state), so the risk is newly introduced by this PR.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| logger.debug("Hostname: %s, Port: %s, IPs: %s", hostnameEntry.getHostname(), hostnameEntry.getPort(), ipAddresses); | ||
|
|
||
| Attack attack = new SSRFDetector().run( | ||
| Attack attack = SSRFDetector.run( |
There was a problem hiding this comment.
Replacing instance call with static 'SSRFDetector.run' may introduce shared mutable state access and race conditions if run uses non-thread-safe state.
Details
✨ AI Reasoning
1) The change replaced an instance method call 'new SSRFDetector().run(...)' with a static call 'SSRFDetector.run(...)' at the modified line; 2) Calling a static method can introduce shared mutable state access or make previously thread-confined behavior global, increasing risk of race conditions if SSRFDetector.run uses mutable/static fields; 3) This harms maintainability and thread-safety because it changes object lifecycle and sharing semantics in a multi-threaded environment; 4) The violation is true because the diff introduced the change from instance to static invocation which can create thread-safety concerns not present before; 5) It is plausible to fix within the PR by ensuring run is thread-safe or reverting to instance usage.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| Vulnerabilities.Vulnerability vulnerability = new Vulnerabilities.PathTraversalVulnerability(); | ||
| if (filePath instanceof String filePathString) { | ||
| Scanner.scanForGivenVulnerability(vulnerability, operation, new String[]{filePathString}); | ||
| Scanner.scanForGivenVulnerability(Vulnerabilities.PATH_TRAVERSAL, operation, new String[]{filePathString}); |
There was a problem hiding this comment.
Reusing shared Vulnerabilities.PATH_TRAVERSAL may introduce data races if Vulnerability instances are mutable or used concurrently.
Details
✨ AI Reasoning
1) The change replaces per-call creation of a Vulnerabilities.Vulnerability instance with a shared reference Vulnerabilities.PATH_TRAVERSAL and uses it in Scanner.scanForGivenVulnerability.
2) If Vulnerabilities.PATH_TRAVERSAL or the Vulnerability instances are mutable or Scanner modifies the vulnerability object, reusing a single static instance can introduce race conditions when accessed concurrently by multiple threads.
3) This harms maintainability because it changes object lifetimes and introduces potential thread-safety problems that did not exist when each call created its own instance.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
|
|
||
| public class SSRFDetector { | ||
| public Attack run(String hostname, int port, List<String> ipAddresses, String operation) { | ||
| public final class SSRFDetector { |
There was a problem hiding this comment.
Changing SSRFDetector to final and making run(...) static breaks the previous public API (instance method -> static).
Details
🔧 How do I fix it?
Support both old and new parameter names during transition periods. Add new optional parameters with defaults. Keep existing response fields while adding new ones. Focus on backwards compatibility.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| } | ||
| public static final class SQLInjectionVulnerability implements Vulnerability { | ||
|
|
||
| private static final class SQLInjectionVulnerability implements Vulnerability { |
There was a problem hiding this comment.
Switched from creating new Detector instances to using shared INSTANCE singletons, which may expose non-thread-safe Detector state to concurrent threads.
Details
✨ AI Reasoning
1) The changes replaced per-call creation of Detector objects with shared singletons (e.g., SqlDetector.INSTANCE, PathTraversalDetector.INSTANCE, ShellInjectionDetector.INSTANCE) and added public static final Vulnerability instances; 2) Sharing mutable or non-thread-safe Detector instances across threads can introduce race conditions or incorrect behavior if those Detector implementations are not designed for concurrent use; 3) This harms maintainability and safety because callers that previously got fresh Detector instances now share state implicitly; 4) Fixing would meaningfully improve safety by ensuring Detector implementations are thread-safe or preserving per-call instantiation; 5) Using singletons for objects that may have internal mutable state is a common source of concurrency bugs; 6) This is a targeted change and can be addressed in this PR by reverting to per-call instantiation or documenting/making Detectors thread-safe.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| import java.util.regex.Pattern; | ||
|
|
||
| public class SqlDetector implements Detector { | ||
| public static final SqlDetector INSTANCE = new SqlDetector(); |
There was a problem hiding this comment.
Declaring INSTANCE before the static logger risks static initialization-order problems if the instance uses the logger during construction.
Details
✨ AI Reasoning
1) The change adds a public static final INSTANCE at line 11 which instantiates SqlDetector during class static initialization.
2) This can harm maintainability/runtime safety because static fields are initialized in textual order; INSTANCE is created before the static logger field (line 12), so if SqlDetector's constructor or any instance initialization uses the logger, it would see a null logger and could throw or behave incorrectly.
3) This is a real initialization-order hazard introduced by this diff because the logger field was present before but the new INSTANCE was added earlier in the file; the issue did not exist prior to inserting the INSTANCE before the logger. Therefore this is a valid problem introduced by the change.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
bitterpanda63
changed the title
|
Currently being tested on https://github.com/Aikido-demo-apps/zen-demo-java/pull/52/files |
No description provided.