Status

()

defect
RESOLVED FIXED
10 years ago
10 months ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

unspecified
1.3b1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-1.2.3])

Attachments

(1 attachment, 9 obsolete attachments)

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).
Blocks: 508118
Depends on: 429551
Blocks: 522091
Posted patch Patch v.1 (WIP) (obsolete) — Splinter Review
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.)
Depends on: 531718
Posted patch Patch v.2 (WIP) (obsolete) — Splinter Review
This implements enough of WeaveCrypto for the test_crypto_random.js tests to pass. Yay!
Attachment #414891 - Attachment is obsolete: true
Depends on: 513788
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?
You could use the preprocessor for this, or we can add a ctypes API to do it.
Posted patch Patch v.3 (obsolete) — Splinter Review
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.
Attachment #415957 - Attachment is obsolete: true
Depends on: 535685
Posted patch Patch v.4 (WIP) (obsolete) — Splinter Review
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.
Attachment #417856 - Attachment is obsolete: true
Depends on: 518130
Sticking a non-specific milestone on this so we don't keep triaging it, but this is wanted ASAP. :)
Target Milestone: --- → Future
Blocks: 547603
Bug 513788 is now landed, so this can proceed.

If any additional features or tweaks are needed for this, please ask. :)
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).
Target Milestone: Future → 2.0
Posted patch Patch v.5 (obsolete) — Splinter Review
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.]
Attachment #418301 - Attachment is obsolete: true
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).
Posted patch Patch v.6 (obsolete) — Splinter Review
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.
Attachment #436404 - Attachment is obsolete: true
(The various exports are still needed for tests, of course)
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.]
Posted patch Patch v.7 (obsolete) — Splinter Review
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).
Attachment #436634 - Attachment is obsolete: true
so is this going to land on the 3.6 branch?
It can't, unless we backport the new ctypes APIs to 3.6.
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.
Attachment #436820 - Flags: feedback?(mconnor)
Posted patch Patch v.8 (obsolete) — Splinter Review
Attachment #436820 - Attachment is obsolete: true
Attachment #438641 - Flags: review?
Attachment #436820 - Flags: feedback?(mconnor)
Attachment #438641 - Flags: review?(mconnor)
Attachment #438641 - Flags: review?(dwitte)
Attachment #438641 - Flags: review?
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.
Blocks: 559536
Blocks: 559602
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?
(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.
Posted patch Patch v.9 (obsolete) — Splinter Review
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. ;)
Attachment #438641 - Attachment is obsolete: true
Attachment #439442 - Flags: review?
Attachment #438641 - Flags: review?(mconnor)
Attachment #438641 - Flags: review?(dwitte)
Attachment #439442 - Flags: review? → review?(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);
Attachment #439442 - Flags: review?(mconnor) → review+
(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.
(In reply to comment #23)

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

Oops, these already exist.
Posted patch Patch v.10Splinter Review
Should be good to go now! Going to test again on Windows and Linux x64 prior to checkin.
Attachment #439442 - Attachment is obsolete: true
Tested on OS X 10.6, Windows 7, and Linux x64.

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

\o/
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: 2.0 → 1.3
Blocks: 561005
Target Milestone: 1.3 → 1.3b1
Blocks: 562419
Blocks: 562431
Whiteboard: [fixed-1.2.3]
Landed on 1.2.x branch:
http://hg.mozilla.org/labs/weave/rev/34d265dbfeb2

Only had one slight fuzz warnings but no changes needed.
Blocks: 561480
Blocks: 578436
Component: Firefox Sync: Crypto → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.