Include MASP Transaction hash in the wrapper signature

[murisi]

For any Tx that contains and uses a MASP Transaction (i.e. some field like Transfer::shielded_section_hash contains its TxId) , the Namada Ledger app must produce a wrapper signature that is also over the hash of the MASP Transaction section. (In addition to everything else put into

ledger-namada/app/src/crypto.c

Line 373 in ca9da5e

// Start generating wrapper signature

) Moreover the MASP Transaction section must be hashed only after the placeholders in it have been replaced by valid authorization signatures and a valid binding signature. The reason for this requirement is that the wrapper signature is intended to prevent malicious actors from tampering with any part of a Tx, including any Transaction embedded within it. The hash of a MASP Transaction section is computed as the SHA-256 hash of 0x04 (DISCRIMINANT_MASP_TX) followed by the Transaction bytes (i.e. the SHA-256 hash of the byte vector consumed by readMaspTx).

[chcmedeiros]

Hey @murisi! As we are just about to complete the shielded hash verification, we will start solving this issue, but we have some questions, where you might be able to help us.

So far, the signMasp method that we have is returning the hash of the complete transaction (similarly to zcash), and the spend signatures can be retrieved with the getSpendSignature method.

From our understanding, you are requesting the signMasp to return not a hash but the wrapper signature, similar to the one for non-masp transactions, including the Masp transaction.
But are not understanding what you mean by "Moreover the MASP Transaction section must be hashed only after the placeholders in it have been replaced by valid authorization signatures and a valid binding signature", can you help us make this clearer?
Will you use the spend signatures we produced to regenerate the final transaction for the wrapper signature?

[murisi]

Hi @chcmedeiros , great thanks for the work on the shielded hash verification issue!

From our understanding, you are requesting the signMasp to return not a hash but the wrapper signature, similar to the one for non-masp transactions, including the Masp transaction.

Conceptually a Tx can contain/wrap any data. The wrapped data might be a Bond or a (Transfer + MASP Transaction) or a (MsgTransfer + MASP Transaction) or a plain MsgTransfer for example. So I could imagine a single general app.sign function that takes a Tx (which might or might not contain a MASP Transaction) and returns the raw header signature and wrapper header signature. The getSpendSignature method could then augment this setup to allow retrieval of the spend signatures (if they are needed). In this way sign and getSpendSignature could cover all the signatures required by the client for both MASP and non-MASP cases. But I guess making signMasp return a wrapper signature could also work.

But are not understanding what you mean by "Moreover the MASP Transaction section must be hashed only after the placeholders in it have been replaced by valid authorization signatures and a valid binding signature", can you help us make this clearer?

Ultimately the wrapper signature must cover over every byte in the Tx in order to prevent transaction malleability. If the Tx contains a Transaction, then all the Transaction bytes including the zk-proofs, spend signatures, and binding signature should be covered by the wrapper signature to prevent malicious actors from invalidating a transaction by tampering with the MASP authorizations. We currently achieve this by including the MASP section hash in the wrapper signature (alongside the data section hash, code section hash, and other section hashes which are already implemented at

ledger-namada/app/src/crypto.c

Line 397 in 6e0ac0f

// Concatenate the code and data section hashes

). The MASP section hash is computed as SHA-256(0x04 || Borsh serialization of Transaction) (see https://github.com/anoma/namada/blob/24eebc7ec68c30f66f8c92a257f5d9acef75258f/crates/tx/src/types.rs#L878) where the concerned Transaction has valid authorizations (in the place of the dummy/placeholder authorizations contained by the Transaction that was sent to the hardware wallet for signing). (Side note: we used the Transaction Borsh serialization to compute the MASP section hash because it includes authorization data unlike the TxId digest.)

Will you use the spend signatures we produced to regenerate the final transaction for the wrapper signature?

Would it be possible for the hardware wallet to do this directly? It seems to me that we just need to patch/overwrite the dummy/placeholder spend signatures and binding signature in the Transaction object with the correct values that you have computed. The 64-byte gaps for the spend authorization signatures in the Transaction are already there, so this patching process should leave the Transaction serialization length unchanged. But maybe I'm missing something? Please let me know... Getting the computer to regenerate the final transaction is also a possibility, but this would introduce another round of communication, and I guess the hardware wallet would have to recheck this final transaction for consistency with all the data it has processed so far.

[chcmedeiros]

Hey @murisi ! Thanks for the clarification, I think it will be possible to do it in the app directly. But let me check with some more certainty and discuss with the team, so we can come with the best solution.

[chcmedeiros]

Hey @murisi ! I think we can do it without the need for an extra round of communication. Can you point us to a place in your SDK or documention where we can confirm the binding signature computation?

[murisi]

Hey @murisi ! I think we can do it without the need for an extra round of communication. Can you point us to a place in your SDK or documention where we can confirm the binding signature computation?

Hi @chcmedeiros , great thanks! See this code for computing the binding signature: https://github.com/anoma/masp/blob/8d83b172698098fba393006016072bc201ed9ab7/masp_proofs/src/sapling/prover.rs#L277 . At a higher level, the computations are also described here: https://zips.z.cash/protocol/protocol.pdf#saplingbalance although our code is generalized to support multiple asset generators. Let me know if you have any further questions...

[murisi]

Hi @chcmedeiros ! How is the binding signature computation going? Is there any additional information we can furnish you with for this issue? Thanks!

[chcmedeiros]

Hey @murisi ! We are working on computing the binding signature, so we can replace it by the dummy one.

[chcmedeiros]

Hey @murisi. We made some good progress. Do you think it is possible to have a MASP transaction, with some debug on the expected value of binding sig, so we could run some test in our implementation?
Or maybe some place in your SDK from which we could get some testcases for this.

[murisi]

Hi @chcmedeiros . In order to get the release done sooner, could we please postpone closing the issue? Essentially we've manage to get the namadac client to almost work with the combination of 86dffc6 , #62 , and #61 . (See anoma/namada#3797 ). The drawback with the approach (that sidesteps this issue) is that the user may have to confirm the Tx twice (which had been what we were trying to avoid): once to get the MASP signatures and the second time to get the wrapper signature. But I've checked with the team and confirmed that this user flow is fine for now and does not to be addressed urgently. If we postpone addressing the issue, then I believe the only blocking problems are that (1) the TxId digest computations of the hardware wallet sometimes diverge from that of namadac, and (2) the spend authorization signatures produced by the hardware wallet are being rejected by the validators.

[chcmedeiros]

Hey @murisi. But you will still need for us to add the Masp hash on the wrapper signature, right ?

[murisi]

Hey @murisi. But you will still need for us to add the Masp hash on the wrapper signature, right ?

Ah yes, you're right. The MASP hash is still needed in the wrapper signature. Thanks!

Conceptually a Tx can contain/wrap any data. The wrapped data might be a Bond or a (Transfer + MASP Transaction) or a (MsgTransfer + MASP Transaction) or a plain MsgTransfer for example. So I could imagine a single general app.sign function that takes a Tx (which might or might not contain a MASP Transaction) and returns the raw header signature and wrapper header signature. The getSpendSignature method could then augment this setup to allow retrieval of the spend signatures (if they are needed). In this way sign and getSpendSignature could cover all the signatures required by the client for both MASP and non-MASP cases. But I guess making signMasp return a wrapper signature could also work.

This is the part of this issue that we can leave out for now. Your current Rust crate API is fine and we have already integrated it. And the team is fine with requiring hardware wallet users to do separately the confirmation for the MASP Transactions and one for the wrapper (at least for now). Apologies for the confusion.

[chcmedeiros]

@murisi do you think is possible to have a transaction with the dummy values replaced with computed ones so we can have a test for MASP wrapper signature generation?

[murisi]

Hey @murisi. We made some good progress. Do you think it is possible to have a MASP transaction, with some debug on the expected value of binding sig, so we could run some test in our implementation? Or maybe some place in your SDK from which we could get some testcases for this.

I've reread the specifications and I think that I can make the Namada client namadac compute the binding signature before the call to signMasp. Its computation only requires knowledge of the randomness parameters and the Transaction sighash. And this sighash (different from TxId) does not depend upon the spend authorization signatures nor does it depend upon rsk since rk can be computed as rk = SpendAuthSigSapling.RandomizePublic(𝛼, ak). So could you please leave the dummy value for bindingSigSapling as it is for now? Apologies, I hadn't realized we had sufficient information for this signature computation to be done before the Transaction is sent to the hardware wallet.

It seems to me that only the spend authorization signatures for each spend description must be computed on the hardware wallet since that computation requires knowledge of ask (the spend authorizing key).

[murisi]

@murisi do you think is possible to have a transaction with the dummy values replaced with computed ones so we can have a test for MASP wrapper signature generation?

Yes, let me produce the test vectors for wrapper signatures that cover over MASP Transactions. I'll also separately produce test vectors for the test cases where TxId digest computations on the hardware wallet don't match those of namadac. Thanks.

[murisi]

I've just checked the debugs.txt testvectors that I sent before. These test vectors for the Txs containing MASP sections also contain the hashes of the MASP sections. For a given test vector, the hash for the MASP section can be found in the list under Tx { sections: [ Authorization( Authorization { targets:. If the MASP section (i.e. MaspTx) is the nth section in the sections vector, then its section hash should be at position n+1 in the above list. Also, ignore the MaspBuilder section hash, it shouldn't be computed by the hardware wallet nor should it be included in the wrapper signature.

Given that we will no longer be computing the binding signature on the hardware wallet, is any additional information still required to test MASP wrapper signature generation?

[chcmedeiros]

I don't think we need any additional info regarding that. Only the test vectors for the test cases where TxId digest computations on the hardware wallet don't match

[murisi]

Hi @chcmedeiros . Please also find attached test vectors to show some of the computations happening during the signing process. They look like:

apply_signatures: sighash_bytes: 69475d2b9988040d4e76b4dde81954ab54afc54affc6f48e93a618dbc0c21015
sign: t: 029265371f023246969c03e03eaf10b1cd6891f16946f83cbe0ec29e4b75308274227ca381f5c509f6b28cd66c677186f1b6455f3079dc5e26cd6c5341bf4d5bf217c1bb442c0cfa847fbd59254730f2
sign: r: c930e9c67af6f9c1c9cbd2b6f019f813ff5576657c84239215cfbae3b6051f04
sign: r_g: b2f073645fb16a3c0c1144340da27caa64ede92551e6bf8bf5dc2f392f2d49d6
sign: rbar: b2f073645fb16a3c0c1144340da27caa64ede92551e6bf8bf5dc2f392f2d49d6
sign: H*(Rbar || M): 769765eb80953697dd81efc724a3521f7da08b114ce1206906263bcb38244e00
sign: s: 467856038ac43b5fcb6da494e26d7375456d1083215a7257fb556062ec4b4408
sign: sbar: 467856038ac43b5fcb6da494e26d7375456d1083215a7257fb556062ec4b4408
spend_sig_internal: ask: 0cd47b62dea686adfd1e96f6503216cdf133e575221400fd65eeaa378f3d450b
spend_sig_internal: ar: 1f666be29fde619463f42658f1d161a77b0caddf7febbaef33b511756b9d3709
spend_sig_internal: sighash: 69475d2b9988040d4e76b4dde81954ab54afc54affc6f48e93a618dbc0c21015
spend_sig_internal: rsk: 740df06d1f775171de02f581aee30fce6c055e54a1c453e6f0f388471026ff05
spend_sig_internal: rk: 1918284580da467e5ec723ba86c345cdebf33e882b5c33702804799ac0393802
spend_sig_internal: data_to_be_signed: 1918284580da467e5ec723ba86c345cdebf33e882b5c33702804799ac039380269475d2b9988040d4e76b4dde81954ab54afc54affc6f48e93a618dbc0c21015
sign: t: 05734daa7328fa0c91a33d78b9c256c06d79b4b38fa01658d6e3c39781464323d166c95227c2fe5e121e87f7d5d82f712bf0f7a3fa23585543201936370b4562ae3a6604e1870c67cf232bc48fbe39ba
sign: r: 9357809b6204808e81dbed2d3327d4a84f23737ebb4136e915c7e8d287be4a01
sign: r_g: d7fbecb584e158c90a02d5ff5914ec729f86f119cee9ed03d1c3af438b30e333
sign: rbar: d7fbecb584e158c90a02d5ff5914ec729f86f119cee9ed03d1c3af438b30e333
sign: H*(Rbar || M): f133d071d7477f9bb510b74a7c9740db20605a58208d99bcb8eb12ca995edb06
sign: s: 9ea71021df2d6f1f8240350026cacf2e36ffeb6dbc2d8202de782a8fdc27b40a
sign: sbar: 9ea71021df2d6f1f8240350026cacf2e36ffeb6dbc2d8202de782a8fdc27b40a
Personalization Prefix: 5a636173685478486173685f
Consensus Branch ID: MASP
Consensus Branch ID: 3925833126
Personal: 5a636173685478486173685fa675ffe9
Header Digest: Hash(0x0d28515245442c4f2d26f7f584a8a8f6a05a523ed1e76889a521f827187e3e2c)
Transparent Digest: Hash(0xc33f2e95705faab35f8d533fa61e95c3b7aaba0776b874a9f74fc12784376a59)
Sapling Digest: 1ab0d393f113f6ea8a73914bbede894a9ee55405b9f0370efab29cede1f13780
TxId Digest: 1510c2c0db18a6938ef4c6ff4ac5af54ab5419e8ddb4764e0d0488992b5d4769

masp_signing_testvectors.txt

[chcmedeiros]

@murisi those Sign info is regarding the signature of a spend right? If so I will use this to test the spend signature generation

[murisi]

@murisi those Sign info is regarding the signature of a spend right? If so I will use this to test the spend signature generation

@chcmedeiros Yes exactly, thanks. The link properly formatted: masp_signing_testvectors.txt

[chcmedeiros]

Hey @murisi ! Regarding the spend signing, I was able to verify sbar and rbar calculation process, starting from the data_2_be_signed, rsk and t from your debug vectors. Going to check if the issue now might be on rsk, rk computation or sign_hash.
Meanwhile, I found that on the debug vectors you are not printing the correct s value, my guess is that you are printing the value of s_bar on the s. Not that important, but might be worth mentioning in the case you are using this for other stuff.

[murisi]

Hi @chcmedeiros . Great, thanks. Let me know if anything comes up...

Meanwhile, I found that on the debug vectors you are not printing the correct s value, my guess is that you are printing the value of s_bar on the s. Not that important, but might be worth mentioning in the case you are using this for other stuff.

Thanks. Looking at https://zips.z.cash/protocol/nu5.pdf#concretereddsa , s_bar seems to just be an encoding of s. Whereas looking at crypto.c, s seems to be defined as a sub-expression of s_bar (i.e. H⊛(𝑅 || vk || 𝑀 )). But I guess all is fine if the hardware wallet obtains the same sbar and rbar as the test vectors.

[murisi]

Hi @chcmedeiros . Whenever I try to sign MASP transactions I get the following error after pressing selecting/pressing "Approve" on the hardware wallet: Ledger | App Error: | 28417 apdu sign verify error. I assume the error emanates from the vicinity of crypto_check_masp. Some notes:

• I'm using the ZIP 32 (over SLIP 10) path: m/32'/877'/0' (from the mnemonic equip will roof matter pink blind book anxiety banner elbow sun young).
• The hardware wallet internally uses the extended spending key: 0x03bc4834ed00000080fe5f31e2d3bd521177e96903bafaeea1ff38afbfc87bc92ffd8bafa9f8f939c707bc9208dcd6e622a6aacab839c23c26404cd362a33a6fb5f9e9871b65652e09a1a8f2975e2d21edeac47b57f70ade8fff6ede55049d5be4a1c95dcbec47ed0c90790db1d8232c742fa3c4437e958276951958bb2392c9debd325f306582daa6ad0a2591b6c6ad459fa2d181005eeef982e378b807eb109275ae89f288f86697 .
• The client only knows the extended full viewing key generated from this: 0x03bc4834ed00000080fe5f31e2d3bd521177e96903bafaeea1ff38afbfc87bc92ffd8bafa9f8f939c7031b9d3d838fb0343ac264aeb78ba909e5a731cd7a3b4d601af55bfde40e2b6df0c1485f8201ab1675c00d1201cb18f0eea8c9e92d26189fc9738544c2e4143d90790db1d8232c742fa3c4437e958276951958bb2392c9debd325f306582daa6ad0a2591b6c6ad459fa2d181005eeef982e378b807eb109275ae89f288f86697.
• The client is also given the proof generation key by the hardware wallet (the nsk can be found inside the extended spending key and the ak inside the extended full viewing key).
• The attached vectors contain dummy spend authorization signatures since the client generated them without knowledge of the true spend authorizing keys.
• The nth SpendBuildParams is used in the construction of the nth spend description. Analogously, the same thing is true for the convert descriptions and output descriptions.
• Sometimes more randomness parameters are generated than are actually needed/used. This is intentional because these parameters are generated by namadac before it knows the exact constitution of the Transaction.

See attached some logs of the calls I'm making. Am I doing something wrong?
apdu_sign_verify_error.txt

[chcmedeiros]

Hey @murisi. So if I understood right, the methods to generate the randomness will be called more than the number of the spends/outputs/converts that will be present in the builder? If so, then the problem might be indeed in the crypto_check_masp when we are getting the randomness values to compute cv for comparison. We might not be getting the random vaue that namadac is using.

[murisi]

So if I understood right, the methods to generate the randomness will be called more than the number of the spends/outputs/converts that will be present in the builder?

Hi @chcmedeiros . Yes, this would make integrating the Ledger app into namadac easier.

If so, then the problem might be indeed in the crypto_check_masp when we are getting the randomness values to compute cv for comparison. We might not be getting the random vaue that namadac is using.

Ah, I see. My bad. Is there a way to make this work? I.e. allow namadac to generate M randomness parameters and then generate a Transaction with N descriptors where N<=M, and have the Ledger app ignore the last M-N randomness parameters (i.e. pretend that they were never requested/generated) when doing crypto_check_masp.

[chcmedeiros]

@murisi I am doing some testing to be sure. But I would say if you should use the first N randomness parameters to generate the transaction, I think this should work. And the app will not use the last M-N parameters, because in the check functions we are iterating over the number of spends/outputs/converts