513798 - Rewrite WeaveCrypto in JS

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Justin Dolske [:Dolske]]

Bug 513783 will be implementing js-ctypes for Firefox 3.6. This will allow JS code to interface with native libraries without the need for writing a binary component.

We can use this to allow implementing WeaveCrypto as a JS component, and eliminate the need for a bunch of related complexity... No need to compile anything, no need for a Xulrunner SDK, no need to have binary code checked into Hg.

An easy first step would be to just port the current C++ code to equivalent JS, retaining the WeaveCrypto.idl interface. Nothing in the rest of Weave would need to change, it should  be compatible. A followup change could move the code out of a component, and into just a standalone JS module (eliminating the need to go through XPCOM).

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

First work in progress...

This thwaps around a bunch of stuff in the Weave repo, to switch from compiling a binary module to the new WeaveCrypto.js. The Javascript module just stubs out all the IWeaveCrypto interfaces, except for a almost-working GenerateRandomBytes. (It's successfully calling the API in NSS, just not doing anything with the result.)

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (WIP)

This implements enough of WeaveCrypto for the test_crypto_random.js tests to pass. Yay!

Comment 3

[[e-mail]]

Comment on attachment 415957 [details] [diff] [review]
Patch v.2 (WIP)

>+        Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);
Nit: .getService(); suffices.

>+        if (1)
>+            nssfile.append("libnss3.dylib");
>+        else if (0)
>+            nssfile.append("libnss3.dll");
>+        else if (0)
>+            nssfile.append("libnss3.so");
What, ctypes can't even be bothered to append the correct suffix for you?

Comment 4

[[e-mail]]

You could use the preprocessor for this, or we can add a ctypes API to do it.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Another work-in-progress checkpoint.

test_crypt_crypt test passes now, and I'm about halfway through getting the keypair code done. Getting a bit easier now that I'm familiar with JS-CTypes now, just becoming tedious. :)

One snag I just hit was wanting union support, but should be able to hack around for now. Will find out in the next step.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.4 (WIP)

Woo, all crypto unit tests now pass. In theory, this could now be dropped in to replace the binary module and Weave should work.

Code still needs some cleanup, and I skipped implementing some finally blocks (just commented out), which means this currently leaks a bunch. Will clean that up next.

Comment 7

[Mike Connor [:mconnor]]

Sticking a non-specific milestone on this so we don't keep triaging it, but this is wanted ASAP. :)

Comment 8

[[e-mail]]

Bug 513788 is now landed, so this can proceed.

If any additional features or tweaks are needed for this, please ask. :)

Comment 9

[[e-mail]]

Comment on attachment 418301 [details] [diff] [review]
Patch v.4 (WIP)

>+        // XXX boo, this is ugly.
>+        if (!outputBuffer.length)
>+            return btoa("");
>+        return this.encodeBase64(outputBuffer.addressOfElement(0), outputBuffer.length);

FWIW, you can just use 'outputBuffer.address()' -- for zero safety.

We may also make '.addressOfElement(n)' work for n = outputBuffer.length, though (i.e. one past the end).

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Updated and fixed a bunch of nits and todos. This patch is still against an older Weave changeset (1877ed04cc9a), will look at updating that next. [Mostly just build changes, I suspect, because nothing has changed in WeaveCrypto since then.]

Comment 11

[Justin Dolske [:Dolske]]

And note to self, because I always spend 10 minutes trying to figure out how to run the tests...

export TOPSRCDIR=~/build/weave
export NATIVE_TOPSRCDIR=~/build/weave
export XULRUNNER_BIN=/Applications/Minefield.app/Contents/MacOS/firefox-bin

make xpi # (each time WeaveCrypto.js is modified)
make -C tests/unit
or
make -C tests/unit test_crypto_xxx

output in unittest/test_output/test_crypto_xxx.log

Note that with this older Weave changeset, only the crypto tests pass (even without my changes).

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Patch v.6

More code cleanup and nits, merged to Weave trunk. Note that you'll want the patch from bug 531718 to run tests.

Running tests is a bit easier now:

make
make test
(wait for it to hang running test_auth_manager, that test seems to be busted?)
then make -C tests/unit/ test_crypto_xxx

WeaveCrypto.js is symlinked in staging now, so edits should take effect without having to rebuild anything (modulo any FF/EM caching).

This is just about ready for a review pass, I'd like to give it one more comparison with the original C code to see if anything sticks out.

Comment 13

[Justin Dolske [:Dolske]]

(The various exports are still needed for tests, of course)

Comment 14

[Justin Dolske [:Dolske]]

Good news, everyone! </Professor Farnsworth>

Just tried with the last patch, and I'm able to sync between two profiles with the js-ctypes WeaveCrypto. [With one small change, s/libnss3.dll/nss3.dll/, since this was the first time I've tested on Windows.]

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Patch v.7

Fixes to build system (and other minor tweaks), to leave the old C++ code in the tree, continue packaging the precompiled binaries, and add runtime detection to use the JS-CTypes component if the Gecko platform is new enough. (IE, Firefox 3.6 uses the binaries, FF 3.7 uses js-ctypes).

Comment 16

[David Dahl :ddahl]

so is this going to land on the 3.6 branch?

Comment 17

[[e-mail]]

It can't, unless we backport the new ctypes APIs to 3.6.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 436820 [details] [diff] [review]
Patch v.7

mconnor: I still want to make one more pass on the WeaveCrypto.js changes, but go ahead and take a look at the other build config changes in the patch.

Comment 19

[Justin Dolske [:Dolske]]

Attachment: Patch v.8

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 438641 [details] [diff] [review]
Patch v.8

>diff --git a/Makefile b/Makefile

> help:
>-	@echo "rebuild_crypto (set to 1 when building a new crypto binary)"

should list a target for crypto-obsolete-build for rebuilding the crypto binaries

@echo "crypto-obsolete-build (rebuilds the legacy binary components)"

>+pref("extensions.weave.log.cryptoDebug", false);

I'm kind of curious why you're using a different logging setup, but we can adjust this later if we want.

Will look at WeaveCrypto.js itself later tonight.

Comment 21

[[e-mail]]

Comment on attachment 438641 [details] [diff] [review]
Patch v.8

[ Skipping all the build bits ]

>diff --git a/crypto/WeaveCrypto.js b/crypto/WeaveCrypto.js

>+        observe : function (subject, topic, data) {
>+            let self = this._self;
>+            if (topic == "nsPref:changed") {
>+                let prefName = data;
>+                self.log("got change to " + prefName + " preference");
>+            }
>+        }

Going to do anything meaningful with 'prefName'?

>+        // Open the NSS library.
>+        let nssfile = Services.dirsvc.get("GreD", Ci.nsILocalFile);
>+        switch (Services.appinfo.OS) {
>+          case "WINNT":
>+            nssfile.append("nss3.dll");
>+            break;
>+          case "Darwin":
>+            nssfile.append("libnss3.dylib");
>+            break;
>+          case "Linux":
>+            nssfile.append("libnss3.so");
>+            break;

Mmm, it'd be nice to do something better here, but for now we're stuck with this :/

>+        // XXX really want to be able to pass specific dlopen flags here.
>+        // XXX linux is going to be sucky to figure out, because distros could
>+        //     place libnss who knows where.
>+        let nsslib = ctypes.open(nssfile.path);
>+        if (!nsslib)
>+            throw Components.Exception("couldn't open NSS library", Cr.NS_ERROR_UNEXPECTED);

ctypes.open will throw if loading the library failed. If you want to log something nice, you could catch and rethrow...

>+        // security/nss/lib/softoken/secmodt.h#59
>+        // typedef struct PK11SlotInfoStr PK11SlotInfo; (defined in secmodti.h) 
>+        this.nss_t.PK11SlotInfo = ctypes.void_t;

Hmm. We need a way to specify an opaque struct, rather than an opaque pointer, so you get your typesafety here. Having the equivalent of voidptr_t everywhere sucks. :/

>+        // security/nss/lib/softoken/secmodt.h#61
>+        // typedef struct PK11SymKeyStr PK11SymKey; (defined in secmodti.h)
>+        this.nss_t.PK11SymKey = ctypes.void_t;
>+// XXX maybe better as ctypes.PointerType("PK11SymKey");

You mean ctypes.PointerType("PK11SymKey*"), and then call it PK11SymKeyPtr. That'd work, and give you more typesafety than you have now... but as said above, you probably want opaque struct support.

>+        // security/nss/lib/util/secoidt.h#454
>+        // typedef enum
>+        this.nss_t.SECOidTag = ctypes.int

Needs more ';'.

>+        // XXX placeholder
>+        // Needed for SECKEYPrivateKey struct def'n, but I don't think we need to actually access it.
>+        this.nss_t.PLArenaPool = ctypes.void_t;

Sigh, so need opaque structs.

>+        // security/nss/lib/cryptohi/keythi.h#78
>+        // typedef struct SECKEYRSAPublicKeyStr --> def'n right above it
>+        this.nss_t.SECKEYRSAPublicKey = ctypes.StructType("SECKEYRSAPublicKey", [{ arena:          this.nss_t.PLArenaPool.ptr },

Would like to see some wrapping here :)

>+        this.nss_t.SECKEYPublicKey = ctypes.StructType("SECKEYPublicKey", [{ arena:      this.nss_t.PLArenaPool.ptr    },
>+                                                                           { keyType:    this.nss_t.KeyType            },
>+                                                                           { pkcs11Slot: this.nss_t.PK11SlotInfo.ptr   },
>+                                                                           { pkcs11ID:   this.nss_t.CK_OBJECT_HANDLE   },
>+                                                                           { rsa:        this.nss_t.SECKEYRSAPublicKey } ]);
>+                                                                           // XXX: "rsa" et al into a union here!

Is 'rsa' the only one you're interested in? I assume so, and that all the union members are the same size. If not, we need unions :)

>+        // security/nss/lib/pk11wrap/pk11pub.h#286
>+        // SECStatus PK11_RandomUpdate(void *data, size_t bytes);
>+        this.nss.PK11_GenerateRandom = nsslib.declare("PK11_GenerateRandom",
>+                                                      ctypes.default_abi, this.nss_t.SECStatus,
>+                                                      ctypes.unsigned_char.ptr, ctypes.int);

Do you want void* or unsigned char* here? (If the C prototype uses void* but unsigned char* is clearer, then hey, go for it.)

Also, size_t != int... you want ctypes.size_t there.

>+        // security/nss/lib/pk11wrap/pk11pub.h#466
>+        // SECStatus PK11_SetPrivateKeyNickname(SECKEYPrivateKey *privKey, const char *nickname);
>+        // XXX no const support? (CType).const maybe? EG ctypes.char.ptr.const, ctypes.int.ptr.const.ptr.const?

There's a bug for that. :)

>+    __utfConverter : null, // UCS2 <--> UTF8 string conversion
>+    get _utfConverter() {
>+        if (!this.__utfConverter) {
>+            this.__utfConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>+                                  createInstance(Ci.nsIScriptableUnicodeConverter);
>+            this.__utfConverter.charset = "UTF-8";
>+        }
>+        return this.__utfConverter;
>+    },
>+
>+    _utfConverterReset : function() {
>+        this.__utfConverter = null;
>+    },

Don't need this; see below.

>+    encrypt : function(clearTextUCS2, symmetricKey, iv) {
>+        this.log("encrypt() called");
>+
>+        let clearTextUTF8 = this._utfConverter.ConvertFromUnicode(clearTextUCS2);
>+        clearTextUTF8 += this._utfConverter.Finish();
>+
>+        let inputBuffer = new ctypes.ArrayType(ctypes.unsigned_char, clearTextUTF8.length)();
>+        this.byteCompress(clearTextUTF8, inputBuffer);

Seems like what you're trying to do here is straight-up convert a UCS2 JSString into a UTF8 buffer. In which case:

  let inputBuffer = new ctypes.ArrayType(ctypes.unsigned_char)(clearTextUCS2);

will achieve exactly that end, including autosizing the array to the UTF8 string length + 1 and nullterminating it for you. (Be careful of that fact if you use the length later!)

>+        // When using CBC padding, the output size is the imput size rounded

'input'.

>+    decrypt : function(cipherText, symmetricKey, iv) {
>+        this.log("decrypt() called");
>+
>+        let inputUCS2;
>+        if (!cipherText.length)
>+            inputUCS2 = "";
>+        else
>+            inputUCS2 = atob(cipherText);
>+
>+        let input = new ctypes.ArrayType(ctypes.unsigned_char, inputUCS2.length)();
>+        this.byteCompress(inputUCS2, input);

  let input = new ctypes.ArrayType(ctypes.unsigned_char)(inputUCS2);

And then cast it to an array of length equal to 'length - 1', to drop the nullterminator.

Or have _commonCrypt do that, if all the callers use that semantic?

Maybe we should change our converter to not nullterminate. :/

Or... trickier!... have the converter nullterminate but then do the cast for you. Stealth safety... ;)

>+        let outputBuffer = new ctypes.ArrayType(ctypes.unsigned_char, inputUCS2.length)();

'input.length' or 'input.length - 1' would be clearer here, I think.

>+        let outputUTF8 = this.byteExpand(outputBuffer);
>+        let outputUCS2;
>+
>+        try {
>+            outputUCS2 = this._utfConverter.ConvertToUnicode(outputUTF8);
>+        } catch(e) {
>+            this._utfConverterReset();
>+            throw e;
>+        }

  let outputUCS2 = outputBuffer.readString();

>+    _commonCrypt : function (input, output, symmetricKey, iv, operation) {
[...]
>+            let maxOutputSize = output.length;
>+            let tmpOutputSize = new ctypes.int(); // Note 1: NSS uses a signed int here...

NSS fail at using size_t!

>+            let actualOutputSize = tmpOutputSize.value;
>+            let finalOutput = output.addressOfElement(tmpOutputSize.value);
>+            maxOutputSize -= tmpOutputSize.value;

Nit: not calling up tmpOutputSize.value three times would be more efficient. :)

>+            // PK11_DigestFinal sure sounds like the last step for *hashing*, but it
>+            // just seems to be an odd name -- NSS uses this to finish the current
>+            // cipher operation. You'd think it would be called PK11_CipherOpFinal...
>+            let tmpOutputSize2 = new ctypes.unsigned_int(); // Note 2: ...but an unsigned here!
>+            if (this.nss.PK11_DigestFinal(ctx, finalOutput, tmpOutputSize2.address(), maxOutputSize))
>+                throw Components.Exception("cipher finalize failed", Cr.NS_ERROR_FAILURE);

You may want to propagate the failure code in the exception, up to you.

Also, 'finalOutput' seems like an odd name given that it's just the ending bit of the output, right?

>+    generateKeypair : function(passphrase, salt, iv, out_encodedPublicKey, out_wrappedPrivateKey) {
>+        this.log("generateKeypair() called.");
>+
>+        let pubKey, privKey, slot;
>+        try {
>+            // Attributes for the private key. We're just going to wrap and extract the
>+            // value, so they're not critical. The _PUBLIC attribute just indicates the
>+            // object can be accessed without being logged into the token.
>+            let attrFlags = (this.nss.PK11_ATTR_SESSION | this.nss.PK11_ATTR_PUBLIC | this.nss.PK11_ATTR_SENSITIVE);
>+
>+            pubKey  = new this.nss_t.SECKEYPublicKey.ptr();
>+
>+            let rsaParams = new this.nss_t.PK11RSAGenParams();
>+            rsaParams.keySizeInBits = this.keypairBits; // 1024, 2048, etc.
>+            rsaParams.pe = 65537;                       // public exponent.

Can use constructor syntax as noted before.

>+    generateRandomKey : function() {
>+        this.log("generateRandomKey() called");
>+        let encodedKey;
>+        let keygenMech;
>+        let keySize;

Nit: your style elsewhere is to declare on the same line.

>+    generateRandomIV : function() {
>+        this.log("generateRandomIV() called");
>+
>+        let mech = this.nss.PK11_AlgtagToMechanism(this.algorithm);
>+        let size = this.nss.PK11_GetIVLength(mech);
>+
>+        // Temporary buffer to hold the generated data.
>+        let scratch = new ctypes.ArrayType(ctypes.unsigned_char, size)();
>+        if (this.nss.PK11_GenerateRandom(scratch, size))
>+            throw Components.Exception("PK11_GenrateRandom failed", Cr.NS_ERROR_FAILURE);
>+
>+        return this.encodeBase64(scratch.address(), scratch.length);

Couldn't this be simplified by calling generateRandomBytes()?

>+    wrapSymmetricKey : function(symmetricKey, encodedPublicKey) {
>+        this.log("wrapSymmetricKey() called");
>+
>+        // Step 1. Get rid of the base64 encoding on the inputs.
>+
>+        let pubKeyData = this.makeSECItem(encodedPublicKey, true);
>+        let symKeyData = this.makeSECItem(symmetricKey, true);
>+
>+        // This buffer is much larger than needed, but that's ok.
>+        let keyData = new ctypes.ArrayType(ctypes.unsigned_char, 4096)();
>+        let wrappedKey = new this.nss_t.SECItem();
>+        wrappedKey.type = this.nss.SIBUFFER;
>+        wrappedKey.data = keyData;
>+        wrappedKey.len  = keyData.length;

Ctor form?

>+    // Compress a JS string (2-byte chars) into a normal C string (1-byte chars)
>+    // EG, for "ABC",  0x0041, 0x0042, 0x0043 --> 0x41, 0x42, 0x43
>+    byteCompress : function (jsString, charArray) {

Don't need this anymore; see below.

>+    // Expand a normal C string (1-byte chars) into a JS string (2-byte chars)
>+    // EG, for "ABC",  0x41, 0x42, 0x43 --> 0x0041, 0x0042, 0x0043
>+    byteExpand : function (charArray) {

Or this.

>+    encodeBase64 : function (data, len) {
>+        // Byte-expand the buffer, so we can treat it as a UCS-2 string
>+        // consisting of u0000 - u00FF.
>+        let expanded = "";
>+        let intData = ctypes.cast(data, ctypes.uint8_t.array(len).ptr).contents;
>+        for (let i = 0; i < len; i++)
>+            expanded += String.fromCharCode(intData[i]);

ctypes behavior has changed since you wrote this; casting to unsigned_char will convert to an integer as well.

I assume that 'data' can have *any* value 0x00 -- 0xFF, in which case ctypes' UTF8 --> UTF16 conversion won't work for you, and you have to do it your way. Once we have the new charset API this will become much more efficient.

If you can guarantee that 'data' can't be interpreted as UTF8, then this is equivalent to

  let expanded = ctypes.cast(data, ctypes.unsigned_char.array(len).ptr).contents.readString();

which is about what it'd look like with the new charset API, too.

>+    makeSECItem : function(input, isEncoded) {
>+        if (isEncoded)
>+            input = atob(input);
>+        let outputData = new ctypes.ArrayType(ctypes.unsigned_char, input.length)();
>+        this.byteCompress(input, outputData);

  let outputData = new ctypes.ArrayType(ctypes.unsigned_char)(input);

While this will UTF16 -> UTF8, in this case it's the same behavior as byteCompress.

>+        let secItem = new this.nss_t.SECItem();
>+        secItem.type = this.nss.SIBUFFER;
>+        secItem.data = outputData;
>+        secItem.len  = outputData.length;

  let secItem = new this.nss_t.SECItem(this.nss.SIBUFFER, outputData, outputData.length);

But if you think what you have is more readable, that's cool.

>+    _deriveKeyFromPassphrase : function (passphrase, salt) {
>+        this.log("_deriveKeyFromPassphrase() called.");
>+        let passItem = this.makeSECItem(passphrase, false);
>+        let saltItem = this.makeSECItem(salt, true);
>+
>+        // http://mxr.mozilla.org/seamonkey/source/security/nss/lib/pk11wrap/pk11pbe.c#1261

Seamonkey? Srsly?

>+    _wrapPrivateKey : function(privKey, passphrase, salt, iv) {
[...]
>+            // Use a buffer to hold the wrapped key. NSS says about 1200 bytes for
>+            // a 2048-bit RSA key, so a 4096 byte buffer should be plenty.
>+            let keyData = new ctypes.ArrayType(ctypes.unsigned_char, 4096)();
>+            wrappedKey = new this.nss_t.SECItem();
>+            wrappedKey.type = this.nss.SIBUFFER;
>+            wrappedKey.data = keyData;
>+            wrappedKey.len  = keyData.length;

Ctor form here?

How much of this is covered by existing unit tests? In particular, dealing with internationalized strings?

Comment 22

[Justin Dolske [:Dolske]]

(In reply to comment #21)

> >+        this.nss_t.PK11SymKey = ctypes.void_t;
> >+// XXX maybe better as ctypes.PointerType("PK11SymKey");
> 
> You mean ctypes.PointerType("PK11SymKey*"), and then call it PK11SymKeyPtr.
> That'd work, and give you more typesafety than you have now... but as said
> above, you probably want opaque struct support.

Yeah, I'd really like something just like foo = ctypes.OpaqueType("foo"), so that I can do "foo.ptr" for consistency with other types. [I'd even like ctypes.void.ptr, but iirc that doesn't work?]


> // XXX: "rsa" et al into a union here!
> 
> Is 'rsa' the only one you're interested in? I assume so, and that all the union members are the same size. If not, we need unions :)

RSA is the only one Weave is currently interested in.

The members are unfortunately different sizes. I think the alignment works out ok -- I remember talking with you about the issues and it seemed to check out, though I've since forgotten the details.

We're probably getting away with this usage because WeaveCrypto never allocates one of these itself, NSS always does so this is just a template for accessing the thing it returns. (I'm not sure if there's any NSS usage that has the caller allocate, though that's no excuse to live on the edge!).


> >+        // SECStatus PK11_RandomUpdate(void *data, size_t bytes);
> >+        this.nss.PK11_GenerateRandom = nsslib.declare("PK11_GenerateRandom",
> >+                                                      ctypes.default_abi, this.nss_t.SECStatus,
> >+                                                      ctypes.unsigned_char.ptr, ctypes.int);
> 
> Do you want void* or unsigned char* here? (If the C prototype uses void* but
> unsigned char* is clearer, then hey, go for it.)

Oops, this is a cut'n'paste failure for the comment. I copied PK11_RandomUpdate (it even says right there!) instead of PK11_GenerateRandom. The code is correct.


>   let inputBuffer = new ctypes.ArrayType(ctypes.unsigned_char)(clearTextUCS2);
> 
> will achieve exactly that end, including autosizing the array to the UTF8
> string length + 1 and nullterminating it for you. (Be careful of that fact if
> you use the length later!)

Yeah, I do rely on inputBuffer.length later. I'll look at this again after updating the patch, I might be able to user length-1 if all the callers do the same thing.

> Maybe we should change our converter to not nullterminate. :/

I'd suspect that most callers will want null termination, but it might be nice to have an optional way to not get it. Maybe |new blahblah("foopy", true)|?

Alternatively, we had bounced around the idea at one point of allowing an array's .length to be shrunk. That would help in this case... Null terminate, and then foo.length -= 1.


> >+            if (this.nss.PK11_DigestFinal(ctx, finalOutput, tmpOutputSize2.address(), maxOutputSize))
> >+                throw Components.Exception("cipher finalize failed", Cr.NS_ERROR_FAILURE);
> 
> You may want to propagate the failure code in the exception, up to you.

Not sure what you mean here.

> Also, 'finalOutput' seems like an odd name given that it's just the ending bit
> of the output, right?

Well, it _is_ the output from DigestFinal...

> >+        // http://mxr.mozilla.org/seamonkey/source/security/nss/lib/pk11wrap/pk11pbe.c#1261
> 
> Seamonkey? Srsly?

Heh. It's an old comment from the C++, maybe old enough that the seamonkey CVS was still the only one that indexed NSS.


> How much of this is covered by existing unit tests? In particular, dealing with
> internationalized strings?

The existing unit tests are fairly extensive, and tickle a lot of edge cases.

But they are weak wrt international strings. I'll add some more tests to make sure such strings roundtrip correctly.

Comment 23

[Justin Dolske [:Dolske]]

Attachment: Patch v.9

Updated with most nits, still need to:
 - look at having encrypt/decrypt use ctypes string conversion (length+/-1)
 - ask dwitte some questions about the byteCompress/byteExpand review comments
 - add some tests for international strings

Otherwise ready for mconnor's gentle review pass. ;)

Comment 24

[Mike Connor [:mconnor]]

Comment on attachment 439442 [details] [diff] [review]
Patch v.9

>+    init : function() {
>+        try {
>+            // Preferences. Add observer so we get notified of changes.
>+            this.prefBranch = Services.prefs.getBranch("extensions.weave.log.");
>+            this.prefBranch.QueryInterface(Ci.nsIPrefBranch2);
>+            this.prefBranch.addObserver("", this.observer, false);

If you pass "cryptoDebug" instead of "" you'll filter out everything else, and avoid a bunch of unreleated changes causing you to update .debug for no reason.

>+    initNSS : function() {
>+          case "WINNT":
>+          case "Darwin":
>+          case "Linux":

Please add any platforms we currently support, since we're currently shipping with support for those platforms.

>+    decrypt : function(cipherText, symmetricKey, iv) {

>+        let inputUCS2;
>+        if (!cipherText.length)
>+            inputUCS2 = "";
>+        else
>+            inputUCS2 = atob(cipherText);

let inputUCS2 = "";
if (cipherText.length);
  inputUCS2 = atob(cipherText);

>+        let outputUCS2;
>+
>+        try {
>+            outputUCS2 = this._utfConverter.ConvertToUnicode(outputUTF8);
>+        } catch(e) {
>+            this._utfConverterReset();
>+            throw e;
>+        }
>+
>+        return outputUCS2;

why not just do this?

try {
  return this._utfConverter.ConvertToUnicode(outputUTF8);
} catch(e) {
  this._utfConverterReset();
  throw e;
}



>+        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);
>+
>+            randKey = this.nss.PK11_KeyGen(slot, keygenMech, null, keySize, null);
>+            if (randKey.isNull())
>+                throw Components.Exception("PK11_KeyGen failed.", Cr.NS_ERROR_FAILURE);

>+            if (this.nss.PK11_ExtractKeyValue(randKey))
>+                throw Components.Exception("PK11_ExtractKeyValue failed.", Cr.NS_ERROR_FAILURE);

As discussed, this is a weird API, non-zero retval == failure.  Adding a comment to that effect seems sane here.

>+    generateRandomIV : function() {
>+        this.log("generateRandomIV() called");
>+
>+        let mech = this.nss.PK11_AlgtagToMechanism(this.algorithm);
>+        let size = this.nss.PK11_GetIVLength(mech);
>+
>+        return this.generateRandomBytes(size);
>+    },

A part of me wants this to be Mardak-style, don't bother with temp local vars... not sure it's any better though.  It's a different form of evil? :D

return this.generateRandonBytes(
         this.nss.PK11_GetIVLength(
            this.nss.PK11_AlgtagToMechanism(
              this.algorithm
            );
         );
       );


>+    makeSECItem : function(input, isEncoded) {
>+        if (isEncoded)
>+            input = atob(input);
>+        let outputData = new ctypes.ArrayType(ctypes.unsigned_char, input.length)();
>+        this.byteCompress(input, outputData);
>+
>+        let secItem = new this.nss_t.SECItem(this.nss.SIBUFFER, outputData, outputData.length);
>+        return secItem;

return new this.nss_t.SECItem(this.nss.SIBUFFER, outputData, outputData.length);

Comment 25

[Justin Dolske [:Dolske]]

(In reply to comment #24)

> >+            this.prefBranch.addObserver("", this.observer, false);
> 
> If you pass "cryptoDebug" instead of "" you'll filter out everything else

Yeah; this is just a leftover from thinking there might be multiple prefs to watch.

Comment 26

[Justin Dolske [:Dolske]]

(In reply to comment #23)

> still need to:
>  - add some tests for international strings

Oops, these already exist.

Comment 27

[Justin Dolske [:Dolske]]

Attachment: Patch v.10

Should be good to go now! Going to test again on Windows and Linux x64 prior to checkin.

Comment 28

[Justin Dolske [:Dolske]]

Tested on OS X 10.6, Windows 7, and Linux x64.

Pushed http://hg.mozilla.org/labs/weave/rev/1a9b2498786d

\o/

Comment 29

[Ed Lee :Mardak]

Landed on 1.2.x branch:
http://hg.mozilla.org/labs/weave/rev/34d265dbfeb2

Only had one slight fuzz warnings but no changes needed.