Skip to content

Commit

Permalink
Merge pull request #5921 from FiveOFive/sqli-comments-cleanup
Browse files Browse the repository at this point in the history 
ascanrules: clean up sqli comments
  • Loading branch information
@thc202
thc202 authored Nov 16, 2024
2 parents 4c9f69b + 464a63c commit 8cac506
Showing 1 changed file with 9 additions and 68 deletions.
77 changes: 9 additions & 68 deletions ...s/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,23 +92,19 @@ public class SqlInjectionScanRule extends AbstractAppParamPlugin
private boolean doUnionBased = false;
private boolean doExpressionBased = false;
private boolean doOrderByBased = false;
// private boolean doStackedBased = false; //TODO: use in the stacked based implementation
// how many requests can we fire for each method? will be set depending on the attack strength
private int doErrorMaxRequests = 0;
private int doBooleanMaxRequests = 0;
private int doUnionMaxRequests = 0;
private int doExpressionMaxRequests = 0;
private int doOrderByMaxRequests = 0;
// private int doStackedMaxRequests = 0; //TODO: use in the stacked based implementation
// how many requests have we fired up?
private int countErrorBasedRequests = 0;
private int countExpressionBasedRequests = 0;
private int countBooleanBasedRequests = 0;
private int countUnionBasedRequests = 0;
private int countOrderByBasedRequests = 0;

// private int countStackedBasedRequests = 0; //TODO: use in the stacked based queries
// implementation
/**
* generic one-line comment. Various RDBMS Documentation suggests that this syntax works with
* almost every single RDBMS considered here
Expand Down Expand Up @@ -458,10 +454,8 @@ private static List<String> asList(String... strings) {
"\") UNION ALL select NULL" + SQL_ONE_LINE_COMMENT,
};

/** for logging. */
private static final Logger LOGGER = LogManager.getLogger(SqlInjectionScanRule.class);

/** determines if we should output Debug level logging */
private boolean debugEnabled = LOGGER.isDebugEnabled();

@Override
Expand Down Expand Up @@ -547,10 +541,6 @@ public String getReference() {
public void init() {
LOGGER.debug("Initialising");

// DEBUG only
// this.debugEnabled=true;
// this.setAttackStrength(AttackStrength.LOW);

// set up what we are allowed to do, depending on the attack strength that was set.
if (this.getAttackStrength() == AttackStrength.LOW) {
// do error based (if Threshold allows), and some expression based
Expand All @@ -563,8 +553,6 @@ public void init() {
doUnionMaxRequests = 0;
doOrderByBased = false;
doOrderByMaxRequests = 0;
// doStackedBased = false;
// doStackedMaxRequests = 0;

} else if (this.getAttackStrength() == AttackStrength.MEDIUM) {
// do some more error based (if Threshold allows), some more expression based, some
Expand All @@ -578,8 +566,6 @@ public void init() {
doUnionMaxRequests = 5;
doOrderByBased = false;
doOrderByMaxRequests = 0;
// doStackedBased = false;
// doStackedMaxRequests = 5;

} else if (this.getAttackStrength() == AttackStrength.HIGH) {
// do some more error based (if Threshold allows), some more expression based, some more
Expand All @@ -594,8 +580,6 @@ public void init() {
doUnionMaxRequests = 10;
doOrderByBased = true;
doOrderByMaxRequests = 5;
// doStackedBased = false;
// doStackedMaxRequests = 10;

} else if (this.getAttackStrength() == AttackStrength.INSANE) {
// do some more error based (if Threshold allows), some more expression based, some more
Expand All @@ -609,8 +593,6 @@ public void init() {
doUnionMaxRequests = 100;
doOrderByBased = true;
doOrderByMaxRequests = 100;
// doStackedBased = false;
// doStackedMaxRequests = 100;
}

// if a high threshold is in place, turn off the error based, which are more prone to false
Expand Down Expand Up @@ -684,7 +666,6 @@ public void scan(HttpMessage msg, String param, String origParamValue) {
// some cases
String[] prefixStrings;
if (origParamValue != null) {
// ZAP: Removed getURLDecode()
prefixStrings = new String[] {"", origParamValue};
} else {
prefixStrings = new String[] {""};
Expand All @@ -704,9 +685,6 @@ public void scan(HttpMessage msg, String param, String origParamValue) {
prefixStrings[prefixIndex] + SQL_CHECK_ERR[sqlErrorStringIndex];
setParameter(msg1, param, sqlErrValue);

// System.out.println("Attacking [" + msg + "], parameter [" + param + "] with
// value ["+ sqlErrValue + "]");

// send the message with the modified parameters
try {
sendAndReceive(msg1, false); // do not follow redirects
Expand Down Expand Up @@ -740,7 +718,7 @@ && checkSpecificErrors(rdbms, msg1, param, sqlErrValue)) {
sqlInjectionAttack = sqlErrValue;
break;
}
} // end of the loop to check for RDBMS specific error messages
}

if (this.doGenericErrorBased && !sqlInjectionFoundForUrl) {
Iterator<Pattern> errorPatternIterator =
Expand Down Expand Up @@ -791,9 +769,9 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
sqlInjectionFoundForUrl = true;
continue;
}
} // end of the loop to check for RDBMS specific error messages
}
}
} // for each of the SQL_CHECK_ERR values (SQL metacharacters)
}
}

// ###############################
Expand Down Expand Up @@ -825,7 +803,6 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
return; // Something went wrong, no point continuing
}

// String mResBodyNormal = getBaseMsg().getResponseBody().toString();
mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString();
mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue);

Expand All @@ -836,9 +813,6 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
// first figure out the type of the parameter..
try {
// is it an integer type?
// ZAP: removed URLDecoding because on Variants
// int paramAsInt =
// Integer.parseInt(SqlInjectionScanRule.getURLDecode(origParamValue));
int paramAsInt = Integer.parseInt(origParamValue);

LOGGER.debug("The parameter value [{}] is of type Integer", origParamValue);
Expand Down Expand Up @@ -971,12 +945,9 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
return; // Something went wrong, no point continuing
}

// String mResBodyNormal = getBaseMsg().getResponseBody().toString();
mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString();
mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue);

// boolean booleanBasedSqlInjectionFoundForParam = false;

// try each of the AND syntax values in turn.
// Which one is successful will depend on the column type of the table/view column into
// which we are injecting the SQL.
Expand All @@ -993,7 +964,6 @@ && matchBodyPattern(msg1, errorPattern, sb)) {

// needs a new message for each type of AND to be issued
HttpMessage msg2 = getNewMsg();
// ZAP: Removed getURLDecode()
String sqlBooleanAndTrueValue = origParamValue + SQL_LOGIC_AND_TRUE[i];
String sqlBooleanAndFalseValue = origParamValue + SQL_LOGIC_AND_FALSE[i];

Expand All @@ -1013,7 +983,6 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
}
countBooleanBasedRequests++;

// String resBodyAND = msg2.getResponseBody().toString();
String resBodyANDTrueUnstripped = msg2.getResponseBody().toString();
String resBodyANDTrueStripped =
stripOffOriginalAndAttackParam(
Expand Down Expand Up @@ -1064,10 +1033,6 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
}
countBooleanBasedRequests++;

// String resBodyANDFalse =
// stripOff(msg2_and_false.getResponseBody().toString(),
// SQL_LOGIC_AND_FALSE[i]);
// String resBodyANDFalse = msg2_and_false.getResponseBody().toString();
String resBodyANDFalseUnstripped =
msg2_and_false.getResponseBody().toString();
String resBodyANDFalseStripped =
Expand Down Expand Up @@ -1176,9 +1141,6 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
}
countBooleanBasedRequests++;

// String resBodyORTrue =
// stripOff(msg2_or_true.getResponseBody().toString(), orValue);
// String resBodyORTrue = msg2_or_true.getResponseBody().toString();
String resBodyORTrueUnstripped =
msg2_or_true.getResponseBody().toString();
String resBodyORTrueStripped =
Expand Down Expand Up @@ -1245,13 +1207,12 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
// vuln for a given param, since the database column is of only 1
// type

break; // No further need to loop
break;
}
}
} // if the results of the "AND 1=1" match the original query, we may be onto
// something.
else {
// andTrueBodyOutput[booleanStrippedUnstrippedIndex].compareTo(normalBodyOutput[booleanStrippedUnstrippedIndex])
// the results of the "AND 1=1" do NOT match the original query, for
// whatever reason (no sql injection, or the web page is not stable)
if (this.debugEnabled) {
Expand All @@ -1275,8 +1236,6 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
booleanStrippedUnstrippedIndex]
.split("\\n"))));

// int numberofDifferences = diffpatch.getDeltas().size();

// and convert the list of patches to a String, joining using a newline
StringBuilder tempDiff = new StringBuilder(250);
for (Delta<String> delta : diffpatch.getDeltas()) {
Expand Down Expand Up @@ -1304,9 +1263,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
LOGGER.debug("DIFFS: {}", tempDiff);
}
}
} // end of boolean logic output index (unstripped + stripped)
}
}
// end of check 2

// check 2a: boolean based logic, where the original query returned *no* data. Here we
// append " OR 1=1" in an attempt to extract *more* data
Expand Down Expand Up @@ -1424,11 +1382,10 @@ && matchBodyPattern(msg1, errorPattern, sb)) {

sqlInjectionFoundForUrl = true;

break; // No further need to loop
break;
}
}
}
// end of check 2a

// Check 3: UNION based
// for each SQL UNION combination to try
Expand Down Expand Up @@ -1486,9 +1443,8 @@ && checkUnionErrors(
sqlInjectionAttack = sqlUnionValue;
break;
}
} // end of the loop to check for RDBMS specific UNION error messages
} //// for each SQL UNION combination to try
// end of check 3
}
}

// ###############################

Expand Down Expand Up @@ -1527,15 +1483,13 @@ && checkUnionErrors(
return; // Something went wrong, no point continuing
}

// String mResBodyNormal = getBaseMsg().getResponseBody().toString();
mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString();
mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue);

if (!sqlInjectionFoundForUrl
&& doOrderByBased
&& countOrderByBasedRequests < doOrderByMaxRequests) {

// ZAP: Removed getURLDecode()
String modifiedParamValue = origParamValue + " ASC " + SQL_ONE_LINE_COMMENT;

HttpMessage msg5 = getNewMsg();
Expand Down Expand Up @@ -1592,7 +1546,6 @@ && checkUnionErrors(
// minimise false positives

// use the descending order this time
// ZAP: Removed getURLDecode()
String modifiedParamValueConfirm =
origParamValue + " DESC " + SQL_ONE_LINE_COMMENT;

Expand Down Expand Up @@ -1662,7 +1615,7 @@ && checkUnionErrors(
.raise();

sqlInjectionFoundForUrl = true;
break; // No further need to loop
break;
}
}
}
Expand All @@ -1675,7 +1628,6 @@ && checkUnionErrors(
// alert in addition to the alerts already raised
if (sqlInjectionFoundForUrl) {
boolean loginUrl = false;
// LOGGER.debug("### A SQL Injection may lead to auth bypass..");

// are we dealing with a login url in any of the contexts?
ExtensionAuthentication extAuth =
Expand Down Expand Up @@ -1704,20 +1656,12 @@ && checkUnionErrors(
// params, fragment, and POST params
// are possibly different from the login page.
loginUrl = true;
// DEBUG only
// LOGGER.debug("##### The right login page was found");
break;
} else {
// LOGGER.debug("#### This is not the login page you're looking
// for");
}
} else {
// LOGGER.debug("### This context has no login page set");
}
}
}
if (loginUrl) {
// LOGGER.debug("##### Raising auth bypass");
// raise the alert, using the custom name and description
String vulnname =
Constant.messages.getString(MESSAGE_PREFIX + "authbypass.name");
Expand Down Expand Up @@ -1816,8 +1760,6 @@ private boolean checkUnionErrors(
boolean patternInOrig = matcherOrig.find();
boolean patternInSQLUnion = matcherSQLUnion.find();

// if (! matchBodyPattern(getBaseMsg(), errorPattern, null) && matchBodyPattern(msg,
// errorPattern, sb)) {
if (!patternInOrig && patternInSQLUnion) {
// Likely a UNION Based SQL Injection (by error message). Raise it
String extraInfo =
Expand Down Expand Up @@ -1938,7 +1880,6 @@ private void expressionBasedAttack(
.setOtherInfo(extraInfo)
.setMessage(msg)
.raise();
// SQL Injection has been found
sqlInjectionFoundForUrl = true;
return;
}
Expand Down

0 comments on commit 8cac506

Please sign in to comment.