Ensure userId that generated challenge is the same as userId verifying it

Author: EC2 Default User

README.md

@@ -195,18 +195,13 @@ This package exposes the `MFA.generateChallenge` and `MFA.verifyChallenge` metho
 ````
 Meteor.methods({
   "start:disableMFA":function () {
-    let challenge = MFA.generateChallenge(this.userId, "disableMFA");
+    let challenge = MFA.generateChallenge(this.userId, "disableMFA", MFA.generateConnectionHash(this.connection));
     return challenge;
   },
   
   "complete:disableMFA":function (solvedChallenge) {
-    let isValid = MFA.verifyChallenge("disableMFA", solvedChallenge);
-    
-    if(!isValid) {
-      throw new Meteor.Error(404, "MFA Challenge Failed");
-    }
-    
-    MFA.disableMFA(this.userId);
+    let userId = MFA.verifyChallenge(this.userId, "disableMFA", MFA.generateConnectionHash(this.connection), solvedChallenge);
+    MFA.disableMFA(userId);
   }  
 });
 ````
@@ -381,15 +376,23 @@ Disables MFA for a user. This is an internal method. If you'd like the user to a
 #### MFA.disablePasswordless(userId)
 Disables passwordless for a user. When this method is called, MFA will remain enabled. To disable passwordless and MFA, call `MFA.disableMFA`.
 
-#### MFA.generateChallenge(userId, type)
-Generates a challenge. This is then sent to the client and passed into `MFA.solveChallenge()`
+#### MFA.generateChallenge(userId, type, connectionHash)
+Generates a challenge. This is then sent to the client and passed into `MFA.solveChallenge()`. See connectionHash below.
+
+#### MFA.verifyChallenge(userId, type, connectionHash, solvedChallenge)
+Verifies the solvedChallenge (created by `MFA.solveChallenge` on client). Throws an error on failiure, returns the userId that generated the challenge on success.
 
-#### MFA.verifyChallenge(type, solvedChallenge)
-Verifies the solvedChallenge (created by `MFA.solveChallenge` on client).
+The userId argument is used to verify that the user that you generated the challenge for is the same as the user you are verifying the challenge for, so you should typically pass in this.userId, or however you can get the current user id. In situations where you are performing this kind of verification in another way, set userId to null.
 
 #### MFA.enableDebug()
 Enables debugging
 
+#### MFA.generateConnectionHash(connection)
+Creates a connection hash per the config.
+
+#### connectionHash
+The connection hash ensures that the same device that creates the challenge is the one that verifies/uses it. When using `MFA.generateChallenge` or `MFA.verifyChallenge`, you can use the `MFA.generateConnectionHash` method to create one, or if you do not need it you can pass an empty string.
+
 ### Config Options
 
 **mfaDetailsField *String* (default: "mfa")** The field where the mfa status object is stored (this is the field you can publish to tell whether a user has enabled)

mfa-server.js

@@ -62,7 +62,8 @@ let _defaults = {
     allowU2FAuthorization:true,
     authorizationDisabledMethods:[],
     keepChallenges:false,
-    passwordless:false
+    passwordless:false,
+    ignoreNullUserIdVerifyChallengeTypes:[]
 };
 
 let config = Object.assign({}, _defaults);
@@ -92,13 +93,17 @@ let isExpired = function (expiryDate) {
     return new Date() >= expiryDate;
 };
 
-let generateChallenge = function (userId, type, challengeConnectionHash) {
+const generateChallenge = function (userId, type, challengeConnectionHash) {
     let user = Meteor.users.findOne({_id:userId}, {fields:{"services.mfapublickey":1, "services.mfaenabled":1, "services.mfamethod":1}});
 
     if(!user || !user.services.mfaenabled) {
         throw new Meteor.Error(400);
     }
 
+    if(!challengeConnectionHash && !(challengeConnectionHash === null && type === "login")) {
+        throw new Error("MFA.generateChallenge: Missing connectionHash. If you don't want to use a hash, pass an empty string when generating and validating a chalenge.");
+    }
+    
     check(user.services.mfamethod, Match.OneOf(...mfaMethods));
 
     let response, challengeData;
@@ -203,6 +208,27 @@ let u2fAuthorizationIsValid = function (authorization) {
     return authorization instanceof Object && !authorization.used && !isExpired(authorization.expires) && authorization.authenticated === true;
 };
 
+const nullUserIdVerifyChallengeError = type =>  `
+WARNING: you are passing null as userId for verifyChallenge
+This means that verifyChallenge will not ensure that the current userId
+is the same as the userId that the challenge was generated for.
+
+This can be a major security vulnerability, as a challenge solved by one
+user can be used as a solved challenge for a different user.
+
+You should only use null for userId if you are preventing this kind of
+vulnerability yourself. For example, using the userId value returned by
+MFA.verifyChallenge().
+
+To stop this warning from appearing, set
+config.ignoreNullUserIdVerifyChallengeTypes to an array that includes ${type}:
+
+MFA.setConfig({ignoreNullUserIdVerifyChallengeTypes:["${type}"]});
+
+This error will be thrown in development, but will only be a warning in
+production.
+`;
+
 let invalidateU2FAuthorization = function (authorizationId) {
     MFAAuthorizations.update({_id:authorizationId}, {$set:{
         used:true,
@@ -220,21 +246,35 @@ const invalidateChallenge = function (challengeId) {
     }
 };
 
-const verifyChallenge = function (type, params) {
+const verifyChallenge = function (userId, type, challengeConnectionHash, params) {
     let {challengeId, challengeSecret} = params;
     check(challengeId, String);
     check(challengeSecret, String);
 
-    let challengeConnectionHash = createConnectionHash(this.connection);
+    if(!challengeConnectionHash) {
+        challengeConnectionHash = createConnectionHash(this.connection);
+    }
     let challengeObj = MFAChallenges.findOne({_id:challengeId});
 
+    if(userId === null && type !== "login") {
+        let err = nullUserIdVerifyChallengeError(type);
+        
+        if(Meteor.isDevelopment) {
+            throw new Error(err);
+        }
+        else {
+            console.warn(err);
+        }
+    }
+    
     if(
         !challengeObj
         || challengeObj.type !== type
         || challengeObj.connectionHash !== challengeConnectionHash
         || challengeObj.challengeSecret !== challengeSecret
         || challengeObj.expires < new Date()
         || challengeObj.used === true
+        || (challengeObj.userId !== userId && userId !== null)
     ) {
         throw new Meteor.Error(404);
     }
@@ -488,7 +528,7 @@ Meteor.methods({
         check(params.challengeId, String);
         check(params.challengeSecret, String);
 
-        let userId = verifyChallenge("login", params);
+        let userId = verifyChallenge(null, "login", createConnectionHash(this.connection), params);
 
         return Accounts._attemptLogin(this, 'login', '', {
           type: 'mfa',
@@ -544,7 +584,7 @@ Meteor.methods({
             throw new Meteor.Error(400);
         }
 
-        verifyChallenge(("authorize:" + authorizationId), solvedU2FChallenge);
+        verifyChallenge(this.userId, ("authorize:" + authorizationId), createConnectionHash(this.connection), solvedU2FChallenge);
 
         let code = generateCode();
 
@@ -584,7 +624,7 @@ Accounts.validateLoginAttempt(options => {
             let params = options.methodArguments[2];
 
             try {
-                verifyChallenge("resetPassword", params);
+                verifyChallenge(options.user._id, "resetPassword", createConnectionHash(options.connection), params);
                 return true;
             }
             catch(e) {
@@ -607,4 +647,4 @@ let getCurrentTOTP = function (secret) {
     return authenticator.generate(secret);
 };
 
-export default { disablePasswordless, verifyChallenge, getCurrentTOTP, enableOTP, setConfig, setStrings, disableMFA, generateChallenge, verifyAssertion, verifyAttestation:verifyAssertion };
+export default { generateConnectionHash:createConnectionHash, disablePasswordless, verifyChallenge, getCurrentTOTP, enableOTP, setConfig, setStrings, disableMFA, generateChallenge, verifyAssertion, verifyAttestation:verifyAssertion };

package.js

@@ -1,6 +1,6 @@
 Package.describe({
   name: 'ndev:mfa',
-  version: '0.0.14',
+  version: '0.1.0',
   summary: 'Multi Factor Authentication and Passwordless (supporting U2F, TOTP, and OTP)',
   git: 'https://github.com/TheRealNate/meteor-mfa',
   documentation: 'README.md'