Skip to content

Commit

Permalink
ascanrules: Address SSTI false positive
Browse files Browse the repository at this point in the history 
- CHANGELOG > Fix note
- SstiScanRule > Adjust logic to prevent False Positives.
- SstiScanRuleUnitTest > Updated for adjusted logic.

Signed-off-by: kingthorin <[email protected]>
  • Loading branch information
@kingthorin
kingthorin committed Oct 10, 2024
1 parent 157a8df commit 1572aa6
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 19 deletions.
3 changes: 3 additions & 0 deletions addOns/ascanrules/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
### Changed
- The XML External Entity Attack scan rule now include example alert functionality for documentation generation purposes (Issue 6119).

### Fixed
- A situation where the Server-Side Template Injection (SSTI) scan rule might result in false positives (Issue 8622).

## [68] - 2024-09-24
### Changed
- Maintenance changes.
Expand Down
53 changes: 34 additions & 19 deletions addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SstiScanRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.apache.commons.lang3.RandomStringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand Down Expand Up @@ -71,23 +73,27 @@ public class SstiScanRule extends AbstractAppParamPlugin implements CommonActive
// the first and last tag are the commentaries from twig
// "\\\"\\<th\\:t\\=\\$\\{zj\\}\\#\\foreach"

private static final TemplateFormat[] TEMPLATE_FORMATS = {
new TemplateFormat(" ", " "),
new TemplateFormat("{", "}"),
new TemplateFormat("${", "}"),
new TemplateFormat("#{", "}"),
new TemplateFormat("{#", "}"),
new TemplateFormat("{@", "}"),
new TemplateFormat("{{", "}}"),
new TemplateFormat("{{=", "}}"),
new TemplateFormat("<%=", "%>"),
new TemplateFormat("#set($x=", ")${x}"),
new TemplateFormat("<p th:text=\"${", "}\"></p>"),
new TemplateFormat(
"{", "}", "{@math key=\"%d\" method=\"multiply\" operand=\"%d\"/}"), // Dustjs
new DjangoTemplateFormat(),
new GoTemplateFormat()
};
static List<TemplateFormat> DEFAULT_TEMPLATE_LIST =
List.of(
new TemplateFormat(" ", " "),
new TemplateFormat("{", "}"),
new TemplateFormat("${", "}"),
new TemplateFormat("#{", "}"),
new TemplateFormat("{#", "}"),
new TemplateFormat("{@", "}"),
new TemplateFormat("{{", "}}"),
new TemplateFormat("{{=", "}}"),
new TemplateFormat("<%=", "%>"),
new TemplateFormat("#set($x=", ")${x}"),
new TemplateFormat("<p th:text=\"${", "}\"></p>"),
new TemplateFormat(
"{",
"}",
"{@math key=\"%d\" method=\"multiply\" operand=\"%d\"/}"), // Dustjs
new DjangoTemplateFormat(),
new GoTemplateFormat());

private static List<TemplateFormat> templateFormats = DEFAULT_TEMPLATE_LIST;

private static final String[] WAYS_TO_FIX_CODE_SYNTAX = {"\"", "'", "1", ""};

Expand Down Expand Up @@ -339,7 +345,7 @@ private void searchForMathsExecution(
codeFixPrefixes = WAYS_TO_FIX_CODE_SYNTAX;
}

for (TemplateFormat sstiPayload : TEMPLATE_FORMATS) {
for (TemplateFormat sstiPayload : templateFormats) {

if (fixSyntax) {
templateFixingPrefix = sstiPayload.getEndTag();
Expand Down Expand Up @@ -396,8 +402,12 @@ private void searchForMathsExecution(
+ ".*"
+ DELIMITER
+ "[\\w\\W]*";
Matcher matcher = Pattern.compile(regex).matcher(output);
String strippedTest = renderTest.replaceAll("[^A-Za-z0-9]+", "");

if (output.contains(renderResult) && output.matches(regex)) {
if (output.contains(renderResult)
&& matcher.matches()
&& !matcher.group(0).contains(strippedTest)) {

String attack = getOtherInfo(sink.getLocation(), output);

Expand Down Expand Up @@ -466,4 +476,9 @@ public List<Alert> getExampleAlerts() {
public Map<String, String> getAlertTags() {
return ALERT_TAGS;
}

// For testing purposes only
static void setTemplateFormats(List<TemplateFormat> templates) {
SstiScanRule.templateFormats = templates;
}
}
68 changes: 68 additions & 0 deletions ...s/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SstiScanRuleUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.util.regex.Pattern;
import java.util.stream.Stream;
import org.apache.commons.lang3.StringUtils;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
Expand All @@ -47,6 +48,7 @@
import org.parosproxy.paros.network.HttpMalformedHeaderException;
import org.parosproxy.paros.network.HttpMessage;
import org.zaproxy.addon.commonlib.CommonAlertTag;
import org.zaproxy.zap.extension.ascanrules.ssti.GoTemplateFormat;
import org.zaproxy.zap.testutils.NanoServerHandler;
import org.zaproxy.zap.utils.ZapXmlConfiguration;

Expand All @@ -59,6 +61,11 @@ protected SstiScanRule createScanner() {
return new SstiScanRule();
}

@AfterEach
void reset() {
SstiScanRule.setTemplateFormats(SstiScanRule.DEFAULT_TEMPLATE_LIST);
}

@Override
protected int getRecommendMaxNumberMessagesPerParam(Plugin.AttackStrength strength) {
int recommendMax = super.getRecommendMaxNumberMessagesPerParam(strength);
Expand Down Expand Up @@ -236,6 +243,39 @@ protected Response serve(IHTTPSession session) {
assertThat(alertsRaised.get(0).getConfidence(), equalTo(Alert.CONFIDENCE_HIGH));
}

@Test
void shouldReportGoBasedSsti() throws NullPointerException, IOException {
String test = "/shouldReportGoBasedSsti/";
// Given
nano.addHandler(createGoHandler(test, true));
HttpMessage msg = getHttpMessage(test + "?name=test");
rule.setConfig(new ZapXmlConfiguration());
SstiScanRule.setTemplateFormats(List.of(new GoTemplateFormat()));
rule.init(msg, parent);
// When
rule.scan();
// Then
assertThat(alertsRaised.size(), equalTo(1));
assertThat(alertsRaised.get(0).getRisk(), equalTo(Alert.RISK_HIGH));
assertThat(alertsRaised.get(0).getConfidence(), equalTo(Alert.CONFIDENCE_HIGH));
}

@Test
void shouldNotReportGoBasedSstiWhenDirectiveEchoed() throws NullPointerException, IOException {
String test = "/shouldNotReportGoBasedSstiWhenDirectiveEchoed/";
// Given
nano.addHandler(createGoHandler(test, false));
HttpMessage msg = getHttpMessage(test + "?name=test");
rule.setConfig(new ZapXmlConfiguration());
rule.setAttackStrength(Plugin.AttackStrength.MEDIUM);
SstiScanRule.setTemplateFormats(List.of(new GoTemplateFormat()));
rule.init(msg, parent);
// When
rule.scan();
// Then
assertThat(alertsRaised.size(), equalTo(0));
}

@Test
void shouldReturnExpectedMappings() {
// Given / When
Expand Down Expand Up @@ -321,4 +361,32 @@ private static String getSimpleArithmeticResult(String expression)
throw new IllegalArgumentException("invalid template code");
}
}

private NanoServerHandler createGoHandler(String path, boolean stripPrint) {
return new NanoServerHandler(path) {
@Override
protected Response serve(IHTTPSession session) {
String name = getFirstParamValue(session, "name");
String response;
if (name != null) {
try {
if (name.contains("print")) {
name = name.replaceAll("[^A-Za-z0-9]+", "");
name = stripPrint ? name.replace("print", "") : name;
}
name = templateRenderMock("{", "}", name);
response =
getHtml(
"sstiscanrule/Rendered.html",
new String[][] {{"name", name}});
} catch (IllegalArgumentException e) {
response = getHtml("sstiscanrule/ErrorPage.html");
}
} else {
response = getHtml("sstiscanrule/NoInput.html");
}
return newFixedLengthResponse(response);
}
};
}
}

0 comments on commit 1572aa6

Please sign in to comment.