Page MenuHomePhabricator

Suffix web olm notifs session with cookieID to avoid race condition during multiple simultaneous log-in processes.
ClosedPublic

Authored by marcin on Dec 8 2023, 4:19 AM.
Tags
None
Referenced Files
F3585181: D10245.diff
Sun, Dec 29, 7:23 PM
Unknown Object (File)
Thu, Dec 26, 1:12 AM
Unknown Object (File)
Wed, Dec 25, 11:24 PM
Unknown Object (File)
Mon, Dec 23, 3:27 AM
Unknown Object (File)
Mon, Dec 23, 3:27 AM
Unknown Object (File)
Mon, Dec 23, 3:27 AM
Unknown Object (File)
Mon, Dec 23, 3:27 AM
Unknown Object (File)
Mon, Dec 23, 3:27 AM
Subscribers

Details

Summary

This differential implements solution for a bug described in https://linear.app/comm/issue/ENG-5911/olmbad-message-mac-on-web. The solution works as follows:

  1. Web client creates olm session and corresponding encryption key and persists it under key that is suffixed with its cookieID.
  2. When notifications arrives all keys in IndexedDB are retrieved and filtered against those that keep olm sessions and their encryption keys.
  3. Keys are sorted lexicographically and last ones are used to actually perform decryption. This means that for decryption we will use olm session that was created by lexicographically largest cookie ID. We have strong reasons to believe that keyserver uses olm session associated with the largest cookie ID for encryption.
  4. Other sessions are deleted

It is important to review this code closely since it implements quite complex solution for rather infrequent bug.

Test Plan
  1. Open three pages with logged-out modal. Paste credentials for the same user in each and start log in process in each card. Initially this was the way to reproduce the issue.
  2. Open developer tools in one page and see that there are multiple sessions and encryption keys and each of them is suffixed with cookie id that exists in your MariaDB instance.
  3. Send notification. Ensure that:
    • it is correctly decrypted
    • once it is received there is just one olm session in IndexedDB and it is the one that initially had largest cookie ID

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-5911
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin edited the test plan for this revision. (Show Details)
web/push-notif/notif-crypto-utils.js
310 ↗(On Diff #34411)

This scenario can occur when notification is received but there is olm session creation process in progress - for example encryption key was created and persisted but olm session hasn't been persisted yet.
I spent some time thinking what should we do in such case. Intuitively we could use encryption key and olm session associated with lower cookieID (the highest cookie ID for which we have both encryption key and olm session). but in fact we can't assume that the keyserver used this session to encrypt notification. I actually think that if there is olm session creation process in progress then the keyserver has already associated device_token with cookie ID that we are creating session for. In such case the keyserver will probably see that there is no olm session for this cookie ID and notificaion won't be encrypted.

350 ↗(On Diff #34411)

We need to cover the case of users that won't log-out and log-in after this code is landed and release. They will have session and encryption key without cookie ID suffix. We could return undefined or empty string from this function but intuitively I thought that returning some string (such as no cookie) should be safest.

marcin requested review of this revision.Dec 8 2023, 4:56 AM
marcin added a subscriber: inka.

I talked to @inka and was advices to refactor this code so that session creator function takes keyserver id as an argument and ashoatKeyserverID is hardcoded in web/socket.react.js

marcin edited the test plan for this revision. (Show Details)

Address inka's advice

michal requested changes to this revision.Dec 11 2023, 3:13 AM

Keys are sorted lexicographically

Are you sure about that? Aren't the cookie ids numerically increasing?

web/push-notif/notif-crypto-utils.js
284–285 ↗(On Diff #34438)

Nit: I would have preferred for them to have the same naming convention (either either DBLabel or DBKey)

296 ↗(On Diff #34438)

It's not good practice in JS to throw non-Error values. In particular the try catch in decryptWebNotification depends on the error value to have .message.

348 ↗(On Diff #34438)

This won't give us any more type safety, but is a bit more explicit.

This revision now requires changes to proceed.Dec 11 2023, 3:13 AM

Keys are sorted lexicographically

Are you sure about that? Aren't the cookie ids numerically increasing?

In this code we use cookieID strings which are stringified numbers. We sort stringified numbers lexicographically which has the same effect as sorting original numbers.

  1. Throw Error instance instead of raw string.
  2. Rename encryptionKeyDBLabel -> encryptionKeyDBKey
  3. Enhance typing of getCookieIDFromOlmDBKey

@michal point is valid - if cookieIDs differ in length lexicographic sort can fail to match numerical sort. We need to address this issue by temporarily converting cookieIDs to numbers.

Temporarily convert cookieIDs to numbers during sorting stage.

michal added inline comments.
web/push-notif/notif-crypto-utils.js
358–367

We probably don't need to keep the original string cookie here and just convert the resulting number to string again, but that's fine.

This revision is now accepted and ready to land.Dec 14 2023, 7:55 AM
web/push-notif/notif-crypto-utils.js
358–367

Why do we need to convert cookieID to string back? This function doesn't return any numbers. It returns string keys suffixed by cookieID in the same form as they entered this function. The only difference is that they are sorted according to numerical order of cookieIDs they are suffixed with.

Address inka's advice

Thank you!