Enforce size limit in payjoin-cli #710
Conversation
Pull Request Test Coverage Report for Build 15172527684Details
💛 - Coveralls |
spacebear21
force-pushed
the
cli-size-limit
branch
from
c9a47f3 to
0734972
Compare
nothingmuch
left a comment
nothingmuch
left a comment
There was a problem hiding this comment.
as implemented this check is redundant with the payjoin crate's, since it does not protect against allocation failures
payjoin-cli/src/app/v1.rs
Outdated
| let response_bytes = response.bytes().await?; | ||
| if response_bytes.len() > MAX_CONTENT_LENGTH { |
There was a problem hiding this comment.
if a sufficiently large response is sent then the program might still crash, because first the excessively sized buffer would be allocated (in BytesExt::collect), and only then is its length is compared to the limit.
i think the way to address this is:
- BIP 78 does not specify that content-length is required on the response, if it's set check that it does not exceed the maximum
- read up to
MAX_CONTENT_LENGTHbytes into a buffer, failing if the limit is exceeded and data is still available to read
it seems like response.bytes_stream() could be used instead of response.bytes()
Because these are unbounded allocations a malicious receiver could cause memory exhaustion for the sender. This approach uses a bytes_stream to avoid loading the entire buffer into memory before checking the size limit.
spacebear21
force-pushed
the
cli-size-limit
branch
from
0734972 to
f494dcd
Compare
| // Pass the response body to process_response without loading it all into memory. | ||
| // This prevents a maliciously crafted response from causing an unbounded allocation. | ||
| let stream = | ||
| response.bytes_stream().map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e)); | ||
| let async_reader = StreamReader::new(stream); | ||
| let mut sync_reader = SyncIoBridge::new(async_reader); | ||
|
|
||
| let psbt = ctx.process_response(&mut sync_reader).map_err(|e| { | ||
| log::debug!("Error processing response: {e:?}"); | ||
| anyhow!("Failed to process response {}", e) | ||
| })?; |
There was a problem hiding this comment.
@nothingmuch Does this approach look sane to you? I had to get LLM help to convert the bytes_stream to std::io::Read for process_response, and this adds a few new dependencies.
EDIT: hmm it worked fine when I smoke tested the payjoin-cli receive/send flow but the e2e tests appear to disagree.
There was a problem hiding this comment.
The approach seems correct, avoids duplication of knowledge about the limits, and just trusts the payjoin crate to enforce the limit.
It appears SyncIoBridge requires a nested tokio runtime but that isn't allowed, perhaps in relation to how async tests are set up? That seems a bit brittle but if we can work around it perfectly fine since this is just a payjoin-cli quirk...
If it's simpler, then we can also just buffer twice, or even explicitly require buffering by changing process_response to take a &[u8] instead of a &mut impl std::io::Read, because in either case it would still be easy to have memory exhaustion vulnerable code calling this.
There was a problem hiding this comment.
I am now leaning towards "changing process_response to take a &[u8] instead of a &mut impl std::io::Read", and I think we should do this for the UncheckedProposal::from_request body parameter as well.
- I'm not a big fan of the extra complexity/dependencies introduced by this StreamReader/SyncIoBridge approach.
- Like you said, the app implementer needs to be careful either way to avoid OOM bugs.
- We use
&[u8]for the V2 Sender and Receiversprocess_responsemethods.
This was prompted by the discussion in #710 (comment). Using a reader still left callers susceptible to writing memory exhaustion vulnerable code, and just introduced extra complexity by having to buffer twice. We can recognize this and leave the buffering entirely up to the caller. Also, the V2 sender and receiver response processing functions already take &[u8], so this makes the function signatures more consistent across versions.
|
this has been obsoleted by #745 right? |
Well the point of this PR was to prevent buffer overflows in payjoin-cli, which #745 doesn't prevent. But the current approach taken in this PR is indeed obsolete. payjoin-cli should now just "read up to MAX_CONTENT_LENGTH bytes into a buffer, failing if the limit is exceeded and data is still available to read" as you suggested earlier in this PR. And depending on your thoughts re: #770 (comment), maybe we should also move MAX_CONTENT_LENGTH to live only in payjoin-cli? |
yes, good point!
yeah that sounds right to me |
|
Superseded by #808 |
This was prompted by the discussion in payjoin/rust-payjoin#710 (comment). Using a reader still left callers susceptible to writing memory exhaustion vulnerable code, and just introduced extra complexity by having to buffer twice. We can recognize this and leave the buffering entirely up to the caller. Also, the V2 sender and receiver response processing functions already take &[u8], so this makes the function signatures more consistent across versions.
4 participants
This diff was previously included in #586 but I deleted it by mistake while rebasing on top of master and resolving conflicts.