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

Go through and check that all cryptography assumptions in the design are implemented in the code. #8587

Closed
16 of 17 tasks
aeyakovenko opened this issue Mar 3, 2020 · 8 comments
Labels
security Pull requests that address a security vulnerability
Milestone

Comments

@aeyakovenko
Copy link
Member

aeyakovenko commented Mar 3, 2020

Problem

A large distributed team working asynchronously on a complex codebase may miss some things that everyone would think are obvious.

Proposed Solution

Go through important cryptography assumptions in the design and verify they are implemented in the code. File issues if integration or unit tests are missing.

TX signature checks

  • Don’t vote on blocks with failed signatures
  • sigverify can run asynchronously with replay stage, but needs to mark the block as verified before replay votes
  • Entry verify fails if signature verification fails (ledger/src/entry.rs)
Validator Signature checks
  • before voting on a block
  • entry verify fails: ledger/src/entry.rs
  • before forwarded to the leader
  • core/src/banking_stage.rs filter_valid_packets_for_forwarding only forwards valid transactions
Leader TX Signature Checks
  • before forwarded
  • core/src/banking_stage.rs filter_valid_packets_for_forwarding only forwards valid transactions
  • when received from a validator
  • core/src/tpu.rs, forwarded tx sockets send their transactions to the TransactionSigVerifier
  • after read from gossip
  • core/src/cluster_info_vote_listener.rs calls sigverify::ed25519_verify_cpu(&msgs)
  • after read from transaction packet queue
  • core/src/tpu.rs transaction sockets send their data to TransactionSigVerifier

PoH Verification

  • PoH is verified before voting on a block
  • entry verify fails: ledger/src/entry.rs

Gossip CRDS Values

  • before adding value into the crds
  • before pushing messages forward
  • CrdsValue::verify calls signature verify. ClusterInfo handle_packets calls verify on all received values. This prevents them from entering the push queue.

Turbine

  • shreds are signed by the leader before broadcast
  • shred.rs entries_to_data_shreds shreds are signed. Same in entries_to_coding_shreds
  • shreds are verified by the validator before retransmit
  • shreds that fail the leaders shred signature check are marked for discard. Retransmit is skipped. Currently handled by recv_window in window_service
  • repair requests are signed and verified
  • signed in ledger/src/shred.rs in entries_to_data_shreds and entries_to_coding_shreds
  • repaired shreds are verified
  • TVU receives the repair responses and they are pushed to the ShredSigVerifier before the retransmit stage.

BankHash

  • all modified accounts are merkle hashed into the bank hash
  • accounts.rs bank_hash_info_at is the hash of all the accounts committed for the slot. It’s scans the account storage for all the AppendVecs for the slot and computes the commit merkle. Bank’s hash_internal_state function picks up that value when the bank is frozen, and its stored in bank.hash. All votes contain bank.hash

Snapshots

tag: @mvines @garious

@aeyakovenko aeyakovenko changed the title Cryptography checklist Go through and check that all cryptography assumptions in the design are implemented in the code. Mar 3, 2020
@mvines mvines added this to the v0.23.9 milestone Mar 3, 2020
@ryoqun
Copy link
Member

ryoqun commented Mar 3, 2020

  • ... bank specific state is signed for the snapshot

@aeyakovenko This is not fully currently implemented: #8185 I havn't gotten hands on it due to imminent snapshot issues.

@mvines mvines modified the milestones: v0.23.9, v1.0.2, v1.0.3, v1.0.4 Mar 3, 2020
@ryoqun ryoqun added the security Pull requests that address a security vulnerability label Mar 6, 2020
@mvines mvines modified the milestones: v1.0.4, v1.0.5, v1.0.6 Mar 6, 2020
@aeyakovenko
Copy link
Member Author

@garious Seems like we should have a cryptography security doc in the design that goes over all the signatures and how they are used.

@ryoqun
Copy link
Member

ryoqun commented Mar 10, 2020

Sorry a bit late; but I've just came up with; I know these are pretty obvious and may overlap with existing check points, but just in case

These are what I'd would peek into if I'd determined to steal million SOLs ever. ;)

Genesis hash

  • Is genesis hash is really chained to the actual corresponding ledger?

Poh

  • Is Poh hash is really changed according to inserted TX signatures?
  • When inserting, there should be no bogus TXes?
  • When validating in parallel with GPU, swapping entries causes verification error? I mean, the entire PoH linkage/integrity is ensured?

TX

  • Sighed TXs becomes invalid with tampered recent_blockhash?

Hashing/Signature coverage

The hashing and signing covers the following data structures exactly with no less and no more? When extra data is prepended/appended by tampering from an untrusted stream, it rejects the extra outright?

  • TX
  • GenesisConfig
  • Shred
  • Account data (only hashing applies)

@ryoqun
Copy link
Member

ryoqun commented Mar 10, 2020

  • Also, is our depending crypto crates are working correctly at the currently-depended version? Is its results checked against other impls like OpenSSL, NaCL? And Cargo.lock can detect and loudly reject to build further if encountered bad crate hash?

@ryoqun
Copy link
Member

ryoqun commented Mar 11, 2020

  • Also, our release tarball is signed by the offline keypair somehow or is it a reproducible build?

@mvines mvines modified the milestones: v1.0.7, v1.0.8, v1.0.9 Mar 16, 2020
@mvines mvines modified the milestones: v1.0.9, v1.0.10, v1.0.11, v1.0.12 Mar 24, 2020
@mvines mvines modified the milestones: v1.0.13, v1.1.1 Mar 31, 2020
@mvines mvines modified the milestones: v1.1.1, v1.1.2, v1.1.3 Apr 3, 2020
@mvines mvines modified the milestones: v1.1.3, v1.1.4, v1.1.5 Apr 13, 2020
@mvines mvines modified the milestones: v1.1.5, v1.2.0 Apr 19, 2020
@ryoqun
Copy link
Member

ryoqun commented Apr 21, 2020

  • ... And Cargo.lock can detect and loudly reject to build further if encountered bad crate hash?
  • Also, our release tarball is signed by the offline keypair somehow or is it a reproducible build?

I'm slowly moving this by way of #9180.

@mvines mvines modified the milestones: v1.2.0, v1.3.0 Apr 21, 2020
@mvines mvines modified the milestones: v1.3.0, v1.4.0 Aug 5, 2020
@mvines mvines modified the milestones: v1.4.0, v1.5.0 Oct 8, 2020
@mvines mvines modified the milestones: v1.5.0, v1.6.0 Dec 17, 2020
@mvines mvines modified the milestones: v1.6.0, v1.7.0 Mar 11, 2021
@mvines mvines modified the milestones: v1.7.0, v1.8.0 May 10, 2021
@ryoqun
Copy link
Member

ryoqun commented May 13, 2022

@ryoqun
Copy link
Member

ryoqun commented May 13, 2022

firstly, i audited those versioned tx mechanism. it seems it's correctly sanitizing/sig-verifying/pre-compiling things for both banking /replaying codepath.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Pull requests that address a security vulnerability
Projects
None yet
Development

No branches or pull requests

3 participants