Conversation
|
👋 I see @TheBlueMatt was un-assigned. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
You should try to remove the existing chacha20poly1305rfc module entirely. Is there some limitations of the upstream implementation that prevented that?
|
@TheBlueMatt at the moment, |
Abeeujah
force-pushed
the
switch-chacha
branch
from
4f42780 to
388bbc2
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Looks like you still didn't manage to remove chacha20poly1305rfc.rs (and chacha20.rs and poly1305.rs)?
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
lightning/src/crypto/streams.rs
Outdated
| struct ChaChaDualPolyReader<'a, R: Read> { | ||
| chacha: &'a mut ChaCha20, | ||
| poly: &'a mut Poly1305, | ||
| poly_aad: &'a mut Poly1305, |
There was a problem hiding this comment.
Nah, we definitely don't want to do the full poly1305 validation twice. We'll have to upstream a Clone for Poly1305, mind opening a PR upstream at https://github.com/rust-bitcoin/rust-bitcoin/?
Abeeujah
force-pushed
the
switch-chacha
branch
from
4ef960c to
7f74979
Compare
…ctures 0df1825 dev: implement standard traits for ChaCha20-Poly1305 data types (Abeeujah) Pull request description: In `rust-lightning`, `ChaCha20-Poly1305` encryption is being refactored from the local `ChaCha20Poly1305RFC` to the `chacha20-poly1305` crate. Previous local implementation had support for streaming in `ChaCha20Poly1305RFC` implementations, but the `chacha20-poly1305` doesn't have implementations that supports streaming, this leads to using the `ChaCha20` and `Poly1305` primitives. To avoid computing `Poly1305` validation twice for input and additional authenticated data, it would be cool if `Poly1305` implements `Clone`, which saves computing the MAC twice. [Downstream PR](lightningdevkit/rust-lightning#4360 (comment)) ACKs for top commit: tcharding: ACK 0df1825 apoelstra: ACK 0df1825; successfully ran local tests Tree-SHA512: eae30d8f996d525a2891e0264fdb1996accaf3c57530f5d0134f63425d1bda82ecbd4112678d91892bd15079ae36db61109beccd7c93d5dc914b8284b30c968f
|
GM @TheBlueMatt upstream PR has been merged, I've pointed the crate to the git |
|
Oh good point yea, can you look into upstreaming our fuzzing logic as well? Could use a different |
Abeeujah
force-pushed
the
switch-chacha
branch
3 times, most recently
from
1aa98b9 to
a1dbdb8
Compare
| bech32 = { version = "0.11.0", default-features = false } | ||
| bitcoin = { version = "0.32.4", default-features = false, features = ["secp-recovery"] } | ||
|
|
||
| chacha20-poly1305 = { git = "https://github.com/Abeeujah/rust-bitcoin.git", rev = "fdf898d8651dd876fc36ee77da31350ed35349f9", default-features = false } |
There was a problem hiding this comment.
Issue: Git dependency prevents crates.io publishing
crates.io does not allow git dependencies. Since the lightning crate is published to crates.io, this dependency must come from a published crate before this can be merged/released. Using git + rev is fine for development/testing, but this is a release blocker.
Is there a timeline for the chacha20-poly1305 crate from rust-bitcoin being published to crates.io?
| pub(crate) mod chacha20poly1305rfc; | ||
| pub(crate) mod poly1305; | ||
| pub(crate) mod streams; | ||
| pub(crate) mod utils; |
There was a problem hiding this comment.
Potential issue: Fuzzing support regression
The deleted chacha20.rs and poly1305.rs files contained #[cfg(fuzzing)] no-op implementations:
- Fuzzing
ChaCha20::process_in_placewas a no-op (data passed through unencrypted) - Fuzzing
Poly1305ignored all input and returned the first 16 bytes of the key as the tag
These no-ops allowed fuzzers (e.g., peer_crypt, full_stack, chanmon_consistency) to exercise protocol logic without needing to produce valid ciphertexts/MACs.
Does the external chacha20-poly1305 crate provide equivalent #[cfg(fuzzing)] no-op implementations? If not, fuzzers that rely on transparent crypto will be broken — the handshake MAC checks in PeerChannelEncryptor will almost always fail on arbitrary fuzz input, preventing exploration of post-handshake code paths.
The #[cfg(fuzzing)] blocks preserved in streams.rs (lines 50-53, 144-147, 240-243) only change the Poly1305 key source but don't address the ChaCha20 encryption being real vs. no-op.
| while chacha_stream.read.bytes_remain() { | ||
| let mut buf = [0; 256]; | ||
| chacha_stream.read(&mut buf)?; | ||
| } |
There was a problem hiding this comment.
Pre-existing bug (worth fixing while refactoring): Missing 0-byte read guard
Unlike the ChaChaTriPolyReader drain loop (line 155-159), this drain loop does not check for a 0-byte return from read(). If the underlying reader returns EOF while bytes_remain() is still > 0 (e.g., the reader has less data than expected), this will infinite-loop.
The TriPoly variant correctly handles this:
if chacha_stream.read(&mut buf)? == 0 {
return Err(DecodeError::ShortRead);
}There's even a test for the TriPoly case (short_read_chacha_tri_read_adapter) but no equivalent test for this path. Consider adding the same guard here.
|
|
||
| let mut mac = Poly1305::new(&mac_key[..32]); | ||
| chacha.process_in_place(&mut plaintext[..]); | ||
| let mut mac = Poly1305::new(mac_key[..32].try_into().unwrap()); |
There was a problem hiding this comment.
Inconsistency: No #[cfg(fuzzing)] block for Poly1305 initialization here
The other three Poly1305 initialization sites in this file all have #[cfg(fuzzing)] blocks that use the encryption key directly instead of the derived mac_key. This site does not. In the old code, this was not an issue because the fuzzing Poly1305 module-level #[cfg(fuzzing)] switch handled it automatically. With the external crate, this site always uses mac_key even in fuzzing mode, making it inconsistent with the other sites.
| let seek_pos = (packet_data.len() - pos) as u32; | ||
| let mut chacha = ChaCha20::new(Key::new(keys.rho), Nonce::new([0; 12]), seek_pos); |
There was a problem hiding this comment.
Bug: Block counter vs byte offset mismatch
seek_pos is a byte count (packet_data.len() - pos), but the 3rd argument to ChaCha20::new is the initial block counter (each ChaCha20 block is 64 bytes). This is evident from how the same constructor is used in sign/mod.rs and inbound_payment.rs, where the value passed corresponds to the old code's d[0] block counter.
The old code consumed packet_data.len() - pos individual bytes from the keystream:
for _ in 0..(packet_data.len() - pos) {
let mut dummy = [0; 1];
chacha.process_in_place(&mut dummy);
}The new code sets the block counter to this byte count, which would start the keystream at byte seek_pos * 64 instead of byte seek_pos. For a typical 1300-byte onion packet on the first iteration, this would start at byte 83,200 instead of byte 1300.
This will produce incorrect filler data, breaking onion packet construction and causing all multi-hop payments to fail.
The fix would need to compute the block offset and handle the remainder:
let skip_bytes = packet_data.len() - pos;
let block_offset = (skip_bytes / 64) as u32;
let byte_remainder = skip_bytes % 64;
let mut chacha = ChaCha20::new(Key::new(keys.rho), Nonce::new([0; 12]), block_offset);
if byte_remainder > 0 {
let mut dummy = [0u8; 64];
chacha.apply_keystream(&mut dummy[..byte_remainder]);
}There was a problem hiding this comment.
The third argument to ChaCha20::new is a byte offset (seek), not a block counter. The constructor internally divides by 64 to get the block count and takes the remainder as the byte offset within that block. So passing seek_pos = packet_data.len() - pos correctly seeks to that byte position in the keystream.
| let mut salt = [0 as u8; 12]; | ||
| let block_height_bytes = block_height.to_be_bytes(); | ||
| salt[0..4].copy_from_slice(&block_height_bytes); | ||
| let tx_index_bytes = tx_index.to_be_bytes(); | ||
| salt[4..8].copy_from_slice(&tx_index_bytes); | ||
|
|
||
| let mut chacha = ChaCha20::new(fake_scid_rand_bytes, &salt); | ||
| let mut chacha = ChaCha20::new(Key::new(*fake_scid_rand_bytes), Nonce::new(salt), 0); |
There was a problem hiding this comment.
Bug: Nonce format change breaks keystream compatibility
The salt was changed from 8 bytes to 12 bytes. With an 8-byte nonce, the old ChaCha20::new placed values in the state as:
d[0]=counter, d[1]=0, d[2]=block_height, d[3]=tx_index
With a 12-byte nonce (RFC 8439), the state becomes:
d[0]=counter, d[1]=block_height, d[2]=tx_index, d[3]=0
These produce different keystreams for any non-zero block_height or tx_index. This means:
get_encrypted_voutnow returns different values than before for the same inputsis_valid_phantom/is_valid_interceptwill fail to validate fake SCIDs generated by old code- Any outstanding invoices using phantom or intercept SCIDs will stop working after upgrade
The test change from get_encrypted_vout(0, 0, ...) to get_encrypted_vout(1, 0, ...) below confirms the behavior changed — the old test only passed because with all-zero nonce the 8-byte and 12-byte layouts happened to coincide, but with block_height=1 they diverge.
To preserve backward compatibility, you need to place block_height and tx_index at the same position in the 12-byte nonce as they occupied in the 8-byte nonce:
let mut salt = [0u8; 12];
salt[4..8].copy_from_slice(&block_height_bytes);
salt[8..12].copy_from_slice(&tx_index_bytes);| run: | | ||
| cd fuzz | ||
| RUSTFLAGS="--cfg=fuzzing --cfg=secp256k1_fuzz --cfg=hashes_fuzz" cargo test --verbose --color always --lib --bins -j8 | ||
| RUSTFLAGS="--cfg=fuzzing --cfg=secp256k1_fuzz --cfg=hashes_fuzz --cfg=chacha20_poly1305_fuzz" cargo test --verbose --color always --lib --bins -j8 |
There was a problem hiding this comment.
Issue: --cfg=chacha20_poly1305_fuzz not added to ci-fuzz.sh
This flag is added here for the sanity check, but fuzz/ci-fuzz.sh (line 16) sets its own RUSTFLAGS without --cfg=chacha20_poly1305_fuzz:
export RUSTFLAGS="--cfg=secp256k1_fuzz --cfg=hashes_fuzz"This means the actual honggfuzz runs will compile the chacha20-poly1305 crate without its fuzzing mode, so real ChaCha20-Poly1305 operations will be performed during fuzzing. Fuzzers won't be able to generate valid MACs, severely limiting their ability to explore post-handshake/post-decryption code paths.
| ) -> Result<msgs::OnionPacket, ()> { | ||
| let mut packet_data = [0; ONION_DATA_LEN]; |
There was a problem hiding this comment.
Semantic concern: 8-byte nonce → 12-byte nonce changes keystream for non-zero nonces
All the ChaCha20::new calls in this file use Nonce::new([0; 12]) (all-zero nonce), replacing the old &[0u8; 8]. Since the nonce is all zeros, the ChaCha20 state is identical regardless of nonce length, so there's no behavioral change here.
However, this is a subtle correctness invariant: this only works because the nonce is [0; 12]. If anyone later changes the nonce to a non-zero value based on the old 8-byte pattern without adjusting the position within the 12-byte array, it would silently break (as happened in scid_utils.rs). Worth a brief comment noting this equivalence holds only for all-zero nonces.
|
|
Migrates ChaCha20-Poly1305 encryption from the local crypto module to rust-bitcoin's `chacha20-poly1305` crate. Integrated the crate across all modules (Router, PeerStorage, Onion Utils, etc.) and removed the deprecated internal implementation. Updated test vectors to align with the new 12-byte nonce construction and ensured SCID encryption remains consistent.
|
Cool, so I guess we're just waiting for an upstream release? Can you fix the rustfmt job in the mean time? |
Cargo fmt formats the code using rustfmt and fixes the format diff reported by the ci rustfmt workflow, when there are formatting issues.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4360 +/- ##
==========================================
+ Coverage 84.55% 86.10% +1.54%
==========================================
Files 135 158 +23
Lines 76569 106736 +30167
Branches 76569 106736 +30167
==========================================
+ Hits 64745 91906 +27161
- Misses 9783 12214 +2431
- Partials 2041 2616 +575
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
closes #4316
This PR switches the ChaChaPoly encryption algorithm to the
chacha20-poly1305 = "0.1.2"crate, the following impls have been modifiedEncryptedOurPeerStoragePeerChannelEncryptorThe
chacha20-poly1305dependency was only added because the issue description by @TheBlueMatt requested it be added, and also, thanks to @tnull for the reference implementation provided.