support both the incorrect and correct MAC methods

also do some refactoring to make it easier to support choices in the other
methods in the future

package.json

@@ -82,7 +82,7 @@
     "matrix-mock-request": "^1.2.2",
     "mocha": "^5.2.0",
     "mocha-jenkins-reporter": "^0.4.0",
-    "olm": "https://matrix.org/packages/npm/olm/olm-3.1.0-pre1.tgz",
+    "olm": "https://matrix.org/packages/npm/olm/olm-3.1.0-pre3.tgz",
     "rimraf": "^2.5.4",
     "source-map-support": "^0.4.11",
     "sourceify": "^0.1.0",

spec/unit/crypto/verification/sas.spec.js

@@ -58,105 +58,143 @@ describe("SAS verification", function() {
         expect(spy).toHaveBeenCalled();
     });
 
-    it("should verify a key", async function() {
-        const [alice, bob] = await makeTestClients(
-            [
-                {userId: "@alice:example.com", deviceId: "Osborne2"},
-                {userId: "@bob:example.com", deviceId: "Dynabook"},
-            ],
-            {
-                verificationMethods: [verificationMethods.SAS],
-            },
-        );
-
-        alice.setDeviceVerified = expect.createSpy();
-        alice.getDeviceEd25519Key = () => {
-            return "alice+base64+ed25519+key";
-        };
-        alice.getStoredDevice = () => {
-            return DeviceInfo.fromStorage(
-                {
-                    keys: {
-                        "ed25519:Dynabook": "bob+base64+ed25519+key",
-                    },
-                },
-                "Dynabook",
-            );
-        };
-        alice.downloadKeys = () => {
-            return Promise.resolve();
-        };
-
-        bob.setDeviceVerified = expect.createSpy();
-        bob.getStoredDevice = () => {
-            return DeviceInfo.fromStorage(
-                {
-                    keys: {
-                        "ed25519:Osborne2": "alice+base64+ed25519+key",
-                    },
-                },
-                "Osborne2",
-            );
-        };
-        bob.getDeviceEd25519Key = () => {
-            return "bob+base64+ed25519+key";
-        };
-        bob.downloadKeys = () => {
-            return Promise.resolve();
-        };
-
+    describe("verification", function() {
+        let alice;
+        let bob;
         let aliceSasEvent;
         let bobSasEvent;
+        let aliceVerifier;
+        let bobPromise;
 
-        const bobPromise = new Promise((resolve, reject) => {
-            bob.on("crypto.verification.start", (verifier) => {
-                verifier.on("show_sas", (e) => {
-                    if (!e.sas.emoji || !e.sas.decimal) {
-                        e.cancel();
-                    } else if (!aliceSasEvent) {
-                        bobSasEvent = e;
-                    } else {
-                        try {
-                            expect(e.sas).toEqual(aliceSasEvent.sas);
-                            e.confirm();
-                            aliceSasEvent.confirm();
-                        } catch (error) {
-                            e.mismatch();
-                            aliceSasEvent.mismatch();
+        beforeEach(async function() {
+            [alice, bob] = await makeTestClients(
+                [
+                    {userId: "@alice:example.com", deviceId: "Osborne2"},
+                    {userId: "@bob:example.com", deviceId: "Dynabook"},
+                ],
+                {
+                    verificationMethods: [verificationMethods.SAS],
+                },
+            );
+
+            alice.setDeviceVerified = expect.createSpy();
+            alice.getDeviceEd25519Key = () => {
+                return "alice+base64+ed25519+key";
+            };
+            alice.getStoredDevice = () => {
+                return DeviceInfo.fromStorage(
+                    {
+                        keys: {
+                            "ed25519:Dynabook": "bob+base64+ed25519+key",
+                        },
+                    },
+                    "Dynabook",
+                );
+            };
+            alice.downloadKeys = () => {
+                return Promise.resolve();
+            };
+
+            bob.setDeviceVerified = expect.createSpy();
+            bob.getStoredDevice = () => {
+                return DeviceInfo.fromStorage(
+                    {
+                        keys: {
+                            "ed25519:Osborne2": "alice+base64+ed25519+key",
+                        },
+                    },
+                    "Osborne2",
+                );
+            };
+            bob.getDeviceEd25519Key = () => {
+                return "bob+base64+ed25519+key";
+            };
+            bob.downloadKeys = () => {
+                return Promise.resolve();
+            };
+
+            aliceSasEvent = null;
+            bobSasEvent = null;
+
+            bobPromise = new Promise((resolve, reject) => {
+                bob.on("crypto.verification.start", (verifier) => {
+                    verifier.on("show_sas", (e) => {
+                        if (!e.sas.emoji || !e.sas.decimal) {
+                            e.cancel();
+                        } else if (!aliceSasEvent) {
+                            bobSasEvent = e;
+                        } else {
+                            try {
+                                expect(e.sas).toEqual(aliceSasEvent.sas);
+                                e.confirm();
+                                aliceSasEvent.confirm();
+                            } catch (error) {
+                                e.mismatch();
+                                aliceSasEvent.mismatch();
+                            }
                         }
-                    }
+                    });
+                    resolve(verifier);
                 });
-                resolve(verifier);
+            });
+
+            aliceVerifier = alice.beginKeyVerification(
+                verificationMethods.SAS, bob.getUserId(), bob.deviceId,
+            );
+            aliceVerifier.on("show_sas", (e) => {
+                if (!e.sas.emoji || !e.sas.decimal) {
+                    e.cancel();
+                } else if (!bobSasEvent) {
+                    aliceSasEvent = e;
+                } else {
+                    try {
+                        expect(e.sas).toEqual(bobSasEvent.sas);
+                        e.confirm();
+                        bobSasEvent.confirm();
+                    } catch (error) {
+                        e.mismatch();
+                        bobSasEvent.mismatch();
+                    }
+                }
             });
         });
 
-        const aliceVerifier = alice.beginKeyVerification(
-            verificationMethods.SAS, bob.getUserId(), bob.deviceId,
-        );
-        aliceVerifier.on("show_sas", (e) => {
-            if (!e.sas.emoji || !e.sas.decimal) {
-                e.cancel();
-            } else if (!bobSasEvent) {
-                aliceSasEvent = e;
-            } else {
-                try {
-                    expect(e.sas).toEqual(bobSasEvent.sas);
-                    e.confirm();
-                    bobSasEvent.confirm();
-                } catch (error) {
-                    e.mismatch();
-                    bobSasEvent.mismatch();
-                }
-            }
+        it("should verify a key", async function() {
+            await Promise.all([
+                aliceVerifier.verify(),
+                bobPromise.then((verifier) => verifier.verify()),
+            ]);
+            expect(alice.setDeviceVerified)
+                .toHaveBeenCalledWith(bob.getUserId(), bob.deviceId);
+            expect(bob.setDeviceVerified)
+                .toHaveBeenCalledWith(alice.getUserId(), alice.deviceId);
+        });
+
+        it("should be able to verify using the old MAC", async function() {
+            // pretend that Alice can only understand the old (incorrect) MAC,
+            // and make sure that she can still verify with Bob
+            const origSendToDevice = alice.sendToDevice;
+            alice.sendToDevice = function(type, map) {
+                if (type === "m.key.verification.start") {
+                    // Note: this modifies not only the message that Bob
+                    // receives, but also the copy of the message that Alice
+                    // has, since it is the same object.  If this does not
+                    // happen, the verification will fail due to a hash
+                    // commitment mismatch.
+                    map[bob.getUserId()][bob.deviceId]
+                        .message_authentication_codes = ['hmac-sha256'];
+                }
+                return origSendToDevice.call(this, type, map);
+            };
+            await Promise.all([
+                aliceVerifier.verify(),
+                bobPromise.then((verifier) => verifier.verify()),
+            ]);
+            expect(alice.setDeviceVerified)
+                .toHaveBeenCalledWith(bob.getUserId(), bob.deviceId);
+            expect(bob.setDeviceVerified)
+                .toHaveBeenCalledWith(alice.getUserId(), alice.deviceId);
         });
-        await Promise.all([
-            aliceVerifier.verify(),
-            bobPromise.then((verifier) => verifier.verify()),
-        ]);
-        expect(alice.setDeviceVerified)
-            .toHaveBeenCalledWith(bob.getUserId(), bob.deviceId);
-        expect(bob.setDeviceVerified)
-            .toHaveBeenCalledWith(alice.getUserId(), alice.deviceId);
     });
 
     it("should send a cancellation message on error", async function() {

src/crypto/verification/SAS.js

@@ -143,17 +143,44 @@ function generateEmojiSas(sasBytes) {
     return emojis.map((num) => emojiMapping[num]);
 }
 
+const sasGenerators = {
+    decimal: generateDecimalSas,
+    emoji: generateEmojiSas,
+};
+
 function generateSas(sasBytes, methods) {
     const sas = {};
-    if (methods.includes("decimal")) {
-        sas["decimal"] = generateDecimalSas(sasBytes);
-    }
-    if (methods.includes("emoji")) {
-        sas["emoji"] = generateEmojiSas(sasBytes);
+    for (const method of methods) {
+        if (method in sasGenerators) {
+            sas[method] = sasGenerators[method](sasBytes);
+        }
     }
     return sas;
 }
 
+const macMethods = {
+    "hmac-sha256-bits": "calculate_mac",
+    "hmac-sha256": "calculate_mac_long_kdf",
+};
+
+/* lists of algorithms/methods that are supported.  The key agreement, hashes,
+ * and MAC lists should be sorted in order of preference (most preferred
+ * first).
+ */
+const KEY_AGREEMENT_LIST = ["curve25519"];
+const HASHES_LIST = ["sha256"];
+const MAC_LIST = ["hmac-sha256-bits", "hmac-sha256"];
+const SAS_LIST = Object.keys(sasGenerators);
+
+const KEY_AGREEMENT_SET = new Set(KEY_AGREEMENT_LIST);
+const HASHES_SET = new Set(HASHES_LIST);
+const MAC_SET = new Set(MAC_LIST);
+const SAS_SET = new Set(SAS_LIST);
+
+function intersection(anArray, aSet) {
+    return anArray instanceof Array ? anArray.filter(x => aSet.has(x)) : [];
+}
+
 /**
  * @alias module:crypto/verification/SAS
  * @extends {module:crypto/verification/Base}
@@ -181,11 +208,11 @@ export default class SAS extends Base {
         const initialMessage = {
             method: SAS.NAME,
             from_device: this._baseApis.deviceId,
-            key_agreement_protocols: ["curve25519"],
-            hashes: ["sha256"],
-            message_authentication_codes: ["hmac-sha256"],
+            key_agreement_protocols: KEY_AGREEMENT_LIST,
+            hashes: HASHES_LIST,
+            message_authentication_codes: MAC_LIST,
             // FIXME: allow app to specify what SAS methods can be used
-            short_authentication_string: ["decimal", "emoji"],
+            short_authentication_string: SAS_LIST,
             transaction_id: this.transactionId,
         };
         this._sendToDevice("m.key.verification.start", initialMessage);
@@ -193,19 +220,19 @@ export default class SAS extends Base {
 
         let e = await this._waitForEvent("m.key.verification.accept");
         let content = e.getContent();
-        if (!(content.key_agreement_protocol === "curve25519"
-              && content.hash === "sha256"
-              && content.message_authentication_code === "hmac-sha256"
-              && content.short_authentication_string instanceof Array
-              && (content.short_authentication_string.includes("decimal")
-                  || content.short_authentication_string.includes("emoji")))) {
+        const sasMethods
+              = intersection(content.short_authentication_string, SAS_SET);
+        if (!(KEY_AGREEMENT_SET.has(content.key_agreement_protocol)
+              && HASHES_SET.has(content.hash)
+              && MAC_SET.has(content.message_authentication_code)
+              && sasMethods.length)) {
             throw newUnknownMethodError();
         }
         if (typeof content.commitment !== "string") {
             throw newInvalidMessageError();
         }
+        const macMethod = content.message_authentication_code;
         const hashCommitment = content.commitment;
-        const sasMethods = content.short_authentication_string;
         const olmSAS = new global.Olm.SAS();
         try {
             this._sendToDevice("m.key.verification.key", {
@@ -217,6 +244,7 @@ export default class SAS extends Base {
             // FIXME: make sure event is properly formed
             content = e.getContent();
             const commitmentStr = content.key + anotherjson.stringify(initialMessage);
+            // TODO: use selected hash function (when we support multiple)
             if (olmutil.sha256(commitmentStr) !== hashCommitment) {
                 throw newMismatchedCommitmentError();
             }
@@ -231,7 +259,7 @@ export default class SAS extends Base {
                 this.emit("show_sas", {
                     sas: generateSas(sasBytes, sasMethods),
                     confirm: () => {
-                        this._sendMAC(olmSAS);
+                        this._sendMAC(olmSAS, macMethod);
                         resolve();
                     },
                     cancel: () => reject(newUserCancelledError()),
@@ -245,7 +273,7 @@ export default class SAS extends Base {
                 verifySAS,
             ]);
             content = e.getContent();
-            await this._checkMAC(olmSAS, content);
+            await this._checkMAC(olmSAS, content, macMethod);
         } finally {
             olmSAS.free();
         }
@@ -253,34 +281,31 @@ export default class SAS extends Base {
 
     async _doRespondVerification() {
         let content = this.startEvent.getContent();
-        if (!(content.key_agreement_protocols instanceof Array
-              && content.key_agreement_protocols.includes("curve25519")
-              && content.hashes instanceof Array
-              && content.hashes.includes("sha256")
-              && content.message_authentication_codes instanceof Array
-              && content.message_authentication_codes.includes("hmac-sha256")
-              && content.short_authentication_string instanceof Array
-              && (content.short_authentication_string.includes("decimal")
-                  || content.short_authentication_string.includes("emoji")))) {
+        const keyAgreement
+              = intersection(content.key_agreement_protocols, KEY_AGREEMENT_SET)[0];
+        const hashMethod
+              = intersection(content.hashes, HASHES_SET)[0];
+        const macMethod
+              = intersection(content.message_authentication_codes, MAC_SET)[0];
+        // FIXME: allow app to specify what SAS methods can be used
+        const sasMethods
+              = intersection(content.short_authentication_string, SAS_SET);
+        if (!(keyAgreement !== undefined
+              && hashMethod !== undefined
+              && macMethod !== undefined
+              && sasMethods.length)) {
             throw newUnknownMethodError();
         }
 
         const olmSAS = new global.Olm.SAS();
-        const sasMethods = [];
-        // FIXME: allow app to specify what SAS methods can be used
-        if (content.short_authentication_string.includes("decimal")) {
-            sasMethods.push("decimal");
-        }
-        if (content.short_authentication_string.includes("emoji")) {
-            sasMethods.push("emoji");
-        }
         try {
             const commitmentStr = olmSAS.get_pubkey() + anotherjson.stringify(content);
             this._sendToDevice("m.key.verification.accept", {
-                key_agreement_protocol: "curve25519",
-                hash: "sha256",
-                message_authentication_code: "hmac-sha256",
+                key_agreement_protocol: keyAgreement,
+                hash: hashMethod,
+                message_authentication_code: macMethod,
                 short_authentication_string: sasMethods,
+                // TODO: use selected hash function (when we support multiple)
                 commitment: olmutil.sha256(commitmentStr),
             });
 
@@ -302,7 +327,7 @@ export default class SAS extends Base {
                 this.emit("show_sas", {
                     sas: generateSas(sasBytes, sasMethods),
                     confirm: () => {
-                        this._sendMAC(olmSAS);
+                        this._sendMAC(olmSAS, macMethod);
                         resolve();
                     },
                     cancel: () => reject(newUserCancelledError()),
@@ -316,13 +341,13 @@ export default class SAS extends Base {
                 verifySAS,
             ]);
             content = e.getContent();
-            await this._checkMAC(olmSAS, content);
+            await this._checkMAC(olmSAS, content, macMethod);
         } finally {
             olmSAS.free();
         }
     }
 
-    _sendMAC(olmSAS) {
+    _sendMAC(olmSAS, method) {
         const keyId = `ed25519:${this._baseApis.deviceId}`;
         const mac = {};
         const baseInfo = "MATRIX_KEY_VERIFICATION_MAC"
@@ -330,24 +355,24 @@ export default class SAS extends Base {
               + this.userId + this.deviceId
               + this.transactionId;
 
-        mac[keyId] = olmSAS.calculate_mac(
+        mac[keyId] = olmSAS[macMethods[method]](
             this._baseApis.getDeviceEd25519Key(),
             baseInfo + keyId,
         );
-        const keys = olmSAS.calculate_mac(
+        const keys = olmSAS[macMethods[method]](
             keyId,
             baseInfo + "KEY_IDS",
         );
         this._sendToDevice("m.key.verification.mac", { mac, keys });
     }
 
-    async _checkMAC(olmSAS, content) {
+    async _checkMAC(olmSAS, content, method) {
         const baseInfo = "MATRIX_KEY_VERIFICATION_MAC"
               + this.userId + this.deviceId
               + this._baseApis.getUserId() + this._baseApis.deviceId
               + this.transactionId;
 
-        if (content.keys !== olmSAS.calculate_mac(
+        if (content.keys !== olmSAS[macMethods[method]](
             Object.keys(content.mac).sort().join(","),
             baseInfo + "KEY_IDS",
         )) {
@@ -355,7 +380,7 @@ export default class SAS extends Base {
         }
 
         await this._verifyKeys(this.userId, content.mac, (keyId, device, keyInfo) => {
-            if (keyInfo !== olmSAS.calculate_mac(
+            if (keyInfo !== olmSAS[macMethods[method]](
                 device.keys[keyId],
                 baseInfo + keyId,
             )) {

yarn.lock

@@ -3520,9 +3520,9 @@ object.pick@^1.3.0:
   dependencies:
     isobject "^3.0.1"
 
-"olm@https://matrix.org/packages/npm/olm/olm-3.1.0-pre1.tgz":
-  version "3.1.0-pre1"
-  resolved "https://matrix.org/packages/npm/olm/olm-3.1.0-pre1.tgz#54f14fa901ff5c81db516b3b2adb294b91726eaa"
+"olm@https://matrix.org/packages/npm/olm/olm-3.1.0-pre3.tgz":
+  version "3.1.0-pre3"
+  resolved "https://matrix.org/packages/npm/olm/olm-3.1.0-pre3.tgz#525aa8191b4b6fcb07a3aa6815687780b99be411"
 
 once@1.x, once@^1.3.0:
   version "1.4.0"