From bc3f415647d44304e9a259506667312fdc316288 Mon Sep 17 00:00:00 2001 From: jwed Date: Tue, 28 Jul 2026 16:32:32 -0400 Subject: [PATCH] SECURITY: the captive-portal callback admitted arbitrary MACs and redirected anywhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while triaging every authenticated route that carries no authorisation middleware. Of 91 such routes this was the ONLY one that proved nothing: 35 scope to $request->user(), 51 check ownership in the controller, 4 carry a credential in the URL. TWO DEFECTS. 1. Arbitrary device admission. Everything identifying the device is request-supplied — $clientMac = $request->input('mac', $request->input('id', '')) — and this endpoint WRITES network state: authorizeGuest() puts a MAC on the mapped VLAN and a PortalSession row is created. So any signed-in account, a student or a parent included, could call GET /portal/wifi/callback?mac=&url=… and admit that device, up to the mapping's max_devices. No signature, no nonce, no check that the MAC was even present on the network. Fixed HALFWAY, on purpose. A well-formed MAC is now required, which stops the parameter smuggling junk into authorizeGuest and into the session row — but it cannot prove the caller owns the device, and the comment says so rather than implying the hole is closed. The likely proper fix is already in this codebase: UnifiApiClient::isClientConnected() exists and is used a few lines below for stale-session cleanup, so requiring the MAC to be currently associated would bind it. I did NOT add it, because if a pre-auth guest reports as not-connected that check breaks WiFi onboarding for the entire school, and there is no hardware here to test against. It needs one deliberate test on real APs. The alternative is having UniFi sign the redirect. 2. Open redirect. `return redirect($request->input('url', '/'))`, unvalidated, on an authenticated route — a phishing primitive serving from this school's own domain. A captive portal exists to forward people to an ARBITRARY external site they were already trying to reach, so an allowlist is not available. safeRedirectTarget() instead accepts only absolute http/https URLs and site-relative paths, rejecting javascript:, data: and protocol-relative "//evil.example" values that redirect() would otherwise follow off-site. A rejected value falls back to '/' rather than erroring: the guest is already online by then, and a bad `url` should not look like a failed login. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RDA3fQbsa3gaHCFoV1Q8Gg --- src/Http/Controllers/PortalController.php | 59 +++++++++++++++++++++-- 1 file changed, 56 insertions(+), 3 deletions(-) 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)