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

Fix signature index check in secp256k1 verify #13014

Closed
wants to merge 1 commit into from
Closed

Fix signature index check in secp256k1 verify #13014

wants to merge 1 commit into from

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Oct 20, 2020

Problem

Index check doesn't prevent invalid indices properly. If signature_index is 1 and there is only one instruction data, the check doesn't fail.

Summary of Changes

  • Fix off-by-one

Fixes #

@jstarry jstarry requested a review from sakridge October 20, 2020 09:39
@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #13014 into master will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #13014     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         361      361             
  Lines       85272    85272             
=========================================
- Hits        69886    69879      -7     
- Misses      15386    15393      +7

sakridge
sakridge approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@sakridge sakridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ryoqun
Copy link
Member

ryoqun commented Oct 20, 2020

@sakridge @jstarry Uuum. is this need to be runtime-featured?

$ ./target/release/solana --url http://api.mainnet-beta.solana.com feature status

Stake By Feature Set:
2415322184 - 90% 
2674527397 - 9% 
Feature                                      Description                              Status
MoqiU1vryuCGQSxFKA1SZ316JdLEFFhoAu6cKUNk7dN  pubkey log syscall                       inactive
SECCKV5UVUsr8sTVSVAzULjdm87r7mLPaqH2FGZjevR  inflation kill switch                    inactive
YCKSgA6XmjtkQrHBQjpyNrX6EMhJPcYcLWMVgWn36iv  max program call depth 64                active since slot 41904000
3h1BQWPDS5veRsq6mDBWruEpgPxRJkfwGexg5iiQ9mYg consistent recentblockhashes sysvar      active since slot 41040000
3zydSLUwuqqsV3wL5wBsaVgyvMox3XTHx7zLEuQf1U2Z correct bank timestamps                  inactive
4kpdyrcj5jS47CZb2oJGfVxjYbsMm2Kx97gFyZrxxwXz no overflow rent distribution            inactive
5RzEHTnf6D7JPZCvwEzjM19kzBsyjSU3HoMfXaQmVgnZ ping-pong packet check #12794            inactive
D7KfP7bZxpkYtD4Pc38t9htgs1k5k47Yhxe4rp6WDVi8 sha256 syscall                           inactive
DFBnrgThdzH4W6wZ12uGPoWcMnvfZj11EHnxHcVxLPhD bpf_loader2 program                      active since slot 41040000
E3PHP7w8kB7np3CTQ1qQ2tW3KCtjRSXBQgW9vM2mWv2Y secp256k1 program                        active since slot 41040000
E5JiFDQCwyC6QfT9REFyMpfK2mHcmv1GUDySU1Ue7TYv spl-token multisig fix                   active since slot 41040000
EdM9xggY5y7AhNMskRG8NgGMnaP4JFNsWi8ZZtyT1af5 max invoke call depth 4                  active since slot 41904000
EnvhHCLvg55P7PDtbvR1NwuTuAeodqpusV3MR5QEK8gs instructions sysvar                      active since slot 41040000
FtjnuAtJTWwX3Kx9m24LduNEhzaGuuPfDW6e14SX2Fy5 rent fixes (#10206, #10468, #11342)      inactive
GaBtBJvmS4Arjj5W1NmFcyvPjsHN38UGYDq2MDwbs9Qu pico-inflation                           inactive
HRe7A6aoxgjKzdjbBv6HTy7tJ4YWqE6tVmYCGho6S9Aq ristretto multiply syscall               inactive
HxvjqDSiF5sYdSYuCXsUnS8UeAoWsMT9iGoFP8pgV1mB compute budget balancing                 active since slot 41904000

Feature activation is not allowed at this time

E3PHP7w8kB7np3CTQ1qQ2tW3KCtjRSXBQgW9vM2mWv2Y secp256k1 program active since slot 41040000

because this is already enabled on mainnet-beta and these handling differencies before and after this pr create different transaction results?

@ryoqun ryoqun mentioned this pull request Oct 20, 2020
Merged
@mvines
Copy link
Member

mvines commented Oct 20, 2020

Yep this affects banking_stage. Needs a feature gate :(

ryoqun
ryoqun reviewed Oct 20, 2020
View reviewed changes
sdk/src/secp256k1.rs
@@ -72,7 +72,7 @@ pub fn verify_eth_addresses(

// Parse out signature
let signature_index = offsets.signature_instruction_index as usize;
if signature_index > instruction_datas.len() {
if signature_index >= instruction_datas.len() {
Copy link
Member

@ryoqun ryoqun Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think adding a test for this won't hurt us, considering this is part of the signature verification code. Maybe this should be easy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can add in the morning unless we want this merged sooner

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually a more complete fix and adds tests:
#13026

@ryoqun
Copy link
Member

ryoqun commented Oct 20, 2020

Also, considering we found this on the wild, maybe should we feed more bunch of eth sigs picked from the real eth network into this code to expose more bugs?

@ryoqun
Copy link
Member

ryoqun commented Oct 20, 2020

Also, considering we found this on the wild, maybe should we feed more bunch of eth sigs picked from the real eth network into this code to expose more bugs?

So that we can pack bunch of any bugs under a single feature gate. :)

@jstarry
Copy link
Member Author

jstarry commented Oct 20, 2020

Wouldn't this cause a panic? I don't think it needs a feature gate. I caught it while reading code, don't think it's happened in the wild

sakridge
sakridge requested changes Oct 20, 2020
View reviewed changes
Copy link
Member

@sakridge sakridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs another fix as in: #13026

@mvines mvines closed this Oct 20, 2020
@jstarry jstarry deleted the fix-verify-eth branch November 29, 2021 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryoqun ryoqun ryoqun left review comments

@sakridge sakridge sakridge requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jstarry @ryoqun @mvines @sakridge