Skip to content

Commit bceb952

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #14 from TheBlueMatt/2018-03-fuzz-fixes-1
Impl some message deserialization, initial fuzzing-found bug fixes
2 parents f47ba76 + e9b1af2 commit bceb952

File tree

3 files changed

+141
-49
lines changed

3 files changed

+141
-49
lines changed

‎src/ln/channel.rs

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -43,34 +43,33 @@ pub struct ChannelKeys {
4343

4444
impl ChannelKeys {
4545
pub fn new_from_seed(seed: &[u8; 32]) -> Result<ChannelKeys, secp256k1::Error> {
46-
let sha = Sha256::new();
4746
let mut prk = [0; 32];
48-
hkdf_extract(sha, b"rust-lightning key gen salt", seed, &mut prk);
47+
hkdf_extract(Sha256::new(), b"rust-lightning key gen salt", seed, &mut prk);
4948
let secp_ctx = Secp256k1::new();
5049

5150
let mut okm = [0; 32];
52-
hkdf_expand(sha, &prk, b"rust-lightning funding key info", &mut okm);
51+
hkdf_expand(Sha256::new(), &prk, b"rust-lightning funding key info", &mut okm);
5352
let funding_key = SecretKey::from_slice(&secp_ctx, &okm)?;
5453

55-
hkdf_expand(sha, &prk, b"rust-lightning revocation base key info", &mut okm);
54+
hkdf_expand(Sha256::new(), &prk, b"rust-lightning revocation base key info", &mut okm);
5655
let revocation_base_key = SecretKey::from_slice(&secp_ctx, &okm)?;
5756

58-
hkdf_expand(sha, &prk, b"rust-lightning payment base key info", &mut okm);
57+
hkdf_expand(Sha256::new(), &prk, b"rust-lightning payment base key info", &mut okm);
5958
let payment_base_key = SecretKey::from_slice(&secp_ctx, &okm)?;
6059

61-
hkdf_expand(sha, &prk, b"rust-lightning delayed payment base key info", &mut okm);
60+
hkdf_expand(Sha256::new(), &prk, b"rust-lightning delayed payment base key info", &mut okm);
6261
let delayed_payment_base_key = SecretKey::from_slice(&secp_ctx, &okm)?;
6362

64-
hkdf_expand(sha, &prk, b"rust-lightning htlc base key info", &mut okm);
63+
hkdf_expand(Sha256::new(), &prk, b"rust-lightning htlc base key info", &mut okm);
6564
let htlc_base_key = SecretKey::from_slice(&secp_ctx, &okm)?;
6665

67-
hkdf_expand(sha, &prk, b"rust-lightning channel close key info", &mut okm);
66+
hkdf_expand(Sha256::new(), &prk, b"rust-lightning channel close key info", &mut okm);
6867
let channel_close_key = SecretKey::from_slice(&secp_ctx, &okm)?;
6968

70-
hkdf_expand(sha, &prk, b"rust-lightning channel monitor claim key info", &mut okm);
69+
hkdf_expand(Sha256::new(), &prk, b"rust-lightning channel monitor claim key info", &mut okm);
7170
let channel_monitor_claim_key = SecretKey::from_slice(&secp_ctx, &okm)?;
7271

73-
hkdf_expand(sha, &prk, b"rust-lightning local commitment seed info", &mut okm);
72+
hkdf_expand(Sha256::new(), &prk, b"rust-lightning local commitment seed info", &mut okm);
7473

7574
Ok(ChannelKeys {
7675
funding_key: funding_key,
@@ -367,7 +366,9 @@ impl Channel {
367366
if msg.push_msat > (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 {
368367
return Err(HandleError{err: "push_msat more than highest possible value", msg: None});
369368
}
370-
//TODO Check if dust_limit is sane?
369+
if msg.dust_limit_satoshis > 21000000 * 100000000 {
370+
return Err(HandleError{err: "Peer never wants payout outputs?", msg: None});
371+
}
371372
if msg.max_htlc_value_in_flight_msat > msg.funding_satoshis * 1000 {
372373
return Err(HandleError{err: "Bogus max_htlc_value_in_flight_satoshis", msg: None});
373374
}
@@ -827,13 +828,15 @@ impl Channel {
827828

828829
pub fn accept_channel(&mut self, msg: &msgs::AcceptChannel) -> Result<(), HandleError> {
829830
// Check sanity of message fields:
830-
//TODO Check if dust_limit is sane?
831831
if !self.channel_outbound {
832832
return Err(HandleError{err: "Got an accept_channel message from an inbound peer", msg: None});
833833
}
834834
if self.channel_state != ChannelState::OurInitSent as u32 {
835835
return Err(HandleError{err: "Got an accept_channel message at a strange time", msg: None});
836836
}
837+
if msg.dust_limit_satoshis > 21000000 * 100000000 {
838+
return Err(HandleError{err: "Peer never wants payout outputs?", msg: None});
839+
}
837840
if msg.max_htlc_value_in_flight_msat > self.channel_value_satoshis * 1000 {
838841
return Err(HandleError{err: "Bogus max_htlc_value_in_flight_satoshis", msg: None});
839842
}
@@ -1013,8 +1016,10 @@ impl Channel {
10131016
if htlc_inbound_value_msat + msg.amount_msat > Channel::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis) {
10141017
return Err(HandleError{err: "Remote HTLC add would put them over their max HTLC value in flight", msg: None});
10151018
}
1016-
// Check our_channel_reserve_satoshis:
1017-
if htlc_inbound_value_msat + htlc_outbound_value_msat + msg.amount_msat > (self.channel_value_satoshis - Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 {
1019+
// Check our_channel_reserve_satoshis (we're getting paid, so they have to at least meet
1020+
// the reserve_satoshis we told them to always have as direct payment so that they lose
1021+
// something if we punish them for broadcasting an old state).
1022+
if htlc_inbound_value_msat + htlc_outbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 {
10181023
return Err(HandleError{err: "Remote HTLC add would put them over their reserve value", msg: None});
10191024
}
10201025
if self.next_remote_htlc_id != msg.htlc_id {
@@ -1592,7 +1597,7 @@ impl Channel {
15921597
return Err(HandleError{err: "Cannot send value that would put us over our max HTLC value in flight", msg: None});
15931598
}
15941599
// Check their_channel_reserve_satoshis:
1595-
if htlc_outbound_value_msat + amount_msat > (self.channel_value_satoshis - self.their_channel_reserve_satoshis) * 1000 - htlc_inbound_value_msat {
1600+
if htlc_inbound_value_msat + htlc_outbound_value_msat + amount_msat + (self.channel_value_satoshis * 1000 - self.value_to_self_msat) > (self.channel_value_satoshis - self.their_channel_reserve_satoshis) * 1000 {
15961601
return Err(HandleError{err: "Cannot send value that would put us over our reserve value", msg: None});
15971602
}
15981603

‎src/ln/msgs.rs

Lines changed: 110 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -684,8 +684,20 @@ impl MsgEncodable for ClosingSigned {
684684
}
685685

686686
impl MsgDecodable for UpdateAddHTLC {
687-
fn decode(_v: &[u8]) -> Result<Self, DecodeError> {
688-
unimplemented!();
687+
fn decode(v: &[u8]) -> Result<Self, DecodeError> {
688+
if v.len() < 32+8+8+32+4+1+33+20*65+32 {
689+
return Err(DecodeError::WrongLength);
690+
}
691+
let mut payment_hash = [0; 32];
692+
payment_hash.copy_from_slice(&v[48..80]);
693+
Ok(Self{
694+
channel_id: deserialize(&v[0..32]).unwrap(),
695+
htlc_id: byte_utils::slice_to_be64(&v[32..40]),
696+
amount_msat: byte_utils::slice_to_be64(&v[40..48]),
697+
payment_hash,
698+
cltv_expiry: byte_utils::slice_to_be32(&v[80..84]),
699+
onion_routing_packet: OnionPacket::decode(&v[84..])?,
700+
})
689701
}
690702
}
691703
impl MsgEncodable for UpdateAddHTLC {
@@ -695,8 +707,17 @@ impl MsgEncodable for UpdateAddHTLC {
695707
}
696708

697709
impl MsgDecodable for UpdateFulfillHTLC {
698-
fn decode(_v: &[u8]) -> Result<Self, DecodeError> {
699-
unimplemented!();
710+
fn decode(v: &[u8]) -> Result<Self, DecodeError> {
711+
if v.len() < 32+8+32 {
712+
return Err(DecodeError::WrongLength);
713+
}
714+
let mut payment_preimage = [0; 32];
715+
payment_preimage.copy_from_slice(&v[40..72]);
716+
Ok(Self{
717+
channel_id: deserialize(&v[0..32]).unwrap(),
718+
htlc_id: byte_utils::slice_to_be64(&v[32..40]),
719+
payment_preimage,
720+
})
700721
}
701722
}
702723
impl MsgEncodable for UpdateFulfillHTLC {
@@ -706,8 +727,15 @@ impl MsgEncodable for UpdateFulfillHTLC {
706727
}
707728

708729
impl MsgDecodable for UpdateFailHTLC {
709-
fn decode(_v: &[u8]) -> Result<Self, DecodeError> {
710-
unimplemented!();
730+
fn decode(v: &[u8]) -> Result<Self, DecodeError> {
731+
if v.len() < 32+8 {
732+
return Err(DecodeError::WrongLength);
733+
}
734+
Ok(Self{
735+
channel_id: deserialize(&v[0..32]).unwrap(),
736+
htlc_id: byte_utils::slice_to_be64(&v[32..40]),
737+
reason: OnionErrorPacket::decode(&v[40..])?,
738+
})
711739
}
712740
}
713741
impl MsgEncodable for UpdateFailHTLC {
@@ -717,8 +745,18 @@ impl MsgEncodable for UpdateFailHTLC {
717745
}
718746

719747
impl MsgDecodable for UpdateFailMalformedHTLC {
720-
fn decode(_v: &[u8]) -> Result<Self, DecodeError> {
721-
unimplemented!();
748+
fn decode(v: &[u8]) -> Result<Self, DecodeError> {
749+
if v.len() < 32+8+32+2 {
750+
return Err(DecodeError::WrongLength);
751+
}
752+
let mut sha256_of_onion = [0; 32];
753+
sha256_of_onion.copy_from_slice(&v[40..72]);
754+
Ok(Self{
755+
channel_id: deserialize(&v[0..32]).unwrap(),
756+
htlc_id: byte_utils::slice_to_be64(&v[32..40]),
757+
sha256_of_onion,
758+
failure_code: byte_utils::slice_to_be16(&v[72..74]),
759+
})
722760
}
723761
}
724762
impl MsgEncodable for UpdateFailMalformedHTLC {
@@ -728,8 +766,24 @@ impl MsgEncodable for UpdateFailMalformedHTLC {
728766
}
729767

730768
impl MsgDecodable for CommitmentSigned {
731-
fn decode(_v: &[u8]) -> Result<Self, DecodeError> {
732-
unimplemented!();
769+
fn decode(v: &[u8]) -> Result<Self, DecodeError> {
770+
if v.len() < 32+64+2 {
771+
return Err(DecodeError::WrongLength);
772+
}
773+
let htlcs = byte_utils::slice_to_be16(&v[96..98]) as usize;
774+
if v.len() < 32+64+2+htlcs*64 {
775+
return Err(DecodeError::WrongLength);
776+
}
777+
let mut htlc_signatures = Vec::with_capacity(htlcs);
778+
let secp_ctx = Secp256k1::without_caps();
779+
for i in 0..htlcs {
780+
htlc_signatures.push(secp_signature!(&secp_ctx, &v[98+i*64..98+(i+1)*64]));
781+
}
782+
Ok(Self {
783+
channel_id: deserialize(&v[0..32]).unwrap(),
784+
signature: secp_signature!(&secp_ctx, &v[32..96]),
785+
htlc_signatures,
786+
})
733787
}
734788
}
735789
impl MsgEncodable for CommitmentSigned {
@@ -739,8 +793,18 @@ impl MsgEncodable for CommitmentSigned {
739793
}
740794

741795
impl MsgDecodable for RevokeAndACK {
742-
fn decode(_v: &[u8]) -> Result<Self, DecodeError> {
743-
unimplemented!();
796+
fn decode(v: &[u8]) -> Result<Self, DecodeError> {
797+
if v.len() < 32+32+33 {
798+
return Err(DecodeError::WrongLength);
799+
}
800+
let mut per_commitment_secret = [0; 32];
801+
per_commitment_secret.copy_from_slice(&v[32..64]);
802+
let secp_ctx = Secp256k1::without_caps();
803+
Ok(Self {
804+
channel_id: deserialize(&v[0..32]).unwrap(),
805+
per_commitment_secret,
806+
next_per_commitment_point: secp_pubkey!(&secp_ctx, &v[64..97]),
807+
})
744808
}
745809
}
746810
impl MsgEncodable for RevokeAndACK {
@@ -750,8 +814,14 @@ impl MsgEncodable for RevokeAndACK {
750814
}
751815

752816
impl MsgDecodable for UpdateFee {
753-
fn decode(_v: &[u8]) -> Result<Self, DecodeError> {
754-
unimplemented!();
817+
fn decode(v: &[u8]) -> Result<Self, DecodeError> {
818+
if v.len() < 32+4 {
819+
return Err(DecodeError::WrongLength);
820+
}
821+
Ok(Self {
822+
channel_id: deserialize(&v[0..32]).unwrap(),
823+
feerate_per_kw: byte_utils::slice_to_be32(&v[32..36]),
824+
})
755825
}
756826
}
757827
impl MsgEncodable for UpdateFee {
@@ -954,8 +1024,21 @@ impl MsgEncodable for OnionHopData {
9541024
}
9551025

9561026
impl MsgDecodable for OnionPacket {
957-
fn decode(_v: &[u8]) -> Result<Self, DecodeError> {
958-
unimplemented!();
1027+
fn decode(v: &[u8]) -> Result<Self, DecodeError> {
1028+
if v.len() < 1+33+20*65+32 {
1029+
return Err(DecodeError::WrongLength);
1030+
}
1031+
let mut hop_data = [0; 20*65];
1032+
hop_data.copy_from_slice(&v[34..1334]);
1033+
let mut hmac = [0; 32];
1034+
hmac.copy_from_slice(&v[1334..1366]);
1035+
let secp_ctx = Secp256k1::without_caps();
1036+
Ok(Self {
1037+
version: v[0],
1038+
public_key: secp_pubkey!(&secp_ctx, &v[1..34]),
1039+
hop_data,
1040+
hmac,
1041+
})
9591042
}
9601043
}
9611044
impl MsgEncodable for OnionPacket {
@@ -987,8 +1070,17 @@ impl MsgEncodable for DecodedOnionErrorPacket {
9871070
}
9881071

9891072
impl MsgDecodable for OnionErrorPacket {
990-
fn decode(_v: &[u8]) -> Result<Self, DecodeError> {
991-
unimplemented!();
1073+
fn decode(v: &[u8]) -> Result<Self, DecodeError> {
1074+
if v.len() < 2 {
1075+
return Err(DecodeError::WrongLength);
1076+
}
1077+
let len = byte_utils::slice_to_be16(&v[0..2]) as usize;
1078+
if v.len() < 2 + len {
1079+
return Err(DecodeError::WrongLength);
1080+
}
1081+
Ok(Self {
1082+
data: v[2..len+2].to_vec(),
1083+
})
9921084
}
9931085
}
9941086
impl MsgEncodable for OnionErrorPacket {

‎src/ln/peer_channel_encryptor.rs

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -157,12 +157,11 @@ impl PeerChannelEncryptor {
157157

158158
#[inline]
159159
fn hkdf(state: &mut BidirectionalNoiseState, ss: SharedSecret) -> [u8; 32] {
160-
let sha = Sha256::new();
161160
let mut hkdf = [0; 64];
162161
{
163162
let mut prk = [0; 32];
164-
hkdf_extract(sha, &state.ck, &ss[..], &mut prk);
165-
hkdf_expand(sha, &prk, &[0;0], &mut hkdf);
163+
hkdf_extract(Sha256::new(), &state.ck, &ss[..], &mut prk);
164+
hkdf_expand(Sha256::new(), &prk, &[0;0], &mut hkdf);
166165
}
167166
state.ck.copy_from_slice(&hkdf[0..32]);
168167
let mut res = [0; 32];
@@ -313,10 +312,9 @@ impl PeerChannelEncryptor {
313312

314313
PeerChannelEncryptor::encrypt_with_ad(&mut res[50..], 0, &temp_k, &bidirectional_state.h, &[0; 0]);
315314

316-
sha.reset();
317315
let mut prk = [0; 32];
318-
hkdf_extract(sha, &bidirectional_state.ck, &[0; 0], &mut prk);
319-
hkdf_expand(sha, &prk, &[0;0], &mut final_hkdf);
316+
hkdf_extract(Sha256::new(), &bidirectional_state.ck, &[0; 0], &mut prk);
317+
hkdf_expand(Sha256::new(), &prk, &[0;0], &mut final_hkdf);
320318
ck = bidirectional_state.ck.clone();
321319
res
322320
},
@@ -375,10 +373,9 @@ impl PeerChannelEncryptor {
375373

376374
PeerChannelEncryptor::decrypt_with_ad(&mut [0; 0], 0, &temp_k, &bidirectional_state.h, &act_three[50..])?;
377375

378-
sha.reset();
379376
let mut prk = [0; 32];
380-
hkdf_extract(sha, &bidirectional_state.ck, &[0; 0], &mut prk);
381-
hkdf_expand(sha, &prk, &[0;0], &mut final_hkdf);
377+
hkdf_extract(Sha256::new(), &bidirectional_state.ck, &[0; 0], &mut prk);
378+
hkdf_expand(Sha256::new(), &prk, &[0;0], &mut final_hkdf);
382379
ck = bidirectional_state.ck.clone();
383380
},
384381
_ => panic!("Wrong direction for act"),
@@ -416,11 +413,10 @@ impl PeerChannelEncryptor {
416413
match self.noise_state {
417414
NoiseState::Finished { ref mut sk, ref mut sn, ref mut sck, rk: _, rn: _, rck: _ } => {
418415
if *sn >= 1000 {
419-
let mut sha = Sha256::new();
420416
let mut prk = [0; 32];
421-
hkdf_extract(sha, sck, sk, &mut prk);
417+
hkdf_extract(Sha256::new(), sck, sk, &mut prk);
422418
let mut hkdf = [0; 64];
423-
hkdf_expand(sha, &prk, &[0;0], &mut hkdf);
419+
hkdf_expand(Sha256::new(), &prk, &[0;0], &mut hkdf);
424420

425421
sck[..].copy_from_slice(&hkdf[0..32]);
426422
sk[..].copy_from_slice(&hkdf[32..]);
@@ -447,11 +443,10 @@ impl PeerChannelEncryptor {
447443
match self.noise_state {
448444
NoiseState::Finished { sk: _, sn: _, sck: _, ref mut rk, ref mut rn, ref mut rck } => {
449445
if *rn >= 1000 {
450-
let mut sha = Sha256::new();
451446
let mut prk = [0; 32];
452-
hkdf_extract(sha, rck, rk, &mut prk);
447+
hkdf_extract(Sha256::new(), rck, rk, &mut prk);
453448
let mut hkdf = [0; 64];
454-
hkdf_expand(sha, &prk, &[0;0], &mut hkdf);
449+
hkdf_expand(Sha256::new(), &prk, &[0;0], &mut hkdf);
455450

456451
rck[..].copy_from_slice(&hkdf[0..32]);
457452
rk[..].copy_from_slice(&hkdf[32..]);
@@ -752,7 +747,7 @@ mod tests {
752747
let res = outbound_peer.encrypt_message(&msg);
753748
assert_eq!(res.len(), 5 + 2*16 + 2);
754749

755-
let mut len_header = res[0..2+16].to_vec();
750+
let len_header = res[0..2+16].to_vec();
756751
assert_eq!(inbound_peer.decrypt_length_header(&len_header[..]).unwrap() as usize, msg.len());
757752
assert_eq!(inbound_peer.decrypt_message(&res[2+16..]).unwrap()[..], msg[..]);
758753

0 commit comments

Comments
 (0)