From 81aeed59065b7bbd81c116b1f00efd7104e4e63f Mon Sep 17 00:00:00 2001 From: Dan G Date: Sat, 12 Oct 2024 21:08:02 +0100 Subject: [PATCH] protect nonce by hashing, add TODO re sig verification --- api.planx.uk/modules/auth/controller.ts | 1 + api.planx.uk/modules/auth/middleware.ts | 22 ++++++++++++++---- .../modules/auth/strategy/microsoft-oidc.ts | 23 +++++++++++++++---- api.planx.uk/server.ts | 9 ++++---- 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/api.planx.uk/modules/auth/controller.ts b/api.planx.uk/modules/auth/controller.ts index a03fdc4b17..016c9efa64 100644 --- a/api.planx.uk/modules/auth/controller.ts +++ b/api.planx.uk/modules/auth/controller.ts @@ -39,6 +39,7 @@ function setJWTCookie(returnTo: string, res: Response, req: Request) { maxAge: new Date( new Date().setFullYear(new Date().getFullYear() + 1), ).getTime(), + // jwt/auth cookies are mostly sent between API and editor, but pizzas rely on staging API sameSite: "none", secure: true, }; diff --git a/api.planx.uk/modules/auth/middleware.ts b/api.planx.uk/modules/auth/middleware.ts index 2443f0dace..b280b2b135 100644 --- a/api.planx.uk/modules/auth/middleware.ts +++ b/api.planx.uk/modules/auth/middleware.ts @@ -8,7 +8,7 @@ import type { Authenticator } from "passport"; import type { RequestHandler } from "http-proxy-middleware"; import type { Role } from "@opensystemslab/planx-core/types"; import { AsyncLocalStorage } from "async_hooks"; -import type { Request } from "express"; +import type { CookieOptions, Request } from "express"; export const userContext = new AsyncLocalStorage<{ user: Express.User }>(); @@ -136,15 +136,27 @@ export const getMicrosoftAuthHandler = ( return (req, res, next) => { req.session!.returnTo = req.get("Referrer"); - // generate a nonce to enable us to validate the response from OP + // generate a nonce to enable us to validate the response from OP (mitigates against CSRF attacks) const nonce = generators.nonce(); console.debug(`Generated a nonce: %s`, nonce); - req.session!.nonce = nonce; - // @ts-expect-error (method not typed to accept nonce, but it does pass it to the strategy) + // we hash the nonce to avoid sending it plaintext over the wire in our auth request + const hash = crypto.createHash("sha256").update(nonce).digest("hex"); + console.debug(`Hashed nonce: %s`, hash); + + // we store the original nonce in a short-lived, httpOnly but cross-site cookie + const httpOnlyCookieOptions: CookieOptions = { + maxAge: 15 * 60 * 1000, // 15 mins + sameSite: "none", + httpOnly: true, + secure: true, + }; + res.cookie("ms-oidc-nonce", nonce, httpOnlyCookieOptions); + + // @ts-expect-error (method not typed to accept nonce, but it does include it in the request) return passport.authenticate("microsoft-oidc", { prompt: "select_account", - nonce, + nonce: hash, })(req, res, next); }; }; diff --git a/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts b/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts index cc9c1dc015..82fcc437b1 100644 --- a/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts +++ b/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts @@ -1,3 +1,4 @@ +import crypto from "crypto"; import type { Client, ClientMetadata, @@ -41,18 +42,30 @@ export const getMicrosoftOidcStrategy = (client: Client): Strategy => { }; const verifyCallback: StrategyVerifyCallbackReq = async ( - req: Http.IncomingMessageWithSession, + req: Http.IncomingMessageWithCookies, tokenSet, done, ): Promise => { // TODO: use tokenSet.state to pass the redirectTo query param through the auth flow + // TODO: validate id_token sig with the public key from the jwks_uri (...v2.0/keys) const claims: IdTokenClaims = tokenSet.claims(); - const email = claims.email; - const returned_nonce = claims.nonce; - if (returned_nonce != req.session?.nonce) { - return done(new Error("Returned nonce does not match session nonce")); + // ensure the response is authentic by comparing nonce + const returned_nonce = claims.nonce; + if (!req.cookies || !req.cookies["ms-oidc-nonce"]) { + return done(new Error("No nonce found in appropriate cookie")); } + const original_nonce = req.cookies["ms-oidc-nonce"]; + const hash = crypto.createHash("sha256").update(original_nonce).digest("hex"); + if (returned_nonce != hash) { + return done( + new Error( + "Returned nonce does not match nonce sent with original request", + ), + ); + } + + const email = claims.email; if (!email) { return done(new Error("Unable to authenticate without email")); } diff --git a/api.planx.uk/server.ts b/api.planx.uk/server.ts index 04730a5be0..39062dcafc 100644 --- a/api.planx.uk/server.ts +++ b/api.planx.uk/server.ts @@ -112,7 +112,8 @@ assert(process.env.UNIFORM_SUBMISSION_URL); // needed for storing original URL to redirect to in login flow app.use( cookieSession({ - maxAge: 24 * 60 * 60 * 100, + // we don't need session to persist for long - it's only required for auth flow + maxAge: 2 * 60 * 60 * 1000, // 2hrs name: "session", secret: process.env.SESSION_SECRET, }), @@ -198,9 +199,9 @@ declare global { } namespace Http { - interface IncomingMessageWithSession extends IncomingMessage { - session?: { - nonce: string; + interface IncomingMessageWithCookies extends IncomingMessage { + cookies?: { + "ms-oidc-nonce": string; }; } }