v1 sender should limit response size

[nothingmuch]

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:

rust-payjoin/payjoin/src/receive/v1.rs

Lines 95 to 97 in 34ee78d

	if content_length > 4_000_000 * 4 / 3 {
	return Err(InternalRequestError::ContentLengthTooLarge(content_length).into());
	}

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:

rust-payjoin/payjoin/src/send/v1.rs

Lines 276 to 277 in 34ee78d

	let mut res_str = String::new();
	response.read_to_string(&mut res_str).map_err(InternalValidationError::Io)?;

(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:

rust-payjoin/payjoin-cli/src/app/v1.rs

Line 99 in 34ee78d

let psbt = ctx.process_response(&mut response.bytes().await?.to_vec().as_slice()).map_err(

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)

[DanGould]

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.

[DanGould]

doesn't #586 only enforce send and not receive? Isn't this thus still open?

[DanGould]

@spacebear21

[spacebear21]

Receiver side already enforced the size limit iiuc?

The v1 receiver enforces a reasonable limit on the request:

rust-payjoin/payjoin/src/receive/v1.rs

Lines 95 to 97 in 34ee78d

	if content_length > 4_000_000 * 4 / 3 {
	return Err(InternalRequestError::ContentLengthTooLarge(content_length).into());
	}

It's here now: https://github.com/payjoin/rust-payjoin/blob/master/payjoin/src/receive/v1/exclusive/mod.rs#L60

[nothingmuch]

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

[spacebear21]

I see - @shinghim had included a check in payjoin-cli in #586 but I removed it by mistake while resolving rebase conflicts. See #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.