rust-payjoin
rust-payjoin copied to clipboard
payjoin
v1 sender should limit response size
BIP 78 does not restrict the size of the response, but specifies that content length must be included.
The v1 receiver enforces a reasonable limit on the request:
https://github.com/payjoin/rust-payjoin/blob/34ee78d44e1527210a343f4e08128e41a82a67a1/payjoin/src/receive/v1.rs#L95-L97
however, a similar limit was not included in payjoin-cli, response.bytes().to_vec() could potentially make unbounded allocations given a large enough response body:
https://github.com/payjoin/rust-payjoin/blob/34ee78d44e1527210a343f4e08128e41a82a67a1/payjoin/src/send/v1.rs#L276-L277
(note that the error mapping also seems inaccurate, not sure what happens in the case of invalid utf8 in the response)
nor is a size limit enforced in the payjoin-cli sender:
https://github.com/payjoin/rust-payjoin/blob/34ee78d44e1527210a343f4e08128e41a82a67a1/payjoin-cli/src/app/v1.rs#L99
because these are unbounded allocations a malicious receiver can cause memory exhaustion for the sender.
there may be other issues in v1, and similarly we should check that the in the v2 implementation no such unbounded allocations (e.g. r.bytes().to_vec()) are used for all parties (sender, receiver, directory and relay)
Why are these strictly a v1 concern? &mut response.bytes().await?.to_vec().as_slice() seems to be how it's done in our app, and .bytes() collects under the hood.
I want to make sure #586 is an actual solution and not just a band aid. I also want to make sure and that our response processing has the right interface. In some places body is a &[u8] and in others io::Read, and we ought to settle on one interface as we approach stability.
doesn't #586 only enforce send and not receive? Isn't this thus still open?
Receiver side already enforced the size limit iiuc?
The v1 receiver enforces a reasonable limit on the request:
https://github.com/payjoin/rust-payjoin/blob/34ee78d44e1527210a343f4e08128e41a82a67a1/payjoin/src/receive/v1.rs#L95-L97
It's here now: https://github.com/payjoin/rust-payjoin/blob/master/payjoin/src/receive/v1/exclusive/mod.rs#L60
Receiver side already enforced the size limit iiuc?
that limit is only enforced after the request data was buffered in memory, the resource exhaustion concern is with payjoin-cli itself which first accepts a potentially arbitrarily sized request, and then sends it off to the quoted code, so a malicious sender can crash payjoin-cli by memory exhaustion before that check kicks in
I see - @shinghim had included a check in payjoin-cli in #586 but I removed it by mistake while resolving rebase conflicts. See https://github.com/payjoin/rust-payjoin/pull/710.
However, I don't think the other direction is addressed (malicious sender), and there may be other places where unbounded allocations occur. It seems like a proper audit is in order.
However, I don't think the other direction is addressed (malicious sender), and there may be other places where unbounded allocations occur. It seems like a proper audit is in order.
Isn't this issue mainly focused on the malicious receiver? From the issue description:
because these are unbounded allocations a malicious receiver can cause memory exhaustion for the sender.
I think i might be confusing myself on the sender/receiver terminology, and we should of course make sure that both malicious senders and receivers are handled, but mostly trying to understand if this issue is solved, and if we should create another one for the other direction (malicious sender)
this issue was primarily about the payjoin-cli behavior
both the payjoin and the payjoin-cli crates implements both the sender and receiver roles.
in the payjoin crate, the receiver had limits, but these are more semantic, the data is assumed to be in memory already so it's a correctness issue.
but in the payjoin-cli crate, to_vec() or collect() is used before knowing the size, this means a malicious input can cause allocation errors and waste the victim's bandwidth