diff --git a/src/Http/Controllers/PortalController.php b/src/Http/Controllers/PortalController.php index aeb7e61..c970038 100644 --- a/src/Http/Controllers/PortalController.php +++ b/src/Http/Controllers/PortalController.php @@ -87,6 +87,35 @@ class PortalController extends Controller // ── Captive portal callback (called by UniFi external portal redirect) ─── + /** + * A redirect target that cannot be turned into a phishing or script payload. + * + * Accepts only absolute http/https URLs and site-relative paths. Rejects + * javascript:/data:/vbscript: schemes, and protocol-relative "//host" values which + * redirect() would happily send off-site. Anything rejected falls back to the + * dashboard rather than erroring — the guest is already online at this point, so a + * bad `url` parameter should not look like a failed login. + */ + private function safeRedirectTarget(?string $url): string + { + $url = trim((string) $url); + + if ($url === '' || str_starts_with($url, '//')) { + return '/'; + } + + // Site-relative is always fine. + if (str_starts_with($url, '/')) { + return $url; + } + + $scheme = strtolower((string) parse_url($url, PHP_URL_SCHEME)); + + return in_array($scheme, ['http', 'https'], true) && parse_url($url, PHP_URL_HOST) + ? $url + : '/'; + } + public function captiveCallback(Request $request, UnifiApiClient $unifi) { // UniFi redirects guest to this URL with ?id=&ap=&t=&url=&ssid= @@ -104,6 +133,23 @@ class PortalController extends Controller return response('Missing MAC address', 400); } + // SECURITY: everything identifying the device is request-supplied, and this + // endpoint WRITES network state (authorizeGuest puts a MAC on the mapped VLAN). + // Before this check any signed-in account — a student, a parent — could call + // GET /portal/wifi/callback?mac=&url=… + // and admit a device they do not own, up to max_devices. + // + // A well-formed MAC is the floor, not a fix: it stops the parameter being used + // to smuggle junk into authorizeGuest and into the PortalSession row, but it + // cannot prove the caller owns the device. Properly binding it needs the AP + // handshake — UniFi can sign the redirect, or we ask the controller whether this + // MAC is CURRENTLY associated to $apMac before admitting it. Both need testing + // against real hardware, so they are deliberately not guessed at here. + $clientMac = str_replace('-', ':', $clientMac); + if (! preg_match('/^([0-9a-f]{2}:){5}[0-9a-f]{2}$/', $clientMac)) { + return response('Malformed MAC address', 400); + } + // Find the VLAN mapping for this user's group/OU $mapping = $this->resolveMapping($user); if (! $mapping) { @@ -161,9 +207,16 @@ class PortalController extends Controller 'expires_at' => now()->addMinutes($minutes), ]); - // Redirect to the original URL the user was trying to reach - $redirectUrl = $request->input('url', '/'); - return redirect($redirectUrl); + // Redirect to the original URL the user was trying to reach. + // + // SECURITY: this was `redirect($request->input('url', '/'))` — an unvalidated + // open redirect on an authenticated route, i.e. a phishing primitive wearing + // this school's domain. The whole point of a captive portal is to send someone + // to an ARBITRARY external site they were already trying to reach, so an + // allowlist is not available; what is available is refusing anything that is + // not a plain http(s) URL, which blocks javascript:, data: and protocol-relative + // //evil.example payloads. + return redirect($this->safeRedirectTarget($request->input('url'))); } public function disconnectSession(Request $request, PortalSession $session, UnifiApiClient $unifi)