Page MenuHomePhabricator

Enhance error check in CryptoModule decrypt. Add encrypted message hash to error message.
ClosedPublic

Authored by marcin on Sep 13 2023, 6:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 16, 5:05 AM
Unknown Object (File)
Thu, Oct 31, 1:14 PM
Unknown Object (File)
Thu, Oct 31, 1:14 PM
Unknown Object (File)
Mon, Oct 28, 3:09 PM
Unknown Object (File)
Oct 18 2024, 12:44 AM
Unknown Object (File)
Oct 12 2024, 2:16 AM
Unknown Object (File)
Oct 12 2024, 2:03 AM
Unknown Object (File)
Oct 1 2024, 4:29 PM
Subscribers

Details

Summary

This differential addresses crypto sync follow ups described there: https://linear.app/comm/issue/ENG-4886/nse-error-during-notif-decryption-bad-message-mac#comment-7b6b9979. In particular:

  1. EncryptedData is passed by reference.
  2. Error checking for olm_max_decrypt_plaintext_length is implemented.
  3. Hash of encrypted message is calculated before decryption. In the case of failure, hash of the message is appended to error message.
  4. Notifications Service Extension adds notifications id to error message notification. NOTE: it is NOT id of an entry in notifications table in MariaDB. It is the iosID field of delivery column of an entry in notifications table.
Test Plan
  1. Build iOS app,
  2. Apply this patch to the keyserver code to send each encrypted notification twice: https://gist.github.com/marcinwasowicz/5011d3c1d5418e1a3af06f6be182e739.
  3. Send notification to iOS device.
  4. Ensure that the second notification contains the hash and id of the notification received,

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin edited the test plan for this revision. (Show Details)
native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
343 ↗(On Diff #31043)

In retrospect there is probably no performance gain here, since in the old code C++ compilers would probably optimize to use a move constructor here. We can leave it though, in case we change things in the future at the callsite

This revision is now accepted and ready to land.Sep 15 2023, 3:41 AM