Truncate secret hash #722

tcharding · 2024-08-22T02:14:19Z

The core::hash::Hasher and bitcoin_hashes hash types implement formatting traits slightly differently

We default to displaying in hex but core defaults to using base 10
We truncate with precision not width parameter but core truncates with both

Anywho, this PR fixes the secret display truncation.

tcharding · 2024-08-22T02:15:26Z

This PR fixes the question asked here: https://github.com/rust-bitcoin/rust-bitcoin/blob/b05cf43d22d36bddd90f9e512ce15158736c4812/bitcoin/src/crypto/key.rs#L1529

tcharding · 2024-08-22T02:21:20Z

This is non-urgent so I'll just wait till #699 goes in to debug CI failure.

Kixunil · 2024-08-22T04:18:29Z

hash is a 32 byte integer (I think) and for integral types precision is ignorded

It is but I think many people perceive it mainly as string. Though if we were to truncate it it's unclear if we should truncate the beginning or the end.

src/secret.rs

apoelstra · 2024-08-22T13:13:37Z

A hash is not an integer. In no sense is a hash an integer. You can interpret a hash as an integer but then you need to make a bunch of decisions about endianness.

A hash is an opaque bytestring. If you truncate it then you should take the prefix not the suffix. This is what git does, this is what Bitcoin does in multiple places (e.g. bip32 fingerprints, the sha256 "checksum" on base58 addresses, etc), and this is what's done to construct "truncated hashes".

Kixunil · 2024-08-22T21:12:53Z

@apoelstra this is also what GPG doesn't do - the short key identifiers are suffixes. ;) Yes, I hate it.

Anyway, I do think we should allow it.

apoelstra · 2024-08-22T21:26:05Z

Hah, TIL another crazy thing about GPG. (I guess I did know this actually, and have been annoyed about it because how hard it is visually to compare the short ID to the long fingerprint.)

tcharding · 2024-08-22T21:29:04Z

A hash is not an integer.

TIL, thanks. Raised issue: rust-bitcoin/hex-conservative#101

In order correctly truncate the secret data we need to use recent version of `bitcoin_hashes`. Remove the range dependency and update the lock files.

Currently we are attempting to truncate the hash created using `bitcoin_hashes` by using the "width" formatting parameter instead of the "precision" parameter. `hex-conservative` truncates with the "precision" parameter as is expected since a hash is not an integral type. Use the formatting string `"{:.16}"` which is the "precision" formatting parameter.

tcharding · 2024-08-22T23:19:30Z

Buuuuut, we'd have to remove the range dependency on bitcoin_hashes.

apoelstra · 2024-08-22T23:21:12Z

Let's just remove the range dependency. It doesn't work properly anyway. See #673. I thought we'd removed it already because of that issue.

apoelstra · 2024-08-22T23:22:16Z

Not sure what's up with CI. I think maybe nightly has broken something xargo-related?

tcharding · 2024-08-22T23:30:36Z

Seems so, this works

RUSTUP_TOOLCHAIN=nightly-2024-08-04 xargo run --release --target=x86_64-unknown-linux-gnu

tcharding · 2024-08-22T23:31:08Z

That is the nightly version currently used in #699

Kixunil · 2024-08-23T04:11:32Z

~~WHAT?! What's the problem with ranged dependency?~~

Actually we made other changes that make #673 non-issue, but there's still #723. I'd like to use ranged dependencies for all private dependencies to minimize dependency duplication.

Kixunil · 2024-08-23T04:21:10Z

Cargo.toml

@@ -37,7 +37,7 @@ global-context-less-secure = ["global-context"]
 secp256k1-sys = { version = "0.10.0", default-features = false, path = "./secp256k1-sys" }
 serde = { version = "1.0.103", default-features = false, optional = true }

-hashes = { package = "bitcoin_hashes", version = ">= 0.12, <= 0.14", default-features = false, optional = true }
+hashes = { package = "bitcoin_hashes", version = "0.14", default-features = false, optional = true }


I believe moving the lower bound from 12 to 13 would've been enough - 0.13 already uses hex-conservative. But maybe it requires certain version to work so in that case we would need:

[features] hashes = ["dep:hashes", "dep:hex-conservative"] [dependencies] hex-conservative = { version = ">=0.?", optional = true } hashes = { package = "bitcoin_hashes", version = ">= 0.13, <= 0.14", default-features = false, optional = true }

Where ? in the version would be replaced with the one that had truncating implemented already. Note however that ranged version was broken by #723 so we have to fix it too if we want ranged versions.

But also, we have hex encoding/decoding in the crate anyway, so why not just have it directly as a dependency and use it for all formatting/decoding? Maybe optional with it being off making Debug slower and Display/FromStr not present.

To be clear, this is not why I've requested changes, you don't have to do it. It's just the wrong formatting that's a problem.

Kixunil · 2024-08-23T04:46:50Z

src/secret.rs

@@ -28,7 +28,7 @@ macro_rules! impl_display_secret {
                hasher.write(&self.secret_bytes());
                let hash = hasher.finish();

-                f.debug_tuple(stringify!($thing)).field(&format_args!("#{:016x}", hash)).finish()
+                f.debug_tuple(stringify!($thing)).field(&format_args!("#{:.16x}", hash)).finish()


This removes zero-padding since the type is just u64. Also the commit message is wrong - the type returned from finish is u64, not Hasher. We should just drop this commit.

No sweat, dropped.

Kixunil · 2024-08-23T05:49:23Z

src/key.rs

+        #[cfg(feature = "std")]
+        {
+            let got = format!("{:?}", sk);
+            let want = "SecretKey(#2f4d520bd6874292)";


I've just realized, I don't believe this value is stable - different compiler version/OSes/... may produce something different. We need to check these properties instead:

got.len() == im_too_lazy_to_count_it; got.starts_with("SecretKey(#"); got.ends_with(")"); got[11..(got.len() - 1)].chars().all(|c| c.is_ascii_hexdigit()); !sk.display_secret().to_string().contains(&got[11..(got.len() - 1)]);

Unrelated to this PR :)

It is related. You've added a clearly broken test and the only reason it didn't manifest as broken is that nobody changed the hash algo between 1.63 and 1.80. The doc specifically says: "The internal algorithm is not specified, and so it and its hashes should not be relied upon over releases."

If you don't want to property-test it then please at least remove it. (with a comment that's why) Also it might be prudent to use RandomState::new().build_hasher() at least in tests to make sure nobody adds the test back by accident but potentially to also frustrate an attacker that would like to somehow recover some bits of the private key. (Does it make sense @apoelstra ?)

oh woops I can't read diffs, I was thinking that because I did not touch the std behaviour I did not write the test. My bad. I dropped the test, I think we are all convinced that this PR does what it says it does, re truncation. (I was very testy [0] yesterday afternoon.)

[0] definition of 'testy': https://duckduckgo.com/?q=testy+def&t=canonical&ia=web

@Kixunil I think, ultimately Rust's reduced-siphash would have to be super broken for it to actually leak any bits of the key, and in the limit it can only leak 64 bits because that's how big its output is, but it does make me a little nervous to treat it as a random oracle like this.

Using a randomized hasher would help with this, at the cost of making the "key fingerprints" no longer work as fingerprints when they occur in logs. I think that would be super frustrating and likely even confusing, so we shouldn't do it in non-test contexts. (And yes, I appreciate that users already shouldn't be expecting these "fingerprints" to stay consistent because stdlib might change them....but in practice they probably will.)

Honestly I'm inclined to get rid of the Hasher stuff entirely and just put a <secret key; please enable hashes in rust-secp to see a fingerprint> type message in its place, just as we do for nostd users. I definitely wouldn't have written code that uses siphash like this and it looks like it originates in a time when I was much less strict in reviewed code than I would be with my own.

But will ACK/merge this since it's a strict improvement on the status quo.

All good points, just note that the output of Debug is normally not considered stable.

just put a <secret key; please enable hashes in rust-secp to see a fingerprint> type message in its place

That sounds great to me! Let's do it.

Kixunil

ACK 3d1ce0d

apoelstra

ACK 3d1ce0d successfully ran local tests

tcharding mentioned this pull request Aug 22, 2024

private_key_debug_is_obfuscated test is in the wrong crate rust-bitcoin/rust-bitcoin#3186

Open

tcharding marked this pull request as draft August 22, 2024 02:21

Kixunil reviewed Aug 22, 2024

View reviewed changes

src/secret.rs Outdated Show resolved Hide resolved

src/secret.rs Outdated Show resolved Hide resolved

tcharding force-pushed the 08-22-truncate-secret-hash branch from 0540116 to 371ec9b Compare August 22, 2024 06:27

tcharding force-pushed the 08-22-truncate-secret-hash branch from 371ec9b to 701a70e Compare August 22, 2024 23:12

tcharding marked this pull request as ready for review August 22, 2024 23:12

tcharding added 2 commits August 23, 2024 09:19

Remove bitcoin_hashes range dependency

4b143d6

In order correctly truncate the secret data we need to use recent version of `bitcoin_hashes`. Remove the range dependency and update the lock files.

tcharding force-pushed the 08-22-truncate-secret-hash branch from 701a70e to af997d8 Compare August 22, 2024 23:19

Kixunil mentioned this pull request Aug 23, 2024

Remove useless/broken reexport of hashes #723

Open

Kixunil requested changes Aug 23, 2024

View reviewed changes

tcharding force-pushed the 08-22-truncate-secret-hash branch from af997d8 to 15b3452 Compare August 23, 2024 05:19

Kixunil reviewed Aug 23, 2024

View reviewed changes

tcharding force-pushed the 08-22-truncate-secret-hash branch from 15b3452 to 3d1ce0d Compare August 23, 2024 21:16

Kixunil approved these changes Aug 24, 2024

View reviewed changes

Kixunil mentioned this pull request Aug 24, 2024

Don't use std's siphash to produce secret key fingerprints #725

Closed

apoelstra approved these changes Aug 25, 2024

View reviewed changes

apoelstra merged commit 5d2149f into rust-bitcoin:master Aug 25, 2024
19 of 20 checks passed

tcharding deleted the 08-22-truncate-secret-hash branch August 29, 2024 02:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Truncate secret hash #722

Truncate secret hash #722

tcharding commented Aug 22, 2024 •

edited

Loading

tcharding commented Aug 22, 2024

tcharding commented Aug 22, 2024

Kixunil commented Aug 22, 2024

apoelstra commented Aug 22, 2024

Kixunil commented Aug 22, 2024

apoelstra commented Aug 22, 2024

tcharding commented Aug 22, 2024

tcharding commented Aug 22, 2024

apoelstra commented Aug 22, 2024

apoelstra commented Aug 22, 2024

tcharding commented Aug 22, 2024

tcharding commented Aug 22, 2024

Kixunil commented Aug 23, 2024 •

edited

Loading

Kixunil Aug 23, 2024

Kixunil Aug 23, 2024

Kixunil Aug 23, 2024

tcharding Aug 23, 2024

Kixunil Aug 23, 2024

tcharding Aug 23, 2024

Kixunil Aug 23, 2024

tcharding Aug 23, 2024

apoelstra Aug 24, 2024

apoelstra Aug 24, 2024

Kixunil Aug 24, 2024

Kixunil left a comment

apoelstra left a comment

Truncate secret hash #722

Truncate secret hash #722

Conversation

tcharding commented Aug 22, 2024 • edited Loading

tcharding commented Aug 22, 2024

tcharding commented Aug 22, 2024

Kixunil commented Aug 22, 2024

apoelstra commented Aug 22, 2024

Kixunil commented Aug 22, 2024

apoelstra commented Aug 22, 2024

tcharding commented Aug 22, 2024

tcharding commented Aug 22, 2024

apoelstra commented Aug 22, 2024

apoelstra commented Aug 22, 2024

tcharding commented Aug 22, 2024

tcharding commented Aug 22, 2024

Kixunil commented Aug 23, 2024 • 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

Kixunil left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Aug 22, 2024 •

edited

Loading

Kixunil commented Aug 23, 2024 •

edited

Loading