Avoid probing attacks with same hash lower amounts #395
Conversation
src/ln/channelmanager.rs
Outdated
| @@ -1574,21 +1574,31 @@ impl ChannelManager { | |||
| /// generating message events for the net layer to claim the payment, if possible. Thus, you | |||
| /// should probably kick the net layer to go send messages if this returns true! | |||
| /// | |||
| /// Require to specify expected amount so that we can claim only payment for the correct amount, | |||
There was a problem hiding this comment.
Should probably say something about "why", otherwise I would totally just specify all amounts in case they have free money :).
eg "You must specify the expected amount for this HTLC, and we will only claim HTLCs available within a few percent of the expected amount. This is critical for several reasons - a) it avoids providing senders with 'proof-of-payment' (in the form of the payment_preimage without having provided the full value and b) it avoids certain privacy-breaking recipient-probing attacks which may review payment activity to motivated attackers!".
There was a problem hiding this comment.
Note that, iirc, but I'm not 100% sure, the spec says we have to allow payents that are slightly greater than our expected value, which probably needs to be updated in the PaymentReceived event docs as well.
There was a problem hiding this comment.
The spec :
if the amount paid is more than twice the amount expected:
SHOULD fail the HTLC.
SHOULD return an incorrect_or_unknown_payment_details error.
Note: this allows the origin node to reduce information leakage by altering the amount while not allowing for accidental gross overpayment.
So updated the check with received_amount * 2 > expected_amount ; fail htlc.
Also took your comment, and PaymentReceived event docs is already referring to the spec on this.
ariard
force-pushed
the
2019-11-expected-amt-claim-funds
branch
2 times, most recently
from
58a82df to
4661950
Compare
|
Updated |
|
The |
ariard
force-pushed
the
2019-11-expected-amt-claim-funds
branch
from
4661950 to
dfa9ca3
Compare
|
Okay updated on dfa9ca3 with simply fetching them from PaymentReceived, but added a TODO that we may fetch amounts from fuzz inputs (need further tweakingn to pass test_no_existing_test_breakage) |
| if received_amount < expected_amount || received_amount * 2 > expected_amount { | ||
| let mut htlc_msat_data = byte_utils::be64_to_array(received_amount).to_vec(); | ||
| let mut height_data = byte_utils::be32_to_array(self.latest_block_height.load(Ordering::Acquire) as u32).to_vec(); | ||
| htlc_msat_data.append(&mut height_data); |
There was a problem hiding this comment.
Grrrr, I think spec changed out from underneath us. In pub fn fail_htlc_backwards we only send back the recvd_value field. Can be fixed in a separate pr.
There was a problem hiding this comment.
Yes, I've spotted other onion issues, like PERM|16 and PERM|17 being deprecated, will bundle in one PR.
Require to specify expected amount so that we can claim only payment for thhe correct amount, and reject payments for incorrect amounts (which are probably middle nodes probing to break our privacy). Send back incorrect_or_unknown_payments_details (PERM|15) to avoid the probe node learning that final node is waiting a payment with the routed hash.
ariard
force-pushed
the
2019-11-expected-amt-claim-funds
branch
from
dfa9ca3 to
5e047cc
Compare
|
Yeah, travis should be good, integrated modifs to make new test_duplicate_htlc_different_direction_onchain works. |
Sending back
incorrect_or_unknown_payment_detailsseems the best pick at it's the one telling to the sender "I'm not waiting this payment"