From d0d9c91ab2e0bdc7c343c8b9d3fecc4e4cad9a10 Mon Sep 17 00:00:00 2001 From: Wruczek Date: Tue, 6 Oct 2020 04:13:28 +0200 Subject: [PATCH] PHP 7 - type declarations, use secure pseudo randoms --- src/private/php/Auth.php | 12 +++++-- src/private/php/Utils/CsrfUtils.php | 30 ++++++------------ src/private/php/Utils/TeamSpeakUtils.php | 2 -- src/private/php/Utils/Utils.php | 40 +++++++++++++++++++----- 4 files changed, 50 insertions(+), 34 deletions(-) diff --git a/src/private/php/Auth.php b/src/private/php/Auth.php index 278e8dd..afd4b00 100644 --- a/src/private/php/Auth.php +++ b/src/private/php/Auth.php @@ -89,6 +89,7 @@ class Auth { * @param $poke bool|null true = poke user, false = send a message, null = default value from config * @return string|null|false Returns code as string on success, null when * client cannot be found and false when other error occurs. + * @throws \TeamSpeak3_Adapter_ServerQuery_Exception */ public static function generateConfirmationCode($cldbid, $poke = null) { if ($poke === null) { @@ -98,7 +99,7 @@ class Auth { if (TeamSpeakUtils::i()->checkTSConnection()) { try { $client = TeamSpeakUtils::i()->getTSNodeServer()->clientGetByDbid($cldbid); - $code = (string) mt_rand(100000, 999999); // TODO: replace it with a CSPRNG + $code = (string) Utils::getSecureRandomInt(100000, 999999); $msg = LanguageUtils::tl("LOGIN_CONFIRMATION_CODE", $code); if ($poke) { @@ -113,6 +114,10 @@ class Auth { if ($e->getCode() === 512) { return null; } + + throw $e; + } catch (\Exception $e) { + // ignore exceptions from Utils::getSecureRandomInt } } @@ -225,7 +230,8 @@ class Auth { } // Check if we data is already cached and if we can use it - if (isset($_SESSION["tsuser"]["servergroups"])) { + // no caching in dev mode + if (isset($_SESSION["tsuser"]["servergroups"]) && !__DEV_MODE) { $cached = $_SESSION["tsuser"]["servergroups"]; // Calculate how old is the cached data (in seconds) @@ -253,7 +259,7 @@ class Auth { } // Since the array in indexed with server group ID's, we can just separate the keys - // That gives us an array with ID's if user groups + // That gives us an array with ID's of user groups $serverGroupIds = array_keys($serverGroups); // Cache it in session with current time for later cachebusting diff --git a/src/private/php/Utils/CsrfUtils.php b/src/private/php/Utils/CsrfUtils.php index a5e3a2f..e6e3326 100644 --- a/src/private/php/Utils/CsrfUtils.php +++ b/src/private/php/Utils/CsrfUtils.php @@ -2,29 +2,18 @@ namespace Wruczek\TSWebsite\Utils; - class CsrfUtils { + public const CSRF_TOKEN_BYTES_LENGTH = 32; + /** * Generates and returns a new CSRF token - * @param $length int length in bytes + * @param $bytes int length in bytes * @return string generated CSRF token * @throws \Exception when unable to generate a new token */ - public static function generateToken($length) { - if (function_exists("random_bytes")) { - $token = bin2hex(random_bytes($length)); - } else if (function_exists("mcrypt_create_iv")) { - $token = bin2hex(mcrypt_create_iv($length, MCRYPT_DEV_URANDOM)); - } else { - $token = bin2hex(openssl_random_pseudo_bytes($length)); - } - - if (!is_string($token) || empty($token)) { - throw new \Exception("Cannot generate new CSRF token"); - } - - return $token; + public static function generateToken(int $bytes): string { + return Utils::getSecureRandomString($bytes); } /** @@ -32,13 +21,12 @@ class CsrfUtils { * @return string CSRF token * @throws \Exception When we cannot generate a new CSRF token */ - public static function getToken() { + public static function getToken(): string { if (isset($_SESSION["csrfToken"])) { return $_SESSION["csrfToken"]; } - $length = 16; // in bytes - $token = self::generateToken($length); + $token = self::generateToken(self::CSRF_TOKEN_BYTES_LENGTH); $_SESSION["csrfToken"] = $token; return $token; @@ -49,7 +37,7 @@ class CsrfUtils { * @param $toCheck string token to be checked * @return bool true if tokens match, false otherwise. */ - public static function validateToken($toCheck) { + public static function validateToken(string $toCheck): bool { $knownToken = @$_SESSION["csrfToken"]; if ($knownToken === null) { @@ -63,7 +51,7 @@ class CsrfUtils { * Tries to get CSRF token from the request and then compares it. * If it fails, it returns the error page with message and exits the script. */ - public static function validateRequest() { + public static function validateRequest(): void { if (isset($_POST["csrf-token"])) { $csrfToken = $_POST["csrf-token"]; } else if (isset($_SERVER["HTTP_X_CSRF_TOKEN"])) { diff --git a/src/private/php/Utils/TeamSpeakUtils.php b/src/private/php/Utils/TeamSpeakUtils.php index 4e65acf..260f3b9 100644 --- a/src/private/php/Utils/TeamSpeakUtils.php +++ b/src/private/php/Utils/TeamSpeakUtils.php @@ -2,8 +2,6 @@ namespace Wruczek\TSWebsite\Utils; -use function mt_rand; -use function var_dump; use Wruczek\TSWebsite\Config; /** diff --git a/src/private/php/Utils/Utils.php b/src/private/php/Utils/Utils.php index f59ae72..aa6840d 100644 --- a/src/private/php/Utils/Utils.php +++ b/src/private/php/Utils/Utils.php @@ -20,8 +20,8 @@ class Utils { * @param $string string String to be escaped * @return string escaped string */ - public static function escape($string) { - return htmlspecialchars((string) $string, ENT_QUOTES, "UTF-8"); + public static function escape(string $string): string { + return htmlspecialchars($string, ENT_QUOTES, "UTF-8"); } /** @@ -30,7 +30,7 @@ class Utils { * @param $str * @return bool|string stripped text without the first line or false on failure */ - public static function stripFirstLine($str) { + public static function stripFirstLine(string $str) { $position = strpos($str, "\n"); if($position === false) @@ -47,7 +47,7 @@ class Utils { * @param bool $case set to false for case-insensitivity (default true) * @return bool true if $haystack starts with $needle, false otherwise */ - public static function startsWith($haystack, $needle, $case = true) { + public static function startsWith(string $haystack, string $needle, bool $case = true): bool { if ($case) return strpos($haystack, $needle, 0) === 0; @@ -62,7 +62,7 @@ class Utils { * @param bool $case set to false for case-insensitivity (default true) * @return bool true if $haystack ends with $needle, false otherwise */ - public static function endsWith($haystack, $needle, $case = true) { + public static function endsWith(string $haystack, string $needle, bool $case = true): bool { $expectedPosition = strlen($haystack) - strlen($needle); if ($case) @@ -77,7 +77,7 @@ class Utils { * @return bool|string Censored IP on success, false on failure * @throws \Exception When the IP address is invalid */ - public static function censorIpAddress($ip) { + public static function censorIpAddress(string $ip): string { if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) { $ip = explode(".", $ip); @@ -107,7 +107,7 @@ class Utils { * Falls back to REMOTE_ADDR if empty * @return string IP address */ - public static function getClientIp($useCfip = null) { + public static function getClientIp(bool $useCfip = null): string { if ($useCfip === null) { $useCfip = Config::get("usingcloudflare"); } @@ -128,7 +128,7 @@ class Utils { * Returns currently used news store * @return INewsStore|null */ - public static function getNewsStore() { + public static function getNewsStore(): ?INewsStore { $newsStore = null; // if the current implementation is default @@ -138,4 +138,28 @@ class Utils { return $newsStore; } + + /** + * Generate secure random int in specified bounds + * @see \random_int() + * @param int $min + * @param int $max + * @return int + * @throws \Exception + */ + public static function getSecureRandomInt(int $min, int $max): int { + return \random_int($min, $max); + } + + /** + * Generate secure random bytes and convert them into hex + * @see \random_bytes() + * @see \bin2hex() + * @param int $bytes + * @return string + * @throws \Exception + */ + public static function getSecureRandomString(int $bytes): string { + return \bin2hex(\random_bytes($bytes)); + } }