signalapp / Signal-Android

A private messenger for Android.
https://signal.org
GNU Affero General Public License v3.0
25.19k stars 6.07k forks source link

Not marking sealed-sender messages as sealed-sender, and subsequently leading to unsealed-sender messages sends afterwards #11991

Closed stevie553 closed 2 years ago

stevie553 commented 2 years ago

Bug description

This happens when censorship circumvention is ON and Play Services are NOT installed on the sender device.

Normally Signal marks a recipient as UD-disabled following a non-UD send, which can be seen here: https://github.com/signalapp/Signal-Android/blob/6f5475fc94507618f1c38226e7084365eabddf4d/app/src/main/java/org/thoughtcrime/securesms/jobs/PushTextSendJob.java#L115-L117

and here: https://github.com/signalapp/Signal-Android/blob/6f5475fc94507618f1c38226e7084365eabddf4d/app/src/main/java/org/thoughtcrime/securesms/jobs/PushMediaSendJob.java#L152-L154

Which leads to future sends being non-UD sends until the recipient's UD status is changed.

The problem is that a successfully sent sealed-sender message never gets labeled that way so Signal mistakenly marks the recipient UD-disabled making future sends UNsealed-sender messages.

Prerequisites:

Steps to reproduce

Actual result: Signal doesn't mark a sealed-sender message as sealed-sender. Expected result: Signal should mark a message sealed-sender status correctly.

Screenshots

Device info

Device: AVD Android version: 12 Signal version: 5.32.4

Link to debug log

No need, reliably reproducible.

stevie553 commented 2 years ago

I found a similar usage of the possibly problematic line JsonUtil.fromJson(responseText, SendMessageResponse.class); here https://github.com/signalapp/Signal-Android/blob/9b837d3f02c42c5d721b93535bb187d47e9df757/libsignal/service/src/main/java/org/whispersystems/signalservice/api/services/MessagingService.java#L59-L62

But there was code introduced in https://github.com/signalapp/Signal-Android/commit/b5dcf8e8f14281a019e4f8a33057095a024386c6 that fixes the UD state of the result of JsonUtil.fromJson: https://github.com/signalapp/Signal-Android/blob/9b837d3f02c42c5d721b93535bb187d47e9df757/libsignal/service/src/main/java/org/whispersystems/signalservice/api/services/MessagingService.java#L63

So maybe applying setSentUnidentfied(unidentified); to https://github.com/signalapp/Signal-Android/blob/9b837d3f02c42c5d721b93535bb187d47e9df757/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/push/PushServiceSocket.java#L521 could potentially fix the problem?

Note: Not an experienced software engineer so I could be 100% wrong about my "solution". Also, the way you folks write code is beyond amazing, everything is just so easy to read, thank you for that.

greyson-signal commented 2 years ago

Ah yes your analysis is correct, thank you for the wonderfully detailed report! I've updated things so that we set the unidentified flag properly at that call site. Thanks!

stevie553 commented 2 years ago

I found another method that doesn't mark the message UD status correctly. https://github.com/signalapp/Signal-Android/blob/67f0ba86246dbd5b0095171a0e4b57d36d5fcf70/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/push/PushServiceSocket.java#L530-L537 It's worth noting that this method is not used anywhere (at least based on Android Studio) The reason I'm reporting it is that maybe in the future it'll be used and produce the bug reported originally in this issue.

Also thanks a lot for the fix, I stopped seeing non-UD messages from my friends living in censored countries.