1456659 - Sync should use a constant time comparison for the HMAC verification.

Status: RESOLVED WONTFIX

(Firefox :: Sync, enhancement, P2)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

Specifically, this check https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/services/sync/modules/record.js#161 should be done using a constant time comparison.

From IRC:

<rfkelly> if we have a good constant-time comparison in the desktop client, I think we should change that code to use it from a hygiene perspective
<rfkelly> if the client has some predictable behaviour that it takes in response to a bad hmac, you can imagine a malicious server handing it known bad BSOs to decrypt, observing the clients behaviour in response, and slowly trying to recover the key through timing info
<rfkelly> it seems extraordinarily improbably in practice, but if it doesn't cost us much to prevent it, well...

That said, I have no idea where to find such a function. It's not part of WebCrypto, and AFAICT before we used that we called into NSS via jsctypes D:

Comment 1

[Ryan Kelly [:rfkelly]]

To say a few more words, I'm wondering whether there's a way for a server to do something like:

* Feed the client bad BSOs in a way that triggers a retry loop, which would allow it to harvest HMAC timing data from the clients retry requests
* Use the HMAC timing leak to forge a /crypto/keys record with a valid HMAC
* Feed the forged /crypto/keys record to the client, causing it to start using encryption keys known to the server

That doesn't seem particular plausible, but I'm not prepared to rule it out entirely.

Comment 2

[Lina Butler [:lina]]

Attachment: Bug 1456659 - Use `crypto.subtle.verify` to verify Sync record HMACs. f?tcsc

Review commit: https://reviewboard.mozilla.org/r/240216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240216/

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8971473 [details]
Bug 1456659 - Use `crypto.subtle.verify` to verify Sync record HMACs. f?tcsc

I played around with this a bit on the train.

`crypto.subtle.verify` uses a constant-time comparison (https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/dom/crypto/WebCryptoTask.cpp#1097-1100), and switching us over doesn't seem too cumbersome...the hardest part is figuring out the different encodings and types of the arguments. Array buffers? Binary strings? Base64-encoded strings? Hex-encoded strings, for extension storage?

Also, I don't think many (any?) of our xpcshell tests use real crypto, so this will need a full TPS run. Try is suspiciously green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f016945d5e88d20e14f3e533a0c0c7f62d4afff6

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8971473 [details]
Bug 1456659 - Use `crypto.subtle.verify` to verify Sync record HMACs. f?tcsc

https://reviewboard.mozilla.org/r/240216/#review246486

LGTM, I raised some concerns in IRC but they turned out to be unfounded.

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

What is the behavior here if the HMAC string is the wrong size?

Comment 6

[Lina Butler [:lina]]

I'm not actively working on this bug, and verify_hmac_string in our Rust adapter does a constant-time comparison, so I think we can WONTFIX unless there's a compelling reason to keep it open.

(In reply to Thom Chiovoloni [:tcsc] from comment #5)

What is the behavior here if the HMAC string is the wrong size?

Web Crypto won't call into NSS at all, and will return false. I guess that leaks timing based on the string length, but seems like that's acceptable, based on https://bugzilla.mozilla.org/show_bug.cgi?id=998802#c12.