Added vfsid and hashChallenge to limit stolen signing key issues (#35)

rorylshanks · Mar 30, 2024 · 9363d57 · 9363d57
1 parent d3f0c52
commit 9363d57
Show file tree

Hide file tree

Showing 8 changed files with 90 additions and 56 deletions.
diff --git 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
@@ -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
diff --git 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
@@ -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
diff --git 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
@@ -1,6 +1,7 @@
 {
     "[email protected]": {
         "displayName": "Veriflow Test User",
+        "vfsid": "dce03eb2-5f90-4ada-9966-4da4319a3c39",
         "givenName": "Test",
         "preferredLanguage": "en",
         "surname": "User",
@@ -12,6 +13,7 @@
         ]
     },
     "[email protected]": {
+        "vfsid": "9e4990a0-c9d3-4b1f-acfd-ba1591f8003e",
         "displayName": "Veriflow Denied User",
         "givenName": "Denied",
         "preferredLanguage": "en",