ascanrules: SQLi SQLite split timing tests to new scan rule

Signed-off-by: kingthorin <[email protected]>

Author: kingthorin

addOns/ascanrules/CHANGELOG.md

@@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Changed
 - Maintenance changes.
 - Depends on an updated version of the Common Library add-on.
+- The SQL Injection SQLite scan rule has been broken into two rules; one union based, and one time based (Issue 7341). This includes assigning the timing rule ID 90038, and updating the add-on's help content.
 
 ### Added
 - Rules (as applicable) have been tagged in relation to HIPAA and PCI DSS.

addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionSqLiteScanRule.java

@@ -433,6 +433,13 @@ <H2 id="id-40024">SQL Injection - SQLite</H2>
 <br>
 Alert ID: <a href="https://www.zaproxy.org/docs/alerts/40024/">40024</a>.
 
+<H2 id="id-90038">SQL Injection - SQLite (Time Based)</H2>
+This active scan rule attempts to inject SQLite specific commands into parameter values and analyzes the timing of server responses to see if the commands were effectively executed on the server (indicating a successful SQL injection attack).
+<p>
+Latest code: <a href="https://github.com/zaproxy/zap-extensions/blob/main/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionSqLiteTimingScanRule.java">SqlInjectionSqLiteTimingScanRule.java</a>
+<br>
+Alert ID: <a href="https://www.zaproxy.org/docs/alerts/90038/">90038</a>.
+
 <H2 id="id-40029">Trace.axd Information Leak</H2>
 Tests to see if Trace Viewer (trace.axd) is available. Although this component is convenient for developers and 
 other stakeholders it can leak a significant amount of information which a security analyst or malicious individual

addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionSqLiteTimingScanRule.java

@@ -190,9 +190,10 @@ ascanrules.sqlinjection.postgres.name = SQL Injection - PostgreSQL
 ascanrules.sqlinjection.refs = https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html
 ascanrules.sqlinjection.soln = Do not trust client side input, even if there is client side validation in place.\nIn general, type check all data on the server side.\nIf the application uses JDBC, use PreparedStatement or CallableStatement, with parameters passed by '?'\nIf the application uses ASP, use ADO Command Objects with strong type checking and parameterized queries.\nIf database Stored Procedures can be used, use them.\nDo *not* concatenate strings into queries in the stored procedure, or use 'exec', 'exec immediate', or equivalent functionality!\nDo not create dynamic SQL queries using simple string concatenation.\nEscape all data received from the client.\nApply an 'allow list' of allowed characters, or a 'deny list' of disallowed characters in user input.\nApply the principle of least privilege by using the least privileged database user possible.\nIn particular, avoid using the 'sa' or 'db-owner' database users. This does not eliminate SQL injection, but minimizes its impact.\nGrant the minimum database access that is necessary for the application.
 ascanrules.sqlinjection.sqlite.alert.errorbased.extrainfo = The following known SQLite error message was provoked: [{0}].
-ascanrules.sqlinjection.sqlite.alert.timebased.extrainfo = The query time is controllable using parameter value [{0}], which caused the request to take [{1}] milliseconds, parameter value [{2}], which caused the request to take [{3}] milliseconds, when the original unmodified query with value [{4}] took [{5}] milliseconds.
+ascanrules.sqlinjection.sqlite.alert.timing.extrainfo = The query time is controllable using parameter value [{0}], which caused the request to take [{1}] milliseconds, parameter value [{2}], which caused the request to take [{3}] milliseconds, when the original unmodified query with value [{4}] took [{5}] milliseconds.
 ascanrules.sqlinjection.sqlite.alert.versionnumber.extrainfo = Using a UNION based SQL Injection attack, and by exploiting SQLite's dynamic typing mechanism, the SQLite version was determined to be [{0}].\nWith string-based injection points, full SQLite version information can be extracted, but with numeric injection points, only partial SQLite version information can be extracted.\nMore information on SQLite version [{0}] is available at https://www.sqlite.org/changes.html
 ascanrules.sqlinjection.sqlite.name = SQL Injection - SQLite
+ascanrules.sqlinjection.sqlite.timing.name = SQL Injection - SQLite (Time Based)
 
 ascanrules.ssti.alert.otherinfo = Proof found at [{0}]\ncontent:\n[{1}]
 ascanrules.ssti.desc = When the user input is inserted in the template instead of being used as argument in rendering is evaluated by the template engine. Depending on the template engine it can lead to remote code execution.

addOns/ascanrules/src/main/javahelp/org/zaproxy/zap/extension/ascanrules/resources/help/contents/ascanrules.html

@@ -19,24 +19,17 @@
  */
 package org.zaproxy.zap.extension.ascanrules;
 
-import static fi.iki.elonen.NanoHTTPD.newFixedLengthResponse;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.startsWith;
 
-import fi.iki.elonen.NanoHTTPD.IHTTPSession;
-import fi.iki.elonen.NanoHTTPD.Response;
 import java.util.Map;
 import org.junit.jupiter.api.Test;
-import org.parosproxy.paros.core.scanner.Alert;
 import org.parosproxy.paros.core.scanner.Plugin;
-import org.parosproxy.paros.network.HttpMessage;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
 import org.zaproxy.addon.commonlib.PolicyTag;
 import org.zaproxy.zap.model.Tech;
 import org.zaproxy.zap.model.TechSet;
-import org.zaproxy.zap.testutils.NanoServerHandler;
 
 /** Unit test for {@link SqlInjectionSqLiteScanRule}. */
 class SqlInjectionSQLiteScanRuleUnitTest extends ActiveScannerTest<SqlInjectionSqLiteScanRule> {
@@ -54,11 +47,11 @@ protected int getRecommendMaxNumberMessagesPerParam(Plugin.AttackStrength streng
                 return NUMBER_MSGS_ATTACK_STRENGTH_LOW;
             case MEDIUM:
             default:
-                return NUMBER_MSGS_ATTACK_STRENGTH_MEDIUM + 2;
+                return NUMBER_MSGS_ATTACK_STRENGTH_MEDIUM;
             case HIGH:
-                return NUMBER_MSGS_ATTACK_STRENGTH_HIGH + 10;
+                return NUMBER_MSGS_ATTACK_STRENGTH_HIGH;
             case INSANE:
-                return NUMBER_MSGS_ATTACK_STRENGTH_INSANE + 22;
+                return NUMBER_MSGS_ATTACK_STRENGTH_INSANE + 126;
         }
     }
 
@@ -82,112 +75,6 @@ void shouldNotTargetNonSqLiteSQLTechs() throws Exception {
         assertThat(targets, is(equalTo(false)));
     }
 
-    @Test
-    void shouldAlertIfSqlErrorReturned() throws Exception {
-        String test = "/shouldReportSqlErrorMessage/";
-
-        this.nano.addHandler(
-                new NanoServerHandler(test) {
-                    @Override
-                    protected Response serve(IHTTPSession session) {
-                        String name = getFirstParamValue(session, "name");
-                        String response = "<html><body></body></html>";
-                        if (name != null && name.contains(" randomblob(")) {
-                            response =
-                                    "<html><body>SQL error: no such function: randomblob</body></html>";
-                        }
-                        return newFixedLengthResponse(response);
-                    }
-                });
-
-        HttpMessage msg = this.getHttpMessage(test + "?name=test");
-
-        this.rule.init(msg, this.parent);
-
-        this.rule.scan();
-
-        assertThat(alertsRaised.size(), equalTo(1));
-        assertThat(alertsRaised.get(0).getEvidence(), equalTo("no such function: randomblob"));
-        assertThat(alertsRaised.get(0).getParam(), equalTo("name"));
-        assertThat(
-                alertsRaised.get(0).getAttack(),
-                equalTo("case randomblob(100000) when not null then 1 else 1 end "));
-        assertThat(alertsRaised.get(0).getRisk(), equalTo(Alert.RISK_HIGH));
-        assertThat(alertsRaised.get(0).getConfidence(), equalTo(Alert.CONFIDENCE_MEDIUM));
-    }
-
-    @Test
-    void shouldAlertIfRandomBlobTimesGetLonger() throws Exception {
-        String test = "/shouldReportSqlTimingIssue/";
-
-        this.nano.addHandler(
-                new NanoServerHandler(test) {
-                    private int time = 100;
-
-                    @Override
-                    protected Response serve(IHTTPSession session) {
-                        String name = getFirstParamValue(session, "name");
-                        String response = "<html><body></body></html>";
-                        if (name != null && name.contains(" randomblob(")) {
-                            try {
-                                Thread.sleep(time);
-                            } catch (InterruptedException e) {
-                                // Ignore
-                            }
-                            time += 100;
-                        }
-                        return newFixedLengthResponse(response);
-                    }
-                });
-
-        HttpMessage msg = this.getHttpMessage(test + "?name=test");
-
-        this.rule.init(msg, this.parent);
-        this.rule.setExpectedDelayInMs(90);
-
-        this.rule.scan();
-
-        assertThat(alertsRaised.size(), equalTo(1));
-        assertThat(
-                alertsRaised.get(0).getEvidence(),
-                startsWith("The query time is controllable using parameter value"));
-        assertThat(alertsRaised.get(0).getParam(), equalTo("name"));
-        assertThat(alertsRaised.get(0).getAttack(), startsWith("case randomblob(100"));
-        assertThat(alertsRaised.get(0).getRisk(), equalTo(Alert.RISK_HIGH));
-        assertThat(alertsRaised.get(0).getConfidence(), equalTo(Alert.CONFIDENCE_MEDIUM));
-    }
-
-    @Test
-    void shouldNotAlertIfAllTimesGetLonger() throws Exception {
-        String test = "/shouldReportSqlTimingIssue/";
-
-        this.nano.addHandler(
-                new NanoServerHandler(test) {
-                    private int time = 100;
-
-                    @Override
-                    protected Response serve(IHTTPSession session) {
-                        String response = "<html><body></body></html>";
-                        try {
-                            Thread.sleep(time);
-                        } catch (InterruptedException e) {
-                            // Ignore
-                        }
-                        time += 100;
-                        return newFixedLengthResponse(response);
-                    }
-                });
-
-        HttpMessage msg = this.getHttpMessage(test + "?name=test");
-
-        this.rule.init(msg, this.parent);
-        this.rule.setExpectedDelayInMs(90);
-
-        this.rule.scan();
-
-        assertThat(alertsRaised.size(), equalTo(0));
-    }
-
     @Test
     void shouldReturnExpectedMappings() {
         // Given / When