Merge pull request #5053 from FiveOFive/timing-statistics-improvements

addOns/ascanrules/CHANGELOG.md

@@ -4,7 +4,8 @@ All notable changes to this add-on will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## Unreleased
-
+### Fixed
+- Use high and low delays for linear regression time-based tests to fix false positives from delays that were smaller than normal variance in application response times.
 
 ## [58] - 2023-10-12
 ### Changed

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

@@ -211,11 +211,8 @@ public class CommandInjectionScanRule extends AbstractAppParamPlugin {
     /** The default number of seconds used in time-based attacks (i.e. sleep commands). */
     private static final int DEFAULT_TIME_SLEEP_SEC = 5;
 
-    // time-based attack detection will not send more than the following number of requests
-    private static final int BLIND_REQUEST_LIMIT = 5;
-    // time-based attack detection will try to take less than the following number of seconds
-    // note: detection of this length will generally only happen in the positive (detecting) case.
-    private static final double BLIND_SECONDS_LIMIT = 20.0;
+    // limit the maximum number of requests sent for time-based attack detection
+    private static final int BLIND_REQUESTS_LIMIT = 4;
 
     // error range allowable for statistical time-based blind attacks (0-1.0)
     private static final double TIME_CORRELATION_ERROR_RANGE = 0.15;
@@ -623,8 +620,8 @@ private boolean testCommandInjection(
                     // use TimingUtils to detect a response to sleep payloads
                     isInjectable =
                             TimingUtils.checkTimingDependence(
-                                    BLIND_REQUEST_LIMIT,
-                                    BLIND_SECONDS_LIMIT,
+                                    BLIND_REQUESTS_LIMIT,
+                                    timeSleepSeconds,
                                     requestSender,
                                     TIME_CORRELATION_ERROR_RANGE,
                                     TIME_SLOPE_ERROR_RANGE);

addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/timing/TimingUtils.java

@@ -24,8 +24,8 @@
 /** Utility class to host time-based blind detection algorithms. */
 public class TimingUtils {
 
-    // Minimum requests required for a meaningful result
-    private static final int MINIMUM_REQUESTS = 3;
+    // Minimum requests required for a result
+    private static final int MINIMUM_REQUESTS = 2;
 
     /**
      * Sends time-based blind requests and analyze the response times using simple linear
@@ -36,12 +36,19 @@ public class TimingUtils {
      * expected delay. This implementation also requires a minimum number of request data points to
      * prevent false positives where a website only responded once or twice within the time limit.
      *
+     * <p>This implementation uses a series of alternating high delay and low delay requests to
+     * minimize the possibility of false positives from normal variations in application response
+     * time. For example, if it tested a delay of 1 second and 2 seconds, there's a very real
+     * possibility that the application response times normally vary by 1 second and we get a false
+     * positive. Whereas if it tests 15 seconds and 1 second, there is a much smaller chance that
+     * the application response times normally vary by 14 seconds.
+     *
      * @param requestsLimit the hard limit on how many times at most requestSender will be called.
-     *     in practice, if there is a correlation, within 0-2 to this number of requests will be
-     *     sent. if there is no correlation, most likely far fewer.
-     * @param secondsLimit the soft limit on how much total time at most should be spent on sending
-     *     requests before forcing a verdict. the limit is necessarily soft since we don't control
-     *     how long requestSender takes to resolve.
+     *     In practice, the number of requests will usually be much less, because if a positive is
+     *     clearly not even close, we exit early. Note that 1 more request than the limit may be
+     *     sent because the test actually makes pairs of requests (one high sleep value and one low
+     *     sleep value)
+     * @param highSleepTimeSeconds the high sleep value to send in requests
      * @param requestSender function that takes in the expected time, sends the request, and returns
      *     the actual delay.
      * @param correlationErrorRange the interval of acceptance for the regression correlation. for
@@ -55,69 +62,79 @@ public class TimingUtils {
      */
     public static boolean checkTimingDependence(
             int requestsLimit,
-            double secondsLimit,
+            int highSleepTimeSeconds,
             RequestSender requestSender,
             double correlationErrorRange,
             double slopeErrorRange)
             throws IOException {
 
-        if (secondsLimit < 5) {
-            throw new IllegalArgumentException(
-                    "requires at least 5 seconds to get meaningful results");
-        }
-
         if (requestsLimit < MINIMUM_REQUESTS) {
             throw new IllegalArgumentException(
                     String.format(
-                            "requires at least %d requests to get meaningful results",
-                            MINIMUM_REQUESTS));
+                            "requires at least %d requests to get results", MINIMUM_REQUESTS));
         }
 
         OnlineSimpleLinearRegression regression = new OnlineSimpleLinearRegression();
 
         int requestsLeft = requestsLimit;
-        int requestsMade = 0;
-        double secondsLeft = secondsLimit;
-        int currentDelay = 1;
-
-        // send requests until we're either out of time or out of requests
-        while (requestsLeft > 0 && secondsLeft > 0) {
-
-            // apply the provided function to get the dependent variable
-            double y = requestSender.apply(currentDelay);
 
-            // this is not a general assertion, but in our case, we want to stop early
-            // if the expected delay isn't at LEAST as much as the requested delay
-            if (y < currentDelay) {
+        // send requests until we've hit the max requests
+        // requests are sent in pairs - one high sleep value and one low sleep value
+        // optimized to stop early if correlation is clearly not possible
+        while (requestsLeft > 0) {
+            // send the high sleep value request
+            boolean isCorrelationPossible =
+                    sendRequestAndTestConfidence(regression, requestSender, highSleepTimeSeconds);
+            // return early if we're clearly not close
+            if (!isCorrelationPossible) {
                 return false;
             }
 
-            // update the regression computation with a new time pair
-            regression.addPoint(currentDelay, y);
-
-            // failure case if we're clearly not even close
-            if (!regression.isWithinConfidence(0.3, 1.0, 0.5)) {
+            // send the low value sleep request
+            isCorrelationPossible = sendRequestAndTestConfidence(regression, requestSender, 1);
+            // return early if we're clearly not close
+            if (!isCorrelationPossible) {
                 return false;
             }
 
-            // update seconds left, requests left, and increase the next delay
-            secondsLeft = secondsLeft - y;
-            requestsLeft = requestsLeft - 1;
-            requestsMade++;
-            currentDelay = currentDelay + 1;
-
-            // if doing a longer request next would put us over time, wrap around to sending shorter
-            // requests
-            if (regression.predict(currentDelay) > secondsLeft) {
-                currentDelay = 1;
-            }
+            // update requests left
+            requestsLeft = requestsLeft - 2;
         }
 
         // we want the slope and correlation to both be reasonably close to 1
         // if the correlation is bad, the relationship is non-linear
         // if the slope is bad, the relationship is not positively 1:1
-        return requestsMade >= MINIMUM_REQUESTS
-                && regression.isWithinConfidence(correlationErrorRange, 1.0, slopeErrorRange);
+        return regression.isWithinConfidence(correlationErrorRange, 1.0, slopeErrorRange);
+    }
+
+    /**
+     * Helper function to send a single request and add it to the regression Also has optimizations
+     * to check if the a correlation is clearly not possible
+     *
+     * @return - true if a correlation is still possible, false if a correlation is clearly not
+     *     possible
+     */
+    private static boolean sendRequestAndTestConfidence(
+            OnlineSimpleLinearRegression regression, RequestSender requestSender, int delay)
+            throws IOException {
+        // apply the provided function to get the dependent variable
+        double y = requestSender.apply(delay);
+
+        // this is not a general assertion, but in our case, we want to stop early
+        // if the expected delay isn't at LEAST as much as the requested delay
+        if (y < delay) {
+            return false;
+        }
+
+        // update the regression computation with a new time pair
+        regression.addPoint(delay, y);
+
+        // failure case if we're clearly not even close
+        if (!regression.isWithinConfidence(0.3, 1.0, 0.5)) {
+            return false;
+        }
+
+        return true;
     }
 
     @FunctionalInterface

addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/timing/TimingUtilsUnitTest.java

@@ -26,6 +26,8 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 import java.util.Random;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -45,14 +47,14 @@ void init() {
     }
 
     @Test
-    // verifies that an incrementing sequence of delays is automatically generated
-    void shouldAutoIncrementDelay() throws IOException {
+    // verifies that an alternating sequence of delays is automatically generated
+    void shouldAlternateHighLowDelay() throws IOException {
         // Given
         ArrayList<Double> generatedDelays = new ArrayList<>();
         // When
         boolean result =
                 TimingUtils.checkTimingDependence(
-                        5,
+                        4,
                         15,
                         x -> {
                             generatedDelays.add(x);
@@ -62,30 +64,7 @@ void shouldAutoIncrementDelay() throws IOException {
                         SLOPE_ERROR_RANGE);
         // Then
         assertThat(result, is(true));
-        assertThat(generatedDelays.toArray(), arrayContaining(1.0, 2.0, 3.0, 4.0, 5.0));
-    }
-
-    @Test
-    // incrementing sequence of delays is automatically generated but then loops back to 1
-    void shouldAutoIncrementThenLoop() throws IOException {
-        // Given
-        ArrayList<Double> generatedDelays = new ArrayList<>();
-        // When
-        boolean result =
-                TimingUtils.checkTimingDependence(
-                        10,
-                        20,
-                        x -> {
-                            generatedDelays.add(x);
-                            return x;
-                        },
-                        CORRELATION_ERROR_RANGE,
-                        SLOPE_ERROR_RANGE);
-        // Then
-        assertThat(result, is(true));
-        assertThat(
-                generatedDelays.toArray(),
-                arrayContaining(1.0, 2.0, 3.0, 4.0, 5.0, 1.0, 2.0, 1.0, 1.0));
+        assertThat(generatedDelays.toArray(), arrayContaining(15.0, 1.0, 15.0, 1.0));
     }
 
     @Test
@@ -95,7 +74,7 @@ void shouldGiveUpQuicklyWhenNotInjectable() throws IOException {
         // When
         boolean result =
                 TimingUtils.checkTimingDependence(
-                        5,
+                        4,
                         15,
                         // respond with a low time
                         x -> {
@@ -116,7 +95,7 @@ void shouldGiveUpQuicklyWhenSlowButNotInjectable() throws IOException {
         // When
         boolean result =
                 TimingUtils.checkTimingDependence(
-                        5,
+                        4,
                         15,
                         // source of small error
                         x -> {
@@ -136,7 +115,7 @@ void shouldDetectDependenceWithSmallError() throws IOException {
         // When
         boolean result =
                 TimingUtils.checkTimingDependence(
-                        5,
+                        4,
                         15,
                         // source of small error
                         x -> x + rand.nextDouble() * 0.5,
@@ -147,19 +126,24 @@ void shouldDetectDependenceWithSmallError() throws IOException {
     }
 
     @Test
-    // verify that there is no alert when less requests were made in the time limit than required
-    // for a statistically meaningful result
-    void shouldNotAlertIfMinimumRequestsThresholdWasNotMet() throws IOException {
+    void shouldNotAlertForUncorrelatedTimes() throws IOException {
+        // Series of response times to test
+        List<Double> delays = new ArrayList<>(Arrays.asList(15.377, 12.093, 16.588, 10.752));
+
         // When
-        boolean result =
-                TimingUtils.checkTimingDependence(
-                        5,
-                        20,
-                        // slow response greater than the secondsLimit
-                        x -> 21,
-                        .15,
-                        .30);
+        boolean result = TimingUtils.checkTimingDependence(4, 15, x -> delays.remove(0), .15, .30);
         // Then
         assertThat(result, is(false));
     }
+
+    @Test
+    void shouldAlertForCorrelatedTimes() throws IOException {
+        // Series of response times to test
+        List<Double> delays = new ArrayList<>(Arrays.asList(16.0, 2.0, 17.0, 3.0));
+
+        // When
+        boolean result = TimingUtils.checkTimingDependence(4, 15, x -> delays.remove(0), .15, .30);
+        // Then
+        assertThat(result, is(true));
+    }
 }