Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cloud Metadata Potentially Exposed false positive #7033

Closed
EndPositive opened this issue Jan 18, 2022 · 12 comments · Fixed by zaproxy/zap-extensions#3482
Closed

Cloud Metadata Potentially Exposed false positive #7033

EndPositive opened this issue Jan 18, 2022 · 12 comments · Fixed by zaproxy/zap-extensions#3482
Assignees
@kingthorin
Labels
add-on FalsePositive

Comments

@EndPositive
Copy link

Describe the bug
The Cloud Metadata Potentially Exposed active rule generates false-positives for all 200 pages that instead do errorhandling/redirect using JavaScript.

To Reproduce
Example (reduced) response body that triggers an alert:

HTTP /1.1 200 OK
connection : close
content-length: 375
content-type: text/html

<html>
<body>
<script>
window.location = "https://www."+window.location.hostname.split('.')[window.location.hostname.split('.').length -2]+'.'+window.location.hostname.split('.')[window.location.hostname.split('.').length -1];
</script>
</body>
</html>

Expected behavior
The active rule detects the redirect and does not trigger an alert.

Software versions

  • ZAP: v2.11.1 In Docker
  • Add-on: CloudMetadataScanRule in ascanrulesBeta v39.0.0
  • OS: In Docker
  • Java: In Docker
  • Browser: In Docker

Would you like to help fix this issue?
Of course, but Java is not my best.
@kingthorin
Copy link
Member

Leverage custom error pages? They exist to facilitate this exact issue, when apps/servers don't follow industry standards.

@kingthorin
Copy link
Member

Actually it looks like this rule might need to be adapted to leverage custom error pages, however that's a fairly simple change:
https://github.com/zaproxy/zap-extensions/blob/611124953f200e6f25fed14297337482294f6e0e/addOns/ascanrulesBeta/src/main/java/org/zaproxy/zap/extension/ascanrulesBeta/CloudMetadataScanRule.java#L122-L123

@EndPositive
Copy link
Author

EndPositive commented Jan 18, 2022 •
edited

Custom error pages would indeed be a quick fix for this issue. It would be really cool if we can detect such scenarios automatically. There's some work for this in paros.core.scanner.Analyser.isFileExist, which checks if some other page redirects to the same page. In this issue, we could do the same, but then allowing javascript to run (so that we redirect).

zaproxy/zap/src/main/java/org/parosproxy/paros/core/scanner/Analyser.java

Lines 415 to 460 in 8ac4b10

// get sample with same relative path position when possible.
// if not exist, use the host only
// ZAP: Removed unnecessary cast.
SampleResponse sample = mapVisited.get(sUri);
if (sample == null) {
try {
uri.setPath(null);
} catch (URIException e2) {
}
String sHostOnly = uri.toString();
// ZAP: Removed unnecessary cast.
sample = mapVisited.get(sHostOnly);
}
// check if any analysed result.
if (sample == null) {
if (msg.getResponseHeader().getStatusCode() == HttpStatusCode.OK) {
// no analysed result to confirm, assume file exist and return
return true;
} else {
return false;
}
}
// check for redirect response. If redirect to same location, then file does not exist
if (HttpStatusCode.isRedirection(msg.getResponseHeader().getStatusCode())) {
try {
if (sample.getMessage().getResponseHeader().getStatusCode()
== msg.getResponseHeader().getStatusCode()) {
String location = msg.getResponseHeader().getHeader(HttpHeader.LOCATION);
if (location != null
&& location.equals(
sample.getMessage()
.getResponseHeader()
.getHeader(HttpHeader.LOCATION))) {
return false;
}
}
} catch (Exception e) {
logger.error(e.getMessage(), e);
}
return true;
}

@kingthorin
Copy link
Member

kingthorin commented Jan 18, 2022 •
edited

This scan rule could do a prelim dummy request (/somefilethatshouldnotexistandlisteklydoesnot) and see if that response is the same as the test (metadata request) response. If it does then don't alert, if it doesn't then alert. In your example above both should be the 200 page doing the JS redirect. The active scanner can't execute the JS but the scenario should still pan out.

@EndPositive
Copy link
Author

Cool, I think that would solve the issue in most cases, except when the response body contains (part of) the url, thus not being identical on a random request.

@thc202
Copy link
Member

thc202 commented Jan 18, 2022

@kingthorin did you check if with the custom pages the issue is addressed already? The Analyser does what's being suggested here and iirc the custom pages uses the Analyser.

@kingthorin
Copy link
Member

I haven't checked yet. I do recall that we leveraged the analyser in some of the checks/methods I just don't recall which off the top of my head.

@kingthorin
Copy link
Member

kingthorin commented Jan 18, 2022 •
edited

If the check of the header status code was replaced withisSuccess(HttpMessage) then this would be covered by Custom Pages and Analyser with fallback to status code.

@kingthorin kingthorin self-assigned this Jan 19, 2022
@kingthorin kingthorin mentioned this issue Jan 23, 2022
Merged
@EndPositive
Copy link
Author

Awesome, thanks a lot!

@sgerlach
Copy link
Contributor

sgerlach commented Feb 4, 2022

I know this is closed, but this test seems to be looking for some VERY specific content. I know customer error pages work is complete, but could/should add some response content checking here given that it's targeting some well known patterns?

@kingthorin
Copy link
Member

Sure if there are known/stable patterns to look for.

@github-actions
Copy link

github-actions bot commented May 6, 2022

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators May 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@kingthorin kingthorin

Labels
add-on FalsePositive
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: ascanrulesBeta: Cloud Metadata scan rule address possible FPs kingthorin/zap-extensions
4 participants
@sgerlach @thc202 @kingthorin @EndPositive