refactor(security): improve SecureRandom implementation, tests, and docs

lib/private/Security/SecureRandom.php

@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 /**
- * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-FileCopyrightText: 2016-2025 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
  * SPDX-License-Identifier: AGPL-3.0-only
  */
@@ -12,36 +12,42 @@
 use OCP\Security\ISecureRandom;
 
 /**
- * Class SecureRandom provides a wrapper around the random_int function to generate
- * secure random strings. For PHP 7 the native CSPRNG is used, older versions do
- * use a fallback.
+ * Secure random string generator recommended for tokens, passwords, secrets, and similar security use cases.
  *
- * Usage:
- * \OC::$server->get(ISecureRandom::class)->generate(10);
- * @package OC\Security
+ * @see \OCP\Security\ISecureRandom
  */
 class SecureRandom implements ISecureRandom {
-	/**
-	 * Generate a secure random string of specified length.
-	 * @param int $length The length of the generated string
-	 * @param string $characters An optional list of characters to use if no character list is
-	 *                           specified all valid base64 characters are used.
-	 * @throws \LengthException if an invalid length is requested
-	 */
 	#[\Override]
 	public function generate(
 		int $length,
-		string $characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/',
+		string $characters = ISecureRandom::CHAR_BASE64_RFC4648,
 	): string {
+
 		if ($length <= 0) {
-			throw new \LengthException('Invalid length specified: ' . $length . ' must be bigger than 0');
+			throw new \LengthException(
+				'Invalid length specified: ' . $length . ' must be greater than 0'
+			);
+		}
+
+		if (
+			// Check for ASCII-only (no multibyte characters)
+			!mb_check_encoding($characters, 'ASCII')
+			// Check for uniqueness: number of unique bytes must equal original length
+			|| strlen(count_chars($characters, 3)) !== strlen($characters)
+			// Check minimum length
+			|| strlen($characters) < 4
+		) {
+			throw new \InvalidArgumentException(
+				'Character set must be ASCII-only, unique, and at least four characters long.'
+			);
 		}
 
+		// Build string by selecting random characters from $characters and appending
 		$maxCharIndex = \strlen($characters) - 1;
 		$randomString = '';
-
 		while ($length > 0) {
 			$randomNumber = \random_int(0, $maxCharIndex);
+			// Safe: $characters is guaranteed ASCII; indexed access is byte-correct.
 			$randomString .= $characters[$randomNumber];
 			$length--;
 		}

lib/public/Security/ISecureRandom.php

@@ -10,18 +10,23 @@
 namespace OCP\Security;
 
 /**
- * Class SecureRandom provides a wrapper around the random_int function to generate
- * secure random strings. For PHP 7 the native CSPRNG is used, older versions do
- * use a fallback.
+ * Secure random string generator for tokens, passwords, secrets, and similar security use cases.
  *
- * Usage:
- * \OCP\Server::get(ISecureRandom::class)->generate(10);
+ * A wrapper around PHP's random_int(), utilizing the native CSPRNG.
+ * @link https://www.php.net/manual/en/function.random-int.php
  *
+ * By default, uses the RFC 4648 Base64 alphabet for random string generation, and allows
+ * custom character sets if desired.
+ *
+ * Example usage:
+ *   - Typical (if ISecureRandom $random is provided by DI):
+ *       `$secret = $this->random->generate(48);`
+ *   - Non-DI:
+ *       `$secret = \OCP\Server::get(\OCP\Security\ISecureRandom::class)->generate(48);`
  * @since 8.0.0
  */
 interface ISecureRandom {
 	/**
-	 * Flags for characters that can be used for <code>generate($length, $characters)</code>
 	 * @since 8.0.0
 	 */
 	public const CHAR_UPPER = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
@@ -47,22 +52,40 @@ interface ISecureRandom {
 	public const CHAR_ALPHANUMERIC = self::CHAR_UPPER . self::CHAR_LOWER . self::CHAR_DIGITS;
 
 	/**
-	 * Characters that can be used for <code>generate($length, $characters)</code>, to
-	 * generate human-readable random strings. Lower- and upper-case characters and digits
-	 * are included. Characters which are ambiguous are excluded, such as I, l, and 1 and so on.
-	 *
+	 * Lowercase, uppercase characters, and digits. Ambiguous characters are excluded (e.g., I, l, and 1).
 	 * @since 23.0.0
 	 */
 	public const CHAR_HUMAN_READABLE = 'abcdefgijkmnopqrstwxyzABCDEFGHJKLMNPQRSTWXYZ23456789';
 
 	/**
-	 * Generate a random string of specified length.
-	 * @param int $length The length of the generated string
-	 * @param string $characters An optional list of characters to use if no character list is
-	 *                           specified all valid base64 characters are used.
-	 * @return string
+	 * Standard Base64 alphabet per RFC4648.
+	 * @link https://datatracker.ietf.org/doc/html/rfc4648#section-4
+	 * @since 33.0.0
+	 */
+	public const CHAR_BASE64_RFC4648 = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/';
+
+	/**
+	 * Generate a secure random string of the specified length.
+	 *
+	 * Security notes:
+	 * - For most secure applications (tokens, passwords, CSRF values), an ample and diverse
+	 *   character set, such as the default CHAR_BASE64_RFC4648, is typically a good choice.
+	 * - Overly small (<4), non-unique, or multibyte character sets weaken security and are not permitted.
+	 *
+	 * @param int $length Number of characters (must be > 0).
+	 * @param string $characters Optional list of unique, single-byte (ASCII) characters
+	 *        to use. Defaults to the CHAR_BASE64_RFC4648 alphabet. A custom set should contain at least 4
+	 *        characters, and must not contain duplicates or multibyte (non-ASCII) characters. It is strongly
+	 *        recommended to use predefined constants from ISecureRandom, which all meet the requirements.
+	 * @return string The randomly generated string.
+	 * @throws \LengthException If $length <= 0.
+	 * @throws \InvalidArgumentException if $characters contains non-ASCII characters, duplicates,
+	 *         or fewer than 4 unique characters.
+	 * @since 35.0.0 $characters has to be >4 chars long, non-ASCII characters are rejected, not contain duplicates.
 	 * @since 8.0.0
 	 */
-	public function generate(int $length,
-		string $characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/'): string;
+	public function generate(
+		int $length,
+		string $characters = ISecureRandom::CHAR_BASE64_RFC4648,
+	): string;
 }

tests/lib/Security/SecureRandomTest.php

@@ -16,11 +16,11 @@ class SecureRandomTest extends \Test\TestCase {
 	public static function stringGenerationProvider(): array {
 		return [
 			[1, 1],
+			[16, 16],
+			[31, 31],
+			[64, 64],
 			[128, 128],
-			[256, 256],
 			[1024, 1024],
-			[2048, 2048],
-			[64000, 64000],
 		];
 	}
 
@@ -82,4 +82,53 @@ public function testInvalidLengths($length): void {
 		$generator = $this->rng;
 		$generator->generate($length);
 	}
+
+	public static function invalidCharProviders(): array {
+		return [
+			'invalid_too_short' => ['abc'],
+			'invalid_duplicates' => ['aabcd'],
+			'invalid_non_ascii' => ["abcd\xf0"],
+		];
+	}
+
+	#[\PHPUnit\Framework\Attributes\DataProvider('invalidCharProviders')]
+	public function testInvalidCharacterSets(string $invalidCharset): void {
+		$this->expectException(\InvalidArgumentException::class);
+		$this->rng->generate(10, $invalidCharset);
+	}
+
+	public function testDefaultCharsetBase64Characters(): void {
+		$randomString = $this->rng->generate(100);
+		$this->assertMatchesRegularExpression('/^[A-Za-z0-9\+\/]{100}$/', $randomString);
+	}
+
+	public function testAllOutputsAreUnique(): void {
+		// While collisions are technically possible, extremely unlikely for these sizes
+		$first = $this->rng->generate(1000);
+		$second = $this->rng->generate(1000);
+		$this->assertNotEquals($first, $second, "Random output should not be repeated.");
+	}
+
+	public function testMinimumValidCharset(): void {
+		$charset = 'abcd';
+		$randomString = $this->rng->generate(500, $charset);
+		$this->assertMatchesRegularExpression('/^[abcd]{500}$/', $randomString);
+	}
+
+	public function testLargeCustomCharset(): void {
+		$charset = '';
+		for ($i = 32; $i <= 126; $i++) { // all printable ASCII
+			$charset .= chr($i);
+		}
+		$randomString = $this->rng->generate(200, $charset);
+		foreach (str_split($randomString) as $char) {
+			$this->assertStringContainsString($char, $charset);
+		}
+	}
+
+	public function testUserProvidedValidCharset(): void {
+		$charset = '@#$!';
+		$randomString = $this->rng->generate(64, $charset);
+		$this->assertMatchesRegularExpression('/^[@#$!]{64}$/', $randomString);
+	}
 }