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

ascanrules: SQLi: add checking for redirection after form input #3284

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

ascanrules: SQLi: add checking for redirection after form input #3284

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/ascanrules/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- SQL Injection - Hypersonic SQL
- SQL Injection - MsSQL
- SQL Injection - MySQL
- SQL Injection will now be more tolerant to redirects (Issue 6883).

### Added
- Help entry for the Spring Actuators scan rule (missed during previous promotion).
Expand Down
72 changes: 60 additions & 12 deletions ...s/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.apache.commons.httpclient.URI;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.text.StringEscapeUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand All @@ -43,6 +44,7 @@
import org.parosproxy.paros.core.scanner.AbstractAppParamPlugin;
import org.parosproxy.paros.core.scanner.Alert;
import org.parosproxy.paros.core.scanner.Category;
import org.parosproxy.paros.network.HttpHeader;
import org.parosproxy.paros.network.HttpMessage;
import org.zaproxy.addon.commonlib.CommonAlertTag;
import org.zaproxy.zap.extension.authentication.ExtensionAuthentication;
Expand Down Expand Up @@ -830,6 +832,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
// String mResBodyNormal = getBaseMsg().getResponseBody().toString();
mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString();
mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue);
String locationHeaderNormal =
refreshedmessage.getResponseHeader().getHeader(HttpHeader.LOCATION);

if (!sqlInjectionFoundForUrl
&& doExpressionBased
Expand Down Expand Up @@ -863,7 +867,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
param,
origParamValue,
modifiedParamValueForAdd,
modifiedParamValueConfirmForAdd);
modifiedParamValueConfirmForAdd,
locationHeaderNormal);
// bale out if we were asked nicely
if (isStop()) {
LOGGER.debug("Stopping the scan due to a user request");
Expand All @@ -888,7 +893,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
param,
origParamValue,
modifiedParamValueForMult,
modifiedParamValueConfirmForMult);
modifiedParamValueConfirmForMult,
locationHeaderNormal);
// bale out if we were asked nicely
if (isStop()) {
LOGGER.debug("Stopping the scan due to a user request");
Expand Down Expand Up @@ -977,6 +983,9 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString();
mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue);

locationHeaderNormal =
refreshedmessage.getResponseHeader().getHeader(HttpHeader.LOCATION);

// boolean booleanBasedSqlInjectionFoundForParam = false;

// try each of the AND syntax values in turn.
Expand Down Expand Up @@ -1020,6 +1029,7 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
String resBodyANDTrueStripped =
stripOffOriginalAndAttackParam(
resBodyANDTrueUnstripped, origParamValue, sqlBooleanAndTrueValue);
String headerAndTrue = msg2.getResponseHeader().getHeader(HttpHeader.LOCATION);

// set up two little arrays to ease the work of checking the unstripped output, and
// then the stripped output
Expand Down Expand Up @@ -1077,6 +1087,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
resBodyANDFalseUnstripped,
origParamValue,
sqlBooleanAndFalseValue);
String headerAndFalse =
msg2_and_false.getResponseHeader().getHeader(HttpHeader.LOCATION);

String andFalseBodyOutput[] = {
resBodyANDFalseUnstripped, resBodyANDFalseStripped
Expand Down Expand Up @@ -1138,8 +1150,22 @@ && matchBodyPattern(msg1, errorPattern, sb)) {

sqlInjectionFoundForUrl = true;

break; // No further need to loop through SQL_AND
break; // No further need to loop through header check

} else if (StringUtils.compare(locationHeaderNormal, headerAndTrue) == 0
&& StringUtils.compare(headerAndTrue, headerAndFalse) != 0) {
// or we get redirected, likely an SQL Injection too
sqlInjectionAttack = sqlBooleanAndTrueValue;
newAlert()
.setConfidence(Alert.CONFIDENCE_MEDIUM)
.setParam(param)
.setAttack(sqlInjectionAttack)
.setMessage(msg2)
.raise();

sqlInjectionFoundForUrl = true;

break; // No further need to loop through SQL_AND
} else {
// the results of the always false condition are the same as for the
// original unmodified parameter
Expand Down Expand Up @@ -1186,14 +1212,19 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
String resBodyORTrueStripped =
stripOffOriginalAndAttackParam(
resBodyORTrueUnstripped, origParamValue, orValue);
String headerOrTrue =
msg2_or_true.getResponseHeader().getHeader(HttpHeader.LOCATION);

String orTrueBodyOutput[] = {
resBodyORTrueUnstripped, resBodyORTrueStripped
};

int compareOrToOriginal =
orTrueBodyOutput[booleanStrippedUnstrippedIndex].compareTo(
normalBodyOutput[booleanStrippedUnstrippedIndex]);
normalBodyOutput[
booleanStrippedUnstrippedIndex])
| StringUtils.compare(
locationHeaderNormal, headerOrTrue);
if (compareOrToOriginal != 0) {
LOGGER.debug(
"Check 2, {} html output for OR TRUE condition [{}] different to (refreshed) original results for {}",
Expand Down Expand Up @@ -1346,12 +1377,14 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
countBooleanBasedRequests++;

String resBodyORTrueUnstripped = msg2.getResponseBody().toString();
String locationHeaderOrTrue =
msg2.getResponseHeader().getHeader(HttpHeader.LOCATION);

// if the results of the "OR 1=1" exceed the original query (unstripped, by more
// than a 20% size difference, say), we may be onto something.
// than a 20% size difference, say) or got redirected, we may be onto something.
// TODO: change the percentage difference threshold based on the alert threshold
if ((resBodyORTrueUnstripped.length()
> (mResBodyNormalUnstripped.length() * 1.2))) {
if ((resBodyORTrueUnstripped.length() > (mResBodyNormalUnstripped.length() * 1.2))
|| (StringUtils.compare(locationHeaderNormal, locationHeaderOrTrue) != 0)) {
LOGGER.debug(
"Check 2a, unstripped html output for OR TRUE condition [{}] produced sufficiently larger results than the original message",
sqlBooleanOrTrueValue);
Expand All @@ -1377,14 +1410,21 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
resBodyANDFalseUnstripped,
origParamValue,
sqlBooleanAndFalseValue);
String headerAndFalse =
msg2_and_false.getResponseHeader().getHeader(HttpHeader.LOCATION);

// does the "AND 1=2" version produce the same as the original (for
// stripped/unstripped versions)
boolean verificationUsingUnstripped =
resBodyANDFalseUnstripped.compareTo(mResBodyNormalUnstripped) == 0;
boolean verificationUsingStripped =
resBodyANDFalseStripped.compareTo(mResBodyNormalStripped) == 0;
if (verificationUsingUnstripped || verificationUsingStripped) {
boolean verificationUsingLocationHeader =
StringUtils.compare(headerAndFalse, locationHeaderOrTrue) != 0;

if (verificationUsingUnstripped
|| verificationUsingStripped
|| verificationUsingLocationHeader) {
LOGGER.debug(
"Check 2, {} html output for AND FALSE condition [{}] matches the (refreshed) original results",
(verificationUsingStripped ? "STRIPPED" : "UNSTRIPPED"),
Expand All @@ -1398,7 +1438,7 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
sqlBooleanOrTrueValue,
sqlBooleanAndFalseValue,
"");
} else {
} else if (verificationUsingUnstripped) {
extraInfo =
Constant.messages.getString(
MESSAGE_PREFIX + "alert.booleanbased.extrainfo",
Expand Down Expand Up @@ -1852,7 +1892,8 @@ private void expressionBasedAttack(
String param,
String originalParam,
String modifiedParamValue,
String modifiedParamValueConfirm)
String modifiedParamValueConfirm,
String originalLocationHeader)
throws IOException {
// those of you still paying attention will note that if handled as expressions (such as by
// a database), these represent the same value.
Expand All @@ -1876,12 +1917,14 @@ private void expressionBasedAttack(
stripOffOriginalAndAttackParam(
modifiedExpressionOutputUnstripped, originalParam, modifiedParamValue);
String normalBodyOutputStripped = stripOff(mResBodyNormalStripped, modifiedParamValue);
String locationHeaderModified = msg.getResponseHeader().getHeader(HttpHeader.LOCATION);

if (!sqlInjectionFoundForUrl && countExpressionBasedRequests < doExpressionMaxRequests) {
// if the results of the modified request match the original query, we may be onto
// something.

if (modifiedExpressionOutputStripped.compareTo(normalBodyOutputStripped) == 0) {
if (modifiedExpressionOutputStripped.compareTo(normalBodyOutputStripped) == 0
&& StringUtils.compare(locationHeaderModified, originalLocationHeader) == 0) {
LOGGER.debug(
"Check 4, STRIPPED html output for modified expression parameter [{}] matched (refreshed) original results for {}",
modifiedParamValue,
Expand Down Expand Up @@ -1913,11 +1956,16 @@ private void expressionBasedAttack(
confirmExpressionOutputUnstripped,
originalParam,
modifiedParamValueConfirm);
String confirmExpressionLocationHeader =
msg.getResponseHeader().getHeader(HttpHeader.LOCATION);
Comment on lines +1959 to +1960
Copy link
Member

Choose a reason for hiding this comment

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

Could extract a convenience method for this accepting HttpMessage and returning String.

Then leverage it throughout.


normalBodyOutputStripped =
stripOff(mResBodyNormalStripped, modifiedParamValueConfirm);

if (confirmExpressionOutputStripped.compareTo(normalBodyOutputStripped) != 0) {
if (confirmExpressionOutputStripped.compareTo(normalBodyOutputStripped) != 0
|| StringUtils.compare(
confirmExpressionLocationHeader, locationHeaderModified)
!= 0) {
// the confirm query did not return the same results. This means that arbitrary
// queries are not all producing the same page output.
// this means the fact we earlier reproduced the original page output with a
Expand Down
Loading