search

LedgerHQ / app-near

Ledger repo for Near app
MIT License
11 stars 9 forks source link

feat: implement NEP-413 #30

Open medicz opened 10 months ago

medicz commented 10 months ago

Implements: https://github.com/near/NEPs/blob/master/neps/nep-0413.md

Pending automated tests.

Tested using https://github.com/LedgerHQ/speculos/tree/master

Screenshots below for Nano S+, ran emulator for all supported devices.

  1. Initial screen, message for recipient. Screenshot 2023-10-20 at 23 56 17
  2. Recipient, any string, but most probably URL or some account. Screenshot 2023-10-20 at 23 56 23
  3. Approve step Screenshot 2023-10-20 at 23 56 37
  4. Signed response Screenshot 2023-10-20 at 23 56 44
medicz commented 10 months ago

This code is unfamiliar to me, but I just want to verify that this solution provides no way to sign a payload that could possibly itself be a valid transaction; we can't sign any particular string, only ones that match our NEP413 format and where all required keys have values.

If it is possible for someone to send a payload that is a valid transaction, then this is a security problem as it would allow someone to craft a message sign request that could then be used to actually perform an action against the users account. We should have a test that proves that any payload not conforming to NEP413 exactly will be rejected, and that we are signing the entire payload; we cannot ignore any part of the NEP413 payload.

By the NEP-413 itself, it is impossible for any message to represent valid transaction.

That is achieved by the fact that first field of a transaction is signer_id, which, when borsch serialized, will start with it's length that is not longer than 64 bytes. Therefore, when the first 4 bytes represent number 2147484061, we know it can not be valid transaction.

More about it: https://github.com/near/NEPs/blob/master/neps/nep-0413.md#how-to-ensure-the-message-is-not-a-transaction.

That said, I completely agree about adding tests, and require help with how to properly do it in this codebase, which is why they are missing.

trechriron commented 10 months ago

A comment from the Ledger team;

Hi @firatNear , I just run the CI and quickly had a look to the PR. I feel like it's not that clear that we're signing a message when looking at the flow. Did you have a look to the Ethereum app EIP-191 implementation/flow ? Maybe you could align on a similar flow ?

dj8yfo commented 8 months ago

@medicz @firatNEAR if you want to discuss doing this pr in a rust version of this app, please give a shout in https://t.me/NEAR_Tools_Community_Group . I plan on porting it to a rust version anyway, so no action may be required on your part, apart from adjusting client code, as i plan to do this functionality under a separate cmd->ins (#define INS_NEP_413_MSG_SIGN 0x..)