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

A malicious slot leader can drop signatures #3568

Closed
garious opened this issue Mar 29, 2019 · 9 comments
Closed

A malicious slot leader can drop signatures #3568

garious opened this issue Mar 29, 2019 · 9 comments

Comments

@garious
Copy link
Contributor

garious commented Mar 29, 2019

Problem

A malicious slot leader could drop all but the first transaction signatures, causing the first signer to pay the transaction fee, but then see the program reject it because of missing signatures.

Proposed Solution

sigverify should reject transactions when there is a mismatch between tx.signatures.len() and tx.message().num_required_signatures.

tag: @sakridge

@garious garious added this to the The Future! milestone Mar 29, 2019
@aeyakovenko
Copy link
Member

aeyakovenko commented Mar 29, 2019

@garious how is the first signature valid? Each signature signs the entire transaction, which includes all the public keys, and if each key requires a signature.

@garious
Copy link
Contributor Author

garious commented Mar 29, 2019

The signer can't sign the entire transaction, because that'd include the signature, which doesn't exist yet. It also doesn't sign the length of the signatures vector. Instead, it signs the whole Message struct, which includes a num_required_signatures. Ideally, that serialized data would be first in the serialized transaction, but instead it's placed after the signatures vector. Therefore, the signatures length is encoded in two places (each consuming 1 byte) and they have the same value. Only the first one is used by sigverify and that data is unsigned and can be modified by an attacker.

@aeyakovenko
Copy link
Member

@garious The signer signs the list of pubkeys, and which are expected to provide a signature, as well as the message. The signatures are appended after. The transaction should be rejected as invalid before payment if any of the signatures fail the check.

@garious
Copy link
Contributor Author

garious commented Mar 29, 2019

The signer signs a list of pubkeys which includes some which need a corresponding signature and some that don't. If the attacker reduces the number in the first byte of the transaction, then sigverify will successfully verify fewer signatures than the number in Message::num_required_signatures.

@aeyakovenko
Copy link
Member

@garious, the signature covers all the pubkeys, including the signers as well as the num required.

@garious
Copy link
Contributor Author

garious commented Mar 30, 2019

But sigverify uses the first byte (unsigned) to determine how many signatures to verify, not Message::num_required_signatures, which is signed.

@aeyakovenko
Copy link
Member

Reducing the required number changes the message hash and breaks all the signatures. So a leader can’t do that.

A malicious drone can’t change the number either, because it breaks some of the signatures and the entire TX is marked as failed.

@aeyakovenko
Copy link
Member

@garious, that’s funny. I guess we just need to check it’s the same value in tx.verify

@mvines mvines modified the milestones: The Future!, Mainnet v1.0.0 May 4, 2019
ryoqun added a commit to ryoqun/solana that referenced this issue Oct 4, 2019
Reject transactions when there is a mismatch between tx.signatures.len() and
tx.message().num_required_signatures.

Fixes solana-labs#3568
ryoqun added a commit to ryoqun/solana that referenced this issue Oct 16, 2019
Reject transactions when there is a mismatch between tx.signatures.len() and
tx.message().num_required_signatures.

Fixes solana-labs#3568
@mvines
Copy link
Member

mvines commented Oct 21, 2019

Fixed

@mvines mvines closed this as completed Oct 21, 2019
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

No branches or pull requests

3 participants