From fa748d61d366132b88a6339be8899cd028d36ead Mon Sep 17 00:00:00 2001 From: magdev Date: Sat, 24 Jan 2026 14:31:13 +0100 Subject: [PATCH] Fix security vulnerabilities identified in audit - Add JSON encoding error handling in ResponseSignature to prevent silent failures - Sanitize exception messages to prevent information disclosure - Fix header normalization to treat empty values as null - Add SSRF protection with URL validation and private IP blocking - Replace custom key derivation with RFC 5869 compliant hash_hkdf() - Add input validation in DTO fromArray() methods - Add DateTime exception handling in DTOs Co-Authored-By: Claude Opus 4.5 --- src/Dto/ActivationResult.php | 7 +++ src/Dto/LicenseInfo.php | 14 ++++- src/Dto/LicenseStatus.php | 39 ++++++++++++- src/LicenseClient.php | 83 ++++++++++++++++++++++++++- src/SecureLicenseClient.php | 91 +++++++++++++++++++++++++++++- src/Security/ResponseSignature.php | 19 +++++-- 6 files changed, 241 insertions(+), 12 deletions(-) diff --git a/src/Dto/ActivationResult.php b/src/Dto/ActivationResult.php index a47ca2b..1048392 100644 --- a/src/Dto/ActivationResult.php +++ b/src/Dto/ActivationResult.php @@ -14,6 +14,13 @@ final readonly class ActivationResult public static function fromArray(array $data): self { + if (!isset($data['success']) || !is_bool($data['success'])) { + throw new \InvalidArgumentException('Invalid response: missing or invalid success field'); + } + if (!isset($data['message']) || !is_string($data['message'])) { + throw new \InvalidArgumentException('Invalid response: missing or invalid message field'); + } + return new self( success: $data['success'], message: $data['message'], diff --git a/src/Dto/LicenseInfo.php b/src/Dto/LicenseInfo.php index 0d9c551..33ce48a 100644 --- a/src/Dto/LicenseInfo.php +++ b/src/Dto/LicenseInfo.php @@ -15,9 +15,21 @@ final readonly class LicenseInfo public static function fromArray(array $data): self { + if (!isset($data['product_id']) || !is_int($data['product_id'])) { + throw new \InvalidArgumentException('Invalid response: missing or invalid product_id'); + } + $expiresAt = null; if (isset($data['expires_at']) && $data['expires_at'] !== null) { - $expiresAt = new \DateTimeImmutable($data['expires_at']); + try { + $expiresAt = new \DateTimeImmutable($data['expires_at']); + } catch (\Exception $e) { + throw new \InvalidArgumentException( + 'Invalid response: invalid date format for expires_at', + 0, + $e + ); + } } return new self( diff --git a/src/Dto/LicenseStatus.php b/src/Dto/LicenseStatus.php index 68400b9..380bfcc 100644 --- a/src/Dto/LicenseStatus.php +++ b/src/Dto/LicenseStatus.php @@ -26,14 +26,49 @@ final readonly class LicenseStatus public static function fromArray(array $data): self { + // Validate required fields + if (!isset($data['valid']) || !is_bool($data['valid'])) { + throw new \InvalidArgumentException('Invalid response: missing or invalid valid field'); + } + if (!isset($data['status']) || !is_string($data['status'])) { + throw new \InvalidArgumentException('Invalid response: missing or invalid status field'); + } + if (!isset($data['domain']) || !is_string($data['domain'])) { + throw new \InvalidArgumentException('Invalid response: missing or invalid domain field'); + } + if (!isset($data['activations_count']) || !is_int($data['activations_count'])) { + throw new \InvalidArgumentException('Invalid response: missing or invalid activations_count field'); + } + if (!isset($data['max_activations']) || !is_int($data['max_activations'])) { + throw new \InvalidArgumentException('Invalid response: missing or invalid max_activations field'); + } + $expiresAt = null; if (isset($data['expires_at']) && $data['expires_at'] !== null) { - $expiresAt = new \DateTimeImmutable($data['expires_at']); + try { + $expiresAt = new \DateTimeImmutable($data['expires_at']); + } catch (\Exception $e) { + throw new \InvalidArgumentException( + 'Invalid response: invalid date format for expires_at', + 0, + $e + ); + } + } + + try { + $status = LicenseState::from($data['status']); + } catch (\ValueError $e) { + throw new \InvalidArgumentException( + 'Invalid response: unknown license status value', + 0, + $e + ); } return new self( valid: $data['valid'], - status: LicenseState::from($data['status']), + status: $status, domain: $data['domain'], expiresAt: $expiresAt, activationsCount: $data['activations_count'], diff --git a/src/LicenseClient.php b/src/LicenseClient.php index b3e657e..b6dc657 100644 --- a/src/LicenseClient.php +++ b/src/LicenseClient.php @@ -18,6 +18,16 @@ final class LicenseClient implements LicenseClientInterface private const API_PATH = '/wp-json/wc-licensed-product/v1'; private const CACHE_TTL = 300; // 5 minutes + /** @var string[] Private IPv4 ranges (CIDR notation) */ + private const PRIVATE_IP_RANGES = [ + '10.0.0.0/8', + '172.16.0.0/12', + '192.168.0.0/16', + '127.0.0.0/8', + '169.254.0.0/16', + '0.0.0.0/8', + ]; + private readonly LoggerInterface $logger; public function __construct( @@ -26,8 +36,10 @@ final class LicenseClient implements LicenseClientInterface ?LoggerInterface $logger = null, private readonly ?CacheItemPoolInterface $cache = null, private readonly int $cacheTtl = self::CACHE_TTL, + bool $allowInsecureHttp = false, ) { $this->logger = $logger ?? new NullLogger(); + $this->validateBaseUrl($baseUrl, $allowInsecureHttp); } public function validate(string $licenseKey, string $domain): LicenseInfo @@ -165,13 +177,15 @@ final class LicenseClient implements LicenseClientInterface } catch (LicenseException $e) { throw $e; } catch (\Throwable $e) { + // Log full error for debugging but sanitize for user-facing message $this->logger->error('License API request failed', [ 'endpoint' => $endpoint, - 'error' => $e->getMessage(), + 'exception_class' => $e::class, + 'error_code' => $e->getCode(), ]); throw new LicenseException( - 'Failed to communicate with license server: ' . $e->getMessage(), + 'Failed to communicate with license server', null, 0, $e @@ -187,4 +201,69 @@ final class LicenseClient implements LicenseClientInterface } return $key; } + + /** + * Validate the base URL to prevent SSRF attacks. + * + * @throws \InvalidArgumentException If the URL is invalid or potentially dangerous + */ + private function validateBaseUrl(string $url, bool $allowInsecureHttp): void + { + if ($url === '') { + throw new \InvalidArgumentException('Base URL cannot be empty'); + } + + $parsed = parse_url($url); + if ($parsed === false || !isset($parsed['scheme'], $parsed['host'])) { + throw new \InvalidArgumentException('Invalid base URL format'); + } + + $scheme = strtolower($parsed['scheme']); + $host = strtolower($parsed['host']); + + // Require HTTPS unless explicitly allowed for localhost + if ($scheme !== 'https') { + if ($scheme !== 'http') { + throw new \InvalidArgumentException('Base URL must use HTTP or HTTPS scheme'); + } + $isLocalhost = $host === 'localhost' || $host === '127.0.0.1'; + if (!$allowInsecureHttp && !$isLocalhost) { + throw new \InvalidArgumentException( + 'Base URL must use HTTPS for non-localhost hosts. ' . + 'Set allowInsecureHttp=true to allow HTTP (not recommended for production).' + ); + } + } + + // Resolve hostname and check for private IPs + $ip = gethostbyname($host); + if ($ip !== $host && $this->isPrivateIp($ip)) { + throw new \InvalidArgumentException( + 'Base URL resolves to a private IP address, which is not allowed' + ); + } + } + + /** + * Check if an IP address is in a private range. + */ + private function isPrivateIp(string $ip): bool + { + $ipLong = ip2long($ip); + if ($ipLong === false) { + return false; + } + + foreach (self::PRIVATE_IP_RANGES as $range) { + [$subnet, $bits] = explode('/', $range); + $subnetLong = ip2long($subnet); + $mask = -1 << (32 - (int) $bits); + + if (($ipLong & $mask) === ($subnetLong & $mask)) { + return true; + } + } + + return false; + } } diff --git a/src/SecureLicenseClient.php b/src/SecureLicenseClient.php index 5f44bab..250a212 100644 --- a/src/SecureLicenseClient.php +++ b/src/SecureLicenseClient.php @@ -33,6 +33,16 @@ final class SecureLicenseClient implements LicenseClientInterface { private const CACHE_TTL = 300; + /** @var string[] Private IPv4 ranges (CIDR notation) */ + private const PRIVATE_IP_RANGES = [ + '10.0.0.0/8', + '172.16.0.0/12', + '192.168.0.0/16', + '127.0.0.0/8', + '169.254.0.0/16', + '0.0.0.0/8', + ]; + private readonly LoggerInterface $logger; private readonly StringEncoder $encoder; private readonly string $apiPath; @@ -52,10 +62,12 @@ final class SecureLicenseClient implements LicenseClientInterface private readonly int $cacheTtl = self::CACHE_TTL, private readonly bool $verifyIntegrity = false, ?StringEncoder $encoder = null, + bool $allowInsecureHttp = false, ) { $this->logger = $logger ?? new NullLogger(); $this->encoder = $encoder ?? new StringEncoder(); $this->apiPath = $this->encoder->decode(self::ENCODED_API_PATH); + $this->validateBaseUrl($baseUrl, $allowInsecureHttp); if ($this->verifyIntegrity) { $this->checkIntegrity(); @@ -199,13 +211,15 @@ final class SecureLicenseClient implements LicenseClientInterface } catch (LicenseException | SignatureException $e) { throw $e; } catch (\Throwable $e) { + // Log full error for debugging but sanitize for user-facing message $this->logger->error('License API request failed', [ 'endpoint' => $endpoint, - 'error' => $e->getMessage(), + 'exception_class' => $e::class, + 'error_code' => $e->getCode(), ]); throw new LicenseException( - 'Failed to communicate with license server: ' . $e->getMessage(), + 'Failed to communicate with license server', null, 0, $e @@ -239,7 +253,13 @@ final class SecureLicenseClient implements LicenseClientInterface foreach ($headers as $name => $values) { // HTTP client returns arrays of values; take the first one - $normalized[$name] = is_array($values) ? ($values[0] ?? '') : $values; + // Empty arrays or empty strings should be treated as missing (null) + if (is_array($values)) { + $value = $values[0] ?? null; + $normalized[$name] = ($value !== '' && $value !== null) ? $value : null; + } else { + $normalized[$name] = ($values !== '' && $values !== null) ? $values : null; + } } return $normalized; @@ -298,4 +318,69 @@ final class SecureLicenseClient implements LicenseClientInterface } return $key; } + + /** + * Validate the base URL to prevent SSRF attacks. + * + * @throws \InvalidArgumentException If the URL is invalid or potentially dangerous + */ + private function validateBaseUrl(string $url, bool $allowInsecureHttp): void + { + if ($url === '') { + throw new \InvalidArgumentException('Base URL cannot be empty'); + } + + $parsed = parse_url($url); + if ($parsed === false || !isset($parsed['scheme'], $parsed['host'])) { + throw new \InvalidArgumentException('Invalid base URL format'); + } + + $scheme = strtolower($parsed['scheme']); + $host = strtolower($parsed['host']); + + // Require HTTPS unless explicitly allowed for localhost + if ($scheme !== 'https') { + if ($scheme !== 'http') { + throw new \InvalidArgumentException('Base URL must use HTTP or HTTPS scheme'); + } + $isLocalhost = $host === 'localhost' || $host === '127.0.0.1'; + if (!$allowInsecureHttp && !$isLocalhost) { + throw new \InvalidArgumentException( + 'Base URL must use HTTPS for non-localhost hosts. ' . + 'Set allowInsecureHttp=true to allow HTTP (not recommended for production).' + ); + } + } + + // Resolve hostname and check for private IPs + $ip = gethostbyname($host); + if ($ip !== $host && $this->isPrivateIp($ip)) { + throw new \InvalidArgumentException( + 'Base URL resolves to a private IP address, which is not allowed' + ); + } + } + + /** + * Check if an IP address is in a private range. + */ + private function isPrivateIp(string $ip): bool + { + $ipLong = ip2long($ip); + if ($ipLong === false) { + return false; + } + + foreach (self::PRIVATE_IP_RANGES as $range) { + [$subnet, $bits] = explode('/', $range); + $subnetLong = ip2long($subnet); + $mask = -1 << (32 - (int) $bits); + + if (($ipLong & $mask) === ($subnetLong & $mask)) { + return true; + } + } + + return false; + } } diff --git a/src/Security/ResponseSignature.php b/src/Security/ResponseSignature.php index 98c44f6..78fa25e 100644 --- a/src/Security/ResponseSignature.php +++ b/src/Security/ResponseSignature.php @@ -112,14 +112,19 @@ final class ResponseSignature /** * Derive a unique key from the license key and server secret. * - * Uses HKDF-like key derivation to create a unique key per license. + * Uses RFC 5869 HKDF to create a unique key per license. */ public static function deriveKey(string $licenseKey, string $serverSecret): string { - // Use HKDF expansion with license key as info - $prk = hash_hmac('sha256', $licenseKey, $serverSecret, true); + // Use PHP's native HKDF implementation (RFC 5869) + // - IKM (input keying material): server secret + // - Length: 32 bytes (256 bits for SHA-256) + // - Info: license key (context-specific info) + // - Salt: empty (uses hash-length zero bytes as per RFC 5869) + $binaryKey = hash_hkdf('sha256', $serverSecret, 32, $licenseKey); - return hash_hmac('sha256', $prk . "\x01", $serverSecret); + // Convert to hex for consistent string handling + return bin2hex($binaryKey); } private function buildSignaturePayload(array $responseData, int $timestamp): string @@ -130,6 +135,12 @@ final class ResponseSignature // Create deterministic JSON representation $jsonBody = json_encode($responseData, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + if ($jsonBody === false) { + throw new \RuntimeException( + 'Failed to encode response data for signature verification: ' . json_last_error_msg() + ); + } + // Combine timestamp and body for signature return $timestamp . ':' . $jsonBody; }