-
Notifications
You must be signed in to change notification settings - Fork 192
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 CVE-2019-9670 Zimbra XXE Detector #580
base: master
Are you sure you want to change the base?
Conversation
Hello @LeonardoE95, thanks for your contribution! I reviewed your plugin and found that while the detection is working, it would be better to leverage Tsunami’s built-in mechanism. A simple way to implement out-of-band (OOB) detection is by including an external DTD pointing to the callback server:
If the callback server is enabled, Tsunami should generate a OOB payload for your template; otherwise, it can fall back to a reflective approach. For reference, you can check these PRs:
Feel free to reach out if you have any questions - I’d be happy to provide more details and assistance. |
Added the OOB detection strategy if the callback server is available. |
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 @LeonardoE95, thanks for the updates!
With my suggestion, I was hoping to unify the detection logic for both cases. However, while reviewing the isServiceVulnerable
function, I found that the default payload for SSRF that does not use the callback server relies on http://public-firing-range.appspot.com/
. The response contains HTML tags, which in the case of an XXE prevents us from using the same template and detection logic for both approaches. The reflected one will return the same error as the patched version: Body cannot be parsed
.
For now, we can stick with the "two templates approach". The suggested implementation will be easier to modify if the default behavior ever changes in our favor.
Other changes include:
- The use of annotations in
oobSleepDuration
withinsleepUninterruptibly
. This is required when running tests (please update them where needed). More details on how to implement annotations for injecting the sleep duration can be found in this comment by one of my colleagues. You will also need to add dependencies to thebuild.gradle
file. - Some minor fixes to the
isZimbra
fingerprint function.
As a last suggestion, consider merging the tests into a single file named Cve20199670DetectorTest.java
.
Let me know if anything isn't clear, and I'll be happy to provide additional information!
Specifically, by using the XXE (CVE-2019-9670) it is possible to read a configuration file that contains an LDAP password for the zimbra account. The zimbra credentials are then used to get a user authentication cookie with an AuthRequest message. Using the user cookie, a SSRF (CVE-2019-9621) in the Proxy Servlet is used to proxy an AuthRequest with the zimbra credentials to the admin port to retrieve an admin cookie. After gaining an admin cookie the Client Upload servlet is used to upload a JSP webshell that can be triggered from the web server to obtain RCE. | ||
|
||
**Affected Versions** | ||
:-----:| |
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.
Solved.
|
||
@Inject | ||
Cve20199670Detector( | ||
@UtcClock Clock utcClock, HttpClient httpClient, PayloadGenerator payloadGenerator) { | ||
this.utcClock = checkNotNull(utcClock); | ||
this.httpClient = checkNotNull(httpClient).modify().setFollowRedirects(false).build(); | ||
this.payloadGenerator = checkNotNull(payloadGenerator); | ||
} |
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.
Introduce oobSleepDuration
to allow a more configurable implementation
@Inject | |
Cve20199670Detector( | |
@UtcClock Clock utcClock, HttpClient httpClient, PayloadGenerator payloadGenerator) { | |
this.utcClock = checkNotNull(utcClock); | |
this.httpClient = checkNotNull(httpClient).modify().setFollowRedirects(false).build(); | |
this.payloadGenerator = checkNotNull(payloadGenerator); | |
} | |
private final int oobSleepDuration; | |
@Inject | |
Cve20199670Detector( | |
@UtcClock Clock utcClock, | |
HttpClient httpClient, | |
PayloadGenerator payloadGenerator, | |
@OobSleepDuration int oobSleepDuration) { | |
this.utcClock = checkNotNull(utcClock); | |
this.httpClient = checkNotNull(httpClient).modify().setFollowRedirects(false).build(); | |
this.payloadGenerator = checkNotNull(payloadGenerator); | |
this.oobSleepDuration = 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.
Added Annotation support for oobSleepDuration as requested.
try { | ||
HttpRequest request = HttpRequest.get(targetUri).withEmptyHeaders().build(); | ||
HttpResponse response = response = this.httpClient.send(request, 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.
try { | |
HttpRequest request = HttpRequest.get(targetUri).withEmptyHeaders().build(); | |
HttpResponse response = response = this.httpClient.send(request, networkService); | |
HttpRequest request = HttpRequest.get(targetUri).withEmptyHeaders().build(); | |
try { | |
HttpResponse response = this.httpClient.send(request, 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.
Solved.
(response.status().code() == 200) | ||
&& response.bodyString().map(body -> body.contains(ZIMBRA_FINGERPRING)).orElse(false); | ||
} catch (IOException e) { | ||
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.
return false; | |
logger.atWarning().withCause(e).log("Request to target '%s' failed", targetUri); | |
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.
Logs added to catch blocks.
targetUri = NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + AUTODISCOVER_PATH; | ||
try { | ||
HttpRequest request = HttpRequest.get(targetUri).withEmptyHeaders().build(); | ||
HttpResponse response = response = this.httpClient.send(request, networkService); | ||
isZimbra = isZimbra && (response.status().code() == 200); | ||
} catch (IOException e) { | ||
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.
targetUri = NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + AUTODISCOVER_PATH; | |
try { | |
HttpRequest request = HttpRequest.get(targetUri).withEmptyHeaders().build(); | |
HttpResponse response = response = this.httpClient.send(request, networkService); | |
isZimbra = isZimbra && (response.status().code() == 200); | |
} catch (IOException e) { | |
return false; | |
targetUri = NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + AUTODISCOVER_PATH; | |
request = HttpRequest.get(targetUri).withEmptyHeaders().build(); | |
try { | |
HttpResponse response = this.httpClient.send(request, networkService); | |
isZimbra &= response.status().code() == 200; | |
} catch (IOException e) { | |
logger.atWarning().withCause(e).log("Request to target '%s' failed", targetUri); | |
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.
Solved.
private boolean isServiceVulnerable(NetworkService networkService) { | ||
if (payloadGenerator.isCallbackServerEnabled()) { | ||
return testWithCallbackServer(networkService); | ||
} else { | ||
return testWithoutCallbackServer(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.
Here is my suggested implementation of the isServiceVulnerable
function.
private boolean isServiceVulnerable(NetworkService networkService) { | |
if (payloadGenerator.isCallbackServerEnabled()) { | |
return testWithCallbackServer(networkService); | |
} else { | |
return testWithoutCallbackServer(networkService); | |
} | |
} | |
private boolean isServiceVulnerable(NetworkService networkService) { | |
PayloadGeneratorConfig config = | |
PayloadGeneratorConfig.newBuilder() | |
.setVulnerabilityType(PayloadGeneratorConfig.VulnerabilityType.SSRF) | |
.setInterpretationEnvironment( | |
PayloadGeneratorConfig.InterpretationEnvironment.INTERPRETATION_ANY) | |
.setExecutionEnvironment(PayloadGeneratorConfig.ExecutionEnvironment.EXEC_ANY) | |
.build(); | |
Payload payload = payloadGenerator.generate(config); | |
String callbackUrl = payload.getPayload(); | |
String xmlPayload; | |
if (payload.getPayloadAttributes().getUsesCallbackServer()) { | |
xmlPayload = String.format(PAYLOAD_TEMPLATE_OOB, callbackUrl); | |
} else { | |
xmlPayload = String.format(PAYLOAD_TEMPLATE_REFLECTED, callbackUrl); | |
} | |
String targetUri = | |
NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + AUTODISCOVER_PATH; | |
HttpRequest request = prepareRequest(targetUri, xmlPayload); | |
HttpResponse response = null; | |
try { | |
response = httpClient.send(request, networkService); | |
} catch (IOException e) { | |
logger.atWarning().withCause(e).log("Request to target '%s' failed", targetUri); | |
} | |
if (response == null || response.bodyString().isEmpty()) { | |
// This may be a situation where error handling and logging are needed. | |
return false; | |
} | |
if (payload.getPayloadAttributes().getUsesCallbackServer()) { | |
logger.atInfo().log("Waiting for RCE callback."); | |
Uninterruptibles.sleepUninterruptibly(Duration.ofSeconds(oobSleepDuration)); | |
return payload.checkIfExecuted(response.bodyString().get()); | |
} | |
return response.bodyString().get().contains(callbackUrl); | |
} |
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.
Both cases have now been merged into isServiceVulnerable
.
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.
Here I've omitted the line if (response == null || response.bodyString().isEmpty())
.
Thinking is that no exception has been launched by the httpClient.send
method at that point, therefore the response should not be null
. And if it is empty, the current code will cover that case and return false
.
Rest was adjusted as suggested, thanks!
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 line payload.checkIfExecuted(response.bodyString().get())
has also been changed to remove the arguments into payload.checkIfExecuted()
, to force the scanner to check with the callback server.
This might not be completely required, however I wanted to be more explicit about it.
private boolean testWithCallbackServer(NetworkService networkService) { | ||
boolean isVulnerable = false; | ||
|
||
if (!payloadGenerator.isCallbackServerEnabled()) { | ||
// Callback server is required | ||
return false; | ||
} | ||
|
||
PayloadGeneratorConfig config = | ||
PayloadGeneratorConfig.newBuilder() | ||
.setVulnerabilityType(PayloadGeneratorConfig.VulnerabilityType.SSRF) | ||
.setInterpretationEnvironment( | ||
PayloadGeneratorConfig.InterpretationEnvironment.INTERPRETATION_ANY) | ||
.setExecutionEnvironment(PayloadGeneratorConfig.ExecutionEnvironment.EXEC_ANY) | ||
.build(); | ||
Payload payload = payloadGenerator.generate(config); | ||
if (!payload.getPayloadAttributes().getUsesCallbackServer()) { | ||
// Callback server is required | ||
return false; | ||
} | ||
|
||
// Construct the payload with URL that points to callback server | ||
String oobCallbackUrl = payload.getPayload(); | ||
String oobPayload = String.format(PAYLOAD_TEMPLATE_OOB, oobCallbackUrl); | ||
String targetUri = | ||
NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + AUTODISCOVER_PATH; | ||
HttpRequest request = prepareRequest(targetUri, oobPayload); | ||
|
||
try { | ||
HttpResponse response = httpClient.send(request, networkService); | ||
logger.atInfo().log("Waiting for XXE callback."); | ||
Uninterruptibles.sleepUninterruptibly(Duration.ofSeconds(10)); | ||
isVulnerable = payload.checkIfExecuted(); | ||
} catch (IOException e) { | ||
logger.atWarning().withCause(e).log("Request to target '%s' failed", targetUri); | ||
return false; | ||
} | ||
|
||
return isVulnerable; | ||
} | ||
|
||
private boolean testWithoutCallbackServer(NetworkService networkService) { | ||
boolean isVulnerable = false; | ||
|
||
String reflectedPayload = String.format(PAYLOAD_TEMPLATE_REFLECTED, TEST_STRING); | ||
String targetUri = | ||
NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + AUTODISCOVER_PATH; | ||
HttpRequest request = prepareRequest(targetUri, reflectedPayload); | ||
|
||
try { | ||
HttpResponse response = httpClient.send(request, networkService); | ||
isVulnerable = | ||
(response.status().code() == 503) | ||
&& response | ||
.bodyString() | ||
// To decrease false positive rate, match also with specific error message | ||
.map(body -> (body.contains(TEST_STRING) && body.contains(ERROR_MSG))) | ||
.orElse(false); | ||
} catch (IOException e) { | ||
logger.atWarning().withCause(e).log("Request to target '%s' failed", targetUri); | ||
return false; | ||
} | ||
|
||
return isVulnerable; | ||
} |
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.
We can remove these functions altogether since they are now merged into the caller.
private boolean testWithCallbackServer(NetworkService networkService) { | |
boolean isVulnerable = false; | |
if (!payloadGenerator.isCallbackServerEnabled()) { | |
// Callback server is required | |
return false; | |
} | |
PayloadGeneratorConfig config = | |
PayloadGeneratorConfig.newBuilder() | |
.setVulnerabilityType(PayloadGeneratorConfig.VulnerabilityType.SSRF) | |
.setInterpretationEnvironment( | |
PayloadGeneratorConfig.InterpretationEnvironment.INTERPRETATION_ANY) | |
.setExecutionEnvironment(PayloadGeneratorConfig.ExecutionEnvironment.EXEC_ANY) | |
.build(); | |
Payload payload = payloadGenerator.generate(config); | |
if (!payload.getPayloadAttributes().getUsesCallbackServer()) { | |
// Callback server is required | |
return false; | |
} | |
// Construct the payload with URL that points to callback server | |
String oobCallbackUrl = payload.getPayload(); | |
String oobPayload = String.format(PAYLOAD_TEMPLATE_OOB, oobCallbackUrl); | |
String targetUri = | |
NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + AUTODISCOVER_PATH; | |
HttpRequest request = prepareRequest(targetUri, oobPayload); | |
try { | |
HttpResponse response = httpClient.send(request, networkService); | |
logger.atInfo().log("Waiting for XXE callback."); | |
Uninterruptibles.sleepUninterruptibly(Duration.ofSeconds(10)); | |
isVulnerable = payload.checkIfExecuted(); | |
} catch (IOException e) { | |
logger.atWarning().withCause(e).log("Request to target '%s' failed", targetUri); | |
return false; | |
} | |
return isVulnerable; | |
} | |
private boolean testWithoutCallbackServer(NetworkService networkService) { | |
boolean isVulnerable = false; | |
String reflectedPayload = String.format(PAYLOAD_TEMPLATE_REFLECTED, TEST_STRING); | |
String targetUri = | |
NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + AUTODISCOVER_PATH; | |
HttpRequest request = prepareRequest(targetUri, reflectedPayload); | |
try { | |
HttpResponse response = httpClient.send(request, networkService); | |
isVulnerable = | |
(response.status().code() == 503) | |
&& response | |
.bodyString() | |
// To decrease false positive rate, match also with specific error message | |
.map(body -> (body.contains(TEST_STRING) && body.contains(ERROR_MSG))) | |
.orElse(false); | |
} catch (IOException e) { | |
logger.atWarning().withCause(e).log("Request to target '%s' failed", targetUri); | |
return false; | |
} | |
return isVulnerable; | |
} |
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.
Both cases have now been merged into isServiceVulnerable.
protected void configurePlugin() { | ||
registerPlugin(Cve20199670Detector.class); | ||
} | ||
} |
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.
Here, the configurations related to the oobSleepDuration
variable introduced in the detector can be added.
} | |
@Provides | |
@OobSleepDuration | |
int provideOobSleepDuration(Cve20199670DetectorConfigs configs) { | |
if (configs.oobSleepDuration == 0) { | |
return 10; | |
} | |
return configs.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.
Added Annotation support for oobSleepDuration as requested.
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.
Thank you! Once you apply this two typo fixes down below, I believe we've addressed everything.
HttpRequest request = HttpRequest.get(targetUri).withEmptyHeaders().build(); | ||
|
||
try { | ||
HttpResponse response = response = this.httpClient.send(request, 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.
HttpResponse response = response = this.httpClient.send(request, networkService); | |
HttpResponse response = this.httpClient.send(request, 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.
Fixed typo.
request = HttpRequest.get(targetUri).withEmptyHeaders().build(); | ||
|
||
try { | ||
HttpResponse response = response = this.httpClient.send(request, 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.
HttpResponse response = response = this.httpClient.send(request, networkService); | |
HttpResponse response = this.httpClient.send(request, 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.
Fixed typo.
LGTM - Approved Reviewer: Giacomo, Doyensec |
Hi there,
This PR contains the implementation for the CVE-2019-9670 detector.
Below it is possible to find the necessary information for review:
Thank you.