Closed Bug 1234305 Opened 5 years ago Closed 5 years ago

WeaveCrypto uses ctypes to access NSS directly and breaks all assumptions about NSS shutdown

Categories

(Firefox :: Sync, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox46 --- affected
firefox49 --- fixed

People

(Reporter: keeler, Assigned: eoger)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

WeaveCrypto uses ctypes to access NSS directly, which is problematic for a number of reasons. ctypes combines the underlying memory unsafety of c with the limited static checks of javascript, so we're starting off with the worst of both worlds here. That's mostly a philosophical point, though. More concretely, WeaveCrypto currently violates all of Gecko's assumptions about how we properly (and safely) shut down NSS. That is, at some point when shutting down Gecko, NSS will go away. Before that point, anything that uses NSS resources and functions must ensure that NSS won't be yanked out from under them. There's no way to do this with how WeaveCrypto is currently implemented. At that point, all NSS resources must be released so they don't leak. We can indirectly do this, but there is a better, unified mechanism that can't be accessed from WeaveCrypto. After that point, no NSS resources or functions may be used. Again, there's a workaround to make this work, but again there's a better, unified mechanism that can't be directly accessed.

WeaveCrypto should really look more like services/crypto/modules/utils.js which (mostly) uses interfaces to primitives implemented in PSM (in C++) that are aware of and can properly handle NSS shutdown.
Flags: firefox-backlog+
David, can you help me understand the priority of this bug and impact of not doing it?
Flags: needinfo?(dkeeler)
Basically, until this is addressed, we can't automatically detect new leaks of NSS resources used by Gecko. In the past few years, only a few meaningful leaks have been introduced as far as I can tell (see in particular bug 1230234 and bug 1230312, although my investigation isn't complete). Once we've fixed them, it would be nice to introduce measures to prevent more.
Flags: needinfo?(dkeeler)
David or Mark, can you help me understand the scope of work here? S, M, L task?
Flags: needinfo?(markh)
Flags: needinfo?(dkeeler)
I'd estimate it's a medium task. In some cases, there is already functionality that I think can be used as a drop-in replacement (e.g. generateRandomBytes can be replaced with nsIRandomGenerator.generateRandomBytes). In other cases, there is minimal functionality, but I think it would have to be extended (key generation/derivation, for example - see nsIKeyObject/nsIKeyObjectFactory).

Short of using/extending replacements, there's probably a way to hack together a 90% solution that would require less work in the short run, but isn't great long-term.
Flags: needinfo?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> I'd estimate it's a medium task
> ...
> Short of using/extending replacements, there's probably a way to hack
> together a 90% solution that would require less work in the short run, but
> isn't great long-term.

Yep, agreed - kinda "large-medium" for a clean solution versus "small-medium" for a hacky shim that ends up addressing this bug but adds more XXX/*sob*/etc comments to the code...
Flags: needinfo?(markh)
Priority: -- → P3
Assignee: nobody → edouard.oger
Status: NEW → ASSIGNED
Comment on attachment 8751601 [details]
MozReview Request: Bug 1234305 - Replace WeaveCrypto NSS implementation with Web Crypto. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52129/diff/1-2/
Comment on attachment 8751601 [details]
MozReview Request: Bug 1234305 - Replace WeaveCrypto NSS implementation with Web Crypto. r=keeler

https://reviewboard.mozilla.org/r/52129/#review49205

This looks great. One thing in general, though, is that many of the helper functions still take a length argument that I don't think is necessary now that everything is either Uint8Arrays or js strings. Also, I think someone who has more experience with promises should double-check those here (in particular, are rejected promises properly handled?).

::: services/crypto/modules/WeaveCrypto.js:83
(Diff revision 2)
> -
>      decrypt : function(cipherText, symmetricKey, iv) {
>          this.log("decrypt() called");
> -
> -        let inputUCS2 = "";
>          if (cipherText.length)

nit: probably best to add braces around the conditional body

Also, should we throw if cipherText isn't of the expected form? (i.e. some base64 string that atob can be called on)

::: services/crypto/modules/WeaveCrypto.js:100
(Diff revision 2)
> +     *
> +     * @args
> +     * data: data to encrypt/decrypt (ArrayBuffer)
> +     * symKeyStr: symmetric key (Base64 String)
> +     * ivStr: initialization vector (Base64 String)
> +     * operation: operation to apply (either OPERATIONS.ENCRYPT or OPERATIONS.DECRYPT)

Maybe this should throw if operation isn't one of the two options?

::: services/crypto/modules/WeaveCrypto.js:114
(Diff revision 2)
>          // for AES. Neither do we want one smaller; throw in that case.
> -        if (iv.length < this.blockSize)
> -            throw "IV too short; must be " + this.blockSize + " bytes.";
> -        if (iv.length > this.blockSize)
> -            iv = iv.slice(0, this.blockSize);
> -
> +        if (ivStr.length < AES_CBC_IV_SIZE) {
> +            throw "IV too short; must be " + AES_CBC_IV_SIZE + " bytes.";
> +        }
> +        if (ivStr.length > AES_CBC_IV_SIZE) {
> +            ivStr = ivStr.slice(0, AES_CBC_IV_SIZE);

Huh - I would expect that this should throw in either case (seems like a bug to pass in an incorrectly-sized IV).

::: services/crypto/modules/WeaveCrypto.js:121
(Diff revision 2)
> +
> +        let iv = new Uint8Array(AES_CBC_IV_SIZE);
> +        this.byteCompressInts(ivStr, iv, ivStr.length);
> +        let symKey = this.importSymKey(symKeyStr, operation);
> +        let cryptMethod = ((operation === OPERATIONS.ENCRYPT) ?
> +                          crypto.subtle.encrypt : crypto.subtle.decrypt)

nit: this formatting is a little unclear. I might write this like so:

let cryptMethod = (operation === OPERATIONS.ENCRYPT
                   ? crypto.subtle.encrypt
                   : crypto.subtle.decrypt)
                  .bind(crypto.subtle);

::: services/crypto/modules/WeaveCrypto.js:140
(Diff revision 2)
>          this.log("generateRandomKey() called");
> -        let slot, randKey, keydata;
> -        try {
> -            slot = this.nss.PK11_GetInternalSlot();
> -            if (slot.isNull())
> -                throw Components.Exception("couldn't get internal slot", Cr.NS_ERROR_FAILURE);
> +        return Async.promiseSpinningly(
> +            crypto.subtle.generateKey({
> +                name: CRYPT_ALGO,
> +                length: CRYPT_ALGO_LENGTH
> +            }, true, [])

It's a little unexpected that we're generating a key with no usages here, but maybe that's ok since we're exporting it and then later importing it with an actual usage (which, now that I think about it, seems a little inefficient, but I don't think it's a big deal).

::: services/crypto/modules/WeaveCrypto.js:162
(Diff revision 2)
>  
> -        return this.encodeBase64(this._randomByteBufferAddr, byteCount);
> +        return this.encodeBase64(randBytes, byteCount);
>      },
>  
>      //
>      // PK11SymKey memoization.

nit: let's either update this comment or remove it

::: services/crypto/modules/WeaveCrypto.js
(Diff revision 2)
>      //
>      // Utility functions
>      //
>  
> -    /**
> -     * Compress a JS string into a C uint8 array. count is the number of

Might be nice to keep/update some documentation here.

::: services/crypto/modules/WeaveCrypto.js:210
(Diff revision 2)
> -     * Compress a JS string into a C uint8 array. count is the number of
> -     * elements in the destination array. If the array is smaller than the
> -     * string, the string is effectively truncated. If the string is smaller
> -     * than the array, the array is not 0-padded.
> -     */
>      byteCompressInts : function byteCompressInts (jsString, intArray, count) {

Are there occasions where count != jsString.length? If not, this should probably be simplified.

::: services/crypto/modules/WeaveCrypto.js:231
(Diff revision 2)
>  
> -    // Returns a filled SECItem *, as returned by SECITEM_AllocItem.
> -    //
> -    // Note that this must be released with freeSECItem, which will also
> -    // deallocate the internal buffer.
>      makeSECItem : function(input, isEncoded) {

nit: doesn't make much sense to call this a SECItem any longer (since that's an NSS-specific term). Maybe makeUint8Array?

::: services/crypto/modules/WeaveCrypto.js:232
(Diff revision 2)
> -    // Returns a filled SECItem *, as returned by SECITEM_AllocItem.
> -    //
> -    // Note that this must be released with freeSECItem, which will also
> -    // deallocate the internal buffer.
>      makeSECItem : function(input, isEncoded) {
>          if (isEncoded)

nit: brances around conditional body

::: services/crypto/modules/WeaveCrypto.js:259
(Diff revision 2)
> -
> -        keyLength  = keyLength || 0;    // 0 = Callee will pick.
> -        let iterations = KEY_DERIVATION_ITERATIONS;
> -
> -        let algid, slot, symKey, keyData;
> -        try {
> +                false,
> +                ["deriveKey"]
> +            )
> +            .then(key =>
> +                crypto.subtle.deriveKey({
> +                    name: KEY_DERIVATION_ALGO,

The formatting is a bit unclear here again - maybe factor out the algorithm and derivedKeyType into their own variables/constants?

::: services/crypto/modules/WeaveCrypto.js:262
(Diff revision 2)
> -
> -        let algid, slot, symKey, keyData;
> -        try {
> -            algid = this.nss.PK11_CreatePBEV2AlgorithmID(pbeAlg, cipherAlg, prfAlg,
> -                                                         keyLength, iterations,
> -                                                         saltItem);
> +            .then(key =>
> +                crypto.subtle.deriveKey({
> +                    name: KEY_DERIVATION_ALGO,
> +                    salt: saltItem,
> +                    iterations: KEY_DERIVATION_ITERATIONS,
> +                    hash: { name: KEY_DERIVATION_HASHING_ALGO},

nit: space before }

::: services/crypto/tests/unit/test_crypto_crypt.js:103
(Diff revision 2)
>    // Fill it too short.
> -  cryptoSvc.byteCompressInts("MMM", intData, 8);
> +  cryptoSvc.byteCompressInts("MMM", item1, 8);
>    for (let i = 0; i < 3; ++i)
> -    do_check_eq(intData[i], [77, 77, 77][i]);
> +    do_check_eq(item1[i], [77, 77, 77][i]);
>  
>    // Fill it too much. Doesn't buffer overrun.

Since we're not using ctypes any more, this isn't really relevant.
Attachment #8751601 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/52129/#review49205

> nit: probably best to add braces around the conditional body
> 
> Also, should we throw if cipherText isn't of the expected form? (i.e. some base64 string that atob can be called on)

Atob throws when there are invalid characters.
Comment on attachment 8751601 [details]
MozReview Request: Bug 1234305 - Replace WeaveCrypto NSS implementation with Web Crypto. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52129/diff/2-3/
David could you check my PR one last time please? I corrected it with your comments.
Flags: needinfo?(dkeeler)
https://reviewboard.mozilla.org/r/52129/#review49764

Looks good, but there are a couple issues that need to be addressed.

::: services/crypto/modules/WeaveCrypto.js:63
(Diff revisions 2 - 3)
>            this.debug = false;
>          }
>      },
>  
> -    log : function (message) {
> -        if (!this.debug)
> +    get encoder() {
> +        delete this.encoder;

deleting/recreating the encoder/decoder each time seems inefficient. Is this necessary?

::: services/crypto/modules/WeaveCrypto.js:164
(Diff revisions 2 - 3)
>          this.log("generateRandomBytes() called");
>  
>          let randBytes = new Uint8Array(byteCount);
>          crypto.getRandomValues(randBytes);
>  
>          return this.encodeBase64(randBytes, byteCount);

encodeBase64 doesn't take a length argument any more

::: services/crypto/modules/WeaveCrypto.js:226
(Diff revisions 2 - 3)
> +        return arrayBuffer;
>      },
>  
> -    expandData : function expandData(data, len) {
> +    expandData(data) {
>          let expanded = "";
>          data = new Uint8Array(data);

This is always already a Uint8Array, right?

::: services/crypto/modules/WeaveCrypto.js:260
(Diff revisions 2 - 3)
> -                    hash: { name: KEY_DERIVATION_HASHING_ALGO},
> -                },
> -                key,
> +            hash: { name: KEY_DERIVATION_HASHING_ALGO },
> +        };
> +        let derivedKeyType = {
> -                {
> -                    name: DERIVED_KEY_ALGO,
> +            name: DERIVED_KEY_ALGO,
> -                    length: DERIVED_KEY_LENGTH,
> +            length: DERIVED_KEY_LENGTH,

This should be keyLength * 8, right?

::: services/crypto/modules/WeaveCrypto.js:269
(Diff revisions 2 - 3)
> -            )
> +            .then(derivedKey => crypto.subtle.exportKey("raw", derivedKey))
> -            .then(key => crypto.subtle.exportKey("raw", key))
>              .then(keyBytes => {
> -                return this.expandData(keyBytes, keyLength);
> +                let derivedStr = this.expandData(keyBytes);
> +                if (keyLength) {
> +                    derivedStr = derivedStr.slice(0, keyLength);

If line 260 is fixed, this shouldn't be necessary.

::: services/crypto/tests/unit/test_crypto_crypt.js:200
(Diff revisions 2 - 3)
> +    err = ex;
> +  }
> +  do_check_true(!!err);
>  
>    key = "c5hG3YG+NC61FFy8NOHQak1ZhMEWO79bwiAfar2euzI=";
> -  iv  = "gsgLRDaxWvIfKt75RjuvFWERt83FFsY2A0TW+0b2iVk=";
> +  iv  = "oLjkfrLIOnK2bDRvW4kXYA==";

In the original implementation, overlong IVs were truncated, right? So, in theory, we should be able to reproduce the same output if we just provide correctly-sized IVs. It's good to add an overlong IV testcase, but we should also keep the original ones (i.e. lines like 'do_check_eq(cipherText, "...");' shouldn't change).
https://reviewboard.mozilla.org/r/52129/#review49786

::: services/crypto/tests/unit/test_crypto_deriveKey.js:15
(Diff revision 3)
>    let k = cryptoSvc.deriveKeyFromPassphrase(pp, salt, 16);
>    do_check_eq(16, k.length);
>    do_check_eq(btoa(k), "d2zG0d2cBfXnRwMUGyMwyg==");
> -  
> +
>    // Test different key lengths.
>    k = cryptoSvc.deriveKeyFromPassphrase(pp, salt, 32);

We should find out what key this generates with the current implementation and ensure that the new implementation generates the same key.
Flags: needinfo?(dkeeler)
https://reviewboard.mozilla.org/r/52129/#review49764

> deleting/recreating the encoder/decoder each time seems inefficient. Is this necessary?

The getter is removed and replaced by the actual encoder/decoder, we only create them once.
Comment on attachment 8751601 [details]
MozReview Request: Bug 1234305 - Replace WeaveCrypto NSS implementation with Web Crypto. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52129/diff/3-4/
Issues corrected!
Flags: needinfo?(dkeeler)
https://reviewboard.mozilla.org/r/52129/#review50742

Great!

::: services/crypto/tests/unit/test_crypto_crypt.js:197
(Diff revision 4)
>    clearText = cryptoSvc.decrypt(cipherText, key, iv);
>    do_check_eq(cipherText, "DLGx8BWqSCLGG7i/xwvvxg==");
>    do_check_eq(clearText, mySecret);
>  
>    key = "c5hG3YG+NC61FFy8NOHQak1ZhMEWO79bwiAfar2euzI=";
> -  iv  = "gsgLRDaxWvIfKt75RjuvFWERt83FFsY2A0TW+0b2iVk=";
> +  iv  = "gsgLRDaxWvIfKt75RjuvFW==";

Cool. I think it might be good to also check that the implementation throws if passed an iv that is too long or too short, though.
Flags: needinfo?(dkeeler)
Comment on attachment 8751601 [details]
MozReview Request: Bug 1234305 - Replace WeaveCrypto NSS implementation with Web Crypto. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52129/diff/4-5/
I added a test case if the iv is too long (we had one for when it's too short) and fixed the encoder getters that gave us a warning in the console.
Flags: needinfo?(dkeeler)
Great!
Flags: needinfo?(dkeeler)
Thanks David!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0e747813be13
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.