[PM-5750] Make encrypt service decrypt faster by replacing constant time equal

[quexten]

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [x] Other

Objective

Yet another partial fix for #1597

Don't merge without discussing first please
During profiling, another discovered slow point in vault "decryption" (unlock) was comparing the macs for the encstrings. For each encstring, the mac itself is calculated, and then double randomized hmac verification is done, in an attempt to protect against timing attacks. This is quite costly, in terms of compute, since for large vaults, this is tens of thousands, if not hundreds of thousands of hmacs being calculated, buffers being allocated and garbage collected, and so on.

Double randomized hmac is usually done when there is an actual timing channel, like a webserver verifying a session cookie. For decrypting ciphers, I don't see this timing channel, so I'm not sure in what scenario an attacker would be able to use this. But again, I'm unsure here, please let me know if I'm overlooking something here.

Again, as with #7582, the results are quite noisy and dependent on the browser, so I will refrain from claiming concrete, general improvement numbers here, but for one sample run in Firefox, this PR brought decryption time from 6.4 seconds to 4 seconds, combining with #7582 brought it to 2 seconds for my 10k cipher test vault.

Code changes

• encrypt.service.implementation.ts: Replace cryptoservice equal with non-cryptographic equality and add note about no timing channel

Screenshots

Before you submit

• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team
• Ensure that all UI additions follow WCAG AA requirements

[bitwarden-bot]

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-5750

[quexten]

Like #7582 this also needs more reliable benchmarks.

[quexten]

Also note that the same non-constant time comparison is used for the checksum uris:

clients/libs/common/src/vault/models/domain/login-uri.ts

Lines 53 to 54 in af0d2f5

	const remoteChecksum = await this.uriChecksum.decrypt(orgId, encKey);
	return remoteChecksum === localChecksum;

(Even though there is technically a very tiny timing channel in the above case, since the time until an icon request happens varies. I doubt it can be used though since the remote checksum in this case is not arbitrarily modifiable)

[MGibson1]

Sorry, hit the close and comment when I wanted to cancel...

Also note that the same non-constant time comparison is used for the checksum uris:

To clarify, this isn't susceptible to timing since any modification would be done to the remote uriChecksums encrypted form, which is HMAC validated against middle-man fiddling with constant time comparison upon decryption in line 53.

[ccben87]

Is there a hold up on this? I'm really keen to see this get implemented as the performance impact we are seeing with over 11k ciphers is significantly impacting us.

[quexten]

Is there a hold up on this? I'm really keen to see this get implemented as the performance impact we are seeing with over 11k ciphers is significantly impacting us.

@ccben87 Just beware that while #6465 mostly just needs the feature flag integration, this PR needs actual discussion to make sure no vulnerability is introduced. While I believe the double hmac verification is not needed for bulk decryption, because there is no usable timing channel, it needs to be independently verified.

There might be alternative approaches that fit better to reduce the time spent on MACs (leverage some browser API to do the constant time comparison without the expensive double HMAC, or reduce the amount of MACs needed to be done in general by doing MAC per cipher instead of per EncString, though the latter would be a major cryptographic change, with other cryptographic benefits / potentially additional software architectural complexity).

And finally, I'm not entirely sure if it's still needed after #6465. #6465 alone might be enough to make unlock nicely usable for most users.