Reject TXs when there is a mismatch by ryoqun · Pull Request #6236 · solana-labs/solana

ryoqun · 2019-10-04T06:53:13Z

Hi, this is my second small PR.

Problem

Summary of Changes

Made sigverify fail when there is a mismatch between tx.signatures.len() and tx.message().num_required_signatures.
- Specifically, made get_packets_offset return 0 as sig_len to propagate such a rejection into the ultimate result of sigverify. This changes the runtime semantics of sigverify for the case of with sig_len being 0. So be careful when reviewing!
- I believe this is the best way under the various constraints to localize the needed changes, considering both the CPU implementation and the GPU one.
(Additionally to the A malicious slot leader can drop signatures #3568), I fixed a bug in deserializing logic in get_packets_offset. (Should I create a separete pull request?)

garious · 2019-10-04T15:52:06Z

@sakridge @CriesofCarrots, fyi. Not sure if the issue referenced is still an issue after the credit-only rewrite. Can you take a peek at #3568?

ryoqun · 2019-10-05T00:51:06Z

@sakridge @CriesofCarrots, fyi. Not sure if the issue referenced is still an issue after the credit-only rewrite. Can you take a peek at #3568?

Thanks for checking this. I'll also try to understand how this relates to the credit-only rewrite.

CriesofCarrots · 2019-10-07T20:14:04Z

@sakridge @CriesofCarrots, fyi. Not sure if the issue referenced is still an issue after the credit-only rewrite. Can you take a peek at #3568?

Yep, this does appear to still be an issue, as sigverify trusts the signature vec length per this line here:

solana/core/src/sigverify.rs

Line 85 in 17f169f

let (sig_len, sig_size) = decode_len(&packet.data);

ryoqun · 2019-10-09T16:00:06Z

@CriesofCarrots Thanks for checking this out! I'll work on to finish this up tomorrow's work-time in JST. :)

ryoqun · 2019-10-10T19:41:13Z

    let mut pubkey_start = pubkey_start as usize;
    let msg_start = msg_start as usize;

+    if sig_len == 0 {


As mentioned in the pull request description, this part changes the runtime semantics. I'm assuming there will be no transaction with zero signature...

ryoqun · 2019-10-10T19:41:42Z

-            for _ in 0..sig_len {
-                signature_offsets.push(sig_offset);
-                sig_offset += size_of::<Signature>() as u32;
+            if sig_len > 0 {


As mentioned in the pull request description, this part changes the runtime semantics. I'm assuming there will be no transaction with zero signature...

ryoqun · 2019-10-10T19:42:35Z

 pub struct MessageHeader {
    /// The number of signatures required for this message to be considered valid. The
    /// signatures must match the first `num_required_signatures` of `account_keys`.
+    /// NOTE: Serialization-related changes must be paired with the direct read at sigverify.


As bonuses, I added precautionary comments to prevent this bug from reoccurring in the future...

Can you also add a test that fails if the serialization changes in a way to affect that?

Well, I could do that of course, however I think that will be out of scope for this PR, assuming you're meaning to write some robust set of unit tests for each plausible outcome of get_packet_offsets. This PR getting relatively large as one from an outside contributor already.

I've already wrote units tests, directly spotting the changed behavior: 1, 2 and 3.

I understand those yet-to-be-written unit tests will be invaluable, considering sigverify will directly be faced to the outside world (= the public Internet for the permission-less DLT like solana). But the concern already are covered by the issues like #5414 and #6339.

Anyway, if there are some thoughts I'm missing or better unit test scenario, please tell me!

Ok, no problem, we can take on writing that test.

Thanks for consideration! Hopefully, writing it will be me if I find some free time!

ryoqun · 2019-10-10T19:48:14Z

+    // Ultimately, the actual sigverify will determine the uncertainity.
+    let sig_len_maybe_trusted = packet.data[msg_start_offset] as usize;

-    let (_pubkey_len, pubkey_size) = decode_len(&packet.data[msg_start_offset..]);


Sadly, this was a blatant wrongdoing of reading a packet...

In short, this old code incorrectly reads bytes serialized from MessageHeader (three of u8s) as the length (ShortU16) of short_vec. (Moderate pun intended... :).

So, if MessageHeader.required_num_sigs are above 0x7f (according to the comment of Short16), pubkey_size will be 2 and subsequent offset calculation gets bogus, resulting in false negative results of sigverify. This have been working because most of time MessageHeader.required_num_sigs is very small compared to the 0x7f.

To make sure, I traced this back to the origin of introduction of this bug. Let me know if it's needed!

(By the way, this is the first bug I've ever found in the production code of solana, I think I've finally managed to make tangible contribution... yay!)

Also, accompanying unit test is test_large_sigs

This is a nice catch. That was my oversight; thanks!

FYI, this is split into #6388.

ryoqun · 2019-10-10T19:52:14Z

@CriesofCarrots @garious Sorry for not being able to deliver the promised blush-up in time... But I've finally managed to finish it! Could you review this pull request and run the CI?

codecov · 2019-10-10T23:54:17Z

Codecov Report

Merging #6236 into master will increase coverage by 1.2%.
The diff coverage is 74.5%.

@@           Coverage Diff            @@
##           master   #6236     +/-   ##
========================================
+ Coverage    77.4%   78.6%   +1.2%     
========================================
  Files         216     219      +3     
  Lines       42592   41922    -670     
========================================
+ Hits        32969   32985     +16     
+ Misses       9623    8937    -686

CriesofCarrots

This approach looks okay to me. Just a couple small suggestions.
But I'd love for @sakridge to take a look; adding as reviewer.

sakridge · 2019-10-14T16:56:56Z

This looks pretty good, but I would like to see a test that puts the malicious transaction into the verify path to show the issue in the original code and then that it's fixed with the updated offsets logic.

sakridge · 2019-10-14T17:16:16Z

I actually would like to see this split into the fix for the issue #3568 and the deserialization fix.

ryoqun · 2019-10-15T14:07:13Z

Thanks for various comments! Now that #6251 is almost finished, I'll work on this tomorrow!

ryoqun · 2019-10-16T12:10:14Z

@sakridge

I actually would like to see this split into the fix for the issue #3568 and the deserialization fix.

I created one! Please take a look at #6388. Currently, both are based on the same parent git commit. And semantically, this PR are based on #6388, expecting that hotfix-like PR will get merged first. After the merge this PR will look like this: https://github.com/ryoqun/solana/compare/unaligned-pubkey-read..mismatched-sig-len.

This looks pretty good, but I would like to see a test that puts the malicious transaction into the verify path to show the issue in the original code and then that it's fixed with the updated offsets logic.

I think this newly-added test case covers that scenario. If anything is missing, please tell me!

CriesofCarrots · 2019-10-16T19:09:06Z

@ryoqun , can you rebase on master, now that #6388 is merged? Thank you!

ryoqun · 2019-10-16T19:59:50Z

Oops, I created a merge commit to preserve old review comments. Is the rebase way preferred? I'm fine to do that!

mvines · 2019-10-16T20:17:51Z

Oops, I created a merge commit to preserve old review comments. Is the rebase way preferred? I'm fine to do that!

Rebase please. No need to preserve the old review comments

Reject transactions when there is a mismatch between tx.signatures.len() and tx.message().num_required_signatures. Fixes solana-labs#3568

ryoqun · 2019-10-16T20:31:12Z

@mvines done!

Oops, I created a merge commit to preserve old review comments. Is the rebase way preferred? I'm fine to do that!

Rebase please. No need to preserve the old review comments

Ok! Rebased!

ryoqun · 2019-10-16T23:01:31Z

@CriesofCarrots @sakridge Thanks for quick reviews! I'm relieved the introduction of PacketOffsets isn't off too much, so I finished it up fixing all nits!

ryoqun · 2019-10-16T23:11:57Z

(Wow, it seems CI starts without bothering you to attach the CI label anymore... I'm very happy! Thank you very much!)

mvines · 2019-10-16T23:13:06Z

(Wow, it seems CI starts without bothering you to attach the CI label anymore... I'm very happy! Thank you very much!)

Yep, I fixed that for you :)

Pull request has been modified.

ryoqun · 2019-10-18T01:29:54Z

Oops, github created a merge commit.... I'll force push...

ryoqun · 2019-10-18T16:17:10Z

@CriesofCarrots Thanks for reviewing again! I addressed them!

CriesofCarrots · 2019-10-18T16:48:53Z

Thanks, @ryoqun !

mvines added the CI Pull Request is ready to enter CI label Oct 4, 2019

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 4, 2019

ryoqun changed the title ~~[WIP] Reject TXs when there is a mismatch~~ Reject TXs when there is a mismatch Oct 10, 2019

ryoqun marked this pull request as ready for review October 10, 2019 18:55

ryoqun commented Oct 10, 2019

View reviewed changes

Comment thread core/src/sigverify.rs

ryoqun commented Oct 10, 2019

View reviewed changes

CriesofCarrots added the CI Pull Request is ready to enter CI label Oct 10, 2019

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 10, 2019

CriesofCarrots reviewed Oct 11, 2019

View reviewed changes

Comment thread core/src/sigverify.rs Outdated

Comment thread core/src/sigverify.rs Outdated

Comment thread core/src/sigverify.rs Outdated

Comment thread core/src/sigverify.rs Outdated

CriesofCarrots requested a review from sakridge October 11, 2019 21:26

CriesofCarrots mentioned this pull request Oct 11, 2019

No runtime check to ensure transaction fee-payer is a debitable account #6339

Closed

sakridge reviewed Oct 14, 2019

View reviewed changes

Comment thread core/src/sigverify.rs Outdated

ryoqun mentioned this pull request Oct 16, 2019

Fix unaligned read of short_vec pubkey_size in sigverify #6388

Merged

mvines added the CI Pull Request is ready to enter CI label Oct 16, 2019

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 16, 2019

Reject TXs when there is a mismatch

3079bdb

Reject transactions when there is a mismatch between tx.signatures.len() and tx.message().num_required_signatures. Fixes solana-labs#3568

ryoqun force-pushed the mismatched-sig-len branch from 2de7267 to 3079bdb Compare October 16, 2019 20:29

CriesofCarrots reviewed Oct 16, 2019

View reviewed changes

Comment thread core/src/sigverify.rs Outdated

CriesofCarrots reviewed Oct 16, 2019

View reviewed changes

Comment thread core/src/sigverify.rs Outdated

Comment thread core/src/sigverify.rs Outdated

Comment thread core/src/sigverify.rs Outdated

ryoqun added 3 commits October 17, 2019 07:41

Dont substract by itself...

819ab30

Unify too similar condition clauses

81e022a

Just pass packet_offsets...

b7f518d

ryoqun requested review from CriesofCarrots and sakridge October 16, 2019 23:01

CriesofCarrots previously approved these changes Oct 17, 2019

View reviewed changes

sakridge reviewed Oct 17, 2019

View reviewed changes

Comment thread core/src/sigverify.rs Outdated

ryoqun requested a review from sakridge October 18, 2019 01:28

ryoqun force-pushed the mismatched-sig-len branch 2 times, most recently from 1cd32f6 to b7f518d Compare October 18, 2019 01:32

CriesofCarrots reviewed Oct 18, 2019

View reviewed changes

Comment thread core/src/sigverify.rs Outdated

Comment thread core/src/sigverify.rs Outdated

Comment thread core/src/sigverify.rs Outdated

Address review comments

f8379a7

CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Oct 18, 2019

solana-grimes merged commit 193c9a0 into solana-labs:master Oct 18, 2019

Conversation

ryoqun commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

garious commented Oct 4, 2019

Uh oh!

ryoqun commented Oct 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CriesofCarrots commented Oct 7, 2019

Uh oh!

ryoqun commented Oct 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakridge Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Oct 10, 2019

Uh oh!

codecov Bot commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sakridge commented Oct 14, 2019

Uh oh!

sakridge commented Oct 14, 2019

Uh oh!

ryoqun commented Oct 15, 2019

Uh oh!

ryoqun commented Oct 16, 2019

Uh oh!

CriesofCarrots commented Oct 16, 2019

Uh oh!

ryoqun commented Oct 16, 2019

Uh oh!

mvines commented Oct 16, 2019

Uh oh!

ryoqun commented Oct 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryoqun commented Oct 16, 2019

Uh oh!

ryoqun commented Oct 16, 2019

Uh oh!

mvines commented Oct 16, 2019

Uh oh!

ryoqun commented Oct 4, 2019 •

edited

Loading

ryoqun commented Oct 5, 2019 •

edited

Loading

sakridge Oct 14, 2019 •

edited

Loading

codecov Bot commented Oct 10, 2019 •

edited

Loading