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.

Sign up for GitHub

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

Jump to bottom

[Feature]: signatures on WSTS packets need to be verified before feeding them into the state machines #578

Open
2 tasks
xoloki opened this issue Sep 27, 2024 · 8 comments · May be fixed by #723 or #787
Open
2 tasks

[Feature]: signatures on WSTS packets need to be verified before feeding them into the state machines #578

xoloki opened this issue Sep 27, 2024 · 8 comments · May be fixed by #723 or #787
Assignees
@xoloki
Labels
sbtc signer binary The sBTC Bootstrap Signer. signer communication Communication across sBTC bootstrap signers.
Milestone
sBTC: Deposit ready

Comments

@xoloki
Copy link
Contributor

xoloki commented Sep 27, 2024

Feature - Verify WSTS packet signatures

1. Description

Applications which use wsts state machines must verify the signatures on the packets before processing them.

1.1 Context & Purpose

All wsts network packets are signed, to guarantee that they come from the purported source, and have not been tampered with. So they must be verified before processing.

But wsts applications typically run both coordinator and signer state machines, so it’s better to verify them outside the state machines themselves. Also, coordinator selection is external to the state machines.

2. Technical Details:

Call Packet::verify with the current signer and coordinator public keys after receiving packets, before feeding them into the machines. Bad packets should be dropped.

2.1 Acceptance Criteria:

  • all packets are verified
  • tests added

3. Related Issues and Pull Requests (optional):
@djordon djordon added the duplicate This issue or pull request already exists label Sep 27, 2024
@djordon
Copy link
Contributor

djordon commented Sep 27, 2024

I think that this is a duplicate of #296.

@djordon djordon added sbtc signer binary The sBTC Bootstrap Signer. signer communication Communication across sBTC bootstrap signers. labels Sep 27, 2024
@djordon djordon added this to the sBTC Release Ready milestone Sep 27, 2024
@djordon djordon mentioned this issue Sep 27, 2024
Open
@xoloki
Copy link
Contributor Author

xoloki commented Sep 27, 2024

I think that this is a duplicate of #296.

I don’t think so. 296 is about signing bitcoin transactions AFAICT.

@djordon
Copy link
Contributor

djordon commented Sep 27, 2024

Oh yeah you are right. It is not a duplicate.

@djordon
Copy link
Contributor

djordon commented Sep 27, 2024 •
edited
Loading

Oh man, we do here

sbtc/signer/src/transaction_signer.rs

Lines 219 to 223 in 53d43bb

async fn handle_signer_message(&mut self, msg: &network::Msg) -> Result<(), Error> {
if !msg.verify() {
tracing::warn!("unable to verify message");
return Err(Error::InvalidSignature);
}
but do not here

sbtc/signer/src/transaction_coordinator.rs

Lines 332 to 377 in c8fa6c5

async fn relay_messages_to_wsts_state_machine_until_signature_created(
&mut self,
bitcoin_chain_tip: &model::BitcoinBlockHash,
coordinator_state_machine: &mut wsts_state_machine::CoordinatorStateMachine,
txid: bitcoin::Txid,
) -> Result<wsts::taproot::SchnorrProof, Error> {
loop {
let msg = self.network.receive().await?;
if &msg.bitcoin_chain_tip != bitcoin_chain_tip {
tracing::warn!(?msg, "concurrent wsts signing round message observed");
continue;
}
let message::Payload::WstsMessage(wsts_msg) = msg.inner.payload else {
continue;
};
let packet = wsts::net::Packet {
msg: wsts_msg.inner,
sig: Vec::new(),
};
let (outbound_packet, operation_result) =
match coordinator_state_machine.process_message(&packet) {
Ok(val) => val,
Err(err) => {
tracing::warn!(?packet, reason = %err, "ignoring packet");
continue;
}
};
if let Some(packet) = outbound_packet {
let msg = message::WstsMessage { txid, inner: packet.msg };
self.send_message(msg, bitcoin_chain_tip).await?;
}
match operation_result {
Some(wsts::state_machine::OperationResult::SignTaproot(signature)) => {
return Ok(signature)
}
None => continue,
Some(_) => return Err(Error::UnexpectedOperationResult),
}
}
}

Okay, that fell through the cracks. I think the best place to do this is here:

sbtc/signer/src/network/mod.rs

Lines 95 to 113 in 53d43bb

async fn receive(&mut self) -> Result<Msg, Error> {
loop {
tokio::select! {
_ = self.term.wait_for_shutdown() => {
return Err(Error::SignerShutdown);
},
recv = self.signal_rx.recv() => {
match recv {
Ok(SignerSignal::Event(SignerEvent::P2P(P2PEvent::MessageReceived(msg)))) => {
return Ok(msg);
},
// We're only interested in the above messages, so we ignore
// the rest.
_ => continue,
}
}
}
}
}

That way we won't need to track everything down all the time. Hmmm, this might be straightforward.

Edit: Oh maybe not. We also need to validate the public key against the list of accepted public keys. I wonder if we can skip this entirely, since the networking layer also does this.

@djordon djordon self-assigned this Sep 27, 2024
@djordon djordon removed the duplicate This issue or pull request already exists label Sep 27, 2024
@djordon djordon removed their assignment Oct 1, 2024
@cylewitruk
Copy link
Member

cylewitruk commented Oct 7, 2024 •
edited
Loading

I wonder if we can skip this entirely, since the networking layer also does this.

It does not at present -- that's where I raised the Q about which messages should be guarded and not (since a pending/joining node may still need to be able to communicate with the network in some capacity). Right now we assume every message is a SignedMessage, so we could do this in the networking layer if we wanted to, at least until we (maybe) introduce some other network-management messages where we'd need to adjust the impl.

We could also just implement it for wsts messages in the networking layer (for now).

@xoloki
Copy link
Contributor Author

xoloki commented Oct 28, 2024 •
edited
Loading

We could also just implement it for wsts messages in the networking layer (for now).

I've been looking over this, and while we can verify messages at the network layer, it won't solve the problem this issue was created to solve.

ecdsa::Signed<message::SignerMessage> wraps the message, signature, and pubkey, so we can call Signed<>::verify. This will at least show that the pub key was used to sign the message, and it has not been altered in transport since being signed.

But wsts needs more than this. wsts::net::Packet contains a wsts::net::Message, and the subtypes of the latter usually contain a signer_id. The signer_id allows us to actually check authentication, not just integrity, and later authorization. So we need to be able to see signer_id, and map it to a PublicKey.

The wsts state machines do have this knowledge; the SignerStateMachine has a PublicKeys field, a the CoordinatorStateMachines have a Config field with similar information.

So it probably makes more sense for the purposes of this issue to do the checks inside TxSignerEventLoop::relay_message

@xoloki
Copy link
Contributor Author

xoloki commented Oct 28, 2024

Actually, it looks like TxSignerEventLoop ::handle_wsts_message already cracks open the messages themselves, so we would have access to signer_id with no refactoring necessary.

@xoloki
Copy link
Contributor Author

xoloki commented Oct 28, 2024

Here's a draft PR which does this for public and private shares: #723

@xoloki xoloki linked a pull request Oct 28, 2024 that will close this issue
check that wsts messages are signed by the specified signer_id #723
Open
This was referenced Nov 4, 2024
Open
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@xoloki xoloki

Labels
sbtc signer binary The sBTC Bootstrap Signer. signer communication Communication across sBTC bootstrap signers.
Projects
sBTC
Status: In Review
Milestone
sBTC: Deposit ready
4 participants
@xoloki @aldur @djordon @cylewitruk