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.

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

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.

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.