Page MenuHomePhabricator

Implement notification decryption in CommNotificationsHandler
ClosedPublic

Authored by marcin on May 19 2023, 7:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 20 2024, 6:36 AM
Unknown Object (File)
Feb 20 2024, 5:27 AM
Unknown Object (File)
Feb 20 2024, 5:16 AM
Unknown Object (File)
Feb 20 2024, 4:06 AM
Unknown Object (File)
Feb 20 2024, 4:05 AM
Unknown Object (File)
Feb 20 2024, 4:03 AM
Unknown Object (File)
Feb 20 2024, 4:02 AM
Unknown Object (File)
Feb 20 2024, 3:12 AM
Subscribers

Details

Summary

This differential implements notification decryption in CommNotificationsHandler

Test Plan
  1. Pull cookie id of currently logged in Android user from the db.
  2. Use method implemented in previous differential to encrypt android notification in send.js. Hardcode cookie id from step.1.
  3. Send notification. Ensure it is currectly displayed.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.May 22 2023, 2:48 AM
tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
90–97 ↗(On Diff #26670)

What do you think about being more explicit in conditions when it comes to encrypted values?

232–234 ↗(On Diff #26670)

I don't think it is beneficial to create an ArrayList just to iterate on it. We can directly iterate Set.

234–250 ↗(On Diff #26670)

Why do we encrypt/decrypt field-by-field? Aren't we leaking some info by doing so? Won't it be safer to encrypt the whole e.g. JSON with all the fields?

This revision now requires changes to proceed.May 22 2023, 2:48 AM
marcin added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
90–97 ↗(On Diff #26670)

Introducing this change might lead us into NullPointerException since we don't assign encrypted field to every notification, even after landing this entire stack. This field is used as follows:

  1. There is no encrypted field: notification was not supposed to be encrypted.
  2. There is encrypted field and it is set to 0: notification was supposed to be encrypted but encryption failed on the keyserver side.
  3. There is encrypted field and it is set to 1: notification was supposed to be encrypted and encryption was successfull.
232–234 ↗(On Diff #26670)

We don't modify which keys are present and which are not, but we modify values that are stored under those keys. Therefore according to the docs: https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#keySet-- iterating directly over keySet output might not be safe. That is why I created the copy of keys present in the map first.

234–250 ↗(On Diff #26670)

There are a couple of reasons here:

  1. Not every field is encrypted, but I understand we could just store encrypted JSON blob under some key along with other keys that are not encrypted.
  2. On iOS we encrypt field by field since some keys presence it probably required by APN.
  3. No security concerns about field by field encryption were raised in discussions under relevant Linear issues: https://linear.app/comm/issue/ENG-2864/decide-which-parts-of-notifications-for-each-platforms-are-going-to-be.
  4. We should ask @anunay whether encrypting entire JSON blob is more efficient in terms of size than encrypting each field separately. My intuition is that there is not single answer to this question but rather there is some treshold which changes the preferred approach.

In conclusion:

  1. If there are security issues about field-byfield approach then we should implement field by field approach.
  2. If there are not security issues about field-by-field approach we should decide based on which is less likely to overflow fcm size limit.

cc @ashoat

tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
90–97 ↗(On Diff #26670)

There shouldn't be an NPE - to avoid that you're using an inverted syntax (notice: encrypted.equals("0") which NPEs vs "0".equals(encrypted) which doesn't). This is the exact reason behind using this strange syntax.

232–234 ↗(On Diff #26670)

Ok, makes sense. There's a more functional approach using Map.replaceAll, but using an intermediate array may also stay.

234–250 ↗(On Diff #26670)

My intuition tells me that encrypting field-by-field is more vulnerable, because an attacker has a couple of independent encrypted values instead of one, bigger one (but intuition isn't a good tool when it comes to cryptography). From size perspective it also feels like encrypting all at once is more efficient - if e.g. a padding is added, it is added once instead of once per field.

So for me it seems like encrypting all at once should be better. But not sure if the difference is significant enough that we should change the solution. Also, I might be wrong and field-by -field is better overall.

This revision is now accepted and ready to land.May 22 2023, 6:52 AM

Agreed with @ashoat that Android notification payload should be encrypted in one pass. Regarding iOS we have to first test whether we can just omit certain fields.

This revision is now accepted and ready to land.May 23 2023, 4:18 AM
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
44 ↗(On Diff #26863)
  1. If Android notification was supposed to be encrypted and was successfully encrypted it will contain encryptedPayload field.
  2. If Android notification was supposed to be encrypted but was not successfully encrypted it will contain encryptionFailed field.

As far as I know JSON cannot contain nulls or undefined so we cannot differentiate between 1 and 2 with empty encryptedPayload field.
Setting a flag encryptionSuccessful that would be either 1 or 0 and then additionally setting encryptedPayload would be a waste of space.

93 ↗(On Diff #26863)

This exception should never occurr. JsonReader class is designed to cover the case of online stream of json tokens and this is why IOException appears in this method signature. However we are dealing with offline in-memory data.

97 ↗(On Diff #26863)

At line 256 we repeatedly call nextString method since based on parent differential we expect that encrypted payload of Android notifications is a string-> string map. This method will throw IllegalStateException if we call nextString but the currently read field is not string. In our case it would mean the server side developer changed Android notification structure but they forgot to update native Android notifications code. We can consider it as native invariant.

103 ↗(On Diff #26863)

JsonReader methods: beginObject, endObject, hasNext and nextString might throw either IOException or IllegalStateException which have already been handled. Therefore any other exception must come from NotificationsCryptoModule. that is why we print this error with "Failed to decrypt encrypted notification" message,

Fix CI by fixing flow error in parent diff

tomek requested changes to this revision.May 24 2023, 1:46 AM
tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
253–259 ↗(On Diff #26867)

It is a really low-level API - can we find something that returns Map instead of iterating through the fields or tokes? The problem with the current approach is that we're assuming that all the values are strings - is it always the case? Can't we send a notif where a prop is an object, or array? Supporting nested structure in this approach could be challenging.

103 ↗(On Diff #26863)

It still can throw any unchecked exception. But regardless, this error message is fine.

This revision now requires changes to proceed.May 24 2023, 1:46 AM
marcin added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
253–259 ↗(On Diff #26867)

It is a really low-level API - can we find something that returns Map instead of iterating through the fields or tokes?

We could use this library: https://www.baeldung.com/java-org-json, but we would need to add it to the project.

The problem with the current approach is that we're assuming that all the values are strings - is it always the case? Can't we send a notif where a prop is an object, or array?

I have answered this question in one of the comments. Unfortunately it was lost during rebase + arc diff so I will quote it:

At line 256 we repeatedly call nextString method since based on parent differential we expect that encrypted payload of Android notifications is a string-> string map. This method will throw IllegalStateException if we call nextString but the currently read field is not string. In our case it would mean the server side developer changed Android notification structure but they forgot to update native Android notifications code. We can consider it as native invariant.

Supporting nested structure in this approach could be challenging.

When we decide to create a nested structure in Android notification we can revisit this code, add org.json and change the implementation. The way this code is currently implemented will inform us that such a change has to be made. For the current use case I think this approach is fine.

native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
253–259 ↗(On Diff #26867)

Ok, makes sense, but having a warning on native client might not be enough to let us know about the issue. We should add some comments in the code that generates notifs that all the Android clients assume that notif is flat.

This revision is now accepted and ready to land.May 24 2023, 3:26 AM

Use high level org.json library.