1083800 - Password Storage uses MD5, when it could use PBKDF2 instead

Status: RESOLVED INVALID

(Firefox OS Graveyard :: Gaia, defect)

Description

[Wilfred Mathanaraj [:WDM]]

Impact: The stored password is only marginally safer when stored as MD5. The hash is too easy to break

Fix: Use PBKDF2 as provided by the Web Crypto API. This means we could remove the depending on the  CryptoJS library we introduced. This is in  js/privacy-panel.js:244-245 & remote_privacy_protection.js:273,301,329

Comment 1

[marta]

Where can we find documentation on how to use Web Crypto API? Current MDN web page does not provide any information

Comment 2

[Wilfred Mathanaraj [:WDM]]

Frederik - could you point here in the right direction?

Comment 3

[Frederik Braun [:freddy]]

Ha, I was going to clear your needinfo and respond right-away:

Unfortunately, MDN does not include the Web Crypto API yet.
The spec requires some understanding of Promises and TypedArrays and isn't the greatest read either <http://www.w3.org/TR/WebCryptoAPI/>. So I recommend you check out the JSConfEU talk & demo code from my colleague Tim Taubert: http://timtaubert.de/blog/2014/09/keeping-secrets-with-javascript/

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

I can definitely provide some guidance here. No docs as Freddy said but there are tests :/

https://mxr.mozilla.org/mozilla-central/source/dom/crypto/test/

Comment 5

[Wilfred Mathanaraj [:WDM]]

marta - please find above info

Comment 6

[marta]

How crucial is it to have it in the first version? Cause we could implement the webCrypto as the update along with the location fuzzing algorithm changes. 
If we really need to do it:
What are Mozilla’s accepted values for:
PBKDF2’s iteration count (10000), hash function (sha-2), salt byte length, …

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Here's some more example code. That's maybe a little more readable than the tests:

https://github.com/ttaubert/secret-notes

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to marta from comment #6)
> What are Mozilla’s accepted values for:

Not sure that we have "accepted values", Freddy might know though.

> PBKDF2’s iteration count (10000)

That's kind of dependent on the device itself. I think something like 80ms CPU time is what iOS8 uses [1].

> hash function (sha-2)

SHA-2 family is preferred I think but SHA-1 would kind of work as well due to the KDF being HMAC. I *would* prefer SHA-2 but unfortunately NSS doesn't support PBKDF2 with anything other than SHA-1 for now :( See bug 554827.

> salt byte length, …

64bits / 8 bytes should be sufficient.



[1] http://blog.cryptographyengineering.com/2014/10/why-cant-apple-decrypt-your-iphone.html

Comment 9

[Frederik Braun [:freddy]]

Yes, 80ms CPU time sounds fair.
It's OK to use SHA-1 for now. We can move to SHA-2 once we support it.
Salt can be generated using crypto.getRandomValues, 8 bytes.

Comment 11

[marta]

Attachment: webCrypto.patch

Patch to add webCrypto to our code. Tested and works, please advice if we need to change anything

Comment 12

[Frederik Braun [:freddy]]

Comment on attachment 8510116 [details] [diff] [review]
webCrypto.patch

Review of attachment 8510116 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK, but I wonder if you need localforage when you have settings access anyway.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8510116 [details] [diff] [review]
webCrypto.patch

Review of attachment 8510116 [details] [diff] [review]:
-----------------------------------------------------------------

Thought I'd give my two cents as well. I agree with Freddy, localforage doesn't seem to be necessary although convenient. Didn't Gaia have a localforage-like API anyway?

::: dev_apps/privacy-panel/js/passphrase.js
@@ +10,5 @@
> +
> +  PassPhrase.prototype = {
> +    buffer: encode('topsecret'),
> +
> +    exists: function() {

exists() is a misleading name as it doesn't return or resolve to a boolean. Maybe call it _mac() similar to _salt() below? If you want a function accessible from the outside then you should probably wrap it like:

exists() {
  return this._mac().then(mac => !!mac);
}

@@ +12,5 @@
> +    buffer: encode('topsecret'),
> +
> +    exists: function() {
> +      return localforage.getItem(this.macDest).then(function(mac) {
> +        return mac;

Nit: return localforage.getItem(this.macDest);

@@ +17,5 @@
> +      });
> +    },
> +
> +    verify: function(password) {
> +      return this.exists.then(function(mac) {

return this.exists().then(... [or this._mac() ...]

@@ +25,5 @@
> +
> +        return this._retrieveKey(password).then(function(key) {
> +          return crypto.subtle.verify('HMAC', key, mac, this.buffer)
> +            .then(function(valid) {
> +              return valid;

Nit: return crypto.subtle.verify('HMAC', key, mac, this.buffer);

@@ +28,5 @@
> +            .then(function(valid) {
> +              return valid;
> +            });
> +        }.bind(this));
> +      }.bind(this));

Could use arrow functions here.

@@ +37,5 @@
> +        return crypto.subtle.sign('HMAC', key, this.buffer)
> +          .then(function(mac) {
> +            return localforage.setItem(this.macDest, mac);
> +          }.bind(this));
> +      }.bind(this));

Arrow functions?

@@ +45,5 @@
> +      return localforage.getItem(this.saltDest).then(function(salt) {
> +        if (salt) {
> +          return salt;
> +        }
> +        salt = crypto.getRandomValues(new Uint8Array(8));

The "8" should probably go into a const like SALT_NUM_BYTES or similar.

Comment 14

[Frederik Braun [:freddy]]

Thanks for the additional comments(In reply to Tim Taubert [:ttaubert] from comment #13) 
> Thought I'd give my two cents as well. I agree with Freddy, localforage
> doesn't seem to be necessary although convenient. Didn't Gaia have a
> localforage-like API anyway?
> 

Yes thanks for providing the in-depth review. This was really only drive-by and I'm on a conference this week and I wanted to get something out fast.
The built-in thing in Gaia is called async-storage and it's in /shared.

Comment 15

[marta]

Attachment: Fixed as proposed

Please review the patch and tell us if this is ok.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8510999 [details] [diff] [review]
Fixed as proposed

Review of attachment 8510999 [details] [diff] [review]:
-----------------------------------------------------------------

You didn't really address most of my comments from the first review. Please take a look at those again, I explicitly wrote down the proposed code changes. Please make sure to use |this._mac()| in |verify()| instead of the currently invalid |this.exists.then(...| call.

That said, I am not an official Gaia peer, I can give feedback but not a final r+. Please ask someone who can.

::: apps/privacy-panel/js/rpp/passphrase.js
@@ +24,5 @@
> +        asyncStorage.getItem(key, function(value) {
> +          resolve(value);
> +        });
> +      });
> +      return promise;

return new Promise(resolve => {
  asyncStorage.getItem(key, resolve);
});

@@ +33,5 @@
> +        asyncStorage.setItem(key, value, function() {
> +          resolve(value);
> +        });
> +      });
> +      return promise;

return new Promise(resolve => {
  asyncStorage.setItem(key, value, () => resolve(value));
});

@@ +42,2 @@
>          return mac;
>        });

_mac() {
  return this._getItem(this.macDest);
},

exists() {
  return this._mac().then(mac => !!mac);
},

In general you might make use of the new object property notation if that's available in Gaia.

@@ +66,1 @@
>            }.bind(this));

return this._retrieveKey(password).then(key => {
  return crypto.subtle.sign('HMAC', key, this.buffer)
    .then(mac => this._setItem(this.macDest, mac));
});

Or maybe:

let promise = this._retrieveKey(password);

promise = promise.then(key => {
  return crypto.subtle.sign('HMAC', key, this.buffer);
});

return promise.then(mac => {
  return this._setItem(this.macDest, mac);
});

You don't have an equivalent to Task.js(m) in Gaia, right? That would make things a lot nicer...

@@ +76,4 @@
>          if (salt) {
>            return salt;
>          }
>          salt = crypto.getRandomValues(new Uint8Array(8));

const SALT_NUM_BYTES = 8; (somewhere at the top)

and...

salt = crypto.getRandomValues(new Uint8Array(SALT_NUM_BYTES));

Comment 17

[marta]

All of the comments were included in the code that went into the apps. Closing as INVALID