Skip to content
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.

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

Parse sender params #30

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion bip78/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ base64 = "0.13.0"
rand = { version = "0.8.4", optional = true }
bip21 = "0.1.1"
url = "2.2.2"
log = { version = "0.4.14", optional = true }

[dev-dependencies]
bitcoind = { version = "0.18.0", features = ["0_21_1"] }
env_logger = "0.9.0"
log = "0.4.14"
14 changes: 14 additions & 0 deletions bip78/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,21 @@ mod uri;
pub(crate) mod weight;
#[cfg(any(feature = "sender", feature = "receiver"))]
pub(crate) mod fee_rate;
#[cfg(any(feature = "receiver"))]
pub(crate) mod params;
#[cfg(any(feature = "sender", feature = "receiver"))]
pub(crate) mod psbt;

pub use uri::{Uri, PjUri, UriExt, PjUriExt, PjParseError};

#[cfg(feature = "log")]
macro_rules! log_warn {
($($args:tt)*) => { log::warn!($($args)*) }
}

#[cfg(not(feature = "log"))]
macro_rules! log_warn {
($($args:tt)*) => {}
}
pub(crate) use log_warn;

108 changes: 108 additions & 0 deletions bip78/src/params.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
use std::borrow::Borrow;
use std::fmt;

use crate::log_warn;
use crate::fee_rate::FeeRate;

#[derive(Debug)]
pub(crate) struct Params {
// version
// v: usize,
// disableoutputsubstitution
pub disable_output_substitution: bool,
// maxadditionalfeecontribution, additionalfeeoutputindex
pub additional_fee_contribution: Option<(bitcoin::Amount, usize)>,
// minfeerate
pub min_feerate: FeeRate,
}

impl Default for Params {
fn default() -> Self {
Params {
disable_output_substitution: false,
additional_fee_contribution: None,
min_feerate: FeeRate::ZERO,
}
}
}

impl Params {
#[cfg(feature = "receiver")]
pub fn from_query_pairs<K, V, I>(pairs: I) -> Result<Self, ParamsError>
where
I: Iterator<Item = (K, V)>,
K: Borrow<str> + Into<String>,
V: Borrow<str> + Into<String>,
{
let mut params = Params::default();

let mut additional_fee_output_index = None;
let mut max_additional_fee_contribution = None;

for (k, v) in pairs {
match (k.borrow(), v.borrow()) {
("v", v) => if v != "1" {
return Err(ParamsError::UnknownVersion)
},
("additionalfeeoutputindex", index) => {
additional_fee_output_index = match index.parse::<usize>() {
Ok(index) => Some(index),
Err(_error) => {
log_warn!("bad `additionalfeeoutputindex` query value '{}': {}", index, _error);
None
}
}
},
("maxadditionalfeecontribution", fee) => {
max_additional_fee_contribution = match bitcoin::Amount::from_str_in(&fee, bitcoin::Denomination::Bitcoin) {
Ok(contribution) => Some(contribution),
Err(_error) => {
log_warn!("bad `maxadditionalfeecontribution` query value '{}': {}", fee, _error);
None
}
}
}
("minfeerate", feerate) => {
params.min_feerate = match feerate.parse::<u64>() {
Ok(rate) => FeeRate::from_sat_per_vb(rate),
Err(e) => return Err(ParamsError::FeeRate(e)),
}
}
("disableoutputsubstitution", v) => params.disable_output_substitution = v == "true",
_ => (),
}
}

match (max_additional_fee_contribution, additional_fee_output_index) {
(Some(amount), Some(index)) => params.additional_fee_contribution = Some((amount, index)),
(Some(_), None) | (None, Some(_)) => { log_warn!("only one additional-fee parameter specified: {:?}", params); },
_ => ()
}
DanGould marked this conversation as resolved.
Show resolved Hide resolved

Ok(params)
}
}

#[derive(Debug)]
pub(crate) enum ParamsError {
UnknownVersion,
FeeRate(std::num::ParseIntError),
}

impl fmt::Display for ParamsError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
ParamsError::UnknownVersion => write!(f, "unknown version"),
ParamsError::FeeRate(_) => write!(f, "could not parse feerate"),
}
}
}

impl std::error::Error for ParamsError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self {
ParamsError::FeeRate(error) => Some(error),
_ => None,
}
}
}
1 change: 1 addition & 0 deletions bip78/src/receiver/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pub(crate) enum InternalRequestError {
InvalidContentType(String),
InvalidContentLength(std::num::ParseIntError),
ContentLengthTooLarge(u64),
SenderParams(crate::params::ParamsError)
}

impl From<InternalRequestError> for RequestError {
Expand Down
18 changes: 18 additions & 0 deletions bip78/src/receiver/mock.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
use std::collections::HashMap;

pub struct MockHeaders(HashMap<String, String>);

impl crate::receiver::Headers for MockHeaders {
fn get_header(&self, key: &str) -> Option<&str> {
self.0.get(key).map(|e| e.as_str())
}
}

impl MockHeaders {
pub fn from_slice(body: &[u8]) -> MockHeaders {
let mut h = HashMap::new();
h.insert("content-type".to_string(), "text/plain".to_string());
h.insert("content-length".to_string(), body.len().to_string());
MockHeaders(h)
}
}
176 changes: 169 additions & 7 deletions bip78/src/receiver/mod.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,42 @@
use bitcoin::util::psbt::PartiallySignedTransaction as Psbt;
use bitcoin::{Script, TxOut};
use bitcoin::{util::psbt::PartiallySignedTransaction as Psbt, AddressType, Script, TxOut};

use crate::params::Params;
mod error;

pub use error::RequestError;
use error::InternalRequestError;


pub trait Headers {
fn get_header(&self, key: &str) -> Option<&str>;
}

pub struct UncheckedProposal {
psbt: Psbt,
params: Params,
}

pub struct MaybeInputsOwned {
psbt: Psbt,
params: Params,
}

pub struct MaybeMixedInputScripts {
psbt: Psbt,
params: Params,
}

pub struct MaybeInputsSeen {
psbt: Psbt,
params: Params,
}

impl UncheckedProposal {
pub fn from_request(body: impl std::io::Read, query: &str, headers: impl Headers) -> Result<Self, RequestError> {
pub fn from_request(
body: impl std::io::Read,
query: &str,
headers: impl Headers,
) -> Result<Self, RequestError> {
use crate::bitcoin::consensus::Decodable;

let content_type = headers.get_header("content-type").ok_or(InternalRequestError::MissingHeader("Content-Type"))?;
Expand All @@ -37,28 +58,115 @@ impl UncheckedProposal {
let reader = base64::read::DecoderReader::new(&mut limited, base64::STANDARD);
let psbt = Psbt::consensus_decode(reader).map_err(InternalRequestError::Decode)?;

let pairs = url::form_urlencoded::parse(query.as_bytes());
let params = Params::from_query_pairs(pairs).map_err(InternalRequestError::SenderParams)?;

// TODO check params are valid for request Origianl PSBT

Ok(UncheckedProposal {
psbt,
params,
})
}

/// The Sender's Original PSBT
pub fn get_transaction_to_check_broadcast(&self) -> bitcoin::Transaction {
self.psbt.clone().extract_tx()
}

pub fn assume_broadcastability_was_verified(self) -> UnlockedProposal {
UnlockedProposal {
/// Call after checking that the Original PSBT can be broadcast.
///
/// Receiver MUST check that the Original PSBT from the sender
/// can be broadcast, i.e. `testmempoolaccept` bitcoind rpc returns { "allowed": true,.. }
/// for `get_transaction_to_check_broadcast()` before calling this method.
DanGould marked this conversation as resolved.
Show resolved Hide resolved
///
/// Do this check if you generate bitcoin uri to receive PayJoin on sender request without manual human approval, like a payment processor.
/// Such so called "non-interactive" receivers are otherwise vulnerable to probing attacks.
/// If a sender can make requests at will, they can learn which bitcoin the receiver owns at no cost.
/// Broadcasting the Original PSBT after some time in the failure case makes incurs sender cost and prevents probing.
///
/// Call this after checking downstream.
pub fn assume_tested_and_scheduled_broadcast(self) -> MaybeInputsOwned {
MaybeInputsOwned {
psbt: self.psbt,
params: self.params,
}
}

pub fn this_is_purely_interactive_wallet(self) -> UnlockedProposal {
UnlockedProposal {
/// Call this method if the only way to initiate a PayJoin with this receiver
/// requires manual intervention, as in most consumer wallets.
///
/// So-called "non-interactive" receivers, like payment processors, that allow arbitrary requests are otherwise vulnerable to probing attacks.
/// Those receivers call `get_transaction_to_check_broadcast()` and `attest_tested_and_scheduled_broadcast()` after making those checks downstream.
pub fn assume_interactive_receive_endpoint(self) -> MaybeInputsOwned {
MaybeInputsOwned {
psbt: self.psbt,
params: self.params,
}
}
}

impl MaybeInputsOwned {
/// The receiver should not be able to sign for any of these Original PSBT inputs.
///
/// Check that none of them are owned by the receiver downstream before proceeding.
Copy link
Owner

Choose a reason for hiding this comment

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

Separate into paragraphs.

pub fn iter_input_script_pubkeys(&self) -> Vec<Result<&Script, RequestError>> {
todo!() // return impl '_ + Iterator<Item = Result<&Script, RequestError>>
}

/// Check that the Original PSBT has no receiver-owned inputs.
/// Return original-psbt-rejected error or otherwise refuse to sign undesirable inputs.
///
/// An attacker could try to spend receiver's own inputs. This check prevents that.
/// Call this after checking downstream.
pub fn assume_inputs_not_owned(self) -> MaybeMixedInputScripts {
MaybeMixedInputScripts {
psbt: self.psbt,
params: self.params,
}
}
}

impl MaybeMixedInputScripts {
/// If there is only 1 input type, the receiver should be able to produce the same
/// type.
///
/// Check downstream before proceeding.
pub fn iter_input_script_types(&self) -> Vec<Result<&AddressType, RequestError>> {
todo!() // return Iterator<Item = Result<&AddressType, RequestError>>
}

/// Verify the original transaction did not have mixed input types
/// Call this after checking downstream.
///
/// Note: mixed spends do not necessarily indicate distinct wallet fingerprints.
/// This check is intended to prevent some types of wallet fingerprinting.
pub fn assume_no_mixed_input_scripts(self) -> MaybeInputsSeen {
Copy link
Owner

Choose a reason for hiding this comment

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

We can actually do this check internally, no need to burden the consumer with it. The consumer only needs to get one address type which is the same for all inputs and check that one.

Copy link
Author

Choose a reason for hiding this comment

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

Is this true? Can we stay up to date on all the possible bitcoin scripts?

Copy link
Owner

Choose a reason for hiding this comment

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

It's a good point that allowing people to manually check would allow them to work around the library not being updated but if we fail to maintain it they probably should fork it rather than using an obsolete library for potentially security-critical software.

Copy link
Author

Choose a reason for hiding this comment

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

it'd be better if the alpha version of this forgoes mixed input script check since we want to use it for loptos. Is it blocking this PR or may we leave it for the next iteration like some of the warns? I'd love for us to get loptos out ASAP

Copy link
Owner

Choose a reason for hiding this comment

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

Even if we skip the check AFAIK there are no wallets currently that support mixed inputs, so that's still useless. If you know of any wallet that does please let me know.

MaybeInputsSeen {
psbt: self.psbt,
params: self.params,
}
}
}

impl MaybeInputsSeen {
/// The receiver should not have sent to or received the Original PSBT's inputs before.
///
/// Check that these are unknown, never before seen inputs before proceeding.
pub fn iter_input_outpoints(&self) -> impl '_ + Iterator<Item=&bitcoin::OutPoint> {
self.psbt.global.unsigned_tx.input.iter().map(|input| &input.previous_output)
}

/// Make sure that the original transaction inputs have never been seen before.
/// This prevents probing attacks. This prevents reentrant PayJoin, where a sender
/// proposes a PayJoin PSBT as a new Original PSBT for a new PayJoin.
///
/// Call this after checking downstream.
pub fn assume_no_inputs_seen_before(self) -> UnlockedProposal {
UnlockedProposal { psbt: self.psbt }
}
}

pub struct UnlockedProposal {
psbt: Psbt,
}
Expand Down Expand Up @@ -113,3 +221,57 @@ pub struct NewOutputOptions {
subtract_fees_from_this: bool,
}

#[cfg(test)]
mod test {
use super::*;

struct MockHeaders {
length: String,
}

impl MockHeaders {
#[cfg(test)]
fn new(length: u64) -> MockHeaders {
MockHeaders { length: length.to_string() }
}
}

impl Headers for MockHeaders {
fn get_header(&self, key: &str) -> Option<&str> {
match key {
"content-length" => Some(&self.length),
"content-type" => Some("text/plain"),
_ => None,
}
}
}

fn get_proposal_from_test_vector() -> Result<UncheckedProposal, RequestError> {

// OriginalPSBT Test Vector from BIP
// | InputScriptType | Orginal PSBT Fee rate | maxadditionalfeecontribution | additionalfeeoutputindex|
// |-----------------|-----------------------|------------------------------|-------------------------|
// | P2SH-P2WPKH | 2 sat/vbyte | 0.00000182 | 0 |
let original_psbt = "cHNidP8BAHMCAAAAAY8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////AtyVuAUAAAAAF6kUHehJ8GnSdBUOOv6ujXLrWmsJRDCHgIQeAAAAAAAXqRR3QJbbz0hnQ8IvQ0fptGn+votneofTAAAAAAEBIKgb1wUAAAAAF6kU3k4ekGHKWRNbA1rV5tR5kEVDVNCHAQcXFgAUx4pFclNVgo1WWAdN1SYNX8tphTABCGsCRzBEAiB8Q+A6dep+Rz92vhy26lT0AjZn4PRLi8Bf9qoB/CMk0wIgP/Rj2PWZ3gEjUkTlhDRNAQ0gXwTO7t9n+V14pZ6oljUBIQMVmsAaoNWHVMS02LfTSe0e388LNitPa1UQZyOihY+FFgABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUAAA=";

let body = original_psbt.as_bytes();
let headers = MockHeaders::new(body.len() as u64);
UncheckedProposal::from_request(body,"?maxadditionalfeecontribution=0.00000182?additionalfeeoutputindex=0", headers)
}

#[test]
fn can_get_proposal_from_request() {
let proposal = get_proposal_from_test_vector();
assert!(proposal.is_ok(), "OriginalPSBT should be a valid request");
}

#[test]
fn unchecked_proposal_unlocks_after_checks() {
let proposal = get_proposal_from_test_vector().unwrap();
let unlocked = proposal
.assume_tested_and_scheduled_broadcast()
.assume_inputs_not_owned()
.assume_no_mixed_input_scripts()
.assume_no_inputs_seen_before();
}
}
Loading