Skip to content

Commit

Permalink
pscanrules: Address Suspicious Comments rule JS FPs
Browse files Browse the repository at this point in the history 
- CHANGELOG > Added fix note.
- InformationDisclosureSuspiciousCommentsScanRule > Updated handling to
target comments in JavaScript more specifically. All alerts are now
raised with Medium Confidence.
- InformationDisclosureSuspiciousCommentsScanRuleUnitTest b> Updated and
added tests.
- Messages.properties > Updated to more detail/report the findings more
specifically based on the new behavior.
- pscanrules.html > Correct occurrence of "add-on" (vs addon).

Signed-off-by: kingthorin <[email protected]>
  • Loading branch information
@kingthorin
kingthorin committed Oct 13, 2024
1 parent b5aa3bd commit e40a6e1
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 41 deletions.
3 changes: 2 additions & 1 deletion addOns/pscanrules/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ All notable changes to this add-on will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## Unreleased

### Fixed
- The Information Disclosure - Suspicious Comments scan rule should now be less false positive prone on JavaScript findings (Issues 6622 & 6736).

## [61] - 2024-09-24
### Changed
Expand Down
73 changes: 48 additions & 25 deletions ...org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@
import java.util.Map;
import java.util.Map.Entry;
import java.util.function.Supplier;
import java.util.regex.MatchResult;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import net.htmlparser.jericho.Element;
import net.htmlparser.jericho.HTMLElementName;
import net.htmlparser.jericho.Source;
Expand Down Expand Up @@ -77,10 +79,32 @@ public class InformationDisclosureSuspiciousCommentsScanRule extends PluginPassi
private static final Supplier<Iterable<String>> DEFAULT_PAYLOAD_PROVIDER =
() -> DEFAULT_PAYLOADS;

// https://github.com/antlr/grammars-v4/blob/c82c128d980f4ce46fb3536f87b06b45b9619922/javascript/javascript/JavaScriptLexer.g4#L49-L50
private static final Pattern JS_MULTILINE_COMMENT =
Pattern.compile("/\\*.*?\\*/", Pattern.DOTALL);
private static final Pattern JS_SINGLELINE_COMMENT = Pattern.compile("//.*");

private static Supplier<Iterable<String>> payloadProvider = DEFAULT_PAYLOAD_PROVIDER;

private List<Pattern> patterns = null;

private static List<String> getJsComments(String content) {
List<String> results = new ArrayList<>();
results.addAll(
JS_SINGLELINE_COMMENT
.matcher(content)
.results()
.map(MatchResult::group)
.collect(Collectors.toList()));
results.addAll(
JS_MULTILINE_COMMENT
.matcher(content)
.results()
.map(MatchResult::group)
.collect(Collectors.toList()));
return results;
}

@Override
public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {

Expand All @@ -90,18 +114,16 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
if (msg.getResponseBody().length() > 0 && msg.getResponseHeader().isText()) {

if (ResourceIdentificationUtils.isJavaScript(msg)) {
// Just treat as text
String[] lines = msg.getResponseBody().toString().split("\n");
for (String line : lines) {
for (String comment : getJsComments(msg.getResponseBody().toString())) {
for (Pattern pattern : patterns) {
Matcher m = pattern.matcher(line);
Matcher m = pattern.matcher(comment);
if (m.find()) {
recordAlertSummary(
alertMap,
new AlertSummary(
pattern.toString(),
line,
Alert.CONFIDENCE_LOW,
comment,
m.group()));
break; // Only need to record this line once
}
Expand All @@ -121,8 +143,8 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
alertMap,
new AlertSummary(
pattern.toString(),
tagStr,
Alert.CONFIDENCE_MEDIUM,
tagStr,
m.group()));
break; // Only need to record this comment once
}
Expand All @@ -132,18 +154,19 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
Element el;
int offset = 0;
while ((el = source.getNextElement(offset, HTMLElementName.SCRIPT)) != null) {
for (Pattern pattern : patterns) {
String elStr = el.toString();
Matcher m = pattern.matcher(elStr);
if (m.find()) {
recordAlertSummary(
alertMap,
new AlertSummary(
pattern.toString(),
elStr,
Alert.CONFIDENCE_LOW,
m.group()));
break; // Only need to record this script once
for (String comment : getJsComments(el.toString())) {
for (Pattern pattern : patterns) {
Matcher m = pattern.matcher(comment);
if (m.find()) {
recordAlertSummary(
alertMap,
new AlertSummary(
pattern.toString(),
Alert.CONFIDENCE_LOW,
comment,
m.group()));
break; // Only need to record this script once
}
}
}
offset = el.getEnd();
Expand Down Expand Up @@ -254,30 +277,30 @@ public List<Alert> getExampleAlerts() {

private static class AlertSummary {
private final String pattern;
private final String detail;
private final int confidence;
private final String detail;
private final String evidence;

public AlertSummary(String pattern, String detail, int confidence, String evidence) {
public AlertSummary(String pattern, int confidence, String detail, String evidence) {
super();
this.pattern = pattern;
this.detail = detail;
this.confidence = confidence;
this.detail = detail;
this.evidence = evidence;
}

public String getPattern() {
return pattern;
}

public String getDetail() {
return detail;
}

public int getConfidence() {
return confidence;
}

public String getDetail() {
return detail;
}

public String getEvidence() {
return evidence;
}
Expand Down
2 changes: 1 addition & 1 deletion ...ain/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ <H2 id="id-10025">Information Disclosure: Referrer</H2>
<H2 id="id-10027">Information Disclosure: Suspicious Comments</H2>
Analyzes web content to identify comments which contain potentially sensitive details. Which may lead to
further attack or exposure of unintended data.
<p><strong>Note:</strong> The strings to look for can be extended by using the Custom Payloads addon.
<p><strong>Note:</strong> The strings to look for can be extended by using the Custom Payloads add-on.
<p>
Latest code: <a href="https://github.com/zaproxy/zap-extensions/blob/main/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRule.java">InformationDisclosureSuspiciousCommentsScanRule.java</a>
<br>
Expand Down
6 changes: 3 additions & 3 deletions ...les/src/main/resources/org/zaproxy/zap/extension/pscanrules/resources/Messages.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,10 @@ pscanrules.informationdisclosurereferrer.otherinfo.sensitiveinfo = The URL in th
pscanrules.informationdisclosurereferrer.otherinfo.ssn = The URL in the HTTP referrer header field appears to contain US Social Security Number(s).
pscanrules.informationdisclosurereferrer.soln = Do not pass sensitive information in URIs.

pscanrules.informationdisclosuresuspiciouscomments.desc = The response appears to contain suspicious comments which may help an attacker. Note: Matches made within script blocks or files are against the entire content not only comments.
pscanrules.informationdisclosuresuspiciouscomments.desc = The response appears to contain suspicious comments which may help an attacker.
pscanrules.informationdisclosuresuspiciouscomments.name = Information Disclosure - Suspicious Comments
pscanrules.informationdisclosuresuspiciouscomments.otherinfo = The following pattern was used: {0} and was detected in the element starting with: "{1}", see evidence field for the suspicious comment/snippet.
pscanrules.informationdisclosuresuspiciouscomments.otherinfo2 = The following pattern was used: {0} and was detected {2} times, the first in the element starting with: "{1}", see evidence field for the suspicious comment/snippet.
pscanrules.informationdisclosuresuspiciouscomments.otherinfo = The following pattern was used: {0} and was detected in likely comment: "{1}", see evidence field for the suspicious comment/snippet.
pscanrules.informationdisclosuresuspiciouscomments.otherinfo2 = The following pattern was used: {0} and was detected {2} times, the first in likely comment: "{1}", see evidence field for the suspicious comment/snippet.
pscanrules.informationdisclosuresuspiciouscomments.soln = Remove all comments that return information that may help an attacker and fix any underlying problems they refer to.

pscanrules.infosessionidurl.desc = URL rewrite is used to track user session ID. The session ID may be disclosed via cross-site referer header. In addition, the session ID might be stored in browser history or server logs.
Expand Down
74 changes: 63 additions & 11 deletions ...oxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRuleUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import org.apache.commons.httpclient.URI;
import org.apache.commons.httpclient.URIException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.parosproxy.paros.Constant;
import org.parosproxy.paros.core.scanner.Alert;
import org.parosproxy.paros.network.HttpMalformedHeaderException;
Expand Down Expand Up @@ -97,11 +99,11 @@ void shouldReturnExpectedMappings() {
}

@Test
void shouldAlertOnSuspiciousCommentInJavaScriptResponse()
void shouldNotAlertOnSuspiciousCommentInJavaScriptResponse()
throws HttpMalformedHeaderException, URIException {

// Given
String line1 = "Some text <script>Some Script Element FIXME: DO something </script>";
String line1 = "myArray = [\"success\",\"FIXME\"]";
String body = line1 + "\nLine 2\n";
HttpMessage msg = createHttpMessageWithRespBody(body, "text/javascript;charset=ISO-8859-1");

Expand All @@ -111,12 +113,39 @@ void shouldAlertOnSuspiciousCommentInJavaScriptResponse()
// When
scanHttpResponseReceive(msg);

// Then
assertEquals(0, alertsRaised.size());
}

@ParameterizedTest
@ValueSource(
strings = {
"/* FIXME: admin:admin01$ */",
"// FIXME: admin:admin01$",
"//FIXME: admin:admin01$"
})
void shouldAlertOnSuspiciousCommentInJavaScriptResponse(String comment)
throws HttpMalformedHeaderException, URIException {

// Given
String line1 = "myArray = [\"success\",\"FIXME\"]";
String line2 = "\n" + comment;
String body = line1 + line2 + "\nLine 3\n";
HttpMessage msg = createHttpMessageWithRespBody(body, "text/javascript;charset=ISO-8859-1");

assertTrue(msg.getResponseHeader().isText());
assertTrue(ResourceIdentificationUtils.isJavaScript(msg));

// When
scanHttpResponseReceive(msg);

// Then
assertEquals(1, alertsRaised.size());
assertEquals(Alert.CONFIDENCE_LOW, alertsRaised.get(0).getConfidence());
assertEquals("FIXME", alertsRaised.get(0).getEvidence());
assertEquals(
wrapEvidenceOtherInfo("\\bFIXME\\b", line1, 1), alertsRaised.get(0).getOtherInfo());
wrapEvidenceOtherInfo("\\bFIXME\\b", comment, 1),
alertsRaised.get(0).getOtherInfo());
}

@Test
Expand All @@ -143,8 +172,9 @@ void shouldCreateOneAlertforMultipleAndEqualSuspiciousComments()
throws HttpMalformedHeaderException, URIException {

// Given
String line1 = "Some text <script>Some Script Element FIXME: DO something";
String line2 = "FIXME: DO something else </script>";
String comment = "// FIXME: DO something";
String line1 = "Some text <script>Some Script Element " + comment;
String line2 = "// FIXME: DO something else </script>";
String body = line1 + "\n" + line2 + "\nLine 2\n";
HttpMessage msg = createHttpMessageWithRespBody(body, "text/javascript;charset=ISO-8859-1");

Expand All @@ -160,7 +190,8 @@ void shouldCreateOneAlertforMultipleAndEqualSuspiciousComments()
assertEquals("FIXME", alertsRaised.get(0).getEvidence());
// detected 2 times, the first in the element
assertEquals(
wrapEvidenceOtherInfo("\\bFIXME\\b", line1, 2), alertsRaised.get(0).getOtherInfo());
wrapEvidenceOtherInfo("\\bFIXME\\b", comment, 2),
alertsRaised.get(0).getOtherInfo());
}

@Test
Expand All @@ -182,11 +213,12 @@ void shouldNotAlertWithoutSuspiciousCommentInJavaScriptResponse()
}

@Test
void shouldAlertOnSuspiciousCommentInHtmlScriptElements()
void shouldAlertOnSuspiciousCommentInHtmlScriptElementWithComment()
throws HttpMalformedHeaderException, URIException {

// Given
String script = "<script>Some Html Element todo DO something </script>";
String comment = "// todo DO something";
String script = "<script>Some Script Element " + comment + "\n</script>";
String body = "<h1>Some text " + script + "</h1>\n<b>No script here</b>\n";
HttpMessage msg = createHttpMessageWithRespBody(body, "text/html;charset=ISO-8859-1");

Expand All @@ -200,7 +232,27 @@ void shouldAlertOnSuspiciousCommentInHtmlScriptElements()
assertEquals(1, alertsRaised.size());
assertEquals(Alert.CONFIDENCE_LOW, alertsRaised.get(0).getConfidence());
assertEquals(
wrapEvidenceOtherInfo("\\bTODO\\b", script, 1), alertsRaised.get(0).getOtherInfo());
wrapEvidenceOtherInfo("\\bTODO\\b", comment, 1),
alertsRaised.get(0).getOtherInfo());
}

@Test
void shouldNotAlertOnSuspiciousContentInHtmlScriptElement()
throws HttpMalformedHeaderException, URIException {

// Given
String script = "<script>myArray = [\"admin\", \"password\"]\n</script>";
String body = "<h1>Some text " + script + "</h1>\n<b>No script here</b>\n";
HttpMessage msg = createHttpMessageWithRespBody(body, "text/html;charset=ISO-8859-1");

assertTrue(msg.getResponseHeader().isText());
assertFalse(ResourceIdentificationUtils.isJavaScript(msg));

// When
scanHttpResponseReceive(msg);

// Then
assertEquals(0, alertsRaised.size());
}

@Test
Expand Down Expand Up @@ -369,15 +421,15 @@ private static String wrapEvidenceOtherInfo(String evidence, String info, int co
if (count == 1) {
return "The following pattern was used: "
+ evidence
+ " and was detected in the element starting with: \""
+ " and was detected in likely comment: \""
+ info
+ "\", see evidence field for the suspicious comment/snippet.";
}
return "The following pattern was used: "
+ evidence
+ " and was detected "
+ count
+ " times, the first in the element starting with: \""
+ " times, the first in likely comment: \""
+ info
+ "\", see evidence field for the suspicious comment/snippet.";
}
Expand Down

0 comments on commit e40a6e1

Please sign in to comment.