Extensive security review of Rust-Lightning codebase

[ariard]

As we move toward production releases, we should establish a clear scope of the Trusted Computing Base, in the sense than any bugs or vulnerabilities occurring inside TCB might jeopardize user funds, and therefore should have our special care.

Here a non-exhaustive list of Things We Must Get Right:

• standard/consensus valid transactions
• sounds and responsive feerate computation for dynamic bumping
• reorg-safe ChannelMonitor
• balanced/correcntess of state machine transition
• enforceable commitment transaction (aka size good enough to propagate)
• sane channel parameters (max_htlc_value_in_flight, ...)
• onchain-to-offchain transition
• safe CLTV deltas
• efficient timing out HTLCs
• spec compliance (or argumentation when we diverge)
• parsing of onchain funds
• onchain verification of channel points (cf utxo-related LN CVEs)
• HTLC packet sanitization
• reasonable default values
• safe storage of witnessscript/output for unilateral claiming
• any kind of DoS
• correctness of funding_signed and funding_tx broadcast sequence
• hash collision
• prevent easy channel congestion (within what current spec allows)
• correct weight computation for static fees
• ...

For each point,we should a) check if code is correct b) document clearly security significance for future modifications and developers c) have extensive unit/functional/fuzzing/mutating testing coverage.

Compare to #565, which should teach library users on the security model/privacy trade-offs and how to comply with them, this should serve as a starting point for future audit of the codebase. At the end of review process, we should commit a document linking each point to its enforcement in the codebase and its tests.

This is really needed because spec compliance isn't enough to be safe, a lot of security implications are loosely documented or let to the wisdom of implementers, and assume a good knowledge of bitcoin first-layer.

(If you any other point we should look on, I invite you to add a comment)

[ariard]

About DoS, maybe we should consider indirect DoS, like the one trying to abuse disk I/O of your full-node or bitcoin server, like a lot of crappy channel_announcement on old blocks.

Even if people have spent a lot of time on DoS risks on Core, you may have indirect DoS from untrusted LN peers through your trusted LN node..

[moneyball]

@ariard thank you for raising awareness of this and creating a list of important areas to be mindful of. Thoughts on turning this into a checklist doc that is added to the repo and linked to from CONTRIBUTING as a checklist all PR authors and reviewers should be mindful of with each and every change? This, along with actual usage of it during PR creation and review, would allow the project to practice this on an ongoing basis as opposed to irregular or one-time audits.

[ariard]

@moneyball I do agree it's far better to do this on an ongoing basis, what I want to be able to convey is the higher security model in some SECURITY.md. Because without you won't be able to understand how each module interact and to be secure you not only care about internals but also about interactions between them.

Yeah a checklist is maybe to limited but we shouldn't definitely invite people to read the security section of the component they're working on/reviewing in CONTRIBUTING.md

[ariard]

See https://github.com/rust-bitcoin/rust-lightning/pull/787#pullrequestreview-585048120.

We should be really careful about chan_id/short_chan_id/htlc_id collisions.

[ariard]

See https://github.com/rust-bitcoin/rust-lightning/pull/890