search

signalapp / libsignal-protocol-c

GNU General Public License v3.0
1.41k stars 296 forks source link

Occasionally failing signature verification #70

Closed gkdr closed 6 years ago

gkdr commented 7 years ago

I have:

Bug description

Several users reported that they cannot build a session because of a failing signature verification. Each time, the signature/bundle was created by the Android application Conversations, and could still be verified by other users of Conversations. It seems to be quite rare as it only was reported to me 3 times, therefore I suspect a weird corner case incompatibility between the C and Java implementations.

I asked the last user this happened to for some more information and was able to reproduce it.

The exact error happens inside session_builder_process_pre_key_bundle, returning SG_ERR_INVALID_KEY.

Steps to reproduce

(All in base64.)

dkonigsberg commented 7 years ago

I was able to reproduce the exact same error by feeding these key values to the Java implementation. So given these inputs, both libraries are failing in the exact same way. This doesn't surprise me much, because they're both running the exact same curve25519 signature verification code on these inputs once you dig down a little bit.

chrisballinger commented 7 years ago

I've seen this happen from the generation side. Sometimes libsignal-protocol-c will generate an invalid bundle, but I haven't been able to reproduce via unit test. I "solved" it by adding bundle verification code after generation, and regenerating if its invalid.

gkdr commented 7 years ago

@dkonigsberg Thank you for trying it out, and sorry if it was too much of a hassle. Since the OMEMO protocol used to wrap the Signal Protocol cannot communicate failures and I am not aware of such a problem with Conversations (and the reporting users told me it works fine), I assumed an interoperability issue. I'll investigate further in that direction, and leave this issue open to report the result.

Am I wrong to assume that if the signature data was somehow corrupted, it wouldn't pass through the deserialization stage?

@chrisballinger In which way was the bundle invalid, and how exactly do you verify it? In this case, the failing bundles have always been created by Conversations though, and actually I could tell users with this issue to let the Conversations-using friend message them to fix it, i.e. Conversations would use the valid bundle issues by libsignal-protocol-c to establish a session (the red-lettered error message probably creates a deer-in-headlights situation so they don't do that by themselves...).

Now I wonder if in all the cases where users told me that "it works fine on Conversations", users simply kept talking to each other because it doesn't display an error in bold, red letters like Pidgin does, and thus "automatically" repaired the session by simply using the other conversation partner's valid bundle.

dkonigsberg commented 7 years ago

If you've definitely been able to detect invalid signatures on the sender side, then one of the following issues could be causing the problem:

The problem with an issue like this, is that its sufficiently rare that its often easy to shrug off. Especially so if you have mechanisms in place to validate pre-key data the sender generates, before its sent to the recipient (such as on an intermediary key store server).

However, it would be great of we could actually come up with the necessary information to deterministically reproduce the issue. The problem is that in order to do this, you'd need to detect a failure and then collect all the key material (public and private) that went into generating that bad signature. Probably also info about the device it happened on. Most of this info is of the sort you don't actually want to ever expose from a user device, unless you subsequently throw it all away and generate new material.

chrisballinger commented 7 years ago

@gkdr I extracted the bundle verification code from session_builder.c:191 // int session_builder_process_pre_key_bundle(session_builder builder, session_pre_key_bundle bundle): https://github.com/ChatSecure/SignalProtocol-ObjC/blob/241f439a80afd68d9199e73e95f1350ade893b53/Classes/Models/SignalPreKeyBundle.m#L92

I don't have a unit test to reproduce the failure, but I'll put a breakpoint in there so if I see it again during development I can post the corresponding key material.

dkonigsberg commented 6 years ago

Closing because no updates have happened in a year, and its not entirely clear that the issue isn't in application code.