From 6c104ff79affd185421b06e2e6d97346f4072b88 Mon Sep 17 00:00:00 2001 From: Edward Lee Date: Thu, 25 Mar 2010 19:23:44 -0700 Subject: [PATCH] Bug 552134 - Ensure that keyring/symmetric key haven't been tampered with [r=mconnor] Store a HMAC with the encrypted symmetric key instead of just the wrapped key and verify that the HMAC matches before unwrapping. Test that normal getting works and a tampered payload/HMAC fails but succeeds on restoring the correct HMAC. --- services/sync/modules/base_records/crypto.js | 31 +++++++++++-- services/sync/tests/unit/head_first.js | 6 +++ .../sync/tests/unit/test_clients_escape.js | 1 - .../sync/tests/unit/test_records_crypto.js | 1 - .../tests/unit/test_records_cryptometa.js | 43 +++++++++++++++++++ 5 files changed, 76 insertions(+), 6 deletions(-) create mode 100644 services/sync/tests/unit/test_records_cryptometa.js diff --git a/services/sync/modules/base_records/crypto.js b/services/sync/modules/base_records/crypto.js index 34a8a32041bc..85bd4927f8f1 100644 --- a/services/sync/modules/base_records/crypto.js +++ b/services/sync/modules/base_records/crypto.js @@ -41,6 +41,7 @@ const Ci = Components.interfaces; const Cr = Components.results; const Cu = Components.utils; +Cu.import("resource://weave/identity.js"); Cu.import("resource://weave/util.js"); Cu.import("resource://weave/base_records/wbo.js"); Cu.import("resource://weave/base_records/keys.js"); @@ -133,8 +134,21 @@ CryptoMeta.prototype = { if (!wrapped_key) throw "keyring doesn't contain a key for " + pubkeyUri; - let unwrappedKey = new String(Svc.Crypto.unwrapSymmetricKey(wrapped_key, - privkey.keyData, passphrase.password, privkey.salt, privkey.iv)); + // Make sure the wrapped key hasn't been tampered with + let localHMAC = Utils.sha256HMAC(wrapped_key.wrapped, this.hmacKey); + if (localHMAC != wrapped_key.hmac) + throw "Server attack?! SHA256 HMAC key fail: " + wrapped_key.hmac; + + // Decrypt the symmetric key and make it a String object to add properties + let unwrappedKey = new String( + Svc.Crypto.unwrapSymmetricKey( + wrapped_key.wrapped, + privkey.keyData, + passphrase.password, + privkey.salt, + privkey.iv + ) + ); unwrappedKey.hmacKey = Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, unwrappedKey); @@ -160,8 +174,17 @@ CryptoMeta.prototype = { delete this.keyring[relUri]; } - this.keyring[new_pubkey.uri.spec] = - Svc.Crypto.wrapSymmetricKey(symkey, new_pubkey.keyData); + // Wrap the symmetric key and generate a HMAC for it + let wrapped = Svc.Crypto.wrapSymmetricKey(symkey, new_pubkey.keyData); + this.keyring[new_pubkey.uri.spec] = { + wrapped: wrapped, + hmac: Utils.sha256HMAC(wrapped, this.hmacKey) + }; + }, + + get hmacKey() { + let passphrase = ID.get("WeaveCryptoID").password; + return Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, passphrase); } }; diff --git a/services/sync/tests/unit/head_first.js b/services/sync/tests/unit/head_first.js index d18688ce661a..fa37973e1f3e 100644 --- a/services/sync/tests/unit/head_first.js +++ b/services/sync/tests/unit/head_first.js @@ -330,3 +330,9 @@ function SyncTestingInfrastructure(engineFactory) { * @usage _(1, 2, 3) -> prints "1 2 3" */ let _ = function(some, debug, text, to) print(Array.slice(arguments).join(" ")); + +_("Setting the identity for passphrase"); +Cu.import("resource://weave/identity.js"); +let passphrase = ID.set("WeaveCryptoID", new Identity()); +passphrase.password = "passphrase"; + diff --git a/services/sync/tests/unit/test_clients_escape.js b/services/sync/tests/unit/test_clients_escape.js index bbc63835f5e0..a165b3b8e8f4 100644 --- a/services/sync/tests/unit/test_clients_escape.js +++ b/services/sync/tests/unit/test_clients_escape.js @@ -4,7 +4,6 @@ Cu.import("resource://weave/base_records/keys.js"); Cu.import("resource://weave/base_records/crypto.js"); function run_test() { - let passphrase = { password: "passphrase" }; let baseUri = "http://fakebase/"; let pubUri = baseUri + "pubkey"; let privUri = baseUri + "privkey"; diff --git a/services/sync/tests/unit/test_records_crypto.js b/services/sync/tests/unit/test_records_crypto.js index 335f10fc1886..04309d9ce2ff 100644 --- a/services/sync/tests/unit/test_records_crypto.js +++ b/services/sync/tests/unit/test_records_crypto.js @@ -59,7 +59,6 @@ function run_test() { log.info("Generating keypair + symmetric key"); PubKeys.defaultKeyUri = "http://localhost:8080/pubkey"; - let passphrase = { password: "my passphrase" }; keys = PubKeys.createKeypair(passphrase, "http://localhost:8080/pubkey", "http://localhost:8080/privkey"); diff --git a/services/sync/tests/unit/test_records_cryptometa.js b/services/sync/tests/unit/test_records_cryptometa.js new file mode 100644 index 000000000000..57fb62bec6be --- /dev/null +++ b/services/sync/tests/unit/test_records_cryptometa.js @@ -0,0 +1,43 @@ +Cu.import("resource://weave/util.js"); +Cu.import("resource://weave/base_records/crypto.js"); +Cu.import("resource://weave/base_records/keys.js"); + +function run_test() { + _("Generating keypair to encrypt/decrypt symkeys"); + let {pubkey, privkey} = PubKeys.createKeypair( + passphrase, + "http://site/pubkey", + "http://site/privkey" + ); + PubKeys.set(pubkey.uri, pubkey); + PrivKeys.set(privkey.uri, privkey); + + _("Generating a crypto meta with a random key"); + let crypto = new CryptoMeta("http://site/crypto"); + let symkey = Svc.Crypto.generateRandomKey(); + crypto.addUnwrappedKey(pubkey, symkey); + + _("Verifying correct HMAC by getting the key"); + crypto.getKey(privkey, passphrase); + + _("Generating a new crypto meta as the previous caches the unwrapped key"); + let crypto = new CryptoMeta("http://site/crypto"); + let symkey = Svc.Crypto.generateRandomKey(); + crypto.addUnwrappedKey(pubkey, symkey); + + _("Changing the HMAC to force a mismatch"); + let goodHMAC = crypto.keyring[pubkey.uri.spec].hmac; + crypto.keyring[pubkey.uri.spec].hmac = "failme!"; + let error = ""; + try { + crypto.getKey(privkey, passphrase); + } + catch(ex) { + error = ex; + } + do_check_eq(error, "Server attack?! SHA256 HMAC key fail: failme!"); + + _("Switching back to the correct HMAC and trying again"); + crypto.keyring[pubkey.uri.spec].hmac = goodHMAC; + crypto.getKey(privkey, passphrase); +}