Alternative: Passing custom hash functions to ECDH by elichai · Pull Request #180 · rust-bitcoin/rust-secp256k1

elichai · 2019-11-09T22:18:07Z

This is an alternative to #164.
It's a lot more code but I feel it's more idiomatic that way.
the ffi::SharedSecret wasn't really needed so I removed it.

Review Tip:

Review commit by commit (obviously :P)
Generated cargo doc locally and compare with https://docs.rs/secp256k1/0.16.0/secp256k1/ecdh/struct.SharedSecret.html

Basically all the user need to do is use an array and do hash.into(). I hope it's an easy enough to use API, I feel like the more complicated our trait system gets the harder it is for users to use, but I still think it's better than runtime checks.

elichai · 2019-11-09T22:37:17Z

Arghh this still isn't safe.
If the closure panics it will panic across into the C code.
and that's UB.

I need to wrap the whole hash_callback in catch_unwind

elichai · 2019-11-10T10:44:38Z

catch_unwind is a std thing :(

I see two options:

Feature gating this for std only. (@shmishleniy @dvdplm Does that work for your usecase?).
Marking it unsafe and documenting that the passed function can not panic and if it does there be dragons.
Combining both 1 and 2 by providing 2 variants of the same function.

@apoelstra Would love to hear your thoughts :)

elichai · 2019-11-10T11:25:37Z

Last commit is an example of how it would look with feature = "std"

dvdplm · 2019-11-11T08:50:29Z

Does that work for your usecase?

std-only should be fine for us, yes.

Re-panicking callbacks, good catch, it's a problem I didn't consider. Going the catch_unwind route would mean excluding switching to aborting panics, which is something we have considered doing in the past for binary size reasons (I think?). It's not a show-stopper for us, mind you, but somewhat unfortunate.
Makes me wonder if adding a new_raw() method wouldn't be an easier way though; wouldn't that help with both the std-only and panicking behaviour problem?

elichai · 2019-11-11T09:34:44Z

You can still compile with panic=abort it just won't catch any aborts.
That's actually the safest route when it comes to FFI

apoelstra · 2019-11-27T20:05:58Z

Yeah, I think we should add an unsafe new_with_hash_no_panic variant which doesn't do the catch_unwind.

Concept ACK the existing code.

elichai · 2019-11-27T22:35:17Z

The exact traits implementation for SharedSecret is still open for debate as I'm not sure what's needed and what not https://docs.rs/secp256k1/0.16.0/secp256k1/ecdh/struct.SharedSecret.html
have Index, Eq, and an as_ptr(). the as_ptr() I don't think should be on the public API here. as it's just a helper struct, it's not an FFI one.
Regarding Index and Eq I don't know. because I implemented both AsRef and Deref to [u8] it should kind of behave and have all of [u8] functions, but on the other hand I had to implement PartialEq

apoelstra · 2019-11-30T14:13:49Z

We should also implement Eq because sometimes you need that explicitly (e.g. when using as a hashmap key) and AsRef is not sufficient. Index is probably fine to just leave implicit.

I have a test case now (communication encryption with a Coldcard wallet) so I will try to use this PR for that today and report back.

apoelstra · 2019-12-01T01:51:59Z

Tested the code; I'm happy with the API and it works (I can communicate with the Coldcard in an encrypted fashion.) Will review in detail tomorrow.

stevenroose · 2019-12-05T15:55:49Z

ACK d2c2e5d did some testing locally, but CI is probably more complete

real-or-random

Looks good to me overall! 👍

real-or-random · 2019-12-05T23:33:07Z

+
+    #[test]
+    #[should_panic(expected = "This is UB")]
+    // This test is technically underined behavior. but it's interesting to see if the behavior changes.


s/underined/undefined

Is it really a good idea to have this test? I mean, it could fail on other C implementations even, no?

Yeah you're right. It was mainly me curious when/where/if this test will ever get broken (even though it's UB it's an indicator of the current instability of unwrapping through FFI, there are some proposals to just make this abort or something similar.

Heh, yeah, we should probably remove this. Or maybe comment it out or something.

apoelstra · 2019-12-06T20:04:20Z

-        &self.0[index]
+    /// Set the length of the object.
+    pub(crate) fn set_len(&mut self, len: usize) {
+        self.len = len;


I wonder if this should be marked unsafe. It is not technically unsafe, in that we're using safe code so it won't allow access to uninitialized memory or OOB access, but it seems a bit footgunny

apoelstra · 2019-12-06T20:09:47Z

+        where F: FnMut([u8; 32], [u8; 32]) -> SharedSecret
+    {
+        let mut ss = SharedSecret::empty();
+        let hashfp: ffi::EcdhHashFn = hash_callback::<F>;


This is really a fascinating use of Rust's type system. I did not know that generics and FFI would be allowed to interact like this.

apoelstra · 2019-12-06T20:10:55Z

+                &mut hash_function as *mut F as *mut c_void,
+            )
+        };
+        debug_assert!(res >= 16); // 128 bit is the minimum for a secure hash function and the minimum we let users.


I like this, though of course users are welcome to bypass it by calling set_len (or just copying into a slice and truncating) on the result.

apoelstra · 2019-12-06T20:16:51Z

ACK except that I would like you to remove the UB test.

(The other nits would be nice to fix, but no big deal.)

apoelstra · 2019-12-09T19:48:35Z

ping @elichai

elichai · 2019-12-09T20:54:16Z

@apoelstra Done

apoelstra

thanks!

elichai changed the title ~~Alternative: Passing custom hash functions~~ Alternative: Passing custom hash functions to ECDH Nov 9, 2019

elichai force-pushed the 2019-11-SharedSecret branch from 5f5880f to eadc88f Compare November 27, 2019 22:25

elichai added 3 commits December 5, 2019 17:45

Replace SharedSecret with a more generic alternative

9759cb0

Add 'new_with_hash' function to SharedSecret

af8fa21

Add tests for the new SharedSecret::new_with_hash() function

f804282

elichai force-pushed the 2019-11-SharedSecret branch from 21d304d to d2c2e5d Compare December 5, 2019 15:47

stevenroose previously approved these changes Dec 5, 2019

View reviewed changes

elichai added 2 commits December 5, 2019 18:05

Fixed secp256k1_ecdh fuzztarget

ca8ea92

feature gate new_with_hash with std only, added catch_unwind

124c1f3

elichai dismissed stevenroose’s stale review via 785a82a December 5, 2019 16:05

elichai force-pushed the 2019-11-SharedSecret branch from d2c2e5d to 785a82a Compare December 5, 2019 16:05

real-or-random self-requested a review December 5, 2019 20:18

real-or-random reviewed Dec 5, 2019

View reviewed changes

apoelstra reviewed Dec 6, 2019

View reviewed changes

apoelstra mentioned this pull request Dec 6, 2019

add posibility to create SharedSecret with applied custom hash function #164

Closed

Add an unsafe variant of new_with_has called new_with_hash_no_panic

5619f2a

Add ECDH to the no-std tests

92c42ca

elichai force-pushed the 2019-11-SharedSecret branch from 785a82a to 92c42ca Compare December 9, 2019 20:54

apoelstra approved these changes Dec 9, 2019

View reviewed changes

apoelstra merged commit 4b74091 into rust-bitcoin:master Dec 9, 2019

This was referenced Dec 9, 2019

Update changelog for v0.17.0 #185

Merged

add c_int return type to EcdhHashFn signature, pub for types mod #161

Closed

elichai deleted the 2019-11-SharedSecret branch January 23, 2020 14:09

real-or-random mentioned this pull request Feb 3, 2020

secp256k1_ecdh_hash_function must return 0 or 1 #196

Closed

apoelstra mentioned this pull request Feb 10, 2022

Limit SharedSecret to 32 byte buffer #402

Merged

Conversation

elichai commented Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elichai commented Nov 9, 2019

Uh oh!

elichai commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elichai commented Nov 10, 2019

Uh oh!

dvdplm commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elichai commented Nov 11, 2019

Uh oh!

apoelstra commented Nov 27, 2019

Uh oh!

elichai commented Nov 27, 2019

Uh oh!

apoelstra commented Nov 30, 2019

Uh oh!

apoelstra commented Dec 1, 2019

Uh oh!

stevenroose commented Dec 5, 2019

Uh oh!

real-or-random left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

real-or-random Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

real-or-random Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

elichai Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

apoelstra Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

apoelstra Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

apoelstra Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Dec 6, 2019

Uh oh!

apoelstra commented Dec 9, 2019

Uh oh!

elichai commented Dec 9, 2019

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elichai commented Nov 9, 2019 •

edited

Loading

elichai commented Nov 10, 2019 •

edited

Loading

dvdplm commented Nov 11, 2019 •

edited

Loading

elichai Dec 6, 2019 •

edited

Loading