Merge branch '5.3' into 5.4

github-actions[bot] · github-actions[bot] · commit cf367b3a4f7c · 2025-04-09T22:35:23.000Z
diff --git a/src/Core/XssSanitiser.php b/src/Core/XssSanitiser.php
@@ -201,7 +201,9 @@ private function getStripAttributeContentsRegex(): string
             $this->splitWithWhitespaceRegex('vbscript:'),
         ];
         // Regex is "starts with any of these, with optional whitespace at the start, case insensitive"
-        return '#^\s*(' . implode('|', $regexes) . ')#iu';
+        // \x08 is the backspace character, though it only causes vulnerabilities if it's at the start
+        // or end of a string
+        return '#^(\s|\x08)*(' . implode('|', $regexes) . ')(\s|\x08)*#iu';
     }
 
     private function splitWithWhitespaceRegex(string $string): string
diff --git a/src/Security/MemberAuthenticator/LostPasswordHandler.php b/src/Security/MemberAuthenticator/LostPasswordHandler.php
@@ -159,38 +159,28 @@ public function redirectToLostPassword()
      */
     public function forgotPassword(array $data, Form $form): HTTPResponse
     {
-        // Run a first pass validation check on the data
-        $dataValidation = $this->validateForgotPasswordData($data, $form);
-        if ($dataValidation instanceof HTTPResponse) {
-            return $dataValidation;
-        }
-
-        $member = $this->getMemberFromData($data);
-
-        // Allow vetoing forgot password requests
-        $results = $this->extend('forgotPassword', $member);
-        if ($results && is_array($results) && in_array(false, $results ?? [], true)) {
-            return $this->redirectToLostPassword();
-        }
-
-        if ($member) {
-            $token = $member->generateAutologinTokenAndStoreHash();
+        return Security::withMinimumExecutionTime(function () use ($data, $form) {
+            // Run a first pass validation check on the data
+            $dataValidation = $this->validateForgotPasswordData($data, $form);
+            if ($dataValidation instanceof HTTPResponse) {
+                return $dataValidation;
+            }
 
-            $success = $this->sendEmail($member, $token);
-            if (!$success) {
-                $form->sessionMessage(
-                    _t(
-                        Member::class . '.EMAIL_FAILED',
-                        'There was an error when trying to email you a password reset link.'
-                    ),
-                    'bad'
-                );
+            $member = $this->getMemberFromData($data);
 
+            // Allow vetoing forgot password requests
+            $results = $this->extend('forgotPassword', $member);
+            if ($results && is_array($results) && in_array(false, $results ?? [], true)) {
                 return $this->redirectToLostPassword();
             }
-        }
 
-        return $this->redirectToSuccess($data);
+            if ($member) {
+                $token = $member->generateAutologinTokenAndStoreHash();
+                $this->sendEmail($member, $token);
+            }
+
+            return $this->redirectToSuccess($data);
+        });
     }
 
     /**
diff --git a/src/Security/MemberAuthenticator/MemberAuthenticator.php b/src/Security/MemberAuthenticator/MemberAuthenticator.php
@@ -33,25 +33,27 @@ public function supportedServices()
 
     public function authenticate(array $data, HTTPRequest $request, ValidationResult &$result = null)
     {
-        // Find authenticated member
-        if (class_exists(Versioned::class)) {
-            [$member, $result] = Versioned::withVersionedMode(function () use ($data) {
-                Versioned::set_stage(Versioned::DRAFT);
+        return Security::withMinimumExecutionTime(function () use ($data, $request, &$result) {
+            // Find authenticated member
+            if (class_exists(Versioned::class)) {
+                [$member, $result] = Versioned::withVersionedMode(function () use ($data) {
+                    Versioned::set_stage(Versioned::DRAFT);
+                    $member = $this->authenticateMember($data, $result);
+                    return [$member, $result];
+                });
+            } else {
                 $member = $this->authenticateMember($data, $result);
-                return [$member, $result];
-            });
-        } else {
-            $member = $this->authenticateMember($data, $result);
-        }
+            }
 
-        // Optionally record every login attempt as a {@link LoginAttempt} object
-        $this->recordLoginAttempt($data, $request, $member, $result->isValid());
+            // Optionally record every login attempt as a {@link LoginAttempt} object
+            $this->recordLoginAttempt($data, $request, $member, $result->isValid());
 
-        if ($member && $request->hasSession()) {
-            $request->getSession()->clear('BackURL');
-        }
+            if ($member && $request->hasSession()) {
+                $request->getSession()->clear('BackURL');
+            }
 
-        return $result->isValid() ? $member : null;
+            return $result->isValid() ? $member : null;
+        });
     }
 
     /**
diff --git a/src/Security/Security.php b/src/Security/Security.php
@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Security;
 
+use InvalidArgumentException;
 use LogicException;
 use Page;
 use ReflectionClass;
@@ -163,6 +164,12 @@ class Security extends Controller implements TemplateGlobalProvider
      */
     private static $login_recording = false;
 
+    /**
+     * Minimum execution time in milliseconds for sensitive execution paths.
+     * Helps to protect against time-based enumeration attacks.
+     */
+    private static int $secure_min_execution_time = 1000;
+
     /**
      * @var boolean If set to TRUE or FALSE, {@link database_is_ready()}
      * will always return FALSE. Used for unit testing.
@@ -1211,6 +1218,43 @@ public static function lost_password_url()
         return Controller::join_links(Director::baseURL(), static::config()->get('lost_password_url'));
     }
 
+    /**
+     * Ensure execution of a callback takes some minimum amount of time by inserting a delay if that execution
+     * time is not elapsed.
+     *
+     * This helps to prevent time-based enumeration attacks by making execution of a sensitive code path always
+     * take the same amount of time. Note that if $minExecutionTime is too low, the enumeration attack will still
+     * be possible - but if it is too high, it could impact the user experience.
+     *
+     * @param integer $minExecutionTime The minimum amount of time in milliseconds that execution should take.
+     * If 0, the secure_min_execution_time configuration property value will be used.
+     * @return mixed The value returned from the callback, if any.
+     */
+    public static function withMinimumExecutionTime(callable $callback, int $minExecutionTime = 0): mixed
+    {
+        if ($minExecutionTime < 0) {
+            throw new InvalidArgumentException('$minExecutionTime must not be negative');
+        }
+        // Start capturing execution time
+        $startTime = hrtime(true);
+        // Execute callback
+        $retVal = $callback();
+        $stopTime = hrtime(true);
+        // Delay by remaining execution time
+        if (!$minExecutionTime) {
+            $minExecutionTime = Security::config()->get('secure_min_execution_time');
+        }
+        // $timeTaken gets converted from nanoseconds to microseconds
+        // $minExecutionTime gets converted from milliseconds to microseconds
+        // $waitFor gets cast to int for use in usleep.
+        $timeTaken = ($stopTime - $startTime) / 1000;
+        $waitFor = (int) round(($minExecutionTime * 1000) - $timeTaken);
+        if ($waitFor > 0) {
+            usleep($waitFor);
+        }
+        return $retVal;
+    }
+
     /**
      * Defines global accessible templates variables.
      *
diff --git a/tests/php/Core/XssSanitiserTest.php b/tests/php/Core/XssSanitiserTest.php
@@ -9,6 +9,9 @@
 
 class XssSanitiserTest extends SapphireTest
 {
+    // This is the backspace character. It needs to be escaped in double-quotes.
+    private const CHAR_BACKSPACE = "\x08";
+
     protected $usesDatabase = false;
 
     public function provideSanitise(): array
@@ -73,6 +76,14 @@ public function provideSanitise(): array
                 'input' => '<a href="javascript:alert(\'ok\')">Lorem ipsum dolor sit amet, consectetur adipisicing elit.</a>',
                 'expected' => '<a>Lorem ipsum dolor sit amet, consectetur adipisicing elit.</a>',
             ],
+            [
+                'input' => '<a href="' . XssSanitiserTest::CHAR_BACKSPACE . 'javascript:alert(\'ok\')">Lorem ipsum dolor sit amet, consectetur adipisicing elit.</a>',
+                'expected' => '<a>Lorem ipsum dolor sit amet, consectetur adipisicing elit.</a>',
+            ],
+            [
+                'input' => '<a href="javascript:alert(\'ok\')' . XssSanitiserTest::CHAR_BACKSPACE . '">Lorem ipsum dolor sit amet, consectetur adipisicing elit.</a>',
+                'expected' => '<a>Lorem ipsum dolor sit amet, consectetur adipisicing elit.</a>',
+            ],
             [
                 // Not exploitable XSS
                 'input' => '<<a href="javascript:evil"/>a href="javascript:evil"/>',
@@ -132,6 +143,10 @@ public function provideSanitise(): array
                 'input' => '<IMG SRC="javascript:alert(\'XSS\');">',
                 'expected' => '<img>',
             ],
+            [
+                'input' => '<IMG SRC="' . XssSanitiserTest::CHAR_BACKSPACE . 'javascript:alert(\'XSS\');">',
+                'expected' => '<img>',
+            ],
             [
                 'input' => '<IMG SRC=javascript:alert(\'XSS\')>',
                 'expected' => '<img>',
@@ -206,6 +221,10 @@ public function provideSanitise(): array
                 'input' => '<BGSOUND SRC="javascript:alert(\'XSS\');">',
                 'expected' => '<bgsound></bgsound>',
             ],
+            [
+                'input' => '<BGSOUND SRC="' . XssSanitiserTest::CHAR_BACKSPACE . 'javascript:alert(\'XSS\');">',
+                'expected' => '<bgsound></bgsound>',
+            ],
             [
                 // Not exploitable XSS
                 'input' => '<BR SIZE="&{alert(\'XSS\')}">',
diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php
@@ -10,6 +10,8 @@
 
 class HTMLEditorSanitiserTest extends FunctionalTest
 {
+    // This is the backspace character. It needs to be escaped in double-quotes.
+    private const CHAR_BACKSPACE = "\x08";
 
     public function provideSanitise(): array
     {
@@ -80,6 +82,18 @@ public function provideSanitise(): array
                 '<a>Test</a>',
                 'Javascript in the href attribute of a link is completely removed'
             ],
+            [
+                'a[href|target|rel]',
+                '<a href="' . HTMLEditorSanitiserTest::CHAR_BACKSPACE . 'javascript:alert(0);">Test</a>',
+                '<a>Test</a>',
+                'Javascript in the href attribute with leading backspace of a link is completely removed'
+            ],
+            [
+                'a[href|target|rel]',
+                '<a href="javascript:alert(0);' . HTMLEditorSanitiserTest::CHAR_BACKSPACE . '">Test</a>',
+                '<a>Test</a>',
+                'Javascript in the href attribute with backspace in middle of a link is completely removed'
+            ],
             [
                 'a[href|target|rel]',
                 '<a href="' . implode("\n", str_split(' javascript:')) . '">Test</a>',
@@ -110,6 +124,12 @@ public function provideSanitise(): array
                 '<iframe></iframe>',
                 'Javascript with tab elements the src attribute of an iframe is completely removed'
             ],
+            [
+                'iframe[src]',
+                '<iframe src="' . HTMLEditorSanitiserTest::CHAR_BACKSPACE . 'javascript:alert(0);"></iframe>',
+                '<iframe></iframe>',
+                'Javascript in the src attribute of an iframe with a backspace is completely removed'
+            ],
             [
                 'object[data]',
                 '<object data="OK"></object>',
@@ -128,6 +148,12 @@ public function provideSanitise(): array
                 '<object></object>',
                 'Object with dangerous javascript content in data attribute with quotes is completely removed'
             ],
+            [
+                'object[data]',
+                '<object data="' . HTMLEditorSanitiserTest::CHAR_BACKSPACE . 'javascript:alert()">',
+                '<object></object>',
+                'Object with dangerous javascript content in data attribute with backspace is completely removed'
+            ],
             [
                 'object[data]',
                 '<object data="data:text/html;base64,PHNjcmlwdD5hbGVydChkb2N1bWVudC5sb2NhdGlvbik8L3NjcmlwdD4=">',
diff --git a/tests/php/Security/SecurityTest.php b/tests/php/Security/SecurityTest.php
@@ -810,6 +810,41 @@ public function doTestChangepasswordForm($oldPassword, $newPassword)
         );
     }
 
+    public function provideWithMinimumExecutionTime(): array
+    {
+        return [
+            'check default is used' => [
+                'useDefault' => false,
+                'minExecution' => 100,
+            ],
+            'check arg is used' => [
+                'useDefault' => true,
+                'minExecution' => 100,
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideWithMinimumExecutionTime
+     */
+    public function testWithMinimumExecutionTime(bool $useDefault, int $minExecution): void
+    {
+        if ($useDefault) {
+            Security::config()->set('secure_min_execution_time', $minExecution);
+            $minExecutionArg = 0;
+        } else {
+            Security::config()->set('secure_min_execution_time', 1);
+            $minExecutionArg = $minExecution;
+        }
+
+        $start = hrtime(true);
+        Security::withMinimumExecutionTime(fn() => null, $minExecutionArg);
+        $timeTaken = hrtime(true) - $start;
+        $this->assertGreaterThanOrEqual($minExecution, $timeTaken / 1000000);
+        // Make sure it wasn't much longer than the min - the test empty callback should take basically no time at all.
+        $this->assertLessThan($minExecution + 1, ($timeTaken / 1000000));
+    }
+
     /**
      * Assert this message is in the current login form errors
      *
