1191776 - Crypto library for Syncto client

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect)

Comment 1

[Michiel de Jong [:michielbdejong]]

Got a first approximation done: https://github.com/michielbdejong/fxsync-webcrypto/issues
Main thing that's not working yet is that for some reason, hmac verifications always return false (even though it can decrypt the record successfully). And I'm a bit worried about performance. But we'll see.

Comment 2

[Michiel de Jong [:michielbdejong]]

hmac now works as well. The library needs a bit of cleanup, more unit tests, and performance testing, but otherwise it seems to be working well now.

Comment 3

[Michiel de Jong [:michielbdejong]]

See https://github.com/michielbdejong/fxsync-webcrypto/pull/17 - @seanlee can you give me some feedback on the code I wrote there? Thanks!!

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Hi Michiel,

I leave a few comments in your commit. Let's discuss them there.

Comment 5

[Michiel de Jong [:michielbdejong]]

As discussed in the meeting just now, this code should land into the Synchronizer app, so this bug depends on the skeleton of that app, bug 1196232

Comment 6

[Michiel de Jong [:michielbdejong]]

r? ferjm https://github.com/mozilla-b2g/gaia/pull/31567

Comment 7

[Michiel de Jong [:michielbdejong]]

This is my first real PR, so there's probably a lot to comment on. :) I would especially like to improve my skills at three things:
* code style (how is the code split into functions, usage of promises/exceptions, variable naming, ...)
* code structure (how do the .js files fit together?)
* test design (what could possibly go wrong, what to test for, what else to test, especially wrt security)

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia PR

Excellent work Michiel!

I left some comments on the PR. Mostly nits.

I'd like someone else more familiar with the Sync crypto stuff to look at this.

Comment 9

[Michiel de Jong [:michielbdejong]]

@ferjm processed your review comments, thanks! I'm still not sure if the line-wrapping is now perfect everywhere, but I tried to reflect your corrections and make it consistent throughout the patch. I made notes of the decisions, so I can apply them already to all my future PRs. Same for using arrow functions where possible. I didn't see how to get rid of the private functions on the FxSyncWebCrypto object though. https://github.com/mozilla-b2g/gaia/pull/31567/commits

Comment 10

[Michiel de Jong [:michielbdejong]]

About security considerations:
We are only implementing a new client in the existing Firefox Sync system, so the security considerations of Firefox Sync in general are out of scope here, right? We only need to worry about possible bugs in our implementation, what worst-case impact they would have, and how we could mitigate them. Furthermore, this patch only implements one moving part of how this new Firefox Sync client works. So all of the following are out of scope here:

* Threat models of the Firefox Sync in general
* Threat models of Firefox OS as a Firefox accounts client
* Sandboxing of the Sync app's runtime memory contents on the Firefox OS device
* Safeguarding the Sync app's source code from being altered by an attacker

So for the Sync feature in general, that leaves us with:

# Safeguarding Sync from being activated on the device against the user's will
* An attacker could convince a user to install a privileged app which has permissions to write to settings. Can we lock down the settings for sync so they can only be changed from Mozilla-trusted code?


# Safeguarding kB from being requested from the token server
* An attacker could convince a user to install a privileged app which has permissions to request kB from the token server. Can we lock down the access to the token server so that our Sync app can request it, but no other app can?

# kB or decrypted data leaking out of the Sync app
* The Sync app should not store kB on for instance the SD-card of a device, where other apps might be also have access.
* After decrypting downloaded data, the Sync app's data adapters should store the data where it needs to be stored, and can store it within the origin of the Sync app, but should not store it anywhere else.

# Data from outside the Sync app ending up on the Sync server
* The Sync app should only upload sanitized data from the appropriate trusted sources (e.g. to the history collection, only upload items from the device's browser history, to the passwords collection, only upload items from the device's passwords store, etc.)

# If there are bugs in this patch
* For decryption, the impact would be limited to corrupt data showing up in the device's data stores. The tests in this patch check that if decryption is unsuccessful, it is reported as such, so then the Sync app knows it should not use/store those results. It is unlikely that the calls to WebCrypto report success with an incorrect result (decryption will only be successful if nothing goes wrong, if anything goes wrong, the result will immediately be recognizable as garbage).
* For encryption, three situations could occur:
  * We upload data that no client can decrypt.
  * We upload data that only our client can decrypt, but other clients cannot.
  * We upload data that all clients can decrypt, but that is easy to decrypt by an attacker who has access to the ciphertext.

@rnewman I'm particularly interested in advice about that last risk. For instance, if the IV that we generate is not random enough, maybe? Would that be a risk? As you can see in the patch, we use crypto.getRandomValues for that.

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8655365 [details] [review]
Gaia PR

Great work Michiel!

Comment 12

[Michiel de Jong [:michielbdejong]]

Review round 2:

https://github.com/michielbdejong/gaia/commit/063fa088748bc93822a384101887df302dee606b

Let me know if that answers your latest comments and if there's anything else I can improve.

Note: we still need some expert eyes on the security consideration before merge, but if that's the only thing left then I can already squash and rebase.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8655365 [details] [review]
Gaia PR

I gave this a very rudimentary once-over, not what I'd call a thorough review.

It looks sane enough, and the test coverage seems good.

If the docs aren't lying, then I believe RandomSource is adequate as IV material.

I would recommend liberally applying Object.freeze on your public objects, as well as the closure-eval approach to modularity that you use now, just for sanity's sake.

I don't see any code in this PR that does key *generation*. If you get to that point, you should get careful review to make sure the keys aren't weak. You will need to do key generation eventually to completely handle interaction with a storage server.

Comment 14

[Michiel de Jong [:michielbdejong]]

@rnewman Thanks a lot for your comments!

(In reply to Richard Newman [:rnewman] from comment #13)
> I don't see any code in this PR that does key *generation*. If you get to
> that point, you should get careful review to make sure the keys aren't weak.
> You will need to do key generation eventually to completely handle
> interaction with a storage server.

Indeed, right now we only support syncing with existing FxSync accounts.

Comment 15

[Michiel de Jong [:michielbdejong]]

Attachment: Review round 3

Applied latest comments from Fernando and Richard, squashed, rebased on master.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8659203 [details] [review]
Review round 3

"let" is not enabled in Gaia yet. Please, go back to "var". The rest looks good.

Comment 17

[Michiel de Jong [:michielbdejong]]

Attachment: let -> var

(do you want me to squash this?)

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8659841 [details]
let -> var

Thank you! Yes, please, squash the commits.

Comment 19

[Michiel de Jong [:michielbdejong]]

Great! :) Rebased and squashed.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/b467811f80e3000ca35e30821bc09f606cf93ee8