Epic: Improvements from Zebra Audit

[mpguerra]

Motivation

We want to ensure that we track and act upon findings from the audit.

Scope

zebra

• [NCC-E005955-XVE] zebra-chain: Inconsistent error management in Add and Sub for Height #6279
  • fixed by change(chain): Refactor the handling of height differences #6330
• [NCC-E005955-DBV] zebra-chain: Unbounded Rejection Sampling with Possibility of Panics #6338
  • fixed by fix(errors): Handle randomness generation and invalid random values as errors in cryptographic code #6385
• [NCC-E005955-M2F] zebra-chain: Check that header block version field is valid when serializing blocks #497
  • fixed by fix(chain): Validate header versions when serializing blocks #6475
• [NCC-E005955-HV6] zebra-network: Buffer length validation after memory allocation #6280
  • fixed by fix(network): validate addr length before reading #6320
• [NCC-E005955-6AN] zebra-network: Power-of-Two-Choices Load Balancing May Deprioritize Honest Peers #6343
  • closed as wont-fix see issue for analysis and decision
• [NCC-E005955-MMC] zebra-network: Uncaught Nonce Reuse and Fragile Nonce Cache Eviction #6339
  • fixed by fix(net): Avoid some self-connection nonce removal attacks #6410
• [NCC-E005955-VM7] zebra-network: Off-by-One Error in zebra-network Retry Parameter #6393
  • fixed by fix(net): Fix off-by-one error in DNS seed peer retries, and clarify logs #6460
• [NCC-E005955-WVM] zebra-consensus: Off-by-One Errors and Inconsistent Usage of PARAMETER_DOWNLOAD_MAX_RETRIES #6340
  • fixed by fix: Inconsistent Docs for Downloading ZKP Params #6464
• [NCC-E005955-GCR] Cargo Audit and RustSec Advisories #6391
  • fixed by change(deps): Update dependencies that only appear in the lock file #6217
• [NCC-E005955-GHX] Incorrectly Disabled Consistency Check #6617
  • This check was verified as redundant and the comment was deleted in PR fix(state): Remove an outdated anchor assertion in a comment #6450.
• [NCC-E005955-7DU] zebra-network: Fragile State Transition During Address Book Update #6672
  • fixed by fix(network): Ignore out of order Address Book changes, unless they are concurrent #6717

ed25519-zebra

• [NCC-E005955-AQM] Private Keys May Be Written to Log Files ed25519-zebra#71
  • fixed by Do not log SigningKey seed, prefix, s, as part of impl Debug ed25519-zebra#70
• [NCC-E005955-3WU] Incomplete zeroization of SigningKey ed25519-zebra#72
  • fixed by Zeroize full signingkey ed25519-zebra#73

librustzcash

• zcash_proofs: Theoretical possibility of overflow leading to panic zcash/librustzcash#786 / [NCC-E005955-NQ6] zcash_proofs: Theoretical possibility of overflow leading to panic (Copy of zcash/librustzcash#786) #6327
  • fixed by Fix a theoretical possibility of overflow leading to panic zcash/librustzcash#805
• Redundant Computation in Sapling and Orchard Note Validation zcash/librustzcash#802 / [NCC-E005955-MU2] zebra-consensus: Redundant Computation in Sapling and Orchard Note Validation #6392
  • Fixed by fix: Avoid a redundant check containing curve point multiplication zcash/librustzcash#840
    • We originally decided not to make any changes here for these reasons. However, we subsequently found an easy fix which are dependent on the ECC team merging. EDIT: It seems like the ECC team are going to implement this in another way.

Optional Tasks

Other miscellaneous remarks that are not considered security vulnerabilities. Fixing these could increase code quality.

• Tracking: TODOs with closed tasks #6281
  • Write script to find TODOs mentioning closed github issues #6278
    • fixed by feat(script): Add binary for finding references to closed issues #6347
  • Make sync, inbound, and block verifier check if a block hash is in any chain or any queue #862 was re-opened as a result of this review
    • fixed by change(state): Stop re-downloading blocks that are in non-finalized side chains #6335
• Potentially incorrect comments in src/batch.rs ed25519-zebra#74
  • fixed by fix documentation about batching ed25519-zebra#75
• Code duplication between reddsa and redjubjub #6341
  • fixed by change: Refactor & optimize the NAF reddsa#63
• XXX used as a TODO Label #6342
  • fixed by change(docs): Replaces XXX with TODO #6417
• Resolve open Peer Starving TODO #6344
  • fixed by doc(net): Explain how we prioritise peer messages, and why it is secure #6488
• Re-enable or delete the assertion in fetch_sprout_final_treestates() #6389
  • fixed by fix(state): Remove an outdated anchor assertion in a comment #6450
• Lack of integration testing. Addressed by Show that duplicate nullifier errors are propagated through the transaction verifier #6399
  • fixed by fix(tests): Add an integration test for error conversions from zebra-state to zebra-consensus #6665
• Outdated Documentation in zebra-state #6673
  • fixed by Replace update_chain_state_with ref with update_chain_tip_with in rustdoc #6781
• Discussion: A closer look at Zebra's Finalized State. Addressed by:
  • Renames to address confusion in zebra's handling of finalized state #6680
  • Update documentation to clarify terminology around block verification #6681
• Fragile Address Limit Implementation in Tests #6674
• Notes on Peer Sourcing #6675
  • Add a new default peers config, using the current config name for an "extra peers" config #6871
  • Zebra should store a persistent list of recent successful peers #1956 - Partially addresses this item
  • Zebra should try harder to discover and distribute good peer addresses #1892 - is another related issue which we decided not to implement

Out of Scope

Other tasks

• Write short "Client Summary Responses" for issues where we've accepted the risk and won't be making any changes:
  • [NCC-E005955-6AN]: Power-of-Two-Choices Load Balancing May Deprioritize Honest Peers - assigned to @teor2345
  • [NCC-E005955-GCR]: Cargo Audit and RustSec Advisories - assigned to @dconnolly
  • [NCC-E005955-MU2]: Redundant Computation in Sapling and Orchard Note Validation - assigned to @upbqdn
  • [NCC-E005955-GHX]: Incorrectly Disabled Consistency Check - assigned to @teor2345

[mpguerra]

We should probably write a summary response for #6617 . I have added a task to do this.

Who can take this one on?

[teor2345]

We should probably write a summary response for #6617 . I have added a task to do this.

Who can take this one on?

I did that PR, and it's very quick to copy my comments from the PR into the doc. So I'm happy to do it now.

[mpguerra]

All done 🎉