Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit SharedSecret to 32 byte buffer #402

Merged
merged 3 commits into from
Feb 24, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Feb 10, 2022

Currently SharedSecret provides a way to get a shared secret using SHA256 as well as a way to use a custom hash function to get the shared secret. Internally SharedSecret uses a 256 byte buffer, this is a tad wasteful. We would like to keep the current functionality but reduce memory usage.

  • Patch 1: Pulls the new_with_hash logic out into a standalone public function that just returns the 64 bytes representing the x,y co-ordinates of the computed shared secret point. Callers are then responsible for hashing this point to get the shared secret (idea by @Kixunil, thanks).
  • Patch 2: Does trivial refactor
  • Patch 3: Uses a 32 byte buffer internally for SharedSecret. This is basically a revert of the work @elichai did to add the custom hashing logic. @elichai please holla if you are not happy with me walking all over this code :)

Note to reviewers

Secret obfuscation is done on top of this in #396, they could be reviewed in order if this work is of interest to you.

src/ecdh.rs Show resolved Hide resolved
src/ecdh.rs Outdated Show resolved Hide resolved
src/ecdh.rs Show resolved Hide resolved
@apoelstra
Copy link
Member

The 256-byte buffer was added in #180 which tried to make the shared secret more generic. Unfortunately the discussion there is overwhelmed by safety discussion regarding panics and callbacks. But I think the motivation was that users might implement a callback that provides larger hashes, or serializes the whole point, or something like that.

I agree that 256 seems extraordinarily large and also looks like it might've been a confusion between bits and bytes. I will try to review this PR more carefully today and see if I can understand.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 10, 2022

If custom hashing is still desired then I think SharedSecret should be generic with [u8; 32] being the default.

or serializes the whole point

To my knowledge this is a very bad idea and I'm not sure if the library should support it. At least without "dangerous" word being part of the API.

@elichai
Copy link
Member

elichai commented Feb 10, 2022

The point was to allow this function: https://docs.rs/secp256k1/latest/secp256k1/ecdh/struct.SharedSecret.html#method.new_with_hash to use something other than sha2, it could for example use sha512, or even something bigger(or a bit smaller like RIPEMD160). So I capped it at 256 bytes, there wasn't a specific reason for that number, I just thought that 64 bytes (e.g. sha512) is definitely reasonable, so I gave it a bit more leeway.

As for safety, why is that dangerous? the API requires at least 128 bits, and should be pretty safe AFAIU

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 10, 2022

As far as I understand if the point is used directly it could unexpectedly interact with other cryptography and cause vulnerabilities. That's why hashing is required.

@elichai
Copy link
Member

elichai commented Feb 10, 2022

As far as I understand if the point is used directly it could unexpectedly interact with other cryptography and cause vulnerabilities. That's why hashing is required.

Yeah, but sadly some applications hash only the x-coordinate while others hash the full compressed point. Also, some applications might want to use a domain separated hash function

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 10, 2022

Sure, I have no problem with custom hash functions.

@tcharding
Copy link
Member Author

tcharding commented Feb 16, 2022

I've implemented all review suggestions but I'm still unsure from the discussion if this PR is a good idea or not?

The point was to allow this function: https://docs.rs/secp256k1/latest/secp256k1/ecdh/struct.SharedSecret.html#method.new_with_hash to use something other than sha2, it could for example use sha512, or even something bigger(or a bit smaller like RIPEMD160).

Does this mean this PR is not useful and we should keep the current implementation in your opinion @elichai?

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 16, 2022

@tcharding I believe if variable-sized hash function is still required then generic parameter is the best way to go.

@tcharding
Copy link
Member Author

I messed around a bit with this but cannot work out how to do it in a way that is cleaner than the code is now. If you feel like writing up some ideas please do but its not really that important. I'll just close this after a while if nothing comes of it.

Cheers

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 17, 2022

I will try to do it to better understand where the problem is.

@elichai
Copy link
Member

elichai commented Feb 17, 2022

I'm not sure what's the motivation here or what's the problem you're trying to fix.

Is it not to "waste" 256 bytes for what's usually just 32 bytes?
if so, then:

  1. Is that really a problem/bottleneck?
  2. We can try and sprinkle some generics over it or make the custom hash one accept a mutable reference for the output array or something like that.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 17, 2022

Not just bottleneck but also makes code confusing and forces us to track the actual length.

@apoelstra
Copy link
Member

I wouldn't mind a PR to change the size to 64 bytes, which would let users get the whole point out (I know, a bad idea, but we've made it fairly unergonomic and this is needed for some deployed systems), and then if they actually need some crazy-wide hash they can just pull the explicit point out of the SharedSecret and do it themselves.

If anything there is value in reducing "256" because it is a confusing number.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 17, 2022

How about having a standalone function computing the point that doesn't use the SharedSecret type? Could be named with dangerous_ prefix and documentation saying it should be hashed.

@apoelstra
Copy link
Member

I'm ambivalent -- my feeling is the custom hash API is already dangerous, because you can put a broken hash in there, and also unergonomic relative to just using the default. So I think the current situation is ok, and adding more_dangerous functions would be an incomplete solution and increase the API surface.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 17, 2022

Sorry, I meant to replace the current custom hash API with a standalone function. SharedSecret would use sha256 only.

@apoelstra
Copy link
Member

I'd be fine with that.

In preparation for simplifying the `SharedSecret` internals pull the
`new_with_hash` function logic out into a standalone public function
that provides similar functionality without use of the `SharedSecret`
struct. Function now returns the 64 bytes of data representing a shared
point on the curve, callers are expected to the hash these bytes to get
a shared secret.
@tcharding
Copy link
Member Author

I've done a total re-write with ideas from the thread of discussion above. The PR message has been re-written. Thanks.

src/ecdh.rs Show resolved Hide resolved
src/ecdh.rs Show resolved Hide resolved
src/ecdh.rs Outdated Show resolved Hide resolved
src/ecdh.rs Outdated Show resolved Hide resolved
fn as_ref(&self) -> &[u8] {
&self.0
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Borrow would be also useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes, this was discussed elsewhere already recently, my bad. Just to clarify, we should use Borrow and remove AsRef, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have src/ecdsa/mod.rs:83:impl AsRef<[u8]> for SerializedSignature {, should this be changed to implement Borrow instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we should implement both AsRef and Borrow for shared secret.
For other types we have to evaluate them one-by-one since Borrow<U> for T requires that T::Eq and T::Hash behave exactly same as U::Eq and U::Hash provided the instance of U was obtained by calling Borrow on T.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we should implement both AsRef and Borrow for shared secret.

Cool, will add.

since Borrow for T requires that T::Eq and T::Hash behave exactly same as U::Eq and U::Hash provided the instance of U was obtained by calling Borrow on T.

I read that in the docs but I do not understand what it implies. I'll have to take direction from others on this until I do get it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code is correct. It would've been incorrect if SharedSecret overrode Hash or Eq implementation to behave differently than one on the slice it returns.


impl SharedSecret {
/// Creates a new shared secret from a pubkey and secret key.
#[inline]
pub fn new(point: &PublicKey, scalar: &SecretKey) -> SharedSecret {
let mut ss = SharedSecret::empty();
let mut buf = [0u8; 32];
let res = unsafe {
ffi::secp256k1_ecdh(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is better than calling shared_secret_point and then hashing it. If it is, then a test that compares them would be useful.

Copy link
Member Author

@tcharding tcharding Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, that is better. Thanks Doing so would introduce a dependency on bitocin_hashes (assuming we used that lib to do the hashing) where as now bitcoin_hashes dependency is optional. Left as is for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good reason, I believe. OTOH it annoys me that there are two SHA256 implementations in the code if you enable bitcoin_hashes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean, the implementation inside libsecp as well as the implementation in bitcoin_hashes? That is a problem for all users of libsecp, even upstream, and has been the subject of much discussion e.g. bitcoin-core/secp256k1#702

For now there's nothing we can do about it from the Rust bindings. (Well, ok, we could actually expose the hash functionality ourselves by patching the vendored library, but I think the solution we'd rather have is to somehow use the bitcoin_hashes implementations when available.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant it. Would be a nice change later if/when it's available.

@tcharding tcharding force-pushed the shared-secret-32-bytes branch 2 times, most recently from 2f2eb35 to b9afd27 Compare February 18, 2022 10:55
@tcharding
Copy link
Member Author

Thanks @Kixunil, implemented suggestions and force pushed!

src/ecdh.rs Show resolved Hide resolved
src/ecdh.rs Show resolved Hide resolved
@tcharding tcharding force-pushed the shared-secret-32-bytes branch 2 times, most recently from c26eeba to d22f1c0 Compare February 20, 2022 13:08
In test code we use multiple pub/sec keys. It is more intuitive if the
'secret 1' is generated by the owner of secret key 1.

Refactor only, no logic changes.
The `SharedSecret` uses sha256 to hash the secret, this implies the
secret is 32 bytes of data.

Currently we use a buffer of 256 bytes, this is unnecessary.

Change the implementation of `SharedSecret` to use a 32 byte buffer.
y.into()
});
assert_ne!(x_arr, [0u8; 32]);
assert_ne!(&y_arr[..], &[0u8; 32][..]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 834f63c:

I think you coulrd've kept these asserts in place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When removing these I could not see what functionality they were testing that was specific to no-std, so I removed them. Or am I missing something? If a test that checks the point returned is non-zero is useful I can add it as a unit test, but again, I don't understand the value of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they were just there as a sanity check. I agree they don't seem to have a very clear value (and also that they should be a unit test, if they exist at all).

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5603d71

@apoelstra
Copy link
Member

@Kixunil do you have open issues here or can I merge this?

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 23, 2022

I don't see any obvious defect but wasn't able to review deeply. The change is quite simple so I guess that's OK.

@apoelstra apoelstra merged commit 8b2edad into rust-bitcoin:master Feb 24, 2022
@tcharding tcharding deleted the shared-secret-32-bytes branch March 1, 2022 16:21
@dspicher
Copy link
Contributor

This PR removed the possibility to create a SharedSecret from a byte serialization, also from the still applying [u8; 32]. Was this intentional?

@Kixunil
Copy link
Collaborator

Kixunil commented Mar 10, 2022

@dspicher good question. I don't think it was intentional but I don't see a huge harm. Why would anyone pretend that some arbitrary bytes are shared secret? Why would anyone compute the actual secret differently than calling SharedSecret::new()?

@dspicher
Copy link
Contributor

dspicher commented Mar 10, 2022

In my case, they are not arbitrary bytes but arise from a previous serialization of a valid SharedSecret, for example for storage in a database.

@Kixunil
Copy link
Collaborator

Kixunil commented Mar 10, 2022

I'm not sure if it's a good idea to store SharedSecret but that use case sounds reasonable. You could also just turn SharedSecret into an array or better your own specific newtype. Still seems better to support it.

@apoelstra
Copy link
Member

Yeah, oops, we should support this. FWIW I think you can get this effect through the serde API.

@apoelstra
Copy link
Member

Oh, it looks like we don't even have serde de/serialization for this type. We should.

dspicher added a commit to dspicher/rust-secp256k1 that referenced this pull request Mar 10, 2022
This was accidentally removed in 8b2edad. See also the discussion
on rust-bitcoin#402
dspicher added a commit to dspicher/rust-secp256k1 that referenced this pull request Mar 10, 2022
This was accidentally removed in 8b2edad. See also the discussion
on rust-bitcoin#402
dspicher added a commit to dspicher/rust-secp256k1 that referenced this pull request Mar 10, 2022
This was accidentally removed in 8b2edad. See also the discussion
on rust-bitcoin#402
dspicher added a commit to dspicher/rust-secp256k1 that referenced this pull request Mar 10, 2022
This was accidentally removed in 8b2edad. See also the discussion
on rust-bitcoin#402
dspicher added a commit to dspicher/rust-secp256k1 that referenced this pull request Mar 10, 2022
This was accidentally removed in 8b2edad. See also the discussion
on rust-bitcoin#402
apoelstra added a commit that referenced this pull request Mar 11, 2022
463148f bump version to 0.22.1 (Dominik Spicher)
9be8e74 Allow SharedSecret to be created from byte array (Dominik Spicher)

Pull request description:

  This was accidentally removed in 8b2edad. See also the discussion
  on #402

  Closes #416.

ACKs for top commit:
  apoelstra:
    ACK 463148f

Tree-SHA512: 04e16226efa2cf6fd461eabb0c78e5b00f347c78e20c1c7561591ffa74a7259fb3265b49a9d7326caf70e4d5ce32a620485f1bd5538c292654f91eb68c2a57dc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants