Incorrect certificate validation

[Sjors]

Currently the pool role (and presumably other roles) incorrectly verifies the Template Provider authority key (and presumable other upstream keys).

https://github.com/stratum-mining/sv2-spec/blob/main/04-Protocol-Security.md#453-server-authentication

The checks happen here:

stratum/protocols/v2/noise-sv2/src/signature_message.rs

Lines 26 to 40 in 3f07a6a

	impl SignatureNoiseMessage {
	pub fn verify(self, pk: &XOnlyPublicKey) -> bool {
	let now = SystemTime::now()
	.duration_since(SystemTime::UNIX_EPOCH)
	.unwrap()
	.as_secs() as u32;
	if self.valid_from <= now && self.not_valid_after >= now {
	let secp = Secp256k1::verification_only();
	let (m, s) = self.split();
	let m = Message::from_hashed_data::<sha256::Hash>(&m[0..10]);
	let s = match Signature::from_slice(&s) {
	Ok(s) => s,
	_ => return false,
	};
	secp.verify_schnorr(&s, &m, pk).is_ok()

1. The spec requires that server_public_key (the upstream static key) is included in the signature hash, but m only covers the 10 bytes of the SIGNATURE_NOISE_MESSAGE. It needs to append the server static public.

2. The message should be signed by the authority key, but it's actually being checked against the static key.

The Template Provider in Bitcoin Core generated signatures with the same two issues. I fixed the bug, but then put in a workaround to re-introduce the bug. That way things keep working.

A fix for the Bitcoin Core side is here: Sjors/bitcoin#29

I'll merge that once the SRI-side is fixed.

[pavlenex]

@plebhash is picking this one 🔥

[pavlenex]

It seems I can only assign you an issue if you comment back on it 😄 We have to add you to organizaiton or repo.

[plebhash]

sounds good! just send me an invite and I'll gladly accept it

[plebhash]

@Sjors while I'm still not at a testing stage, it seems I'll eventually need to test SRI against your Bitcoin Core fork on a specific commit.

I'm looking at Sjors/bitcoin#29 and your last comment says:

I included this into the main PR, with a revert commit.

Could you confirm this is the main PR? bitcoin/bitcoin#29346

I'm not finding the revert commit, thus the confusion.

[plebhash]

Another question to @Sjors:

I built 2024/01/sv2_noise but I'm running into this while trying to start it:

./src/bitcoind -sv2 -sv2port=8442 -debug=sv2 -datadir=$HOME/.bitcoin/
Error: Error parsing command line arguments: Invalid parameter -sv2

[plebhash]

question to @Fi3 :

as stated by @Sjors:

The message should be signed by the authority key, but it's actually being checked against the static key.

So that means we need to fix this line, correct?

stratum/protocols/v2/noise-sv2/src/signature_message.rs

Line 40 in 3f07a6a

secp.verify_schnorr(&s, &m, pk).is_ok()

but where would authority_key come from?

I can see two potential paths, but please correct me if I'm missing something:

• add a new input parameter to fn verify
• add a new field to struct SignatureNoiseMessage

[Sjors]

@plebhash sorry for the confusion here:

1. My main pull request is Stratum v2 Template Provider (take 2) bitcoin/bitcoin#28983, which uses branch 2023/11/sv2-poll. This does not contain the fix (nor a revert commit). You can test it by cherry-picking the commit from Fix certificate generation and validation Sjors/bitcoin#29 (you can't use that branch as is, because we changed the n++ / ++n thing)

2. My most up to date branch is 2024/02/sv2-poll-ellswift, which uses EllSwift. It's not part of the main pull request (yet), because the SRI main branch doesn't support EllSwift yet, pending Elligator swift encoding #737 (and it's too tedious to write a revert commit)

You should write the fix on top of #737 and test it with (2). Otherwise you'll probably have to fix a merge conflict after #737 is merged.

The branch 2024/01/sv2_noise from bitcoin/bitcoin#29346 only implmements the noise protocol, nothing else from Statum v2. So starting with -sv2 should indeed not work.

You indeed need to pass both the authority key and the static key into the verification function. I'm not much of a Rust guru, so no opinion on how to do that.

[plebhash]

thanks for the clarification @Sjors

so for TP I built 2024/02/sv2-poll-ellswift while applying yet another revert to Sjors/bitcoin@027c635 in order to ensure TP performs the correct Handshake.

On SRI side I pulled @lorbax fork and checked out ElligatorSwift branch.
If I launch the pool without changing any code, it breaks with:

thread 'main' panicked at pool/src/lib/template_receiver/mod.rs:56:18:
called `Result::unwrap()` on an `Err` value: CodecError(NoiseSv2Error(InvalidCertificate([0, 0, 25, 46, 197, 101, 255, 255, 255, 255, 13, 138, 18, 236, 6, 46, 121, 189, 246, 52, 197, 25, 40, 219, 156, 20, 158, 187, 191, 103, 184, 79, 95, 149, 2, 192, 57, 36, 119, 155, 148, 59, 121, 194, 4, 199, 10, 138, 13, 8, 88, 46, 87, 191, 131, 67, 159, 221, 233, 220, 28, 173, 54, 87, 181, 103, 19, 78, 91, 50, 25, 44, 145, 210])))

On the verify function body, I adapted m so that if follows the formula from the spec: m = SHA-256(version || valid_from || not_valid_after || server_public_key):

-            let m = Message::from_hashed_data::<sha256::Hash>(&m[0..10]);
+            let m = [&m[0..10], &pk.serialize()].concat();
+            let m = Message::from_hashed_data::<sha256::Hash>(&m);

I also changed verify so that it takes in the authority_pk:

-    pub fn verify(self, pk: &XOnlyPublicKey) -> bool {
+    pub fn verify(self, pk: &XOnlyPublicKey, authority_pk: &XOnlyPublicKey) -> bool {

But if I try to call verify_schnorr against authority_pk

-            secp.verify_schnorr(&s, &m, pk).is_ok()
+            secp.verify_schnorr(&s, &m, authority_pk).is_ok()

the pool still breaks with the same error:

thread 'main' panicked at pool/src/lib/template_receiver/mod.rs:56:18:
called `Result::unwrap()` on an `Err` value: CodecError(NoiseSv2Error(InvalidCertificate([0, 0, 25, 46, 197, 101, 255, 255, 255, 255, 13, 138, 18, 236, 6, 46, 121, 189, 246, 52, 197, 25, 40, 219, 156, 20, 158, 187, 191, 103, 184, 79, 95, 149, 2, 192, 57, 36, 119, 155, 148, 59, 121, 194, 4, 199, 10, 138, 13, 8, 88, 46, 87, 191, 131, 67, 159, 221, 233, 220, 28, 173, 54, 87, 181, 103, 19, 78, 91, 50, 25, 44, 145, 210])))

while if I simply call verify_schnorr against pk (static key), it works fine.

IIUC, we expect verify_schnorr to work against authority_pk, not pk. So that brings the question: are more changes necessary to SRI, to TP, or both?

---

Changes to SRI can be found at plebhash@bdc3880

[Sjors]

My revert commit was missing a spot (in sv2_template_provider.cpp), so it wasn't a complete revert. Fixed in Sjors/bitcoin@6b5ceb7

[plebhash]

thanks!

just to confirm: in order to have a TP with correct certificate behavior, I should check out 2024/02/sv2-poll-ellswift (while making sure it includes 53085dca4cbbad76c6497b6942259296cc342d66), then re-revert the following:

• 027c63547e7ad49915e407eb5335b3412c93ba72
• 53085dca4cbbad76c6497b6942259296cc342d66

is that correct?

---

after doing the above, signature verification breaks both against pk as well as authority_pk

but as pointed out by @Sjors here plebhash@bdc3880#r138457380 maybe authority_pk is not loaded with the correct authority key?

[Sjors]

The specific hash you need to revert may change as I push updates to the 2024/02/sv2-poll-ellswift branch. The commits you'll want to revert are called "[Temporarily break certificate generation and validation]" and "[Temporarily use previous noise protocol name]".

Can you open a (draft) PR with your change? That makes it easier to comment on it.

[Sjors]

I left a more elaborate comment on your commit: plebhash@bdc3880#r138462734

My guess / hope is that you didn't set the right authority key in your config file...

[Sjors]

I found a bug on my end where it kept replacing the static and authority key files. Fixed in the latest push.

I also get an invalid certificate error. I'll do some more digging.

[Sjors]

Also don't forget to make the same changes to sign(), otherwise e.g. the translator can't connect to the pool.

Another issue I found is that the Pool role, which I currently use for testing this, doesn't have a tp_authority_pub_key config option. Instead it was reading IT'S OWN authority_pubkey.

So you need to add in roles/pool/src/lib/mining_pool/mod.rs:

pub struct Configuration {
    pub listen_address: String,
    pub tp_address: String,
    pub tp_authority_pub_key: Secp256k1PublicKey,

And then use config.tp_authority_pub_key.

That still doesn't fix it for me though...

[Sjors]

Oops, I was logging the static key instead of the authority key.

In the process of debugging I added some log statements to make sure:

1. The message we're hashing matches what the template provider signs for. I added a log statement Certificate hashed data:. This includes the static key, which was already logged.

2. We're checking against the correct authority key. I added a log statement to also print the authority key x-coordinate in hex.

[plebhash]

The specific hash you need to revert may change as I push updates to the 2024/02/sv2-poll-ellswift branch. The commits you'll want to revert are called "[Temporarily break certificate generation and validation]" and "[Temporarily use previous noise protocol name]".

@Sjors does this still hold true? I cannot find any commit with "[Temporarily use previous noise protocol name]" anymore.

[plebhash]

Another issue I found is that the Pool role, which I currently use for testing this, doesn't have a tp_authority_pub_key config option. Instead it was reading IT'S OWN authority_pubkey.

Ok that clarifies the authority_pubkey issue. But we will need to add this key to the config file so that it matches TP.
Where can I find this key? Or could you please paste it here?

[Sjors]

Temporarily use previous noise protocol name

That one is no longer needed, as long as you rebase on #737 and use the latest 2024/02/sv2-poll-ellswift branch on the Bitcoin Core side. Sorry for the chaos.

[Sjors]

Where can I find this key? Or could you please paste it here?

Bitcoin Core puts it in the log when you start with -sv2.

[plebhash]

Can you open a (draft) PR with your change? That makes it easier to comment on it.

lorbax#3 here's a draft PR against @lorbax 's fork

it already contains the suggested changes to variable names

[Sjors]

The reason I suggest adding a new tp_authority_pub_key field to the pool config is two fold:

1. It's consistent with the naming used by other roles that can connect to a Template Provider

2. The field authority_public_key refers to the pool itself. This makes sense when the pool runs its own template provider, in which case they would be the same key. But I think the code will be less confusing if we treat them as independent keys. The user can always set the same value for authority_public_key and tp_authority_pub_key