Page MenuHomePhabricator
Differential D13757

[lib] Restart session when any Olm error is thrown
ClosedPublic
Actions

Authored by angelika on Oct 21 2024, 6:20 AM.
Tags
None
Referenced Files
F3331737: D13757.id45300.diff
Wed, Nov 20, 9:09 PM
F3331736: D13757.id45289.diff
Wed, Nov 20, 9:09 PM
F3331723: D13757.id.diff
Wed, Nov 20, 9:09 PM
F3331712: D13757.diff
Wed, Nov 20, 9:09 PM
Unknown Object (File)
Mon, Nov 18, 4:51 PM
Unknown Object (File)
Sun, Nov 17, 5:31 AM
Unknown Object (File)
Sun, Nov 10, 9:23 AM
Unknown Object (File)
Fri, Nov 8, 11:07 PM
View All 42 Files
Subscribers
ashoat
tomek

Details

Reviewers
kamil
tomek
ashoat
Commits
rCOMM4723c3b6eb1d: [lib] Restart session when any Olm error is thrown
Summary

This diff restarts session when any Olm error is thrown by throwing an exception with OLM_ERROR flag set if error occured when decrypting the message.
If flag is included in the exception then the session is reset.

https://linear.app/comm/issue/ENG-9729/session-not-reset-after-getting-bad-message-mac

Test Plan
  1. Simulate BAD_MESSAGE_MAC by changing the last 3 characters of the message passed to session.decrypt() in the code
  2. Verify in the logs that BAD_MESSAGE_MAC is thrown and session is reset.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

angelika created this revision.Oct 21 2024, 6:20 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptOct 21 2024, 6:20 AM
angelika added reviewers: kamil, ashoat.Oct 21 2024, 6:21 AM
Harbormaster completed remote builds in B32315: Diff 45289.Oct 21 2024, 6:38 AM
angelika requested review of this revision.Oct 21 2024, 6:38 AM
angelika edited reviewers, added: tomek; removed: ashoat.Oct 21 2024, 7:12 AM
ashoat added inline comments.Oct 21 2024, 9:05 AM
lib/tunnelbroker/use-peer-to-peer-message-handler.js
307 ↗(On Diff #45289)

I realize that this issue existed before, but there is a risk here... if the code throws a primitive (eg. throw 5), then we will get an error when trying to dereference 5.message

This can be mitigated by using getMessageForException. Given this pattern exists in the DMs code in more places than just here, perhaps it should be handled in a separate diff.

native/cpp/CommonCpp/CryptoTools/Session.cpp
177 ↗(On Diff #45289)

Given that OLM_ERROR has some magical importance, I think we should do more to make sure that somebody doesn't change it.

When we have two constants that need to match between two parts of the codebase, usually we add a code comment referring to the other location, and telling the reader to not change one location without changing the other.

Could you:

  1. Factor this out into a constant
  2. Add a code comment to the constant's definition, as well as the definition in olm-utils.js. Each comment should point to the other location, and should caution the reader to not change one without changing the other
angelika updated this revision to Diff 45300.Oct 21 2024, 1:19 PM

Extract OLM_ERROR_FLAG to constant and add comments

angelika added a child revision: D13761: [lib] Use getMessageForException instead of e.message in DM related code.Oct 21 2024, 1:28 PM
ashoat accepted this revision.Oct 21 2024, 1:35 PM
ashoat added inline comments.
native/cpp/CommonCpp/CryptoTools/Session.h
20 ↗(On Diff #45300)

Should this be static?

This revision is now accepted and ready to land.Oct 21 2024, 1:35 PM
Harbormaster completed remote builds in B32317: Diff 45300.Oct 21 2024, 1:38 PM
angelika updated this revision to Diff 45315.Oct 22 2024, 5:55 AM

Make constant static

Harbormaster completed remote builds in B32326: Diff 45315.Oct 22 2024, 6:30 AM
kamil accepted this revision.Wed, Oct 23, 6:41 AM
kamil added inline comments.
lib/tunnelbroker/use-peer-to-peer-message-handler.js
318 ↗(On Diff #45315)

The fact that this line was removed could cause an issue - when we receive an encrypted message for a non-existent session we should "heal" the state by calling createOlmSessionsWithUser

See that sessionDoesNotExist is different and does not have OLM_ERROR_FLAG

angelika updated this revision to Diff 45369.Thu, Oct 24, 5:16 AM

Bring back sessionDoesNotExist check

Harbormaster completed remote builds in B32364: Diff 45369.Thu, Oct 24, 5:34 AM
Closed by commit rCOMM4723c3b6eb1d: [lib] Restart session when any Olm error is thrown (authored by angelika). · Explain WhyThu, Oct 24, 5:52 AM
This revision was automatically updated to reflect the committed changes.
angelika added a commit: rCOMM4723c3b6eb1d: [lib] Restart session when any Olm error is thrown.

Revision Contents
Changeset List

PathSize
lib/
tunnelbroker/
4 lines
utils/
5 lines
native/
cpp/
CommonCpp/
CryptoTools/
8 lines
web/
shared-worker/
worker/
40 lines

Diff 45369

View Options

lib/tunnelbroker/use-peer-to-peer-message-handler.js

Loading...
View Options

lib/utils/olm-utils.js

Loading...
View Options

native/cpp/CommonCpp/CryptoTools/Session.cpp

Loading...
View Options

web/shared-worker/worker/worker-crypto.js

Loading...