search

whisperfish / libsignal-protocol-rs

A Rust interface to the Signal Protocol. DEPRECATED in favour of https://github.com/signalapp/libsignal-client ! Signal reimplemented the whole thing in Rust.
https://michael-f-bryan.github.io/libsignal-protocol-rs
GNU General Public License v3.0
44 stars 10 forks source link

Add calculate_agreement to PublicKey #63

Closed gferon closed 4 years ago

gferon commented 4 years ago

This is required to implement secondary device provisioning in libsignal-service-rs.

gferon commented 4 years ago

If you want, I can try to fix the potential issue with freeing memory in this PR as well, but I guess it's not really a problem right now.

rubdos commented 4 years ago

@Michael-F-Bryan, you might want to take a look at this, since it contains some crypto and unsafe. @gferon and his colleague are on my #whisperfish Matrix channel, they're helping some extra features into libsignal-service wrt. multi device and registration.

Michael-F-Bryan commented 4 years ago

Sorry for the delay @rubdos, I've got annual leave all week and haven't really touched my laptop.

The proposed code seems alright from a safety and implementation perspective, but relying on Box using libc feels a little brittle... I believe it's guaranteed that Rust will use the system allocator by default but if a consumer decided to use jemalloc instead, they'd run into bizarre crashes. A more robust solution would be to copy the data to a Rust Vec and use the libc crate to free() the original data.

rubdos commented 4 years ago

No problem on the delay. Seems like I pulled some more people into your libsignal crates :-)

gferon commented 4 years ago

Sorry for the delay @rubdos, I've got annual leave all week and haven't really touched my laptop.

The proposed code seems alright from a safety and implementation perspective, but relying on Box using libc feels a little brittle... I believe it's guaranteed that Rust will use the system allocator by default but if a consumer decided to use jemalloc instead, they'd run into bizarre crashes. A more robust solution would be to copy the data to a Rust Vec and use the libc crate to free() the original data.

This is exactly what I wanted to do, but since I found it done exactly this way in hkdf.rs, I didn't want to introduce another way of doing it :smile:

EDIT: thanks for the great feedback, I've applied the changes you proposed and added a test :rocket:

Michael-F-Bryan commented 4 years ago

This is exactly what I wanted to do, but since I found it done exactly this way in hkdf.rs, I didn't want to introduce another way of doing it :smile:

Hehe... oops, busted! I love how my original comment even has the same soundness questions we've brought up here.

gferon commented 4 years ago

I can fix the conflicts if you want, so you can review and merge?

EDIT: I just fixed the conflicts, this PR is ready for review!