Fix of possible DoS attack related session_cipher_decrypt processing

[steweg]

This patch fixes storing of invalid identity key. The scenario when it could happen:

• lets have 3 clients A, B & C
• all clients registers successfully on server and provide set of one-time pre-shared keys
• client C creates malformed pre-key signal message stating that it is from B and sends it to client A
• client A process the incoming pre-key signal message and accidentally stores the wrong identity key to B, so client C can impersonate B or just disable communication between A&B (depending on whether or not the message will be able to be decrypted)
• client B sends valid pre-key signal message to client A
• client A is not able to process the message as processing will never go beyond point of "is_trusted_identity" as identity key of B is different from the one that A has in it's store
• this state preservers until A somehow drops the stored identity key

This is currently easily prevented within Signal-Server code, which doesn't allow client C to send message stating that it is coming client B, but still this information should not be trusted based on fact that it is coming from server (what if server will be somehow compromised?).

Possible solutions:

• if the message is decrypted successfully, then it is up to user/UI to handle the identity key change appropriately
• if the message is not decrypted successfully, there is no user/UI interaction, but the session state will be already screwed, therefore this fix simply removes the stored identity key if decryption fails and identity key was changed

I also noticed that this problem is also present in libsignal-protocol-java or SignalProtocolKit, but in those the saveIdentity methods are having different return codes and therefore the processing is somehow different. E.g. in Signal-Android I see that you are returning in "IsTrustedIdentity" API always true when it comes to receiving, so the impact might be even worse...

I hope I explained everything, if not feel free to ask

[christophhagen]

I don't think this is a problem with libsignal-protocol-c, and is best handled by the surrounding implementation. The Signal policy for identity keys is "trust on first use", which means that the key is considered valid if no conflicting key exists for the recipient. With your scenario, client A will get an SG_ERR_UNTRUSTED_IDENTITY error once he receives the pre_key_signal_message from client B. It is then up to the user/implementation to either:

• delete the old identity key and try again to process the message (thereby storing the new identity)
• ignore the message (and all subsequent ones from the same identity key), effectively pinning the old identity key

Of course in this scenario trusting the old identity key would be wrong, but for this purpose (and to protect against MITM attacks) Signal offers the opportunity to compare the key fingerprint with the other party.

For your suggested changes, I see one problem: identity_key_changed will only be set to 1 if no identity key is present for the sender. If a key is already present, then signal_protocol_identity_is_trusted_identity() within session_builder_process_pre_key_signal_message() will return 0 and a SG_ERR_UNTRUSTED_IDENTITY error will occur. This means that the implementation doesn't solve the problem of needing to delete the old key, but instead makes the behaviour of the method a bit confusing.

Side note: The problem of a DoS attack exist within libsignal-protocol-c, since it is not possible to distinguish a malicious pre_key_signal_message of an attacker from a valid identity change of the correspondent without the ability to compare identities by some other means. This can be solved by the server, which checks that the identity of the sender matches the identity contained in the message. Additionally a client can verify the identity keys through fingerprints and then ignore any messages from different identities to prevent malicious key changes.

[steweg]

Verifying against the fingerprint of course provides the way for end-user to detect that something went wrong. But how many user are actually doing that? I think it is minority of users. less than 10%. So anything can be done to prevent this in automatic manner, it would be better.

The patch is targeting the scenario when A doesn't have any info about B identity key. It prevents just the case when attacker C impersonate B in a way that the the first message data is not able to decrypted. Therefore the "trust on first use" is valid (there is no stored identity key in persistent storage) and code continues. The session is able to be build from A perspective and therefore A stores the identity key. The message content is later on not able to decrypted and as such it will not be pushed to UI at all, therefore user is not aware, while the session data with wrong identity key is being stored in the storage in the background.

From this point B is not able to establish the session with A a A will drop any message due to "untrusted" identity key being used until A does drop the wrong info about B (coming from C).

I agree this can be solved by various surrounding implementations and also on server implementations, but first at all implementer must be aware that they need to handle this somehow. Unfortunately currently the documentation is not stating this anywhere. So if implementer is using libsignal-protocol-c and trust the API definitions, they might easily end-up with problems as that code is actually saving data into permanent storage, while the processing of message is not yet completed. This itself is not a problem, if the code will also rollback all the changes that were done along the processing if something went wrong during the process. In this case it must remove the accidentally stored identity key data.

Hope you get the point

[christophhagen]

I see your point. In general it makes sense not to store the identity key if the pre key signal message can't be decrypted.

However even with this fix it would still be possible for an attacker to send a valid message to A stating that it is from B, and A would store the wrong identity key. libsignal-protocol-c itself can't protect against that.

Unfortunately this fix would mean that the API would change (through save_identity), which should be avoided if possible. My advice would still be to handle this problem by surrounding implementation.

[steweg]

I agree with you, changing the API should be avoided if possible, unfortunately I don't believe it is possible/suitable.

For "valid" messages from C stating impersonating that it is B, this is up to UI to handle, as the message itself will be actually seen in conversations. So the only part problematic part is handling of "invalid" messages.

Anyway you are now at least aware that there is such problem. If you prefer not to merge my proposal, I am ok with that, so feel free to close the PR.