Skip to content

Reduce use of error level logging in pscan rules, tech detection, & retire #6645

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reduce use of error level logging in pscan rules, tech detection, & retire #6645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions addOns/pscanrules/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
### Changed
- Add alert references to HTTP Server Response Header scan rule alerts (Issue 7100, 9050).
- Update alert references to latest locations to fix 404s and resolve redirections.
- Reduced usage of error level logging.

## [66] - 2025-07-25
### Added
Expand Down
112 changes: 53 additions & 59 deletions ...c/main/java/org/zaproxy/zap/extension/pscanrules/CrossDomainMisconfigurationScanRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,66 +85,60 @@ public String getName() {
@Override
public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {

try {
LOGGER.debug(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a lot but it's because all the indentation changed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is likely helpful to "Hide Whitespace" while reviewing this PR.

"Checking message {} for Cross-Domain misconfigurations",
msg.getRequestHeader().getURI());

String corsAllowOriginValue =
msg.getResponseHeader().getHeader(HttpHeader.ACCESS_CONTROL_ALLOW_ORIGIN);
// String corsAllowHeadersValue =
// msg.getResponseHeader().getHeader(HttpHeader.ACCESS_CONTROL_ALLOW_HEADERS);
// String corsAllowMethodsValue =
// msg.getResponseHeader().getHeader(HttpHeader.ACCESS_CONTROL_ALLOW_METHODS);
// String corsExposeHeadersValue =
// msg.getResponseHeader().getHeader(HttpHeader.ACCESS_CONTROL_EXPOSE_HEADERS);

if (corsAllowOriginValue != null && corsAllowOriginValue.equals("*")) {
LOGGER.debug(
"Checking message {} for Cross-Domain misconfigurations",
msg.getRequestHeader().getURI());

String corsAllowOriginValue =
msg.getResponseHeader().getHeader(HttpHeader.ACCESS_CONTROL_ALLOW_ORIGIN);
// String corsAllowHeadersValue =
// msg.getResponseHeader().getHeader(HttpHeader.ACCESS_CONTROL_ALLOW_HEADERS);
// String corsAllowMethodsValue =
// msg.getResponseHeader().getHeader(HttpHeader.ACCESS_CONTROL_ALLOW_METHODS);
// String corsExposeHeadersValue =
// msg.getResponseHeader().getHeader(HttpHeader.ACCESS_CONTROL_EXPOSE_HEADERS);

if (corsAllowOriginValue != null && corsAllowOriginValue.equals("*")) {
LOGGER.debug(
"Raising a Medium risk Cross Domain alert on {}: {}",
HttpHeader.ACCESS_CONTROL_ALLOW_ORIGIN,
corsAllowOriginValue);
// Its a Medium, rather than a High (as originally thought), for the following
// reasons:
// Assumption: if an API is accessible in an unauthenticated manner, it doesn't need
// to be protected
// (if it should be protected, its a Missing Function Level Access Control issue,
// not a Cross Domain Misconfiguration)
//
// Case 1) Request sent using XHR
// - cookies will not be sent with the request at all unless withCredentials = true
// on the XHR request;
// - If a cookie was sent with the request, the browser will not give access to the
// response body via JavaScript unless the response headers say
// "Access-Control-Allow-Credentials: true"
// - If "Access-Control-Allow-Credentials: true" and "Access-Control-Allow-Origin:
// *" in the response, the browser will not give access to the response body.
// (this is an edge case, but is actually really important, because it blocks all
// the useful attacks, and is well supported by modern browsers)
// Case 2) Request sent using HTML Form POST with an iframe, for instance, and
// attempting to access the iframe body (ie, the Cross Domain response) using
// JavaScript
// - the cookie will be sent by the web browser (possibly leading to CSRF, but with
// no impact from the point of view of the Same Origin Policy / Cross Domain
// Misconfiguration
// - the HTML response is not accessible in JavaScript, regardless of the CORS
// headers sent in the response (in all my trials, at least)
// (this is even more restrictive than the equivalent request sent by XHR)

// The CORS misconfig could still allow an attacker to access the data returned from
// an unauthenticated API, which is protected by some other form of security, such
// as IP address white-listing, for instance.

buildAlert(
extractEvidence(
msg.getResponseHeader().toString(),
HttpHeader.ACCESS_CONTROL_ALLOW_ORIGIN))
.raise();
}

} catch (Exception e) {
LOGGER.error(
"An error occurred trying to passively scan a message for Cross Domain Misconfigurations");
"Raising a Medium risk Cross Domain alert on {}: {}",
HttpHeader.ACCESS_CONTROL_ALLOW_ORIGIN,
corsAllowOriginValue);
// Its a Medium, rather than a High (as originally thought), for the following
// reasons:
// Assumption: if an API is accessible in an unauthenticated manner, it doesn't need
// to be protected
// (if it should be protected, its a Missing Function Level Access Control issue,
// not a Cross Domain Misconfiguration)
//
// Case 1) Request sent using XHR
// - cookies will not be sent with the request at all unless withCredentials = true
// on the XHR request;
// - If a cookie was sent with the request, the browser will not give access to the
// response body via JavaScript unless the response headers say
// "Access-Control-Allow-Credentials: true"
// - If "Access-Control-Allow-Credentials: true" and "Access-Control-Allow-Origin:
// *" in the response, the browser will not give access to the response body.
// (this is an edge case, but is actually really important, because it blocks all
// the useful attacks, and is well supported by modern browsers)
// Case 2) Request sent using HTML Form POST with an iframe, for instance, and
// attempting to access the iframe body (ie, the Cross Domain response) using
// JavaScript
// - the cookie will be sent by the web browser (possibly leading to CSRF, but with
// no impact from the point of view of the Same Origin Policy / Cross Domain
// Misconfiguration
// - the HTML response is not accessible in JavaScript, regardless of the CORS
// headers sent in the response (in all my trials, at least)
// (this is even more restrictive than the equivalent request sent by XHR)

// The CORS misconfig could still allow an attacker to access the data returned from
// an unauthenticated API, which is protected by some other form of security, such
// as IP address white-listing, for instance.

buildAlert(
extractEvidence(
msg.getResponseHeader().toString(),
HttpHeader.ACCESS_CONTROL_ALLOW_ORIGIN))
.raise();
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...n/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureDebugErrorsScanRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ private static List<String> loadFile(Path path) {
BufferedReader reader = null;
File f = path.toFile();
if (!f.exists()) {
LOGGER.error("No such file: {}", f.getAbsolutePath());
LOGGER.warn("No such file: {}", f.getAbsolutePath());
return strings;
}
try {
Expand Down
2 changes: 1 addition & 1 deletion ...rc/main/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureInUrlScanRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ private static List<String> loadFile(String file) {
List<String> strings = new ArrayList<>();
File f = new File(Constant.getZapHome() + File.separator + file);
if (!f.exists()) {
LOGGER.error("No such file: {}", f.getAbsolutePath());
LOGGER.warn("No such file: {}", f.getAbsolutePath());
return strings;
}

Expand Down
2 changes: 1 addition & 1 deletion ...main/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureReferrerScanRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ private static List<String> loadFile(String file) {
List<String> strings = new ArrayList<>();
File f = new File(Constant.getZapHome() + File.separator + file);
if (!f.exists()) {
LOGGER.error("No such file: {}", f.getAbsolutePath());
LOGGER.warn("No such file: {}", f.getAbsolutePath());
return strings;
}

Expand Down
2 changes: 1 addition & 1 deletion ...es/src/main/java/org/zaproxy/zap/extension/pscanrules/InsecureAuthenticationScanRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public void scanHttpRequestSend(HttpMessage msg, int id) {
alertRisk = Alert.RISK_HIGH;
}
} catch (IllegalArgumentException e) {
LOGGER.error(
LOGGER.warn(
"Invalid Base64 value for {} Authentication: {}",
authMechanism,
authValues[1]);
Expand Down
144 changes: 69 additions & 75 deletions ...nrules/src/main/java/org/zaproxy/zap/extension/pscanrules/RetrievedFromCacheScanRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,88 +57,82 @@ public class RetrievedFromCacheScanRule extends PluginPassiveScanner
@Override
public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {

try {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a lot but it's because all the indentation changed.

LOGGER.debug(
"Checking URL {} to see if was served from a shared cache",
msg.getRequestHeader().getURI());

// X-Cache: HIT
// X-Cache: HIT from cache.kolich.local <-- was the data actually served from the
// cache (subject to no-cache, expiry, etc.)?
// (if X-Cache: HIT, it implies X-Cache-Lookup: HIT)
// (and if X-Cache-Lookup: MISS, it implies X-Cache: MISS)
// X-Cache-Lookup: HIT from cache.kolich.local:80 <-- was the data *available* in the
// cache? (not whether it was actually served)

// X-Cache: MISS
// X-Cache: MISS from cache.kolich.local
// X-Cache-Lookup: MISS from cache.kolich.local:80

// X-Cache HIT from proxy.domain.tld, MISS from proxy.local
// X-Cache-Lookup HIT from proxy.domain.tld:3128, MISS from proxy.local:3128

List<String> xcacheHeaders = msg.getResponseHeader().getHeaderValues("X-Cache");
if (!xcacheHeaders.isEmpty()) {
for (String xcacheHeader : xcacheHeaders) {
for (String proxyServerDetails : xcacheHeader.split(",")) {
// strip off any leading space for the second and subsequent proxies
if (proxyServerDetails.startsWith(" "))
proxyServerDetails = proxyServerDetails.substring(1);
LOGGER.trace("Proxy HIT/MISS details [{}]", proxyServerDetails);
String[] proxyServerDetailsArray = proxyServerDetails.split(" ", 3);
if (proxyServerDetailsArray.length >= 1) {
String hitormiss =
proxyServerDetailsArray[0].toUpperCase(); // HIT or MISS
if (hitormiss.equals("HIT")) {
// the response was served from cache, so raise it..
String evidence = proxyServerDetails;
LOGGER.debug(
"{} was served from a cache, due to presence of a 'HIT' in the 'X-Cache' response header",
msg.getRequestHeader().getURI());
// could be from HTTP/1.0 or HTTP/1.1. We don't know which.
buildAlert(evidence, false).raise();
return;
}
LOGGER.debug(
"Checking URL {} to see if was served from a shared cache",
msg.getRequestHeader().getURI());

// X-Cache: HIT
// X-Cache: HIT from cache.kolich.local <-- was the data actually served from the
// cache (subject to no-cache, expiry, etc.)?
// (if X-Cache: HIT, it implies X-Cache-Lookup: HIT)
// (and if X-Cache-Lookup: MISS, it implies X-Cache: MISS)
// X-Cache-Lookup: HIT from cache.kolich.local:80 <-- was the data *available* in the
// cache? (not whether it was actually served)

// X-Cache: MISS
// X-Cache: MISS from cache.kolich.local
// X-Cache-Lookup: MISS from cache.kolich.local:80

// X-Cache HIT from proxy.domain.tld, MISS from proxy.local
// X-Cache-Lookup HIT from proxy.domain.tld:3128, MISS from proxy.local:3128

List<String> xcacheHeaders = msg.getResponseHeader().getHeaderValues("X-Cache");
if (!xcacheHeaders.isEmpty()) {
for (String xcacheHeader : xcacheHeaders) {
for (String proxyServerDetails : xcacheHeader.split(",")) {
// strip off any leading space for the second and subsequent proxies
if (proxyServerDetails.startsWith(" "))
proxyServerDetails = proxyServerDetails.substring(1);
LOGGER.trace("Proxy HIT/MISS details [{}]", proxyServerDetails);
String[] proxyServerDetailsArray = proxyServerDetails.split(" ", 3);
if (proxyServerDetailsArray.length >= 1) {
String hitormiss = proxyServerDetailsArray[0].toUpperCase(); // HIT or MISS
if (hitormiss.equals("HIT")) {
// the response was served from cache, so raise it..
String evidence = proxyServerDetails;
LOGGER.debug(
"{} was served from a cache, due to presence of a 'HIT' in the 'X-Cache' response header",
msg.getRequestHeader().getURI());
// could be from HTTP/1.0 or HTTP/1.1. We don't know which.
buildAlert(evidence, false).raise();
return;
}
}
}
}
}

// The "Age" header (defined in RFC 7234) conveys the sender's estimate of the amount of
// time since the response (or its revalidation) was generated at the origin server.
// An HTTP/1.1 server that includes a cache MUST include an Age header field in every
// response generated from its own cache.
// i.e.: a valid "Age" header implies that the response was served from a cache
// lets validate that it is actually a non-negative decimal integer, as mandated by RFC
// 7234, however.
// if there are multiple "Age" headers, just look for one valid value in the multiple
// "Age" headers.. Not sure if this case is strictly valid with the spec, however.
// Note: HTTP/1.0 caches do not implement "Age", so the absence of an "Age" header does
// *not* imply that the response was served from the origin server, rather than a
// cache..
List<String> ageHeaders = msg.getResponseHeader().getHeaderValues("Age");
if (!ageHeaders.isEmpty()) {
for (String ageHeader : ageHeaders) {
LOGGER.trace("Validating Age header value [{}]", ageHeader);
Long ageAsLong = null;
try {
ageAsLong = Long.parseLong(ageHeader);
} catch (NumberFormatException nfe) {
// Ignore
}
if (ageAsLong != null && ageAsLong >= 0) {
String evidence = "Age: " + ageHeader;
LOGGER.debug(
"{} was served from a HTTP/1.1 cache, due to presence of a valid (non-negative decimal integer) 'Age' response header value",
msg.getRequestHeader().getURI());
buildAlert(evidence, true).raise();
return;
}
// The "Age" header (defined in RFC 7234) conveys the sender's estimate of the amount of
// time since the response (or its revalidation) was generated at the origin server.
// An HTTP/1.1 server that includes a cache MUST include an Age header field in every
// response generated from its own cache.
// i.e.: a valid "Age" header implies that the response was served from a cache
// lets validate that it is actually a non-negative decimal integer, as mandated by RFC
// 7234, however.
// if there are multiple "Age" headers, just look for one valid value in the multiple
// "Age" headers.. Not sure if this case is strictly valid with the spec, however.
// Note: HTTP/1.0 caches do not implement "Age", so the absence of an "Age" header does
// *not* imply that the response was served from the origin server, rather than a
// cache..
List<String> ageHeaders = msg.getResponseHeader().getHeaderValues("Age");
if (!ageHeaders.isEmpty()) {
for (String ageHeader : ageHeaders) {
LOGGER.trace("Validating Age header value [{}]", ageHeader);
Long ageAsLong = null;
try {
ageAsLong = Long.parseLong(ageHeader);
} catch (NumberFormatException nfe) {
// Ignore
}
if (ageAsLong != null && ageAsLong >= 0) {
String evidence = "Age: " + ageHeader;
LOGGER.debug(
"{} was served from a HTTP/1.1 cache, due to presence of a valid (non-negative decimal integer) 'Age' response header value",
msg.getRequestHeader().getURI());
buildAlert(evidence, true).raise();
return;
}
}

} catch (Exception e) {
LOGGER.error("An error occurred while checking if a URL was served from a cache", e);
}
}

Expand Down
1 change: 1 addition & 0 deletions addOns/pscanrulesAlpha/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
## Unreleased
### Changed
- Update alert references to latest locations to fix 404s and resolve redirections.
- Reduced usage of error level logging.

## [45] - 2025-06-20
### Added
Expand Down
4 changes: 2 additions & 2 deletions ...a/src/main/java/org/zaproxy/zap/extension/pscanrulesAlpha/ExampleFilePassiveScanRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ private static List<String> loadFile(String file) {
BufferedReader reader = null;
File f = new File(Constant.getZapHome() + File.separator + file);
if (!f.exists()) {
LOGGER.error("No such file: {}", f.getAbsolutePath());
LOGGER.warn("No such file: {}", f.getAbsolutePath());
return strings;
}
try {
Expand All @@ -134,7 +134,7 @@ private static List<String> loadFile(String file) {
}
}
} catch (IOException e) {
LOGGER.error(
LOGGER.debug(
"Error on opening/reading example error file. Error: {}", e.getMessage(), e);
} finally {
if (reader != null) {
Expand Down
1 change: 1 addition & 0 deletions addOns/pscanrulesBeta/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
## Unreleased
### Changed
- Update alert references to latest locations to fix 404s and resolve redirections.
- Reduced usage of error level logging.

## [45] - 2025-09-10
### Changed
Expand Down
Loading