search

vanitasvitae / Smack

A modular and portable open source XMPP client library written in Java for Android and Java (SE) VMs
https://igniterealtime.org/projects/smack/
Apache License 2.0
16 stars 3 forks source link

Crashing when parsing messages from ChatSecure iOS #24

Closed n8fr8 closed 7 years ago

n8fr8 commented 7 years ago

I updated to the latest smack-omemo libraries and they changed something related to parsing message keys and auth tags. If you don't send a 32 byte long message + auth key or something jt throws a runtime error and crashes. The old code that was replaced has a link to https://github.com/ChatSecure/ChatSecure-iOS/issues/647

All in all, it seems ChatSecure iOS is using the "legacy format" whatever that means. This is what @chrisballinger had to say:

"The legacy format shouldn't crash smack-omemo, that's a bug. Consuming both legacy and new format should be supported simultaneously. Likely enough people are using new format that we can move away from sending legacy, but it's not top priority."

vanitasvitae commented 7 years ago

My intention in removing the legacy support was to mitigate the security vulnerybility the legacy format involves. But you are right, just throwing an AssertionError is probably not the right way to handle wrong formats.

What do you suggest? I'd vote for silently dropping messages with wrong (also legacy) format as if they could not be decrypted.

n8fr8 commented 7 years ago

Either outcome still means we can't currently communicate from Zom Android with Zom iOS. If you aren't going to support legacy anymore, then we need to upgrade Zom iOS to not use legacy likely. Or am I confused about how legacy vs new work? Is there a query/handshake process of some sort?

vanitasvitae commented 7 years ago

Implementing the new format is pretty straight forward (took me 30 mins or so). In the old format, the message was just encrypted using an AES key and the output was not further modified. That means, the AuthTag (16 byte) was appended to the encrypted message (I think this is standard behaviour for Cipher.doFinal()?).

In the new format, the authtag is cut from the message and appended to the messageKey that gets encrypted using the double ratchet. (When sending)

When receiving a message with the new format, the implementation has to check, whether the decrypted AES messageKey is 32 byte long and if so, it has to append the last 16 byte to the encrypted message (payload) before decrypting.

In the end its just 5 System.arrayCopy() calls or so. I'd highly suggest ChatSecure-iOS implementing the new format, so that we can move on.

Edit: Here is how Conversations did it: https://github.com/siacs/Conversations/commit/f0c3b31a42ac6269a0ca299f2fa470586f6120be#diff-e9eacf512943e1ab4c1fbc21394b4450R170

n8fr8 commented 7 years ago

Actually I think it already does implement the new format, but perhaps offers the legacy format for backwards compatibility. So i think it is in this process that you are seeing the key of the shorter length, and throwing the AssertionError.

vanitasvitae commented 7 years ago

So i think it is in this process that you are seeing the key of the shorter length, and throwing the AssertionError.

Thats right. smack-omemo assumes that the key MUST be of length 32 at the moment.

chrisballinger commented 7 years ago

The code is there for ChatSecure to send and receive both legacy and new format, but I was waiting until enough people had upgraded to receive the new format. I'd argue the old format isn't necessarily insecure, but I can see the reasoning behind the new format.

I'd recommend to continue parsing old format for legacy purposes instead of silently dropping. I can turn on new format in the next release, but at the very least there probably shouldn't be a remotely triggerable crash.

vanitasvitae commented 7 years ago

I pushed a change that prevents smack-omemo from crashing for now.