Fix OIDC login CSRF by verifying sesskey on callback

[Patryk Mroczko (patmr7)]

No description provided.

[Copilot]

Pull request overview

This PR hardens the OIDC auth-code login flow against CSRF on IdP callbacks by binding the server-side auth_oidc_state record to a browser-held token that will be sent on cross-site form_post responses.

Changes:

• Sets a dedicated auth_oidc_csrf cookie with SameSite=None; Secure; HttpOnly prior to initiating the OIDC authorization request.
• Verifies the callback’s auth_oidc_state.sesskey against the CSRF cookie (falling back to sesskey() when the cookie is absent) and rejects/cleans up state on mismatch.
• Clears the CSRF cookie after successful state validation for both standard auth and admin-consent responses.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (5)

auth/oidc/classes/loginflow/authcode.php:327

• Cookie clearing should match the hardened __Host- cookie name and Path=/ so the CSRF cookie is actually removed and cannot be reintroduced by a Domain cookie from a sibling subdomain.

        setcookie('auth_oidc_csrf', '', [

auth/oidc/classes/loginflow/authcode.php:403

• Cookie clearing should match the hardened __Host- cookie name and Path=/ so the CSRF cookie is actually removed and cannot be reintroduced by a Domain cookie from a sibling subdomain.

        setcookie('auth_oidc_csrf', '', [

auth/oidc/classes/loginflow/authcode.php:239

• The CSRF validation relies on a cookie value. With the current cookie name and path, a sibling-subdomain attacker could potentially "cookie-toss" a Domain cookie named auth_oidc_csrf and influence the value PHP reads, weakening the CSRF binding. Consider switching to an __Host- prefixed cookie (host-only, no Domain) which also requires Path=/ to prevent subdomain injection.

                setcookie('auth_oidc_csrf', sesskey(), [

auth/oidc/classes/loginflow/authcode.php:322

• This reads the CSRF cookie by name; if the cookie is changed to an __Host- prefixed cookie to avoid subdomain cookie injection, this reference also needs to use the new name to keep validation consistent.

This issue also appears in the following locations of the same file:

• line 327
• line 403

        $csrftoken = $_COOKIE['auth_oidc_csrf'] ?? sesskey();

auth/oidc/classes/loginflow/authcode.php:398

• This reads the CSRF cookie by name; if the cookie is hardened using an __Host- prefix to prevent subdomain cookie injection, this reference must use the same cookie name.

        $csrftoken = $_COOKIE['auth_oidc_csrf'] ?? sesskey();

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

auth/oidc/classes/loginflow/authcode.php:411

• On CSRF validation failure, the CSRF cookie is left in place. Clearing it before throwing reduces the lifetime of the exposed sesskey value and helps avoid repeated failures caused by a stale cookie.

        $csrftoken = $_COOKIE['auth_oidc_csrf'] ?? sesskey();
        if (!hash_equals((string)$staterec->sesskey, (string)$csrftoken)) {
            $DB->delete_records('auth_oidc_state', ['id' => $staterec->id]);
            throw new moodle_exception('errorauthunknownstate', 'auth_oidc');
        }

auth/oidc/classes/loginflow/authcode.php:335

• On CSRF validation failure, the CSRF cookie is left in place. Clearing it before throwing reduces the lifetime of the exposed sesskey value and helps avoid repeated failures caused by a stale cookie.

This issue also appears on line 407 of the same file.

        $csrftoken = $_COOKIE['auth_oidc_csrf'] ?? sesskey();
        if (!hash_equals((string)$staterec->sesskey, (string)$csrftoken)) {
            $DB->delete_records('auth_oidc_state', ['id' => $staterec->id]);
            throw new moodle_exception('errorauthunknownstate', 'auth_oidc');
        }

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

auth/oidc/classes/loginflow/authcode.php:443

• Same issue as the admin-consent handler: the code says "expire state" but does not enforce a TTL, and the CSRF verification is conditional. Because the CSRF cookie expires in 5 minutes, callbacks taking longer than that can fail inconsistently depending on when cron last ran. Consider enforcing a TTL via $staterec->timecreated and calling verify_csrf_cookie() unconditionally once the state record is found.

        // Validate and expire state.
        $staterec = $DB->get_record('auth_oidc_state', ['state' => $authparams['state']]);
        if (empty($staterec)) {
            throw new moodle_exception('errorauthunknownstate', 'auth_oidc');
        }

        if (is_https() || !empty($CFG->sslproxy) || !empty($_COOKIE['auth_oidc_csrf'])) {
            $this->verify_csrf_cookie($staterec);
        }

auth/oidc/classes/loginflow/authcode.php:376

• The CSRF verification is conditional here, and the preceding comment says "Validate and expire state" but there is no server-side expiry check. Because the CSRF cookie expires in 5 minutes, callbacks taking longer than that can fail inconsistently depending on when the scheduled cleanup last ran. Consider enforcing a TTL via $staterec->timecreated and calling verify_csrf_cookie() unconditionally once the state record is found.

        if (is_https() || !empty($CFG->sslproxy) || !empty($_COOKIE['auth_oidc_csrf'])) {
            $this->verify_csrf_cookie($staterec);
        }

auth/oidc/classes/loginflow/authcode.php:337

• clear_csrf_cookie() is called unconditionally before the validity check. Because the CSRF cookie is shared per browser session, clearing it on the first successful callback can cause a second in-flight OIDC callback (e.g., two tabs) to fail when the callback uses form_post and the main session cookie is not sent. Consider only clearing the cookie on failure (or leave it to expire) so multiple outstanding state records can still be validated.

        $valid = hash_equals((string) $staterec->sesskey, (string) $csrftoken);

        $this->clear_csrf_cookie();

        if (!$valid) {

auth/oidc/classes/loginflow/authcode.php:323

• The docblock says the CSRF cookie is "Always" cleared, but this is only necessary (and typically desirable) on failure. If you keep the cookie until expiry to support multiple in-flight callbacks, the docblock should reflect that behavior.

     * Verify the state record's sesskey against the CSRF cookie, falling back to the current
     * session's sesskey when the cookie is absent. Always clears the cookie and, on failure,
     * deletes the state record.