546783 - Make sure various randomness is security OK

Status: RESOLVED FIXED

(Cloud Services :: General, defect)

Description

[Ed Lee :Mardak]

One spot outside of NSS is Utils.makeGUID:
http://hg.mozilla.org/labs/weave/file/1.0/source/modules/util.js#l150

Comment 1

[Mike Connor [:mconnor]]

Needs to be done in 1.3.

Comment 2

[Ed Lee :Mardak]

David, can you go over these uses of random to make sure they're okay?

http://mxr.mozilla.org/labs-central/search?string=random&find=%2Fweave%2Fsource

engines:307 syncID is an opaque token that triggers a resync if it changes
engines:417 symmetric key used to encrypt data (bookmarks)
keys:143 salt used to convert the passphrase into a symkey
keys:144 iv used with encrypting the private key
crypto:66 iv used with encrypting data (bookmarks)
util:163 random numbers to generate an opaque GUID
clients:115 opaque token to identify a user/computer
service:173 just like the engine's syncID but at a service level
service:239 sanity check to make sure crypto is working
service:407 random backoff to avoid simultaneous retry
service:1329 same as above with random backoff

Comment 3

[David Chan [:dchan]]

Most of the uses of random look okay. The only concern I have is with the GUID generation function makeGUID in util.js . 

61 bits gives a 50% change for a collision after ~2^31 GUIDs . It may not be a concern in your uses, but I believe there will be collisions during the life time of Weave (assuming the 1 million user target and a couple hundred sync items per user) .

I would recommend increasing the length of the GUID to reduce the chance of collisions. I haven't examined the algorithm enough to say whether it is biased or not.

util:163 random numbers to generate an opaque GUID
See above


engines:307 syncID is an opaque token that triggers a resync if it changes
clients:115 opaque token to identify a user/computer
service:173 just like the engine's syncID but at a service level
The above use Utils.makeGUID() to generate tokens/values. makeGUID() is good enough for the purposes listed above.


engines:417 symmetric key used to encrypt data (bookmarks)
keys:143 salt used to convert the passphrase into a symkey
keys:144 iv used with encrypting the private key
crypto:66 iv used with encrypting data (bookmarks)
The above use Svc.Crypto functions which are based on NSS from my understanding. NSS is FIPS 140-2 compliant, so there isn't a concern.


service:239 sanity check to make sure crypto is working
No concerns here


service:407 random backoff to avoid simultaneous retry
service:1329 same as above with random backoff
Cryptographically secure random isn't needed so this is fine.

Comment 4

[Mike Connor [:mconnor]]

(In reply to comment #3)
> Most of the uses of random look okay. The only concern I have is with the GUID
> generation function makeGUID in util.js . 
> 
> 61 bits gives a 50% change for a collision after ~2^31 GUIDs . It may not be a
> concern in your uses, but I believe there will be collisions during the life
> time of Weave (assuming the 1 million user target and a couple hundred sync
> items per user) .

We care about collisions only within user+collection (i.e. a collision between history and password records wouldn't matter, or between two users within a collection).  We know there's a tradeoff here, but given the scope, we feel like the tradeoff is acceptable.