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

Add clear message signing #74

Closed
bigspider opened this issue Oct 7, 2022 · 21 comments · Fixed by #226
Closed

Add clear message signing #74

bigspider opened this issue Oct 7, 2022 · 21 comments · Fixed by #226
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@bigspider
Copy link
Collaborator

bigspider commented Oct 7, 2022

The app can already sign arbitrary (binary) messages (using the legacy Bitcoin Message signing scheme that prepends "\x18Bitcoin Signed Message:\n" to the message), but only a hash of the message is shown.
We could improve the UX (and potentially the practical security for certain protocols) by showing the actual message if it's not very long and displayable as text.

Ideally, the user should be able to scroll back and forth if the message is long (but that could be non-trivial if we can't hold the entire message in memory; perhaps we can have different thresholds on the length for Nano S versus other devices).

@bigspider bigspider added enhancement New feature or request good first issue Good for newcomers labels Oct 7, 2022
@deepak7488
Copy link

Hey, I want to work on this issue. Can You Guide Me through the process for that?

@bigspider
Copy link
Collaborator Author

Hi @deepak7488, thanks for your interest in contributing! :)

Handler for sign_message: here
Current UI code: here

The state necessary for the UI will need to be added to the global variable here.
Because of memory limitations (it needs to still work on Nano S that has little RAM available), it's important not to increase the size of the ui_state_t structure, which might add some constraints when deciding the UX flows.

The handler could do something similar to this:

  • do a first pass on the entire message (while computing the hash), and check if all the characters are printable (for example the ASCII range between 0x20 and 0x7e should contain all the interesting characters).
  • if not printable, only show the hash (as it is today).
  • if printable, then:
    • if the length is short enough that it could fit all in memory (TBD), show message hash followed by the actual message as a single flow
    • otherwise, show the message in chunks, where at the end of each chunk you have "Continue"/"Reject" options, and at the last chunk you have the usual "Sign message"/"Reject" options as now.

When showing the message in chunks, it would be even better would be to be able to go to the next (or previous) chunk by just using the left/right buttons. However, this might require some digging into the UX framework of the sdk (which is notoriously not very developer-friendly...) to figure out how to do − and not sure there are use cases for very long messages anyway, at this time.

Hope this gives you enough information to get started, feel free to ask more questions!

@deepak7488
Copy link

As my semester exam is going on, I will work on this after 26 Nov. Thank You for your fast response and for giving me this information. I will be very happy to work on this.
Thank You.

@deepak7488
Copy link

Hey, I need to have physical nano s devices to develop this app, or I can do it on my mac os.

@bigspider
Copy link
Collaborator Author

Hey, I need to have physical nano s devices to develop this app, or I can do it on my mac os.

It's more convenient to work with the speculos emulator during development; here you shouldfind everything you need to get started!

@deepak7488
Copy link

Hey, Can you help me set up the development environment I am facing some issues?
you can arrange a Gmeet I will address my issue at that meeting. I am facing a problem with building and loading the app part. i.e., docker image.

@bigspider
Copy link
Collaborator Author

I recommend to join Ledger's developer Discord for these questions, easier to find help there: https://developers.ledger.com/discord-pro

@ef3n9r98
Copy link

Hi @bigspider, I know some devs who'd be interested in working on this. Is there a budget for fixing this issue, and if so, how much?

I'd put this issue up on OpenQ.

Thanks!

@bigspider
Copy link
Collaborator Author

Hi @ef3n9r98. Thanks for your interest, but at this time there are no plans to put on bounties for PRs on the bitcoin application.

@rxbryan
Copy link

rxbryan commented Feb 9, 2023

Hi @bigspider. I think there's enough information here for me to start working on this issue.
Can I get assigned to this?

@bigspider
Copy link
Collaborator Author

Hi @bigspider. I think there's enough information here for me to start working on this issue. Can I get assigned to this?

Sure, that would be great! Let me know if you have any questions on the tentative specs above.

You can use a software wallet like electrum to try the message signing feature, to get an idea of how it would be used in a software wallet.

@rxbryan
Copy link

rxbryan commented Mar 6, 2023

Hi @bigspider, Sorry for the delay. I've been quite busy.
I've got some questions.

if printable, then:
if the length is short enough that it could fit all in memory (TBD), show message hash followed by the actual message as a single flow

What's the memory size? The current UI code displays about 18 characters (9 bytes) of the message hash per screen. Is it safe to assume this limit.

otherwise, show the message in chunks, where at the end of each chunk you have "Continue"/"Reject" options, and at the last chunk you have the usual "Sign message"/"Reject" options as now.

Does this mean we forgo displaying the message hash, if the message is too large and just display the message only?

@rxbryan
Copy link

rxbryan commented Mar 6, 2023

Is ux_sign_message_flow a struct and where is this UX_FLOW macro defined ?

If I am correct that this function ux_flow_init executes the flow declared in the UX_FLOW macro. We would need to code a UX_FLOW that implements the UI from the specs above.

So where is UX_FLOW and ux_flow_init defined? so I could understand the code better.

@rxbryan
Copy link

rxbryan commented Mar 6, 2023

When showing the message in chunks, it would be even better would be to be able to go to the next (or previous) chunk by just using the left/right buttons. However, this might require some digging into the UX framework of the sdk (which is notoriously not very developer-friendly...) to figure out how to do − and not sure there are use cases for very long messages anyway, at this time.

ux_sign_message_flow already displays the hash in chunks so we can build on that. Can you refer me to the "UX framework of the sdk"?

@rxbryan
Copy link

rxbryan commented Mar 6, 2023

@bigspider I am testing signing message using the bitcoin_client in the repo. But I am not getting the option to sign or reject. I am just getting the 6985 denied by user response code.
here's the events

{"text": "Bitcoin Testnet", "x": 41, "y": 3}
{"text": "is ready", "x": 41, "y": 17}
{"text": "Sign", "x": 41, "y": 3}
{"text": "message", "x": 41, "y": 17}
{"text": "Path", "x": 52, "y": 3}
{"text": "44'/1'/0'/0/0", "x": 33, "y": 17}
{"text": "Message hash (1/4)", "x": 15, "y": 3}
{"text": "1BF9AD7CE49ADF6CB", "x": 7, "y": 17}
{"text": "Message hash (2/4)", "x": 15, "y": 3}
{"text": "C707A689B6E1765315", "x": 9, "y": 17}
{"text": "Message hash (3/4)", "x": 15, "y": 3}
{"text": "1E95C1CD8A53F9FCE5", "x": 7, "y": 17}
{"text": "Message hash (4/4)", "x": 14, "y": 3}
{"text": "4D3D51A2A24", "x": 26, "y": 17}
{"text": "Sign", "x": 41, "y": 3}
{"text": "message", "x": 41, "y": 17}
{"text": "Reject", "x": 45, "y": 19}
{"text": "Bitcoin Testnet", "x": 41, "y": 3}
{"text": "is ready", "x": 41, "y": 17}

@bigspider
Copy link
Collaborator Author

Hi @rxbryan, a caveat

The memory is already quite tight on Nano S (meaning: there is not much room to add more space in the global structures − in particular, should make sure to not increase the size of the g_ui_state variable).

The UX_FLOW and related macros are defined in the sdk (eg. here), but they are unfortunately very poorly documented.

Does this mean we forgo displaying the message hash, if the message is too large and just display the message only?

I'd say let's always show also the message hash, as it might be easier for some users (e.g. if they're using a third device to double-check the hash).

ux_sign_message_flow already displays the hash in chunks so we can build on that. Can you refer me to the "UX framework of the sdk"?

Yes, but the way that's written requires all the chunks to be in memory at the same time, which is not feasible if there are many.

@bigspider I am testing signing message using the bitcoin_client in the repo. But I am not getting the option to sign or reject. I am just getting the 6985 denied by user response code. here's the events

{"text": "Bitcoin Testnet", "x": 41, "y": 3}
{"text": "is ready", "x": 41, "y": 17}
{"text": "Sign", "x": 41, "y": 3}
{"text": "message", "x": 41, "y": 17}
{"text": "Path", "x": 52, "y": 3}
{"text": "44'/1'/0'/0/0", "x": 33, "y": 17}
{"text": "Message hash (1/4)", "x": 15, "y": 3}
{"text": "1BF9AD7CE49ADF6CB", "x": 7, "y": 17}
{"text": "Message hash (2/4)", "x": 15, "y": 3}
{"text": "C707A689B6E1765315", "x": 9, "y": 17}
{"text": "Message hash (3/4)", "x": 15, "y": 3}
{"text": "1E95C1CD8A53F9FCE5", "x": 7, "y": 17}
{"text": "Message hash (4/4)", "x": 14, "y": 3}
{"text": "4D3D51A2A24", "x": 26, "y": 17}
{"text": "Sign", "x": 41, "y": 3}
{"text": "message", "x": 41, "y": 17}
{"text": "Reject", "x": 45, "y": 19}
{"text": "Bitcoin Testnet", "x": 41, "y": 3}
{"text": "is ready", "x": 41, "y": 17}

It appears from this event log that the action was indeed rejected?

@bigspider
Copy link
Collaborator Author

A complication for this issue at this time is that it will conflict with the PR #126 that adds support to a new device with completely different UX system (as it's a large touchscreen).

@rxbryan
Copy link

rxbryan commented Mar 7, 2023

Yes, but the way that's written requires all the chunks to be in memory at the same time, which is not feasible if there are many.

We could create a new flow for displaying messages and pass 64 bytes at a time.

A complication for this issue at this time is that it will conflict with the PR #126 that adds support to a new device with completely different UX system (as it's a large touchscreen).

Looking through the code of the PR, It still does make use of the flows as the other models. We can use conditional compilation if the stax device doesn't have memory constraints and pass the message in one chunk.

@bigspider
Copy link
Collaborator Author

Looking through the code of the PR, It still does make use of the flows as the other models. We can use conditional compilation if the stax device doesn't have memory constraints and pass the message in one chunk.

Yes, changes are not drastic for non-stax devices, but I'd suggest waiting for that to be merged before working on this issue.
There's also #112 that is likely to conflict, but I didn't yet schedule when I would work on it, yet.

@rxbryan
Copy link

rxbryan commented Mar 10, 2023

Yes, changes are not drastic for non-stax devices, but I'd suggest waiting for that to be merged before working on this issue.

Alright.

There's also #112 that is likely to conflict, but I didn't yet schedule when I would work on it, yet.

I will remind you xd.

@yash013
Copy link

yash013 commented Mar 14, 2023

Can anyone help me getting started?? I want to contribute in this project not only on a single issue.
What should I do as a next step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants