From 20d3b1a60a69adf8c49ce000f42b50675b5178b0 Mon Sep 17 00:00:00 2001 From: Rory Shanks Date: Sat, 30 Mar 2024 16:50:18 +0100 Subject: [PATCH] Added vfsid and hashChallenge to limit stolen signing key issues --- lib/idp_adapters/googleworkspace.js | 2 + lib/idp_adapters/msgraph.js | 3 +- lib/idp_adapters/none.js | 8 -- lib/idp_adapters/tokenclaims.js | 2 + lib/sso.js | 122 +++++++++++++++++----------- package-lock.json | 6 ++ package.json | 1 + test/e2e/configs/idp_output.json | 2 + 8 files changed, 90 insertions(+), 56 deletions(-) delete mode 100644 lib/idp_adapters/none.js diff --git a/lib/idp_adapters/googleworkspace.js b/lib/idp_adapters/googleworkspace.js index 7485e22..e4ed207 100644 --- a/lib/idp_adapters/googleworkspace.js +++ b/lib/idp_adapters/googleworkspace.js @@ -4,6 +4,7 @@ import {GoogleAuth} from 'google-auth-library'; import { getConfig } from '../../util/config.js'; import Cache from 'cache'; import redisHelper from '../../util/redis.js' +import crypto from 'crypto'; const redisClient = redisHelper.getClient() @@ -49,6 +50,7 @@ async function getUsersAndGroups() { log.info(`Requesting groups for user ${user.primaryEmail}`) const groups = await getUserGroups(client, user.primaryEmail); userData[user.primaryEmail] = { + vfsid: crypto.randomUUID(), displayName: user.name.fullName, givenName: user.name.givenName, preferredLanguage: user.language || 'en', diff --git a/lib/idp_adapters/msgraph.js b/lib/idp_adapters/msgraph.js index 66aacc0..e758cc3 100644 --- a/lib/idp_adapters/msgraph.js +++ b/lib/idp_adapters/msgraph.js @@ -8,6 +8,7 @@ import log from '../../util/logging.js' import fs from 'fs'; import Cache from 'cache'; import redisHelper from '../../util/redis.js' +import crypto from 'crypto'; const redisClient = redisHelper.getClient() @@ -64,7 +65,7 @@ async function getUsersAndGroups(clientId, clientSecret, tenantId) { flatGroups.push(group.displayName) } user.groups = flatGroups - + user.vfsid = crypto.randomUUID() log.debug(`${completedUsers} / ${totalUsers} Finished updating user ${user.id}`) completedUsers++ return user diff --git a/lib/idp_adapters/none.js b/lib/idp_adapters/none.js deleted file mode 100644 index 46003ea..0000000 --- a/lib/idp_adapters/none.js +++ /dev/null @@ -1,8 +0,0 @@ -import log from '../util/logging.js' - -async function runUpdate() { - log.debug("Running idp_update for none") - return {} -} - -export default { runUpdate }; \ No newline at end of file diff --git a/lib/idp_adapters/tokenclaims.js b/lib/idp_adapters/tokenclaims.js index 424d6ed..62f0ffc 100644 --- a/lib/idp_adapters/tokenclaims.js +++ b/lib/idp_adapters/tokenclaims.js @@ -2,6 +2,7 @@ import log from '../../util/logging.js' import Cache from 'cache'; import redisHelper from '../../util/redis.js' import { getConfig } from '../../util/config.js'; +import crypto from 'crypto'; const redisClient = redisHelper.getClient() @@ -34,6 +35,7 @@ async function addNewUserFromClaims(claims) { var userId = claims[currentConfig.idp_provider_user_id_claim] var userData = { + vfsid: crypto.randomUUID(), id: userId, mail: claims.email, ...claims diff --git a/lib/sso.js b/lib/sso.js index 66afe66..4b49953 100644 --- a/lib/sso.js +++ b/lib/sso.js @@ -9,6 +9,7 @@ import { checkAuthHeader } from './token-auth.js' import crypto from 'crypto' import errorpages from '../util/errorpage.js' import idp from './idp.js' +import bcrypt from 'bcryptjs' function addParamsToUrl(baseUrl, params) { const url = new URL(baseUrl); @@ -137,6 +138,22 @@ async function setSessionCookie(req, res) { return } + let userId = decoded.userId + + // Now we compare the challenge hash from the token with the challenge from the IdP + // To limit impact of user impersonation in the event of a stolen signing key + let userFromIdp = await idp.getUserById(userId) + let veriflowUserSpecificSecurityChallenge = userFromIdp.vfsid + let hashToCompare = decoded.challengeHash + + let challengeHashResult = await bcrypt.compare(veriflowUserSpecificSecurityChallenge, hashToCompare) + + if (!userFromIdp || !veriflowUserSpecificSecurityChallenge || !hashToCompare || !challengeHashResult) { + log.error({ error: "User failed challengeHash", token: decoded }) + req.session.destroy() + throw new Error("Challenge hash failed") + } + req.session.loggedin = true req.session.userId = decoded.userId; await addSessionDetails(req) @@ -196,7 +213,7 @@ async function redirectToSsoProvider(req, res) { res.status(400).send(html) return } - // FIXME: Verify that redirect URL is allowed in config + var redirectToken = await decodeJWT(req.query.token) if (!redirectToken) { @@ -208,34 +225,16 @@ async function redirectToSsoProvider(req, res) { var currentConfig = getConfig() var redirectBasePath = getRedirectBasepath() - if (req.session.loggedin) { - await addSessionDetails(req) - let jwtPayload = { - protocol: redirectToken.protocol, - host: redirectToken.host, - path: redirectToken.path, - query: redirectToken.query, - userId: req.session.userId, - cookieExpires: req.session.cookie.expires, - parentSession: req.sessionID - } - var signedJwt = await createJWT(jwtPayload) - - var redirectProtocol = redirectToken.protocol - var redirectHost = redirectToken.host - var redirectPath = redirectBasePath + "/set" - var baseUrl = `${redirectProtocol}://${redirectHost}${redirectPath}` - var redirectParams = { - token: signedJwt - } - var redirectUrl = addParamsToUrl(baseUrl, redirectParams) + // FIXME: Verify that redirect URL is allowed in config + req.session.redirect = redirectToken - res.redirect(redirectUrl); - return; + // If the user is already logged in to veriflow, do not re-initiate the IdP login flow, rather + // just authenticate the user and send them on their way + if (req.session.loggedin) { + await handleRedirectToSetEndpoint(req, res) + return } - req.session.redirect = redirectToken - var oauthIssuer = await Issuer.discover(currentConfig.idp_provider_url) var oauth_client = new oauthIssuer.Client({ @@ -294,31 +293,20 @@ async function verifySsoCallback(req, res) { await idp.addNewUserFromClaims(userClaims) - req.session.loggedin = true; - req.session.userId = userIdClaim; - await addSessionDetails(req) + let userFromIdp = await idp.getUserById(userIdClaim) - let jwtPayload = { - protocol: req.session.redirect.protocol, - host: req.session.redirect.host, - path: req.session.redirect.path, - query: req.session.redirect.query, - userId: userIdClaim, - cookieExpires: req.session.cookie.expires, - parentSession: req.sessionID + if (!userFromIdp) { + log.warn({ error: "User does not exist in IdP", claims: userClaims }) + req.session.destroy() + var html = await errorpages.renderErrorPage(403, "ERR_USER_NOT_IN_IDP", req) + res.status(403).send(html) + return } - var signedJwt = await createJWT(jwtPayload) - var redirectProtocol = req.session.redirect.protocol - var redirectHost = req.session.redirect.host - var redirectPath = redirectBasePath + "/set" - var baseUrl = `${redirectProtocol}://${redirectHost}${redirectPath}` - var redirectParams = { - token: signedJwt - } - var redirectUrl = addParamsToUrl(baseUrl, redirectParams) + req.session.loggedin = true; + req.session.userId = userIdClaim; - res.redirect(redirectUrl); + await handleRedirectToSetEndpoint(req, res) } catch (error) { log.error({ message: "Unknown error occoured in verifySsoCallback", context: { error: error.message, trace: error.stack } }) @@ -327,6 +315,46 @@ async function verifySsoCallback(req, res) { } } +async function handleRedirectToSetEndpoint(req, res) { + var redirectBasePath = getRedirectBasepath() + let userId = req.session.userId + let userFromIdp = await idp.getUserById(userId) + let veriflowUserSpecificSecurityChallenge = userFromIdp?.vfsid + + if (!userFromIdp || !veriflowUserSpecificSecurityChallenge) { + throw new Error("Unable to get user from IdP in handleRedirectToSetEndpoint") + } + + await addSessionDetails(req) + + // Generate a hash of the vfsid of the user stored in redis, to limit issue of user impersonation + // when the signing key is stolen + let challengeHash = await bcrypt.hash(veriflowUserSpecificSecurityChallenge, 5) + + let jwtPayload = { + protocol: req.session.redirect.protocol, + host: req.session.redirect.host, + path: req.session.redirect.path, + query: req.session.redirect.query, + userId: userId, + cookieExpires: req.session.cookie.expires, + parentSession: req.sessionID, + challengeHash: challengeHash + } + var signedJwt = await createJWT(jwtPayload) + + var redirectProtocol = req.session.redirect.protocol + var redirectHost = req.session.redirect.host + var redirectPath = redirectBasePath + "/set" + var baseUrl = `${redirectProtocol}://${redirectHost}${redirectPath}` + var redirectParams = { + token: signedJwt + } + var redirectUrl = addParamsToUrl(baseUrl, redirectParams) + + res.redirect(redirectUrl); +} + async function addSessionDetails(req) { req.session.details = { user_agent: req.get("user-agent"), diff --git a/package-lock.json b/package-lock.json index cc563be..2073232 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,6 +11,7 @@ "dependencies": { "@microsoft/microsoft-graph-client": "^3.0.7", "axios": "^1.6.8", + "bcryptjs": "^2.4.3", "bossbat": "^1.2.0", "cache": "^3.0.0", "chokidar": "^3.6.0", @@ -262,6 +263,11 @@ } ] }, + "node_modules/bcryptjs": { + "version": "2.4.3", + "resolved": "https://registry.npmjs.org/bcryptjs/-/bcryptjs-2.4.3.tgz", + "integrity": "sha512-V/Hy/X9Vt7f3BbPJEi8BdVFMByHi+jNXrYkW3huaybV/kQ0KJg0Y6PkEMbn+zeT+i+SiKZ/HMqJGIIt4LZDqNQ==" + }, "node_modules/bignumber.js": { "version": "9.1.2", "resolved": "https://registry.npmjs.org/bignumber.js/-/bignumber.js-9.1.2.tgz", diff --git a/package.json b/package.json index cdeb651..db2ea3d 100644 --- a/package.json +++ b/package.json @@ -20,6 +20,7 @@ "dependencies": { "@microsoft/microsoft-graph-client": "^3.0.7", "axios": "^1.6.8", + "bcryptjs": "^2.4.3", "bossbat": "^1.2.0", "cache": "^3.0.0", "chokidar": "^3.6.0", diff --git a/test/e2e/configs/idp_output.json b/test/e2e/configs/idp_output.json index a85e3e4..80262ea 100644 --- a/test/e2e/configs/idp_output.json +++ b/test/e2e/configs/idp_output.json @@ -1,6 +1,7 @@ { "test@veriflow.dev": { "displayName": "Veriflow Test User", + "vfsid": "dce03eb2-5f90-4ada-9966-4da4319a3c39", "givenName": "Test", "preferredLanguage": "en", "surname": "User", @@ -12,6 +13,7 @@ ] }, "denied@veriflow.dev": { + "vfsid": "9e4990a0-c9d3-4b1f-acfd-ba1591f8003e", "displayName": "Veriflow Denied User", "givenName": "Denied", "preferredLanguage": "en",