mjg-foundation / passport2-monero

v2.x.x series of firmware for Passport, rebuilt for monero
Other
29 stars 3 forks source link

Formalize and Document Airgapped Signing Standard #13

Open mjg-foundation opened 1 year ago

mjg-foundation commented 1 year ago

I found that monero-wallet-cli supports cold signing, but the only references to it are here and monerodocs.org, which wouldn't load. If someone could dig into the exact formats (both unsigned and signed) this is using, and document it as clearly as done in BIP370 , we could start building a connection with the core wallet, and set the standard for communication in #3 and #6 Alternatively, a new, more transparent and efficient standard could be built.

Bounty: .5 XMR Bounty paid out when a formal document is included in a PR, with ratification from a dev who worked on the core wallet functionality, or signoff from a couple devs if we build a new standard. Bounty can be split among participants equally.

donttracemebruh commented 1 year ago

Adding 1 XMR to the bounty.

Monero-HackerIndustrial commented 1 year ago

Just wanted to add some context for the high level UX process for offline airgapped signing. Since this is the most commonly assumed and recommended setup for airgapped signing.

The assumptions in a typical flow:

The steps:

  1. Generate transaction on the hot wallet. The hot wallet generates a file called "unsigned_monero_tx" (Default name from the cli wallet)
  2. "unsigned_monero_tx" is transferred to the offline airgapped machine.
  3. Run "sign_transfer" from offline cold wallet. The tx details are displayed. User is prompted to sign. If they sign the wallet cli creates a file called "signed_monero_tx"
  4. "signed_monero_tx" is transferred to online machine. Run "submit_transfer" cli command. This uploads the transaction to the network.

The common recommendation is to use existing high level monero apis (directly or through language wrapper libraries). The monero python library for example uses rpc calls behind the scenes for the "sign_transfer_from" api call. This explains why much of the documentation is missing.

Another additonal thing to be aware of is the potential for protocol wide changes in monero that could potentially break wallets in the future. Unlike a lot of bitcoin development, monero has an ethos of upgrading and hardforking the network pretty frequently. Seraphis is a new protocol down the line and includes a new address scheme called "Jamtis". It is important to prepare wallet documentation to match what point in time the software was working with a specific "version" of the monero protocol. Here is a really good overview: https://github.com/monero-project/monero/issues/8157

kayabaNerve commented 1 year ago

It's magic bytes, a version, and then the epee serialization of a unsigned_tx_set (encrypted with the private view key).

epee is an internal binary format detailed here: https://github.com/jeffro256/serde_epee/blob/master/PORTABLE_STORAGE.md

The actual unsigned_tx_set struct is... quite large, containing multiple other quite large structs. It also isn't just the signing data for a specific TX. It additionally includes a update payload as to the wallet's state.

This really isn't designed to be some portable format. It's extremely specific to wallet2 internals. Also, the size of the unsigned transaction file may exceed the Passport's memory with how much data is thrown in here... Anyone want to make a dummy unsigned TX and report the file size?

mjg-foundation commented 1 year ago

I'm open to building a new standard, it just means that no one has anything to test against until the chicken (mobile wallet) and the egg (passport) both exist. If the standard is better, it would be worth it.

r4v3r23 commented 1 year ago

not sure there is a need for a "standard" - offline transactions work in a specific way in monero and the steps are relatively simple. any implemenation will be cross compatible. check these guides: https://docs.featherwallet.org/guides/offline-tx-signing & https://monero.stackexchange.com/a/2916

i suggest adding this bounty amount to the mobile implementation instead. working code is king

mjg-foundation commented 1 year ago

not sure there is a need for a "standard" - offline transactions work in a specific way in monero and the steps are relatively simple. any implemenation will be cross compatible. check these guides: https://docs.featherwallet.org/guides/offline-tx-signing & https://monero.stackexchange.com/a/2916

i suggest adding this bounty amount to the mobile implementation instead. working code is king

When I mean standard, I mean documentation of a format that many mobile wallets and airgapped wallets can agree on for interoperability. I think it's necessary, otherwise we'd have a ton of duplicated work doing the same thing slightly differently in each different implementation.

r4v3r23 commented 1 year ago

not sure there is a need for a "standard" - offline transactions work in a specific way in monero and the steps are relatively simple. any implemenation will be cross compatible. check these guides: https://docs.featherwallet.org/guides/offline-tx-signing & https://monero.stackexchange.com/a/2916 i suggest adding this bounty amount to the mobile implementation instead. working code is king

When I mean standard, I mean documentation of a format that many mobile wallets and airgapped wallets can agree on for interoperability. I think it's necessary, otherwise we'd have a ton of duplicated work doing the same thing slightly differently in each different implementation.

the only difference would be in the method used to transfer payloads between offline/online devices. the rest of the process is the exactly the same. there is nothing to "agree" upon, wallets either implement the correct process as its used in monero's wallet2 or not.

@kayabaNerve whats the average size of an unsigned_monero_tx file?

kayabaNerve commented 1 year ago

I'll specifically comment the Monero unsigned TX file is a kitchen sink format highly premised on internals which I can't recommend as a robust, portable format for security-critical environments.

https://github.com/mjg-foundation/passport2-monero/issues/6#issuecomment-1518440449 commented the data needed.

It's: 1) Inputs. This is 40 bytes + 16 * 64 each? I may be off by ~128 bytes? 2) Payments. These are ~80 bytes each.

For a 2-in, 2-out TX, there should be a minimally representable format which is less than 3kb (and accordingly fitting into a QR code).

The data which needs to be communicated back is the CLSAG, at 96 + (16 * 32) bytes (+- 32). That's 608 bytes per. Accordingly, two 3kb QR codes, one for the TX, one for the signatures, works.

As two other notes: 1) There is no requirement to implement the Monero spec exactly. Just because it is how wallet2 does it, doesn't make it correct nor the best way to. 2) I also don't know the size of an unsigned_monero_tx file. I'd expect it to be much larger than the minimal format being discussed here. I myself asked if someone else would mind providing that piece of data.

r4v3r23 commented 1 year ago

I'll specifically comment the Monero unsigned TX file is a kitchen sink format highly premised on internals which I can't recommend as a robust, portable format for security-critical environments.

#6 (comment) commented the data needed.

It's:

1. Inputs. This is 40 bytes + 16 * 64 each? I may be off by ~128 bytes?

2. Payments. These are ~80 bytes each.

For a 2-in, 2-out TX, there should be a minimally representable format which is less than 3kb (and accordingly fitting into a QR code).

The data which needs to be communicated back is the CLSAG, at 96 + (16 * 32) bytes (+- 32). That's 608 bytes per. Accordingly, two 3kb QR codes, one for the TX, one for the signatures, works.

As two other notes:

1. There is no requirement to implement the Monero spec exactly. Just because it is how wallet2 does it, doesn't make it correct nor the best way to.

2. I also don't know the size of an unsigned_monero_tx file. I'd expect it to be much larger than the minimal format being discussed here. I myself asked if someone else would mind providing that piece of data.

Just created a couple unsigned_monero_tx files:

Only the 1 input tx was able to fit onto a QR in feather, using UR can allow for larger transfers.

mjg-foundation commented 1 year ago

the only difference would be in the method used to transfer payloads between offline/online devices. the rest of the process is the exactly the same. there is nothing to "agree" upon, wallets either implement the correct process as its used in monero's wallet2 or not.

This is what I'm discussing. The format for transferring unsigned and signed transactions between the mobile and cold wallets are very important to agree on.

kayabaNerve commented 1 year ago

@r4v3r23 So if we want to support 2-input transactions encoded into a single QR code (which may not be needed, yet would be pleasant), we need a distinct format. Thanks for the info.

mjg-foundation commented 1 year ago

@r4v3r23 So if we want to support 2-input transactions encoded into a single QR code (which may not be needed, yet would be pleasant), we need a distinct format. Thanks for the info.

I'm hoping to always use UR for transaction signing, so no need to design for very limited transaction sizes.

kayabaNerve commented 1 year ago

If we're discussing a standard, I'd like to note the Passport's support for UR is pleasant, yet doesn't change how the underlying data should be formatted.

Monero-HackerIndustrial commented 1 year ago

This really isn't designed to be some portable format. It's extremely specific to wallet2 internals. Also, the size of the unsigned transaction file may exceed the Passport's memory with how much data is thrown in here... Anyone want to make a dummy unsigned TX and report the file size?

I made a test one a while back and it was 12184 bytes. Good point on the memory constraints of passport.

not sure there is a need for a "standard" - offline transactions work in a specific way in monero and the steps are relatively simple. any implemenation will be cross compatible. check these guides: https://docs.featherwallet.org/guides/offline-tx-signing & https://monero.stackexchange.com/a/2916

The attached feather docs outputs and key images need to be synced between online and offline machine. Do you think from a UX standpoint it is better to have the device only sign the transaction or create and sign it?

Only the 1 input tx was able to fit onto a QR in feather, using UR can allow for larger transfers.

Keep in mind that QR codes have different levels for different data density and error correction. https://www.qrcode.com/en/about/version.html

The 3k limit qr code might not be a good target for smaller devices and lower resolution screens/cameras. The QR codes most are used to using are going to be in levels 1-5. I was pushing it on level 7 to get about 1000 bytes. I think this one might be a bit too dense for smaller screens. You can try it out and generate a qr code and scanning it with a phone scanner (which has good cameras). It needs to be displayed on a large enough resolution. Here is some sample python code for what that would look like: https://github.com/Monero-HackerIndustrial/PortableMoneroQR/blob/main/POC/generateQR.py and how dense the image is. https://github.com/Monero-HackerIndustrial/PortableMoneroQR/blob/main/POC/test.png

The QR codes really need to be small enough to quickly scan with hardware. The larger QR levels with more data are really hard to get a slower camera to read. There are some optimizations and compression that could be made but it would still end up with multiple QR codes for unsigned tx files.

mjg-foundation commented 1 year ago

Multiple QR codes are fine. If the format needs to be so big it pushes us to a rediculous number of UR frames, it may be fine to use some form of compression.

kayabaNerve commented 1 year ago

To be clear, 12kb is well within the passport's memory. I was concerned about the transfer updates. Depending on how many transfers the wallet has...

From a security standpoint, the device should create the TX. Anything it doesn't do itself is an attack vector. Checking the data's integrity on the device is generally equivalent to creating the data itself.

r4v3r23 commented 1 year ago

To be clear, 12kb is well within the passport's memory. I was concerned about the transfer updates. Depending on how many transfers the wallet has...

From a security standpoint, the device should create the TX. Anything it doesn't do itself is an attack vector. Checking the data's integrity on the device is generally equivalent to creating the data itself.

i think we are all assuming a secure device. UX can dictate a review of final tx before broadcasting. do you have any proposals for improving offline txs data security?

kayabaNerve commented 1 year ago

do you have any proposals for improving offline txs data security?

Don't let the online computer decide any part of it. Else the UX has to show every single part of the TX/rebuild it to verify its legitimacy in creation, which is infeasible/pointless.

r4v3r23 commented 1 year ago

do you have any proposals for improving offline txs data security?

Don't let the online computer decide any part of it. Else the UX has to show every single part of the TX/rebuild it to verify its legitimacy in creation, which is infeasible/pointless.

it has to propose the tx for signing. whats wrong with a final confirmation before broadcast?

mjg-foundation commented 1 year ago

To be clear, 12kb is well within the passport's memory. I was concerned about the transfer updates. Depending on how many transfers the wallet has... From a security standpoint, the device should create the TX. Anything it doesn't do itself is an attack vector. Checking the data's integrity on the device is generally equivalent to creating the data itself.

i think we are all assuming a secure device. UX can dictate a review of final tx before broadcasting. do you have any proposals for improving offline txs data security?

The online wallet is not to be trusted. This is the model in passport, which is why we display tx information on the device before signing. If the user verifies the passport code, and verifies the transaction details, no trust is needed. Reviews of the final tx data on the online wallet are no good imo.

I prefer a format with minimal data as the "unsigned transaction" input to passport, and a complete, signed transaction as the output.

mjg-foundation commented 1 year ago

Let's move on to discussion of what the unsigned format is. The signed format should be the exact format of a monero transaction.

r4v3r23 commented 1 year ago

Let's move on to discussion of what the unsigned format is. The signed format should be the exact format of a monero transaction.

right, which goes back to my point of questioning any "standard". there is a certain way to create and sign offline txs in the cli wallet, which if implemented by other wallets will ensure interoperability.

kayabaNerve commented 1 year ago

I prefer a format with minimal data as the "unsigned transaction" input to passport, and a complete, signed transaction as the output.

I completely agree with this. Happy we can focus up to it.

My own work defined the following schema:

  pub fn new(
    protocol: Protocol,
    r_seed: Option<Zeroizing<[u8; 32]>>,
    inputs: Vec<SpendableOutput>,
    mut payments: Vec<(MoneroAddress, u64)>,
    change_address: Option<Change>,
    data: Vec<Vec<u8>>,
    fee_rate: Fee,
  ) -> Result<SignableTransaction, TransactionError> {

For the Passport, I'd propose the simplified:

  pub fn new(
    protocol: Protocol,
    inputs: Vec<SpendableOutput>,
    mut payments: Vec<(MoneroAddress, u64)>,
    data: Vec<Vec<u8>>,
    fee_rate: Fee,
  ) -> Result<SignableTransaction, TransactionError> {

where SpendableOutput is the information needed to spend an input and the ring to use with it.

  /// Absolute difference between the spend key and the key in this output
  pub key_offset: Scalar,
  pub commitment: Commitment,

for the output itself, where commitment is the mask + amount.

Then for the ring:

/// Decoy data, containing the actual member as well (at index `i`).
#[derive(Clone, PartialEq, Eq, Debug, Zeroize, ZeroizeOnDrop)]
pub struct Decoys {
  pub i: u8,
  pub offsets: Vec<u64>,
  pub ring: Vec<[EdwardsPoint; 2]>,
}

A consistency check between an input and its supplied ring does need to be performed.

Also, I understand these bits of Rust code aren't a formal standard/definition. I'm solely commenting what variables are needed.

r4v3r23 commented 1 year ago

To be clear, 12kb is well within the passport's memory. I was concerned about the transfer updates. Depending on how many transfers the wallet has... From a security standpoint, the device should create the TX. Anything it doesn't do itself is an attack vector. Checking the data's integrity on the device is generally equivalent to creating the data itself.

i think we are all assuming a secure device. UX can dictate a review of final tx before broadcasting. do you have any proposals for improving offline txs data security?

The online wallet is not to be trusted. This is the model in passport, which is why we display tx information on the device before signing. If the user verifies the passport code, and verifies the transaction details, no trust is needed. Reviews of the final tx data on the online wallet are no good imo.

I prefer a format with minimal data as the "unsigned transaction" input to passport, and a complete, signed transaction as the output.

like i said, its up to UX to display confirmation of tx details before spending

mjg-foundation commented 1 year ago

Let's move on to discussion of what the unsigned format is. The signed format should be the exact format of a monero transaction.

right, which goes back to my point of questioning any "standard". there is a certain way to create and sign offline txs in the cli wallet, which if implemented by other wallets will ensure interoperability.

The problems with this are that:

  1. The way the CLI does it isn't documented, so if we go with it, we should document it well for interoperability. This is would be considered a "standard".
  2. It seems like there are better ways of doing it being discussed, which could could lead to a better standard.

Either way, there will be a standard. "No standard" means no interoperability, just custom data encoding and decoding on every project. This is a nightmare imo and loses the gains of FOSS, where we can discuss and build something reliable and reusable by following a standard, whatever that may be. If you disagree with the standard we move forward with, making or using another standard is an option.

mjg-foundation commented 1 year ago

like i said, its up to UX to display confirmation of tx details before spending

I see, I thought you were talking about the hot wallet displaying the details after receiving the signed transaction from passport, rather than the passport displaying the details.

kayabaNerve commented 1 year ago

Even wallet2, right now, doesn't display the full details before signing AFAICT. The current unsigned_tx item has >10 fields when you flatten it. It's a mess to verify the consistency/integrity of it.

mjg-foundation commented 1 year ago

Even wallet2, right now, doesn't display the full details before signing AFAICT. The current unsigned_tx item has >10 fields when you flatten it. It's a mess to verify the consistency/integrity of it.

This is a problem we can deal with later in #6 , but we should display everything that can be verified, whether or not the user will. Let's get back to the topic.

Why are the r_seed and change_address fields omitted for passport?

kayabaNerve commented 1 year ago

Because they're behavior of general utility, which Serai uses, yet a Passport user has a 99% chance of never using.

r_seed determines the ephemeral keys in my work. If you fix it, you can acquire an honest-sender-outgoing-view-key. I use it to verify multisignature's wallets transactions. The Passport will likely want to use a randomly generated r_seed, or deterministically derive a r_seed (as you do need ephemeral keys to prove you made a certain payment, so having a way to export ephemeral keys without using storage would be nice).

A malicious computer providing a r_seed would allow it to create ephemeral keys, known by some third party, which then would break the privacy of who was sent to.

As for change address, the change address is almost always the wallet's own address (and you may lose privacy if it isn't). My own work just needs such finely grained coin control.

I did leave the data field, despite the fact that should likely be empty, as there is utility for it. My own project, a DEX, requires embedding data into transactions to inform the DEX of what to do with said TX. If Passport users wanted to swap their Monero for Bitcoin...

That last item is probably a bit out of scope for here specifically, but as we discuss standards, I appreciate the utility. I'd also note a general lack of similar utility behind the fields I removed.

It's also just very hard to, from a UX standpoint, approve an r_seed. They'd look completely random and go over the head of most users.

mjg-foundation commented 1 year ago

Makes sense to me. Could you specify all those fields down to primitives or well known structs, and their sizes? If someone else could then confirm that this format would work well as our standard, we could move on to documenting.

kayabaNerve commented 1 year ago
pub struct UnsignedTransaction {
    protocol: u8,
    inputs: Vec<(SpendableOutput, Ring)>,
    mut payments: Vec<(MoneroAddress, u64)>,
    data: Vec<Vec<u8>>,
    fee_rate: Fee, // There's a fee per weight component, u64, and a quantization mask, also u64
}

pub struct SpendableOutput {
  pub key_offset: Scalar, // The offset from the spend key used by this one time key
  pub commitment: Commitment, // Scalar mask and u64 amount
}

pub struct Ring {
  pub i: u8, // Index of signer
  pub offsets: Vec<u64>, // Global output indexes
  pub ring: Vec<[Point; 2]>, // Public key, commitment
}

where MoneroAddress is two public keys (64 bytes) and 1 or 9 bytes for the payment ID. The same byte that declares if it has a payment ID can also declare if it's a subaddress.

I'm unsure key_offset is best defined. It may be preferable to define it as (u32, u32), Scalar yet don't know at this time. It honestly depends on how nicely wallet2 plays with outputting raw key offsets...

Under protocol v16:

This creates a total size of 1 + 1 + (inputs 1225) + 1 + (outputs 73) + 1 + 16 = 1391 bytes for a 1-in, 2-out TX with no data and without using VarInts. The only consistency check needs is between the input and the ring for it (which can be optimized out, it's just annoying to have to insert into the middle of a ring).

Monero's format is 1.5x as large. The .5x extra from Monero is duplicated data, which would need consistency checking, and hot-wallet determined values, which would need legitimacy checking.

kayabaNerve commented 1 year ago

*This used 1-byte for the UnsignedTransaction vector lengths since Monero has a maximum of 16 outputs, and with 16 outputs, only ~210 inputs.

Monero-HackerIndustrial commented 1 year ago

From a security standpoint, the device should create the TX. Anything it doesn't do itself is an attack vector. Checking the data's integrity on the device is generally equivalent to creating the data itself.

Wanted to be clear what you meant. Would you consider verifying the unsigned tx on the device the same as "creating it on the device". I assume an unsigned tx would need to be verified and the user would have a chance to check the details [amount, to etc]

Don't let the online computer decide any part of it. Else the UX has to show every single part of the TX/rebuild it to verify its legitimacy in creation, which is infeasible/pointless.

I understand the benefit of constructing the tx on the device. Should we push that as the primary or the only way to do that? Right now the "unsigned_tx" file is already being used. It would bring parity with the reference client (monero_cli) to support signing it.

To be fair to this point:

i think we are all assuming a secure device. UX can dictate a review of final tx before broadcasting. do you have any proposals for improving offline txs data security?

The cli should output a hash checksum too. The file is encrypted to the signer pub key I believe.

@kayabaNerve If the unsigned tx is sent using the parameters instead of the whole "unsigned_tx" file then we might want to encrypt the value to the singer public key as a default setting. Since any QR code could give away decoys and weaken sender privacy. (Might be a bit paranoid)

This format should be a lot smaller data transfer as you have mentioned. What wallets can generate these formats as outputs directly?

I'll look into the format (serde_epee that you linked) to see about parsing the created file for some tools beyond the scope of this project. The "unsigned_tx" file format is not super well documented and relies on a large amount of internal monero knowledge which makes it hard to consider it "portable". The format you proposed can be merged into the monero_cli and offered as a "streamlined" unsigned_tx.

jeandudey commented 1 year ago

FWIW I wrote a serde wrapper for Monero's portable storage from epee a while ago on Rust:

https://github.com/xmr-rs/xmr/tree/master/portable-storage

I may say that for an embedded device that is going to perform signing of Monero transactions it won't be easy to work with due to memory constraints as the portable storage format assumes heavily that:

  1. The platform has lots of memory.
  2. The memory doesn't fragment.

It does a lot of small allocations and that fragments the memory a lot when de serializing it.

I think instead of using epee a format defined using CBOR could be made that could be transformed from a CBOR unsigned TX to a Monero unsigned TX deterministically, and also with a signed transaction, CBOR has been designed with embedded devices in mind, also allows doing zero copy de-serialization for large buffers/arrays.

One would also make available a CDDL description so people making wallets in any language can either generate the code for serialization/de-serialization, or just write it from the description. With IoT tech demanding CBOR libraries there will be support for a few languages, instead of using epee which one would have to make bindings to be used in other languages if that's possible.

Using CBOR also integrates nicely with the UR standards and we could talk to them to reserve a specific UR type for Monero with the CDDL description made.

kayabaNerve commented 1 year ago

@Monero-HackerIndustrial

Would you consider verifying the unsigned tx on the device the same as "creating it on the device".

To clarify exactly what I was referring to:

If the hot wallet decides the ephemeral one time keys, the hot wallet can cause the hardware wallet to burn funds. The only way to prevent this is to verify the one time keys are well formed. The easiest way to do so is to verify the one time keys is H(key images || secret). Calculating that hash to check against the provided one time key is pointless. If the hardware wallet already has to generate the one time key, why should it also have it passed in by the computer?

Verification of passed in TX components is almost always equivalent to locally creating those components and performing consistency checks. Accordingly, receiving them in the first place is pointless.


Monero does encrypt it with the private view key. I'd be fine with that being done here.


I would not support further usage of epee. I'd rather define a simple linear binary format. The only structural data needed is the amount of inputs, amount of outputs, amount of data segments, and the length of each data segment. Each of these can be simple, 1-byte prefixes.

jeandudey commented 1 year ago

The only structural data needed is the amount of inputs, amount of outputs, amount of data segments, and the length of each data segment. Each of these can be simple, 1-byte prefixes.

CBOR has a simple encoding format for byte sequences by using length prefixes that are variable in length, so it should cover that too.

r4v3r23 commented 1 year ago

any update on this?

mjg-foundation commented 1 year ago

any update on this?

Nope. This will pretty much be determined by whoever implements signing functionality in passport if it isn't decided here first.

mjg-foundation commented 1 year ago

It seems like someone deleted their bounty contribution, so I'll pitch in an additional 1 XMR myself to make the bounty label accurate.

mjg-foundation commented 1 year ago

I think as a first pass, we should try to use the same format as the CLI wallet that Anonero is now compatible with, and if this is inadequate for larger transactions in the future we could build a new format. Now this issue should be focused on documenting the format used by the CLI wallet, so work can start on implement signing with passport.