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

handleSignRawBytes signs CID instead of bytes directly #172

Open
hugomrdias opened this issue Nov 12, 2024 · 10 comments
Open

handleSignRawBytes signs CID instead of bytes directly #172

hugomrdias opened this issue Nov 12, 2024 · 10 comments

Comments

@hugomrdias
Copy link

hugomrdias commented Nov 12, 2024

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

@eshon
Copy link

eshon commented Nov 12, 2024

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

@hugomrdias
Copy link
Author

hugomrdias commented Nov 14, 2024

More considerations (may be beyond Zondax/Ledger scope):

  • Should be strict with the prefix ? https://eips.ethereum.org/EIPS/eip-191
    • \x19Ethereum Signed Message:\n "0x19 <0x45 (E)> <thereum Signed Message:\n" + len(message)> "
    • \x19Filecoin Signed Message:\n "0x19 <0x46 (F)> <ilecoin Signed Message:\n" + len(message)> "
    • Filecoin Sign Bytes:\n current prefix
  • If its EIP-191 should it required Expert mode enabled ?
  • Should we allow unprefixed signing like lotus and filsnap or should we support EIP-191 there too?
    • This is relevant for other signatures for things like UCANs and others.

/cc @eshon we may need to look into this further

@eshon
Copy link

eshon commented Nov 14, 2024

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
sign(blake2b256(CID(blake2b256("Filecoin Sign Bytes:\n" + len(message) + message)))))

@eshon
Copy link

eshon commented Nov 14, 2024

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?

  • Change the current Filecoin Sign Bytes:\n to \x19Filecoin Signed Message:\n?

Future:

  • Regarding unprefixed signing to match Lotus, I believe Zondax raised a security concern about this earlier so let's collect use cases first and file a new issue.

@sourajyoti-gupta
Copy link

Hi @hugomrdias , thanks for letting us know that. We'll shortly be investigating this and get back to you with some any updates. Thanks!

@ribasushi
Copy link

@eshon @hugomrdias note that the choice of 0x19 is valid for Eth making it a clearly-invalid RLP, but it does not have the same effect in a CBOR context.

Something to consider when determinining what negative-domain-separator to use.

cc @magik6k @rvagg as somewhat in their wheelhouse

@hugomrdias
Copy link
Author

hugomrdias commented Nov 26, 2024

Yes, but the full prefix has the same effect no? A normal tx cbor buffer could never verify against a signature like this.
And if ever a bad msg comes in you can lookup the first couple of bytes and check for eip191 and throw

@eshon
Copy link

eshon commented Dec 4, 2024

signDataCap can be removed, confirmed with FIDL (they are moving away from it anyway)

@hugomrdias
Copy link
Author

filecoin-project/lotus#12761 discussion on the lotus repo

@snissn
Copy link

snissn commented Dec 13, 2024

Thoughts on the Proposed Changes

Regarding the initial suggestions:

signDataCapRemove the extra CID operation (currently not used but might be useful in the future)
signRawBytesRemove the extra CID operation

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

  • Current:

    sign(blake2b256(CID(blake2b256("Filecoin Sign Bytes:\n" + len(message) + message))))
    
  • Proposed:

    sign(blake2b256("\x19Filecoin Signed Message:\n" + len(message) + message))
    

Simplifying to a single blake2b256 hash makes sense, but ensuring compatibility is key.

\x19 Prefix Consideration

In Ethereum, the \x19 prefix helps prevent signed messages from being interpreted as transactions. @ribasushi, you mentioned this might not apply in Filecoin — am I understanding that correctly? If there’s a Filecoin-equivalent safeguard to prevent misinterpretation of signed messages, incorporating it into a standard would be beneficial.

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

5 participants