Skip to content

Enforce size limit in payjoin-cli #710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
spacebear21 wants to merge 1 commit into from
Closed

Enforce size limit in payjoin-cli #710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion payjoin-cli/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,21 @@ bitcoincore-rpc = "0.19.0"
clap = { version = "~4.0.32", features = ["derive"] }
config = "0.13.3"
env_logger = "0.9.0"
futures-util = "0.3"
http-body-util = { version = "0.1", optional = true }
hyper = { version = "1", features = ["http1", "server"], optional = true }
hyper-rustls = { version = "0.26", optional = true }
hyper-util = { version = "0.1", optional = true }
log = "0.4.7"
payjoin = { version = "0.23.0", default-features = false }
rcgen = { version = "0.11.1", optional = true }
reqwest = { version = "0.12", default-features = false }
reqwest = { version = "0.12", default-features = false, features = ["stream"] }
rustls = { version = "0.22.4", optional = true }
serde = { version = "1.0.160", features = ["derive"] }
sled = "0.34"
tokio = { version = "1.38.1", features = ["full"] }
tokio-rustls = { version = "0.25", features = ["ring"], default-features = false, optional = true }
tokio-util = { version = "0.7", features = ["io", "io-util"] }
url = { version = "2.3.1", features = ["serde"] }

[dev-dependencies]
Expand Down
30 changes: 23 additions & 7 deletions payjoin-cli/src/app/v1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::sync::Arc;

use anyhow::{anyhow, Context, Result};
use bitcoincore_rpc::bitcoin::Amount;
use futures_util::TryStreamExt;
use http_body_util::combinators::BoxBody;
use http_body_util::{BodyExt, Full};
use hyper::body::{Buf, Bytes, Incoming};
Expand All @@ -17,9 +18,10 @@ use payjoin::bitcoin::FeeRate;
use payjoin::receive::v1::{PayjoinProposal, UncheckedProposal};
use payjoin::receive::ReplyableError::{self, Implementation, V1};
use payjoin::send::v1::SenderBuilder;
use payjoin::{ImplementationError, Uri, UriExt};
use payjoin::{ImplementationError, Uri, UriExt, MAX_CONTENT_LENGTH};
use tokio::net::TcpListener;
use tokio::sync::watch;
use tokio_util::io::{StreamReader, SyncIoBridge};

use super::config::Config;
use super::wallet::BitcoindWallet;
Expand Down Expand Up @@ -88,12 +90,26 @@ impl AppTrait for App {
"Sent fallback transaction hex: {:#}",
payjoin::bitcoin::consensus::encode::serialize_hex(&fallback_tx)
);
let psbt = ctx.process_response(&mut response.bytes().await?.to_vec().as_slice()).map_err(
|e| {
log::debug!("Error processing response: {e:?}");
anyhow!("Failed to process response {e}")
},
)?;

if let Some(content_length) = response.content_length() {
if content_length > MAX_CONTENT_LENGTH as u64 {
return Err(anyhow!(
"Response content length exceeded the limit of {MAX_CONTENT_LENGTH} bytes"
));
}
}

// 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)
})?;
Comment on lines +102 to +112
Copy link
Collaborator Author

@spacebear21 spacebear21 May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

@nothingmuch nothingmuch May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@spacebear21 spacebear21 Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. I'm not a big fan of the extra complexity/dependencies introduced by this StreamReader/SyncIoBridge approach.
  2. Like you said, the app implementer needs to be careful either way to avoid OOM bugs.
  3. We use &[u8] for the V2 Sender and Receivers process_response methods.


self.process_pj_response(psbt)?;
Ok(())
Expand Down
Loading