From c4df0cb30b287b1d7866da0ed6ddb45f75673bfc Mon Sep 17 00:00:00 2001 From: Karl Seryani Date: Sat, 11 Apr 2026 18:37:35 -0400 Subject: [PATCH] ascanrules: Reduce SQL Injection boolean-based false positives Add a control request to the boolean-based SQL injection detection to verify page stability before raising alerts. After AND FALSE or OR TRUE differs from baseline, the scan re-sends the original parameter value. If the control response also differs, the page is unstable and the alert is raised at CONFIDENCE_LOW instead of CONFIDENCE_MEDIUM. This reduces false positives from pages with dynamic content (CSRF tokens, timestamps, view counters) while still surfacing potential findings for manual verification. Fixes zaproxy/zaproxy#9289 Signed-off-by: Karl Seryani --- addOns/ascanrules/CHANGELOG.md | 3 + .../ascanrules/SqlInjectionScanRule.java | 114 ++++++- .../SqlInjectionScanRuleUnitTest.java | 310 ++++++++++++++++++ 3 files changed, 424 insertions(+), 3 deletions(-) diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md index f0b25d5beaf..04b5f0a533d 100644 --- a/addOns/ascanrules/CHANGELOG.md +++ b/addOns/ascanrules/CHANGELOG.md @@ -9,6 +9,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - The scan rules now have new tags for the OWASP Top 10 2025, and API Top 10 2023. - Depends on an updated version of the Common Library add-on. +### Fixed +- Reduced false positives in SQL Injection boolean-based checks by adding a control request to detect page instability (Issue 9289). + ## [80] - 2026-03-02 ### Added - Checks for cloud metadata from IBM and OpenStack. diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java index 87cac8df18b..873ed3febad 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java @@ -1071,6 +1071,20 @@ private void testBooleanBasedSqlInjection(String param, String origParamValue) sqlBooleanAndFalseValue, refreshedmessage.getRequestHeader().getURI()); + // Confirm the page is stable by re-sending the original value + int confidence = + checkPageStability( + param, + origParamValue, + normalResponse, + "Check 2, AND FALSE path"); + if (confidence < 0) { + if (isStop()) { + return; + } + continue; + } + // it's different (suggesting that the "AND 1 = 2" appended on gave // different results because it restricted the data set to nothing // Likely a SQL Injection. Raise it @@ -1078,7 +1092,7 @@ private void testBooleanBasedSqlInjection(String param, String origParamValue) // Bypass" alert, if necessary sqlInjectionAttack = sqlBooleanAndTrueValue; newAlert() - .setConfidence(Alert.CONFIDENCE_MEDIUM) + .setConfidence(confidence) .setParam(param) .setAttack(sqlInjectionAttack) .setOtherInfo( @@ -1139,6 +1153,20 @@ private void testBooleanBasedSqlInjection(String param, String origParamValue) orValue, refreshedmessage.getRequestHeader().getURI()); + // Confirm the page is stable by re-sending the original value + int confidence = + checkPageStability( + param, + origParamValue, + normalResponse, + "Check 2, OR TRUE path"); + if (confidence < 0) { + if (isStop()) { + return; + } + continue; + } + // it's different (suggesting that the "OR 1 = 1" appended on gave // different results because it broadened the data set from nothing // to something @@ -1146,7 +1174,7 @@ private void testBooleanBasedSqlInjection(String param, String origParamValue) // "Authentication Bypass" alert, if necessary sqlInjectionAttack = orValue; newAlert() - .setConfidence(Alert.CONFIDENCE_MEDIUM) + .setConfidence(confidence) .setParam(param) .setAttack(sqlInjectionAttack) .setOtherInfo( @@ -1266,11 +1294,50 @@ private void testBooleanBasedNoDataSqlInjection(String param, String origParamVa "Check 2, {} html output for AND FALSE condition [{}] matches the (refreshed) original results", (verificationUsingStripped ? "STRIPPED" : "UNSTRIPPED"), sqlBooleanAndFalseValue); + + // Confirm the page is stable by re-sending the original value + HttpMessage msgControl = getNewMsg(); + setParameter(msgControl, param, origParamValue); + if (isStop()) { + return; + } + try { + sendAndReceive(msgControl, false); + } catch (SocketException ex) { + LOGGER.debug( + "Caught {} {} when accessing: {}", + ex.getClass().getName(), + ex.getMessage(), + msgControl.getRequestHeader().getURI()); + continue; + } + countBooleanBasedRequests++; + + String controlBodyUnstripped = msgControl.getResponseBody().toString(); + String controlBodyStripped = stripOff(controlBodyUnstripped, origParamValue); + + boolean controlMatchesUnstripped = + controlBodyUnstripped.compareTo(mResBodyNormalUnstripped) == 0; + boolean controlMatchesStripped = + controlBodyStripped.compareTo(mResBodyNormalStripped) == 0; + + int confidence; + if (!controlMatchesUnstripped && !controlMatchesStripped) { + // Control differs from baseline - page may be unstable + LOGGER.debug( + "Check 2a, control request for original value [{}] showed page is unstable for {}", + origParamValue, + refreshedmessage.getRequestHeader().getURI()); + confidence = Alert.CONFIDENCE_LOW; + } else { + confidence = Alert.CONFIDENCE_MEDIUM; + } + // raise the alert, and save the attack string for the "Authentication // Bypass" alert, if necessary sqlInjectionAttack = sqlBooleanOrTrueValue; newAlert() - .setConfidence(Alert.CONFIDENCE_MEDIUM) + .setConfidence(confidence) .setParam(param) .setAttack(sqlInjectionAttack) .setOtherInfo( @@ -1737,6 +1804,47 @@ private void expressionBasedAttack( // here doesn't matter. Anything between 0 and 1 works. private static final float HEURISTIC_WEIGHT = .99f; + /** + * Send a control request with the original parameter value to verify page stability. Returns + * {@link Alert#CONFIDENCE_MEDIUM} if the page is stable, {@link Alert#CONFIDENCE_LOW} if + * unstable, or {@code -1} if the request could not be completed (caller should skip this + * iteration). + */ + private int checkPageStability( + String param, + String origParamValue, + ComparableResponse normalResponse, + String checkDescription) + throws IOException { + HttpMessage msgControl = getNewMsg(); + setParameter(msgControl, param, origParamValue); + if (isStop()) { + return -1; + } + try { + sendAndReceive(msgControl, false); + } catch (SocketException ex) { + LOGGER.debug( + "Caught {} {} when accessing: {}", + ex.getClass().getName(), + ex.getMessage(), + msgControl.getRequestHeader().getURI()); + return -1; + } + countBooleanBasedRequests++; + ComparableResponse controlResponse = new ComparableResponse(msgControl, origParamValue); + + if (compareResponses(normalResponse, controlResponse) < 1) { + LOGGER.debug( + "{}: control request for original value [{}] showed page is unstable for {}", + checkDescription, + origParamValue, + refreshedmessage.getRequestHeader().getURI()); + return Alert.CONFIDENCE_LOW; + } + return Alert.CONFIDENCE_MEDIUM; + } + /** * 0 means very different and 1 very similar. Note that this is the opposite from most compareTo * implementations but it matches the behavior of the compareWith function and heuristics in diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java index a0dd3f9b9af..b32e7476fa7 100644 --- a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java +++ b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java @@ -919,6 +919,316 @@ void shouldNotFailIfNormalAndModified301RedirectWithNoLocationHeaders() throws E // Then assertThat(alertsRaised, hasSize(1)); } + + // False positive case - https://github.com/zaproxy/zaproxy/issues/9289 + // Page content varies between requests (e.g., CSRF tokens), + // causing AND FALSE to appear different from baseline when it's just instability. + // The control check re-sends the original value; if it also differs, report at low + // confidence. + @Test + void shouldAlertWithLowConfidenceIfControlRequestShowsPageIsUnstable() throws Exception { + // Given + String param = "param"; + String normalValue = "payload"; + String andTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + String andFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + + // Custom handler: uses an incrementing counter for the original value + // so each request with the original value returns slightly different + // content (simulating CSRF token rotation). + // AND TRUE returns "normal response" (matching baseline after stripping). + // AND FALSE returns "different response" (triggering the alert path). + // The key: responseBodyHeuristic() strips param values from response + // bodies before comparing. After stripping, AND TRUE's body becomes + // "normal response" which matches the stripped baseline "normal response". + // But the control re-request gets a new counter value, making the + // stripped body "normal response counterN" differ from baseline. + nano.addHandler( + new NanoServerHandler("/") { + private int counter = 0; + + @Override + protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) { + String value = getFirstParamValue(session, param); + if (normalValue.equals(value)) { + counter++; + // Content changes each time the original value is sent. + // After stripping "payload", body is "normal response counterN" + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "normal response counter" + counter); + } else if (andTrueValue.equals(value)) { + // After stripping both "payload" and the AND TRUE payload, + // the body is "normal response". But baseline body after + // stripping "payload" is "normal response counterN". + // These won't match either -- so AND TRUE won't match. + // We need AND TRUE to match, so include the same counter + // as the most recent baseline. + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "normal response counter" + counter); + } else if (andFalseValue.equals(value)) { + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "different response"); + } else { + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "normal response counter" + counter); + } + } + }); + + rule.init(getHttpMessage("/?" + param + "=" + normalValue), parent); + + // When + rule.scan(); + + // Then - alert raised at low confidence because the control re-request shows + // page instability + assertThat(alertsRaised, hasSize(1)); + assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_LOW))); + assertNoParams(alertsRaised.get(0)); + } + + // Positive case confirming the control check does not break real SQLi detection. + // When AND TRUE matches baseline, AND FALSE differs, and the control request + // returns the same response as baseline, the page is stable and the alert is valid. + @Test + void shouldAlertIfControlConfirmsStablePage() throws Exception { + // Given + String param = "id"; + String normalValue = "2"; + String andTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + String andFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + + UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml("Results for ID 2: record found") + .whenParamValueIs(andTrueValue) + .thenReturnHtml("Results for ID 2: record found") + .whenParamValueIs(andFalseValue) + .thenReturnHtml("Results for ID 2: no records") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?" + param + "=" + normalValue), parent); + + // When + rule.scan(); + + // Then - alert raised because the control re-request of the original value + // returns the same response, confirming the page is stable + assertThat(alertsRaised, hasSize(1)); + Alert actual = alertsRaised.get(0); + assertThat(actual.getParam(), is(equalTo(param))); + assertThat(actual.getAttack(), is(equalTo(andTrueValue))); + assertThat(actual.getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); + assertNoParams(alertsRaised.get(0)); + } + + // Regression test for https://github.com/zaproxy/zaproxy/issues/9289 + // App parses parameter as integer, ignoring non-numeric suffixes like " AND 1=1". + // All SQL payloads produce the same response as the original, so no alert. + @Test + void shouldNotAlertIfNumericParameterStripsNonNumericInput() throws Exception { + // Given + String param = "id"; + String normalValue = "2"; + String andTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + String andFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + + // Handler that simulates parseInt behavior (like PHP intval() or MySQL implicit cast): + // extracts leading digits only, ignores everything after the first non-digit character. + // "2 AND 1=1 -- " -> leading "2" -> ID 2 (same as original) + // This means all payloads produce the same response as the original. + nano.addHandler( + new NanoServerHandler("/") { + @Override + protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) { + String value = getFirstParamValue(session, param); + // Extract leading digits only (simulating intval behavior) + int numericId = 0; + if (value != null) { + String digits = value.replaceAll("^(\\d+).*", "$1"); + if (!digits.isEmpty()) { + try { + numericId = Integer.parseInt(digits); + } catch (NumberFormatException e) { + numericId = 0; + } + } + } + String body = + "Results for ID " + numericId + ""; + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, NanoHTTPD.MIME_HTML, body); + } + }); + + rule.init(getHttpMessage("/?" + param + "=" + normalValue), parent); + + // When + rule.scan(); + + // Then - no alert because all responses are identical (app ignores SQL suffixes) + assertThat(alertsRaised, hasSize(0)); + } + + @Test + void shouldAlertWithLowConfidenceIfOrTrueControlRequestShowsPageIsUnstable() + throws Exception { + // Given + String param = "param"; + String normalValue = "payload"; + String andTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + String andFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + String orTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_OR_TRUE[0]; + + // AND TRUE matches normal, AND FALSE also matches normal (enters OR TRUE path), + // OR TRUE differs from normal, but the control re-request shows page instability. + // The counter increments only on origParamValue requests, so AND TRUE and + // AND FALSE see the same counter as the most recent refresh, matching it. + // The control re-request increments the counter, making it differ from baseline. + nano.addHandler( + new NanoServerHandler("/") { + private int counter = 0; + + @Override + protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) { + String value = getFirstParamValue(session, param); + if (andTrueValue.equals(value)) { + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "normal response counter" + counter); + } else if (andFalseValue.equals(value)) { + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "normal response counter" + counter); + } else if (orTrueValue.equals(value)) { + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "different response from or true"); + } else { + // Original value: content changes each time (unstable page) + counter++; + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "normal response counter" + counter); + } + } + }); + + rule.init(getHttpMessage("/?" + param + "=" + normalValue), parent); + + // When + rule.scan(); + + // Then - alert raised at low confidence because the control re-request shows + // page instability on the OR TRUE path + assertThat(alertsRaised, hasSize(1)); + assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_LOW))); + assertNoParams(alertsRaised.get(0)); + } + + @Test + void shouldAlertWithLowConfidenceIfNoDataControlRequestShowsPageIsUnstable() + throws Exception { + // Given + String param = "param"; + String normalValue = "payload"; + + // No-data path (check 2a): OR TRUE returns >20% more data than original, + // AND FALSE matches original, but the control re-request returns different + // content (unstable page). + // For check 2 to not trigger: AND TRUE returns different content from normal. + // Use HIGH strength to provide enough request budget for both check 2 and 2a. + rule.setAttackStrength(AttackStrength.HIGH); + String shortContent = "short content here"; + // Must be >20% longer than shortContent (18 chars * 1.2 = ~22 chars minimum) + String longContent = + "this is a much longer response body that significantly exceeds " + + "the twenty percent threshold needed for the no-data path"; + + nano.addHandler( + new NanoServerHandler("/") { + private int normalCallCount = 0; + + @Override + protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) { + String value = getFirstParamValue(session, param); + if (value == null) { + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + shortContent); + } + if (value.contains(" OR ") || value.startsWith(normalValue + "%")) { + // OR TRUE and LIKE variants return much longer content + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + longContent); + } else if (value.contains(" AND 1=1") + || value.contains("' AND '1'='1") + || value.contains("\" AND \"1\"=\"1") + || value.contains(SqlInjectionScanRule.SQL_LIKE)) { + // AND TRUE and LIKE returns different so check 2 skips + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "completely different for and true"); + } else if (value.contains(" AND 1=2") + || value.contains("' AND '1'='2") + || value.contains("\" AND \"1\"=\"2")) { + // AND FALSE returns same as baseline + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + shortContent); + } else if (normalValue.equals(value)) { + // Original param value: returns stable content on first + // calls (baseline/refresh), but later calls differ + // (simulating page instability for the control check). + normalCallCount++; + if (normalCallCount <= 2) { + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + shortContent); + } + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + "different unstable content"); + } + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, + NanoHTTPD.MIME_HTML, + shortContent); + } + }); + + rule.init(getHttpMessage("/?" + param + "=" + normalValue), parent); + + // When + rule.scan(); + + // Then - alert raised at low confidence because the control re-request shows + // page instability on the no-data path + assertThat(alertsRaised, hasSize(1)); + assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_LOW))); + assertNoParams(alertsRaised.get(0)); + } } @Nested