-
Notifications
You must be signed in to change notification settings - Fork 17
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
handleSignRawBytes signs CID instead of bytes directly #172
Comments
I had wondered if this might be related to signDataCap and signClientDeal, methods for Fil+, but Hugo pointed out that these have separate methods: https://github.com/LedgerHQ/app-filecoin/blob/develop/app/src/apdu_handler.c#L540-L559 |
More considerations (may be beyond Zondax/Ledger scope):
/cc @eshon we may need to look into this further |
FYI - I'm asking the Fil+ team how this might impact their current workflows here: https://filecoinproject.slack.com/archives/C01DLAPKDGX/p1731581852642289 Update: the Fil+ team would also like the extra CID operation below removed to match Lotus |
Summary of next steps from the Fil+ thread. Can you please:
In addition, can we modify the prefix above to match EIP-191 as Hugo is suggesting?
Future:
|
Hi @hugomrdias , thanks for letting us know that. We'll shortly be investigating this and get back to you with some any updates. Thanks! |
@eshon @hugomrdias note that the choice of Something to consider when determinining what negative-domain-separator to use. |
Yes, but the full prefix has the same effect no? A normal tx cbor buffer could never verify against a signature like this. |
|
filecoin-project/lotus#12761 discussion on the lotus repo |
Thoughts on the Proposed ChangesRegarding the initial suggestions:
These changes seem logical, as reducing unnecessary operations simplifies the process. However, do these modifications impact any live apps or products in the ecosystem? Breaking user flows is a concern, and if we decide to move forward, a migration plan would be prudent — including auditing current users, notifying stakeholders, setting timelines, and coordinating the transition. Current vs. Proposed Signing Format
Simplifying to a single
|
As a follow up to our slack thread im moving the discussion here.
From my understanding the ledger app is trying to follow eip191 which does :
sign(keccak256("\x19Ethereum Signed Message:\n" + len(message) + message)))
With that in mind for filecoin it should do
sign(blake2b256("\x19Filecoin Signed Message:\n" + len(message) + message))).
But currently it does
sign(blake2b256(CID(blake2b256("Filecoin Sign Bytes:\n" + len(message) + message)))))
And the cid has the same tags as the lotus msg signing cbor blake2b but here for raw bytes the payload is not necessarily cbor. In fact the point of following eip191 is to protect against accidental signing transactions.
Lotus cmd
lotus wallet sign
signs the bytes directly without eip191 prefix or CID(payload)https://github.com/filecoin-project/lotus/blob/master/cli/wallet.go#L460
https://github.com/filecoin-project/lotus/blob/3d0112e47178541038f32d6ee42014d42bf36f00/node/impl/full/wallet.go#L46
https://github.com/filecoin-project/lotus/blob/master/api/api_wallet.go#L15
IMO the extra CID does not add any value and could be avoided as for eip191 seems useful for its own reasons (avoid accidental transactions signing) and maybe it should be used in Lotus and other wallets.
To end, is it even worth it to change this at this point? If not we should at least document this somewhere visible.
/cc @eshon @ribasushi
🔗 zboto Link
The text was updated successfully, but these errors were encountered: