Obfuscate shared secret when printing #396

tcharding · 2022-02-09T06:13:55Z

Currently printing the SharedSecret using Display or Debug prints the real secret, this is sub-optimal. We have a solution for other secrets in the project where printing is obfuscated and we provide a display_secret method for explicitly printing.

Mirror the logic for other secrets and obfuscate the SharedSecret when printing.

Patches 1 - 5: Clean up.
Patch 6: The meat and potatoes.

This is the final change needed to:
Resolve: #226

tcharding · 2022-02-09T06:55:31Z

I'm confused as to why CI is failing? This PR does not make material changes to the 256 byte array so why are PartialEq etc. missing for Rust 1.29?

elichai · 2022-02-09T17:46:40Z

src/ecdh.rs

+    #[inline]
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut slice = [0u8; BUF_SIZE * 2];
+        let hex = to_hex(&self.secret, &mut slice).expect("fixed-size hex serializer failed");


This serializes the full buffer, which can be longer than the actual secret.
Maybe DisplaySecret should wrap the SharedSecret type and use its Deref impl to get the full secret?

I think storing reference is better, see below.

Now that this PR is being done on top of #402, these comments are resolved I believe.

src/ecdh.rs

Kixunil · 2022-02-09T20:27:47Z

src/secret.rs

@@ -91,7 +91,7 @@ pub struct DisplaySecret {
 impl fmt::Debug for DisplaySecret {


Unrelated but I think this should be called just Display - same as std::path::Display

I'm going to leave this as is for now, can re-visit if you feel strongly.

Kixunil · 2022-02-09T20:30:23Z

src/ecdh.rs

+/// Formats the explicit byte value of the secret as a little-endian hexadecimal string using the
+/// provided formatter.
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct DisplaySecret {


Name could be Display but ecdh::Display is weird. ecdh::shared_secret::Display would look better but since secret is the only type here the additional module looks weird. IDK what's best.

As above, I'll leave this for now. Thanks.

src/ecdh.rs

tcharding · 2022-02-10T10:40:47Z

Converting to draft until #402 is merged or closed since 402 will effect how this PR (and review suggestions) are implemented.

tcharding · 2022-02-10T10:41:52Z

You can't derive these because const generics are unsupported in older Rust versions and arrays have the traits implemented only up to size of 32.

Cool, thanks. That explains the mysterious (to me) Rust 1.29 CI fail.

dr-orlovsky · 2022-02-17T16:30:04Z

JFYI previous work on a similar matter: #312

tcharding · 2022-02-18T12:00:24Z

JFYI previous work on a similar matter: #312

Yep, I tying in with what you did there! Thanks

tcharding · 2022-02-18T12:25:48Z

Strange that nightly and stable CI runs pass but not the others (its a rustdoc test that fails, should not be effected by the toolchain)?

Kixunil · 2022-02-18T13:51:18Z

src/secret.rs

@@ -35,7 +36,7 @@ macro_rules! impl_display_secret {

                hasher.write(DEBUG_HASH_TAG);


I believe the test fails because DefaultHasher::new() above returns hasher seeded with a different random seed each time. Which also means this implementation is actually wrong if we want to allow comparing different secrets.

Interesting, thanks for debugging it for me. So shared_secret_point is non-trivial to use then. I'll put some more work into it tomorrow. Cheers.

On further thought, I don't get that, the same data hashed multiple times should give the same output irrespective of the hashing implementation. I haven't looked at the code yet, I was just pondering your message.

Edit: something doesn't seem right about this, DefaultHasher::new is guaranteed to return same hasher and the implementation uses fixed seed 0, 0.

Original message:

std hasher is one intended for HashMap and implements DoS protection by randomizing among other things. It produces same hash for same input for same instances of the hasher but since we call new on each debug we're randomizing it.

if we want to allow comparing different secrets

I meant humans comparing, not computers here. Only the Debug implementation is wrong.
Unless we want to implement our own hasher or store DefaultHasher in static variable I suggest to just remove the hash when compiled without bitcoin_hashes.

Kixunil · 2022-02-19T11:33:14Z

src/secret.rs

+    /// let key = secp256k1::ONE_KEY;
+    ///
+    /// // Normal display hides value.
+    /// assert_eq!("SecretKey(#2518682f7819fb2d)", format!("{:?}", key));


Oh, I see now, this is actually hard coded. The test is wrong because std doesn't guarantee any particular hash function or value. I suggest to test it with bitcoin_hashes only.

I think std should guarantee that the shared secrets don't change, and it is a breaking change if they do.

Having said this, we shouldn't guarantee the debug output of anything.

They don't guarantee it in case a better hash function is available so it can be switched. Notice that it's specifically called DefaultHasher - there was SipHasher before. I believe that's fine, if people want a specific hash function they should just explicitly use it (as a crate or NIH).

Agree on not guaranteeing debug output.

Happened to be the same hasher version that produced the expected output. So different versions of Rust produce different hashes - as documented.

Left an example of debug output in the rustdocs of SecretKey, removed the assertion. Removed all mention of debug output for the other two secrets (KeyPair and SharedSecret).

They don't guarantee it in case a better hash function is available so it can be switched. Notice that it's specifically called DefaultHasher - there was SipHasher before. I believe that's fine, if people want a specific hash function they should just explicitly use it (as a crate or NIH).

I really don't like this. The implication that if users try to share a secret, and one user has a slightly different version of rust-secp than the other, that they will be unable to communicate because their crypto doesn't work.

That's non-issue because we only use this for debug output which must not be relied on. Creation of SharedSecret still uses sha256.

apoelstra · 2022-02-21T15:19:57Z

src/key.rs

    #[inline]
-    pub fn serialize_secret(&self) -> [u8; constants::SECRET_KEY_SIZE] {
+    pub fn into_secret(&self) -> [u8; constants::SECRET_KEY_SIZE] {


In 2ccd54b:

I don't know about this. into_secret I would expect to produce a SecretKey or maybe a SharedSecret, not a byte array. A byte array I'd expect to get from something whose name was serialization-related.

Something bothered me about this and now I see what. secret.into_secret() is weird. Even worse it takes &self, which shouldn't use into_ prefix. I think the name should stay or _secret should be removed. Or perhaps call it to_bytes() to remove the impression that there's costly serialization.

Nice. I'd be happy with to_bytes.

The system works, to_bytes it is.

Funny side note, while doing this change originally I looked up that naming table (again), and managed to read it incorrectly and write the commit log based on this :) Naming things really is hard.

We need to write our own, non-confusing version. :)

apoelstra

ACK ff39f28

This is a breaking change because it removes a bunch of the SharedSecret API. But I also contend that it's a breaking change because it changes the sharedsecret hash function.

Kixunil · 2022-02-22T21:01:29Z

@apoelstra did you confuse it with the other PR?

apoelstra · 2022-02-22T22:43:39Z

My comment refers to 41dc861 which I think is only in this PR.

apoelstra · 2022-02-22T22:44:40Z

Ah, no -- the first three commits are from an old version of #402, which is where the breaking changes come in.

tcharding · 2022-02-23T08:00:34Z

My bad, this often happens to me, at first splitting the PRs seems like a good idea but if/as they grow it becomes entangled. And rebasing these was a PITA :)

apoelstra · 2022-02-23T18:54:10Z

@tcharding is your intent here to build on #402?

tcharding · 2022-02-24T08:43:07Z

Builds on top of the correct current tip of #402 now, thanks for noticing.

@Kixunil

5603d71 Limit SharedSecret to 32 byte buffer (Tobin Harding) d5eeb09 Use more intuitive local var numbering (Tobin Harding) 834f63c Separate new_with_hash into public function (Tobin Harding) Pull request description: 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. ACKs for top commit: apoelstra: ACK 5603d71 Tree-SHA512: 48982a4a6a700a111e4c1d5d21d62503d34f433d8cb303d11ff018d2f2be2467fa806107018db16b6d0fcc5ff1a0325dd5790c62c47831c7cd2141a1b6f9467d

apoelstra · 2022-02-24T15:25:38Z

#402 is merged. Can you rebase this? (It actually looks like the tip of 402 is still different from the commit in this PR)

Hashing the debug output for secrets can be done with `bitcoin_hashes` not just `std`. Mention this in the obfuscated string output when neither are available.

In array initialisation we use magic number 64, this is the secret bytes length multiplied by 2. Please note; we still use the magic number 32, left as such because it is used in various ways and its not immediately clear that using a single const would be any more descriptive. Use `SECRET_KEY_SIZE * 2` instead of magic number 64.

The identifier `i` is predominantly used for indexing an array but we are using it as a place holder for the iterated value of an array that is then printed. The identifier `byte` is more descriptive. Done in preparation for adding similar code to the `ecdh` module.

tcharding · 2022-02-24T19:52:28Z

It actually looks like the tip of 402 is still different from the commit in this PR

It is hard to get good help :)

Kixunil · 2022-02-25T08:06:45Z

src/key.rs

@@ -212,9 +212,9 @@ impl SecretKey {
        SecretKey(sk)
    }

-    /// Serializes the secret key as byte value.
+    /// Gets the secret key as a byte value.


"Returns" would sound more natural to me.

Thanks, use as suggested.

Kixunil · 2022-02-25T08:12:57Z

src/key.rs

    #[inline]
-    pub fn serialize_secret(&self) -> [u8; constants::SECRET_KEY_SIZE] {
+    pub fn to_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] {


Since this is key pair I think it'd be clearer to name this method secret_bytes() (Was also thinking of to_ prefix but that usually isn't used for getters.)

Weak ACK -- I think it would be clearer to label this method with secret somehow.

Too trigger happy with the the to_ conversions huh :) Use secret_bytes as suggested. Cheers.

The `serialize_secret` method is a getter method, it does not do any serialisation. However we use the method on secret keys and key types so in order for the name to be uniform use the descriptive name `secret_bytes`. Rename `serialize_secret` to be `secret_bytes`.

Improve rustdocs on `display_secret` by doing: - Minor improvements to the rustdocs to aid readability in the editor. - Do not guarantee (`assert_eq!`) debug output

Currently printing the `SharedSecret` using `Display` or `Debug` prints the real secret, this is sub-optimal. We have a solution for other secrets in the project where printing is obfuscated and we provide a `display_secret` method for explicitly printing. Mirror the logic for other secrets and obfuscate the `SharedSecret` when printing.

tcharding · 2022-02-28T07:25:11Z

Implemented suggestions and force push. The only changes to the PR are the two suggested (docs fix and method name).

apoelstra

ACK cf6badf

tcharding force-pushed the obfuscate-debug-shared-secret branch from d12edb6 to de3527f Compare February 9, 2022 06:42

tcharding force-pushed the obfuscate-debug-shared-secret branch from de3527f to 04726d9 Compare February 9, 2022 06:58

elichai reviewed Feb 9, 2022

View reviewed changes

Kixunil reviewed Feb 9, 2022

View reviewed changes

tcharding marked this pull request as draft February 10, 2022 10:40

tcharding mentioned this pull request Feb 18, 2022

Limit SharedSecret to 32 byte buffer #402

Merged

tcharding force-pushed the obfuscate-debug-shared-secret branch from 04726d9 to 6788d4b Compare February 18, 2022 11:41

tcharding force-pushed the obfuscate-debug-shared-secret branch 2 times, most recently from f7b229e to 8535fa1 Compare February 18, 2022 12:15

tcharding marked this pull request as ready for review February 18, 2022 12:25

Kixunil reviewed Feb 18, 2022

View reviewed changes

Kixunil reviewed Feb 19, 2022

View reviewed changes

tcharding force-pushed the obfuscate-debug-shared-secret branch 5 times, most recently from 5db4115 to 85fc336 Compare February 21, 2022 13:51

apoelstra reviewed Feb 21, 2022

View reviewed changes

tcharding force-pushed the obfuscate-debug-shared-secret branch from 85fc336 to ff39f28 Compare February 22, 2022 11:51

apoelstra previously approved these changes Feb 22, 2022

View reviewed changes

apoelstra mentioned this pull request Feb 23, 2022

update libsecp to latest ersion; bump major version number #384

Merged

tcharding dismissed apoelstra’s stale review via ea6a378 February 24, 2022 08:39

tcharding force-pushed the obfuscate-debug-shared-secret branch 2 times, most recently from ea6a378 to ea20bbb Compare February 24, 2022 08:42

tcharding force-pushed the obfuscate-debug-shared-secret branch from ea20bbb to 79c5a1e Compare February 24, 2022 09:03

tcharding added 3 commits February 24, 2022 19:48

Mention bitcoin_hashes in obfuscated secret msg

6dca996

Hashing the debug output for secrets can be done with `bitcoin_hashes` not just `std`. Mention this in the obfuscated string output when neither are available.

tcharding force-pushed the obfuscate-debug-shared-secret branch from 79c5a1e to 47c9adf Compare February 24, 2022 19:50

Kixunil reviewed Feb 25, 2022

View reviewed changes

tcharding added 3 commits February 28, 2022 07:11

Improve rustdocs for displaying secrets

e4be664

Improve rustdocs on `display_secret` by doing: - Minor improvements to the rustdocs to aid readability in the editor. - Do not guarantee (`assert_eq!`) debug output

tcharding force-pushed the obfuscate-debug-shared-secret branch from 47c9adf to cf6badf Compare February 28, 2022 07:24

apoelstra approved these changes Feb 28, 2022

View reviewed changes

apoelstra merged commit ab6df6f into rust-bitcoin:master Feb 28, 2022

tcharding deleted the obfuscate-debug-shared-secret branch March 1, 2022 16:21

		@@ -91,7 +91,7 @@ pub struct DisplaySecret {
		impl fmt::Debug for DisplaySecret {

		@@ -35,7 +36,7 @@ macro_rules! impl_display_secret {

		hasher.write(DEBUG_HASH_TAG);

Obfuscate shared secret when printing #396

Obfuscate shared secret when printing #396

Conversation

tcharding commented Feb 9, 2022 • edited Loading

tcharding commented Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Feb 10, 2022

tcharding commented Feb 10, 2022

dr-orlovsky commented Feb 17, 2022

tcharding commented Feb 18, 2022

tcharding commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Feb 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Feb 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil commented Feb 22, 2022

apoelstra commented Feb 22, 2022

apoelstra commented Feb 22, 2022

tcharding commented Feb 23, 2022 • edited Loading

apoelstra commented Feb 23, 2022

tcharding commented Feb 24, 2022 • edited Loading

apoelstra commented Feb 24, 2022

tcharding commented Feb 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Feb 28, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Feb 9, 2022 •

edited

Loading

tcharding commented Feb 9, 2022 •

edited

Loading

Kixunil Feb 19, 2022 •

edited

Loading

Kixunil Feb 20, 2022 •

edited

Loading

tcharding commented Feb 23, 2022 •

edited

Loading

tcharding commented Feb 24, 2022 •

edited

Loading