Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OMEMO - older messages are lost #2241

Closed
licaon-kter opened this issue Sep 21, 2020 · 28 comments
Closed

OMEMO - older messages are lost #2241

licaon-kter opened this issue Sep 21, 2020 · 28 comments
Milestone

Comments

@licaon-kter
Copy link
Contributor

licaon-kter commented Sep 21, 2020

HEAD (0a7dff4), Firefox 80, using IndexDB

To Reproduce
Steps to reproduce the behavior:

  1. login
  2. send or receive messages in 1:1 or MUC, with OMEMO
  3. logout
  4. login

Expected behavior
Older messages are available

Actual behavior
All past messages (both from me and my contacts) are replaced with I sent you an OMEMO encrypted message but your client doesn’t seem to support that. Find more information on https://conversations.im/omemo
Moreover in 1:1, all the messages are deleted, only the last one is kept, but it's actually the string above.
Sometimes some messages (eg. pulled from MAM if Converse was offline) are not corrupted, but on the next login they'll be.

Note: key is NOT regenerated on login.

Related to: #2235

@licaon-kter licaon-kter changed the title OMEMO - older messaged are lost OMEMO - older messages are lost Sep 21, 2020
@jcbrand jcbrand added the bug label Sep 22, 2020
@jcbrand jcbrand added this to the 7.0.0 milestone Sep 22, 2020
@jcbrand
Copy link
Member

jcbrand commented Oct 23, 2020

I had a look at this, but didn't find the fix yet.

It's not necessary to log out in order to reproduce this, you can just leave and rejoin the MUC. The problem appears to be related to browser storage.

When messages are fetched after rejoining the MUC, the models array is empty (as if there weren't any messages stored), although I can see them in localStorage via the developer console.

Looks like somehow (probably when the MUC gets closed) the messages collection gets persisted to localStorage in such a way that it wipes the references to the saved messages. Upon rejoin, messages are recreated from MAM, (which AFAIK doesn't provide enough info to decrypt them, if they were sent by us).

@licaon-kter
Copy link
Contributor Author

Reopen please, some messages are still mangled after a while.

@jcbrand
Copy link
Member

jcbrand commented Oct 29, 2020

Sorry, but that's too vague.

@licaon-kter
Copy link
Contributor Author

Which part is vague?

"a while"? It's because I wasn't paying attention, can't specify how much time has passed.

"mangled"? One message has transformed into I sent you an OMEMO encrypted message but your client doesn’t seem to support that. Find more information on https://conversations.im/omemo. Then I wrote some more, done other stuff elsewhere, come back (maybe logged out, maybe not), now a second random message has become this.

@licaon-kter
Copy link
Contributor Author

licaon-kter commented Oct 29, 2020

I can repro at will, it appears that the last MUC message gets axed, some sort of off-by-one?
beforeafter

then the last one gets mangled.

Console says:

 ERROR: Higher reported acknowledge count than unacknowledged stanzas. Reported Acknowledged Count: 8 -Unacknowledged Stanza Count: 7 -New: 56 - Previous: 48 log.js:64:19
    log log.js:64
    error log.js:81
    rd converse-smacks.js:44
    run core.js:1259
    _dataRecv core.js:2515
    forEachChild core.js:309
    _dataRecv core.js:2504
    _onMessage websocket.js:506
    onmessage websocket.js:277
    (Async: EventHandlerNonNull)
    _replaceMessageHandler websocket.js:277
    _onInitialMessage websocket.js:262
    onmessage websocket.js:161
    (Async: EventHandlerNonNull)
    _connect websocket.js:161
    connect core.js:1772
    connect connection.js:103
    Wc converse-core.js:1146
    login converse-core.js:1108
    login converse-core.js:533
    reconnect connection.js:123
    reconnect converse-core.js:400
    onDisconnected connection.js:234
    onConnectStatusChanged connection.js:279
    _changeConnectStatus core.js:2383
    _doDisconnect core.js:2424
    _onClose websocket.js:360
    onclose websocket.js:159
    (Async: EventHandlerNonNull)
    _connect websocket.js:159
    connect core.js:1772
    connect connection.js:103
    Wc converse-core.js:1146
    login converse-core.js:1108
    login converse-core.js:533
    reconnect connection.js:123
    reconnect converse-core.js:400
    onDisconnected connection.js:234
    onConnectStatusChanged connection.js:279
    _changeConnectStatus core.js:2383
    _doDisconnect core.js:2424
    _onClose websocket.js:360
    onclose websocket.js:159

and

 ERROR: MessageCounterError Message key not found. The counter was repeated or the key was not filled. log.js:64:19
    log log.js:64
    error log.js:81
    t converse-omemo.js:192
    t converse-omemo.js:1243
    e converse-core.js:467
    (Async: promise callback)
    e converse-core.js:467
    hook converse-core.js:467
    parseMUCMessage stanza.js:687
    messages converse-mam.js:53
    handleMAMResult converse-mam.js:52
    fetchArchivedMessages converse-mam.js:109
    fetchNewestMessages converse-mam.js:40
    initialize converse-mam.js:219
    wi events.js:281
    yi events.js:264
    e events.js:50
    trigger events.js:254
    trigger converse-core.js:441
    onConnectionStatusChanged converse-muc.js:539
    wi events.js:282
    yi events.js:264
    e events.js:50
    trigger events.js:254
    onOwnPresence converse-muc.js:2349
    onPresence converse-muc.js:2315
    presence_handler converse-muc.js:699
    run core.js:1259
    _dataRecv core.js:2515
    forEachChild core.js:309
    _dataRecv core.js:2504
    _onMessage websocket.js:506
    onmessage websocket.js:277
    (Async: EventHandlerNonNull)
    _replaceMessageHandler websocket.js:277
    _onInitialMessage websocket.js:262
    onmessage websocket.js:161
    (Async: EventHandlerNonNull)
    _connect websocket.js:161
    connect core.js:1772
    connect connection.js:103

and

Uncaught (in promise) MessageCounterError: Message key not found. The counter was repeated or the key was not filled.
    doDecryptWhisperMessage libsignal-protocol.js:1982
    promise callback*doDecryptWhisperMessage/< libsignal-protocol.js:1982
    promise callback*doDecryptWhisperMessage libsignal-protocol.js:1981
    decryptWithSessionList libsignal-protocol.js:1971
    decryptWhisperMessage libsignal-protocol.js:1973
    promise callback*decryptWhisperMessage/< libsignal-protocol.js:1972
    promise callback*queueJobForNumber libsignal-protocol.js:1995
    decryptWhisperMessage libsignal-protocol.js:1972
    t converse-omemo.js:188
    t converse-omemo.js:1243
    e converse-core.js:467
    promise callback*hook/e< converse-core.js:467
    hook converse-core.js:467
    parseMUCMessage stanza.js:687
    messages converse-mam.js:53
    handleMAMResult converse-mam.js:52
    fetchArchivedMessages converse-mam.js:109
    fetchNewestMessages converse-mam.js:40
    initialize converse-mam.js:219
    wi events.js:281
    yi events.js:264
    e events.js:50
    trigger events.js:254
    trigger converse-core.js:441
    onConnectionStatusChanged converse-muc.js:539
    wi events.js:282
    yi events.js:264
    e events.js:50
    trigger events.js:254
    onOwnPresence converse-muc.js:2349
    onPresence converse-muc.js:2315
    presence_handler converse-muc.js:699
    run core.js:1259
    _dataRecv core.js:2515
    forEachChild core.js:309
    _dataRecv core.js:2504
    _onMessage websocket.js:506
    onmessage websocket.js:277
    _replaceMessageHandler websocket.js:277
    _onInitialMessage websocket.js:262
    onmessage websocket.js:161
    _connect websocket.js:161
    connect core.js:1772
    connect connection.js:103
libsignal-protocol.js:1982:151

These tend to keep repeating for some reason albeit there's no MUC activity

Hope these help.

@jcbrand
Copy link
Member

jcbrand commented Oct 29, 2020

Thank you, that helps a lot.

I think it's an off-by-one error, like you suspected, which causes the last cached message to be returned from MAM and the MAM entry then overwrites the decrypted plaintext.

@jcbrand jcbrand reopened this Oct 29, 2020
@jcbrand jcbrand modified the milestones: 7.0.0, 7.0.1 Nov 18, 2020
@Echolon Echolon added the OMEMO label Apr 2, 2021
@afriedmanGlacier
Copy link
Contributor

afriedmanGlacier commented Nov 4, 2021

It looks like the bug fix for #1481 has some relation to this as well. If I close and then re-enter a group now, I'm seeing the error message from getJIDForDecryption in plugins/omemo/utils.js

EDIT: Never mind, that is resolved if I set muc_clear_messages_on_leave to false.

@jcbrand
Copy link
Member

jcbrand commented Nov 4, 2021

@afriedmanGlacier: Can you paste the traceback of the error here?

@afriedmanGlacier
Copy link
Contributor

afriedmanGlacier commented Nov 4, 2021

Sure...do you need debug included? I set muc_clear_messages_on_leave to false and it didn't have issues decrypting, but here's the trace when I set it back to true. And we've occasionally seen the error reported above, "MessageCounterError Message key not found", but that's not what's happening here.

log.js:64 ERROR: Error Could not find JID to decrypt OMEMO message for
log @ log.js:64
error @ log.js:81
decryptWhisperMessage @ utils.js:375
parseEncryptedMessage @ utils.js:219
(anonymous) @ core.js:256
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
m @ polyfills-es5.js:1
Promise.then (async)
y @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
w @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ core.js:256
hook @ core.js:256
parseMUCMessage @ parsers.js:249
async function (async)
parseMUCMessage @ parsers.js:218
(anonymous) @ utils.js:81
handleMAMResult @ utils.js:80
async function (async)
handleMAMResult @ utils.js:78
fetchArchivedMessages @ utils.js:146
async function (async)
fetchArchivedMessages @ utils.js:132
fetchNewestMessages @ utils.js:223
(anonymous) @ index.js:48
triggerEvents @ events.js:281
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
trigger @ core.js:230
onRoomEntered @ muc.js:329
async function (async)
onRoomEntered @ muc.js:312
onConnectionStatusChanged @ muc.js:344
triggerEvents @ events.js:282
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
onOwnPresence @ muc.js:2601
async function (async)
onOwnPresence @ muc.js:2593
onPresence @ muc.js:2565
presence_handler.shared_converse.connection.addHandler.ignoreNamespaceFragment @ muc.js:589
run @ core.js:1264
(anonymous) @ core.js:2526
forEachChild @ core.js:316
_dataRecv @ core.js:2515
_onMessage @ websocket.js:506
socket.onmessage @ websocket.js:277
S @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
p @ polyfills-es5.js:1
h @ polyfills-es5.js:1
log.js:64 ERROR: Error Could not find JID to decrypt OMEMO message for
log @ log.js:64
error @ log.js:81
decryptWhisperMessage @ utils.js:375
parseEncryptedMessage @ utils.js:219
(anonymous) @ core.js:256
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
m @ polyfills-es5.js:1
Promise.then (async)
y @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
w @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ core.js:256
hook @ core.js:256
parseMUCMessage @ parsers.js:249
async function (async)
parseMUCMessage @ parsers.js:218
(anonymous) @ utils.js:81
handleMAMResult @ utils.js:80
async function (async)
handleMAMResult @ utils.js:78
fetchArchivedMessages @ utils.js:146
async function (async)
fetchArchivedMessages @ utils.js:132
fetchNewestMessages @ utils.js:223
(anonymous) @ index.js:48
triggerEvents @ events.js:281
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
trigger @ core.js:230
onRoomEntered @ muc.js:329
async function (async)
onRoomEntered @ muc.js:312
onConnectionStatusChanged @ muc.js:344
triggerEvents @ events.js:282
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
onOwnPresence @ muc.js:2601
async function (async)
onOwnPresence @ muc.js:2593
onPresence @ muc.js:2565
presence_handler.shared_converse.connection.addHandler.ignoreNamespaceFragment @ muc.js:589
run @ core.js:1264
(anonymous) @ core.js:2526
forEachChild @ core.js:316
_dataRecv @ core.js:2515
_onMessage @ websocket.js:506
socket.onmessage @ websocket.js:277
S @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
p @ polyfills-es5.js:1
h @ polyfills-es5.js:1
log.js:64 ERROR: Error Could not find JID to decrypt OMEMO message for
log @ log.js:64
error @ log.js:81
decryptWhisperMessage @ utils.js:375
parseEncryptedMessage @ utils.js:219
(anonymous) @ core.js:256
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
m @ polyfills-es5.js:1
Promise.then (async)
y @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
w @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ core.js:256
hook @ core.js:256
parseMUCMessage @ parsers.js:249
async function (async)
parseMUCMessage @ parsers.js:218
(anonymous) @ utils.js:81
handleMAMResult @ utils.js:80
async function (async)
handleMAMResult @ utils.js:78
fetchArchivedMessages @ utils.js:146
async function (async)
fetchArchivedMessages @ utils.js:132
fetchNewestMessages @ utils.js:223
(anonymous) @ index.js:48
triggerEvents @ events.js:281
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
trigger @ core.js:230
onRoomEntered @ muc.js:329
async function (async)
onRoomEntered @ muc.js:312
onConnectionStatusChanged @ muc.js:344
triggerEvents @ events.js:282
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
onOwnPresence @ muc.js:2601
async function (async)
onOwnPresence @ muc.js:2593
onPresence @ muc.js:2565
presence_handler.shared_converse.connection.addHandler.ignoreNamespaceFragment @ muc.js:589
run @ core.js:1264
(anonymous) @ core.js:2526
forEachChild @ core.js:316
_dataRecv @ core.js:2515
_onMessage @ websocket.js:506
socket.onmessage @ websocket.js:277
S @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
p @ polyfills-es5.js:1
h @ polyfills-es5.js:1
libsignal-protocol.js:36194 decryptWhisperMessage - could not decryptWithSessionList
(anonymous) @ libsignal-protocol.js:36194
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
m @ polyfills-es5.js:1
Promise.then (async)
y @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
w @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ core.js:256
hook @ core.js:256
parseMUCMessage @ parsers.js:249
async function (async)
parseMUCMessage @ parsers.js:218
(anonymous) @ utils.js:81
handleMAMResult @ utils.js:80
async function (async)
handleMAMResult @ utils.js:78
fetchArchivedMessages @ utils.js:146
async function (async)
fetchArchivedMessages @ utils.js:132
fetchNewestMessages @ utils.js:223
(anonymous) @ index.js:48
triggerEvents @ events.js:281
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
trigger @ core.js:230
onRoomEntered @ muc.js:329
async function (async)
onRoomEntered @ muc.js:312
onConnectionStatusChanged @ muc.js:344
triggerEvents @ events.js:282
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
onOwnPresence @ muc.js:2601
async function (async)
onOwnPresence @ muc.js:2593
onPresence @ muc.js:2565
presence_handler.shared_converse.connection.addHandler.ignoreNamespaceFragment @ muc.js:589
run @ core.js:1264
(anonymous) @ core.js:2526
forEachChild @ core.js:316
_dataRecv @ core.js:2515
_onMessage @ websocket.js:506
socket.onmessage @ websocket.js:277
S @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
p @ polyfills-es5.js:1
h @ polyfills-es5.js:1
libsignal-protocol.js:36194 decryptWhisperMessage - could not decryptWithSessionList
(anonymous) @ libsignal-protocol.js:36194
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
m @ polyfills-es5.js:1
Promise.then (async)
y @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
w @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ core.js:256
hook @ core.js:256
parseMUCMessage @ parsers.js:249
async function (async)
parseMUCMessage @ parsers.js:218
(anonymous) @ utils.js:81
handleMAMResult @ utils.js:80
async function (async)
handleMAMResult @ utils.js:78
fetchArchivedMessages @ utils.js:146
async function (async)
fetchArchivedMessages @ utils.js:132
fetchNewestMessages @ utils.js:223
(anonymous) @ index.js:48
triggerEvents @ events.js:281
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
trigger @ core.js:230
onRoomEntered @ muc.js:329
async function (async)
onRoomEntered @ muc.js:312
onConnectionStatusChanged @ muc.js:344
triggerEvents @ events.js:282
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
onOwnPresence @ muc.js:2601
async function (async)
onOwnPresence @ muc.js:2593
onPresence @ muc.js:2565
presence_handler.shared_converse.connection.addHandler.ignoreNamespaceFragment @ muc.js:589
run @ core.js:1264
(anonymous) @ core.js:2526
forEachChild @ core.js:316
_dataRecv @ core.js:2515
_onMessage @ websocket.js:506
socket.onmessage @ websocket.js:277
S @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
p @ polyfills-es5.js:1
h @ polyfills-es5.js:1
libsignal-protocol.js:36194 decryptWhisperMessage - could not decryptWithSessionList
(anonymous) @ libsignal-protocol.js:36194
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
m @ polyfills-es5.js:1
Promise.then (async)
y @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
w @ polyfills-es5.js:1
value @ polyfills-es5.js:1
(anonymous) @ core.js:256
hook @ core.js:256
parseMUCMessage @ parsers.js:249
async function (async)
parseMUCMessage @ parsers.js:218
(anonymous) @ utils.js:81
handleMAMResult @ utils.js:80
async function (async)
handleMAMResult @ utils.js:78
fetchArchivedMessages @ utils.js:146
async function (async)
fetchArchivedMessages @ utils.js:132
fetchNewestMessages @ utils.js:223
(anonymous) @ index.js:48
triggerEvents @ events.js:281
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
trigger @ core.js:230
onRoomEntered @ muc.js:329
async function (async)
onRoomEntered @ muc.js:312
onConnectionStatusChanged @ muc.js:344
triggerEvents @ events.js:282
triggerApi @ events.js:264
eventsApi @ events.js:50
Events.trigger @ events.js:254
onOwnPresence @ muc.js:2601
async function (async)
onOwnPresence @ muc.js:2593
onPresence @ muc.js:2565
presence_handler.shared_converse.connection.addHandler.ignoreNamespaceFragment @ muc.js:589
run @ core.js:1264
(anonymous) @ core.js:2526
forEachChild @ core.js:316
_dataRecv @ core.js:2515
_onMessage @ websocket.js:506
socket.onmessage @ websocket.js:277
S @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
value @ polyfills-es5.js:1
p @ polyfills-es5.js:1
h @ polyfills-es5.js:1
...

@jcbrand
Copy link
Member

jcbrand commented Nov 6, 2021

You are only able to decrypt OMEMO encrypted messages once. The double ratchet prevents subsequent decryptions.

So if you set muc_clear_messages_on_leave to true, then messages are deleted when you leave the MUC and when they're fetched from the archive they're encrypted and can't be decrypted again.

This is mentioned in the documentation for that setting: https://conversejs.org/docs/html/configuration.html#muc-clear-messages-on-leave

That's the one issue.

The other one: Error Could not find JID to decrypt OMEMO message for

This can happen if a user's nickname has changed. The archived messages are fetched, and they still have the old nickname, so Converse can't find the JID based on the nickname.

XEP-0421 provides a way to fix this, by adding persistent occupant IDs to MUC message and presence stanzas.

Do you know whether your server supports it? Newer versions of Prosody do. I'm in the process of adding support for XEP-0421.

jcbrand added a commit that referenced this issue Nov 6, 2021
This let's us populate the `from_real_jid` attribute for messages in
cases where the user's nickname has changed.

Updates #2241
jcbrand added a commit that referenced this issue Nov 6, 2021
This let's us populate the `from_real_jid` attribute for messages in
cases where the user's nickname has changed.

Only save the occupant-id if the MUC supports it

Store all advertised features on the `chatbox.features` model.
This allows us to look up a feature without using the async
`disco.supports` API.

Updates #2241
@jcbrand
Copy link
Member

jcbrand commented Nov 6, 2021

@afriedmanGlacier: I've now merged the branch which adds support for XEP-0421. If your XMPP server supports it, then Converse should now be able to decrypt messages whose nicknames don't match any current users (as long as those messages weren't decrypted already, as explained above).

@afriedmanGlacier
Copy link
Contributor

We use ejabberd, and it does not appear to support that XEP yet: processone/ejabberd#3397

@weiss do you know if there are plans for this or if it's been implemented yet?

@weiss
Copy link
Contributor

weiss commented Nov 8, 2021

do you know if there are plans for this or if it's been implemented yet?

I'm not aware of such plans. I agree it would be nice to have, if that helps! 😃

(Wondering about this use case though: Converse.js supports OMEMO in anonymous groups? Isn't the value of end-to-end encryption questionable if you don't know who the other end is?)

@weiss
Copy link
Contributor

weiss commented Nov 8, 2021

Converse.js supports OMEMO in anonymous groups?

Oh, or maybe it's just using the room JID rather than the real JID even in non-anonymous groups?

@jcbrand
Copy link
Member

jcbrand commented Nov 8, 2021

@weiss: It's not that the group is anonymous, it's that groupchat messages don't include the real JID of the sender, even in non-anonymous MUCs.

If a user's nickname changes, then MAM messages might be received from the old nickname and without XEP-0421 the client doesn't have a way to look up the real JID of the sender and so the message can't be decrypted.

@weiss
Copy link
Contributor

weiss commented Nov 8, 2021

Right, totally makes sense to me now. Thanks (and sorry for side tracking the topic)!

@futurealecks
Copy link

Adding on to @licaon-kter @afriedmanGlacier; getting these random messages that are not able to be decrypted in 1:1 messages as well. In the screenshot below, the 3 messages I sent at 6:21 PM were all within a minute. One of these messages was unable to be decrypted. The contents of this particular message was "Sure".

image

I'm not showing any relevant errors in debug that I can tell. It also seems that these only appear when I scroll back up in history. I also think they only appear when you "reopen" the app.

My setup is one Android user and one Converse user (MAM).

@licaon-kter
Copy link
Contributor Author

Using HEAD I don't see the exact same error, but the whole thing is unstable is general.

Eg. Messages sent while Converse was not connected are almost surely not decrypted. Note: no key change, no IndexDB clean up. The old messages (already decrypted) are shown fine though.

@afriedmanGlacier
Copy link
Contributor

@licaon-kter you said "Messages sent while Converse was not connected are almost surely not decrypted." Is that only in MUC or in 1:1 messages also?

We see that same issue in MUC, but it generally doesn't seem to happen if the sending device/client is connected when we re-open Converse.

One cause seems to be that the MUC occupant nickname that is linked to the jid is not populated until the user is online or some other event, so when MAM/offline messages come in, it didn't have a way to identify the contact or real_jid. I know "presence" is one thing that populates the nickname/jid (which is why it works in this particular case when the other device is online), but that shouldn't be necessary. In testing with other contacts, sometimes the nickname was stored and we didn't have this issue.

When entering a group, there is a "fetchMembers" function that queries for group affiliations/occupants. According to the xmpp specs, the server CAN return group nickname with this query, but in our case it doesn't.

@licaon-kter
Copy link
Contributor Author

1:1 testing for me

@jcbrand
Copy link
Member

jcbrand commented Nov 20, 2021

One cause seems to be that the MUC occupant nickname that is linked to the jid is not populated until the user is online or some other event, so when MAM/offline messages come in, it didn't have a way to identify the contact or real_jid

When entering a group, there is a "fetchMembers" function that queries for group affiliations/occupants. According to the xmpp specs, the server CAN return group nickname with this query, but in our case it doesn't.

Ok, so I studied the code, and I think what you're describing might be an issue.

When a MAM message is received, we don't get the MUC occupant's real JID in the MAM message stanza, only the nickname (and the XEP-0421 occupant id if the XMPP supports it, but your's doesn't).

In order to decrypt a message, we need the sender's real JID, which means we need a way to look up the real JID from the MUC nickname.

That's why, when Converse joins a MUC, it first calls fetchMembers to get the member list, before the MAM query is done, so that it has a list of occupants with their nicknames and real JIDs, so that we can look up the real JID by nickname when a MAM message is received.

Then, as a MAM messages come in, it gets parsed and it's crucial that the from_real_jid value is found, otherwise it won't be possible to decrypt the message. See here:

'from_real_jid': chatbox.occupants.findOccupant(attrs)?.get('jid'),

However, if the members list that's returned by the XMPP server doesn't return nicks, then findOccupant won't return an occupant and from_real_jid won't be defined and so the encrypted MAM message can't be decrypted.

@afriedmanGlacier Can you provide an example of a members list IQ response from your XMPP server?

I'm not sure what can be done if the members list that's returned doesn't include nicknames or XEP-0421 occupant ids.

I wonder what other XMPP clients do about this issue. I think the problem can be mitigated/solved by storing occupant data indefinitely.

@afriedmanGlacier Are you using localStorage or IndexedDB as your store? Converse does keep the occupant data between logins, as long as you don't keep the MUC open. If you leave it via the UI (by clicking the dropdown and then leave), then all the MUC data (e.g. messages and occupants) are deleted and you'll also lose your decrypted messages and won't be able to decrypt them again.

This last point seems to me to also be causing a lot of confusion among people. One potential solution there is to not delete the data when you leave a MUC. With localStorage this isn't really an option, due to the storage limit, but IndexedDB doesn't have a limit.

@jcbrand
Copy link
Member

jcbrand commented Nov 20, 2021

@iNPUTmice informed me that for a non-anon MUC, you do get real JIDs in MAM results. So I have a fix for the above problem in the works.

jcbrand added a commit that referenced this issue Nov 20, 2021
That solves the problem of not being able to look up OMEMO session data
from incoming MAM messages.

See here: #2241 (comment)

Updates #2241
@jcbrand
Copy link
Member

jcbrand commented Nov 21, 2021

@licaon-kter Could you please check if things are better now?

@afriedmanGlacier
Copy link
Contributor

@jcbrand sorry, was offline over the weekend. We are using IndexedDB. Do you still need me to provide an example of a members list IQ response?

Looks like you may have figured out the solution without that, so we'll try what you've done first. Thanks!

@afriedmanGlacier
Copy link
Contributor

I updated from your commit and I'm still having the decryption issue with groups when I reopen Converse if messages came in groups while I was disconnected. I'll try to verify what's happening and send a log later today or tomorrow.

Also, while the fix you made in theory should help what I'm seeing with group messages, I'm guessing it's not the same issue for @licaon-kter since that's 1:1?

@afriedmanGlacier
Copy link
Contributor

After looking at this further, it seems like the issue I'm referencing is probably different than what @licaon-kter and some of the guys that I work with have seen. I'm guessing I should create a separate issue for what I'm seeing?

In the case of this original ticket, the messages are decrypted upon arrival but later on after reconnecting a message can be "lost" and changed to not decrypted.

What I'm seeing is that messages that come in to a MUC (and sometimes 1:1) while the client is closed don't get decrypted upon opening the client and doing a MAM fetch.

Just to be clear, we are using a version of converse-desktop that has been updated to 8.0.1 plus most of the latest commits. So it's possible that the web client doesn't have the issue I'm seeing because it seems like it could be a timing issue.

After the update for getting sender real JID, we are correctly receiving the JID, but the decryption still often fails when opening the app for messages that arrived while closed. In looking at the message attributes during the decryption process, I noticed that it fails with no warning in plugins/omemo/utils.parseEncryptedMessage because it doesn't have attrs.encrypted.key.

In the attrs: "is_encrypted":true,"encrypted":{"device_id":"406865841"}. In other words it doesn't have the iv, key, etc.

However, the correct rid is clearly among the keys in the message. In headless/shared/parsers.getEncryptionAttributes, the line const device_id = _converse.omemo_store?.get('device_id'); doesn't return a device_id. The store appears to exist but device_id doesn't seem to be accessible yet when the first MAM messages come in for some reason.

@jcbrand
Copy link
Member

jcbrand commented Nov 25, 2021

@licaon-kter Can this ticket be closed?

@licaon-kter
Copy link
Contributor Author

licaon-kter commented Feb 3, 2022

Closing in favour of #2733

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants