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

Ledger | App Error: | 28417 apdu sign verify error #66

Closed
murisi opened this issue Sep 18, 2024 · 15 comments
Closed

Ledger | App Error: | 28417 apdu sign verify error #66

murisi opened this issue Sep 18, 2024 · 15 comments

Comments

@murisi
Copy link
Contributor

murisi commented Sep 18, 2024

Whenever I try to sign MASP transactions on Ledger app v0.0.28 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
Copy link
Collaborator

Hey @murisi, one question regarding what we talked before, are you using the first N random values on the builder ?

@chcmedeiros
Copy link
Collaborator

I am trying to debug this on zemu, using the info from your debug file

@chcmedeiros
Copy link
Collaborator

@murisi I have one question requerdinf the process you followed:

  1. Generate randomness with spend/output/convert randomness methods;
  2. Generate the spends signatures using the sign_masp_spends;
  3. Extract the spends signatures;
  4. Sign complete masp blob with sign method;
    This error is appearing on step number 2 ?

@murisi
Copy link
Contributor Author

murisi commented Sep 18, 2024

Hey @murisi, one question regarding what we talked before, are you using the first N random values on the builder ?

Yes, I believe so. The nth spend description should be using the nth alpha and rcv values. See https://github.com/anoma/masp/blob/f2b0cae3e495e4f7d482e587432ec4e5f2793528/masp_primitives/src/transaction/components/sapling/builder.rs#L1000-L1010 . The nth convert description should be using the nth rcv value. See https://github.com/anoma/masp/blob/f2b0cae3e495e4f7d482e587432ec4e5f2793528/masp_primitives/src/transaction/components/sapling/builder.rs#L1053-L1058 . The nth output description should be using the nth rcm and rseed values. See: https://github.com/anoma/masp/blob/f2b0cae3e495e4f7d482e587432ec4e5f2793528/masp_primitives/src/transaction/components/sapling/builder.rs#L1088-L1094 . Though I'll recheck this code, especially its interaction with dummy notes.

@chcmedeiros
Copy link
Collaborator

I believe the problem might be in the computation of the cv values for converts. We didn't have transactions example with converts until now, and I can see the computed cv is invalid.

@murisi
Copy link
Contributor Author

murisi commented Sep 18, 2024

@murisi I have one question requerdinf the process you followed:

1. Generate randomness with spend/output/convert randomness methods;

2. Generate the spends signatures using the sign_masp_spends;

3. Extract the spends signatures;

4. Sign complete masp blob with sign method;

Hi @chcmedeiros . I believe that I've followed the above process. See: https://github.com/anoma/namada/blob/5a3b3c7af8276df74a457bb48edfcc0afb1894f5/crates/apps_lib/src/client/tx.rs#L934-L1067 . Breaking it down:

  1. Line 947 generates the spend randomness
  2. Line 958 generates the convert randomness
  3. Line 967 generates the output randomness
  4. Line 981 builds a MASP Tx based on the randomness parameters
  5. Line 1021 signs the MASP Transaction. This is where this issue's error is displayed after the user presses "approve"
  6. Line 1030 is not reached, but this is where the spend authorization signature is extracted
  7. Line 1067 is not reached, but this is where the sign method is called to sign the entire Tx with spend authorization signatures attached
   This error is appearing on step number 2 ?

Yes exactly, the error happens on step 2. The hardware wallet displays the entire MASP transaction along with "approve" and "reject" buttons. Pressing the "approve" button causes the CLI to display, "Ledger | App Error: | 28417 apdu sign verify error".

@murisi
Copy link
Contributor Author

murisi commented Sep 18, 2024

Hi @chcmedeiros . Also, I've integrated the clean_randomness_buffers as the step 0 here: https://github.com/anoma/namada/blob/aa8aaa022c3350b5b1df79bfee51b016eeb894b0/crates/apps_lib/src/client/tx.rs#L837 . This call seems to be working, but I just wanted to check whether I'm using it correctly since it takes two parameters: a BIP 44 path and a Tx blob. Since the Tx has not been built at this point, I supply it with a dummy Tx and path. Is this okay/correct usage of the function? How does the function use these two parameters?

@chcmedeiros
Copy link
Collaborator

My bad we did not need the blob! I will fix that input parameters.

@chcmedeiros
Copy link
Collaborator

@murisi need your input here on the cv computation for the converts. We should use the generator and value rigth ?

@murisi
Copy link
Contributor Author

murisi commented Sep 18, 2024

@murisi need your input here on the cv computation for the converts. We should use the generator and value rigth ?

@chcmedeiros I think it should be computed as cv = asset_generator*value + VALUE_COMMITMENT_RANDOMNESS_GENERATOR*rcv (similar to how it's done for spend and output descriptions). See https://github.com/anoma/masp/blob/8d83b172698098fba393006016072bc201ed9ab7/masp_primitives/src/convert.rs#L65 and https://github.com/anoma/masp/blob/8d83b172698098fba393006016072bc201ed9ab7/masp_primitives/src/sapling.rs#L194 .

  • asset_generator is here:
    CHECK_ERROR(readBytes(ctx, &tmp.ptr, tmp.len))
  • value is here:
    CHECK_ERROR(readUint64(ctx, &tmp_64))
  • rcv is generated by the hardware wallet when namadac calls NamadaApp::get_convert_randomness()
  • VALUE_COMMITMENT_RANDOMNESS_GENERATOR is the same as the one used to compute the value commitment for spend and output descriptions

Thanks!

@chcmedeiros
Copy link
Collaborator

Hey @murisi are you able to test using the fixes branch ?

@murisi
Copy link
Contributor Author

murisi commented Sep 20, 2024

Hey @murisi are you able to test using the fixes branch ?

Hi @chcmedeiros , thanks for looking into this. I've ran the test using the fixes branch, but the 28417 apsu sign verify error is still occurring at the same place. The issue seems to be in checkSpends. I'm currently rechecking the Transactions produced by namadac, perhaps there's an error in the computations there...

@chcmedeiros
Copy link
Collaborator

chcmedeiros commented Sep 20, 2024

@murisi The only check on the check spends that is not covered by zemu is the rk computation, but I have the computation being validated in cpp tests with some of your test vectors. Can this be a difference in the ask ?

@chcmedeiros
Copy link
Collaborator

We can try and remove the rk verification to check if indeed the issue is there. The cv comparison is similar to the converts and outputs, and at least in zemu it is working without problem.
I added the checks to check if the correct randomness values were indeed the ones being used.

@murisi
Copy link
Contributor Author

murisi commented Sep 20, 2024

@chcmedeiros Apologies, the remaining problem was on my side. In order to be able to test the Ledger app with namadac, I've been applying #61 on top of the fixes branch. (This is because the namada repo including the Transaction builder interface, CLI interfaces, and the software wallet are built around extended spending keys and extended full viewing keys.) However there was a bug in that PR that I've now fixed with 60ded6c . So this 28417 apdu sign verify error error is now gone, MASP transaction signing now works, and the signatures produced are accepted by node validation functions. I'll continue testing though... Thanks.

For context on the extended key issue, see the integration with the hardware wallet's key generation here: https://github.com/anoma/namada/blob/a3227fe8214c3d82f3ec15c42b1a11617ebd6dbf/crates/apps_lib/src/client/tx.rs#L848-L940 .

@chcmedeiros chcmedeiros mentioned this issue Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants