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

Payloads MUST be hashed using a cryptographic hash function #120

Open
nplasterer opened this issue Sep 27, 2023 · 4 comments
Open

Payloads MUST be hashed using a cryptographic hash function #120

nplasterer opened this issue Sep 27, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@nplasterer
Copy link
Contributor

You should not be able to sign non hashed payloads

payloads MUST be hashed using a cryptographic hash function as part of the ECDSA spec.
Some Crypto library API's, expected hashed strings so that implementors can choose which hash function to use in their cryptosystem
Allowing developers to misuse hash functions in our library is dangerous, and not inline with defensible code practices.
XMTP sdks should only expose functions which expect raw strings
XMTPv2 uses two different hashing functions, so explicit functions should be created to ensure that developers never invoke sign on unhashed data
Even though the underlying library prevents this in android this function https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/messages/PrivateKey.kt#L80-L95 needs to explicitly disallow being called without a hashed payload.

@nplasterer nplasterer added the enhancement New feature or request label Sep 27, 2023
@codingtosh
Copy link

Hey @nplasterer , I would like to pick this up. I'm a native android dev, have been away from open source for a year or so but looking to become active again.

I've set up the codebase locally and gone through some of your documentation/youtube videos to understand what the project is about. I'm ready to open my first PR here, but these two statements seem conflicting to me:

" Allowing developers to misuse hash functions in our library is dangerous, and not inline with defensible code practices. XMTP sdks should only expose functions which expect raw strings"

vs.

" https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/messages/PrivateKey.kt#L80-L95 needs to explicitly disallow being called without a hashed payload."

If we expect devs to provide a raw string, it is naturally going to be unhashed. We wouldn't want to disallow that, right?
My understanding is that we should expect the user to provide a raw string, and the method should hash it internally. Reasoning:

  1. Much smaller change surface. We can just flip the needToHash boolean to true. We can guarantee that the string will be hashed with a secure algo, which is SHA3 in this case.
  2. Afaik, we can't practically disntinguish between hashed and unhashed strings in order to disallow the unhashed ones. Even if we did, we can not guarantee that the hashing algo the dev used meets the desired security standards.

WDYT? Please let me know if I am missing out on something.

@nplasterer
Copy link
Contributor Author

@codingtosh that sounds great. I think on the surface the change is simple but it will be a bit more complicated. The sign function should really be internal and not used externally. Any place internally we call sign now will have to be modified. This will be a breaking change for any developers who might be calling it directly as well. I think diving in the direction you outlined sounds great. The test coverage should let you know if any issues arise. Ping me on the PR whenever you have it open!

@cdhiraj40
Copy link

@codingtosh that sounds great. I think on the surface the change is simple but it will be a bit more complicated. The sign function should really be internal and not used externally. Any place internally we call sign now will have to be modified. This will be a breaking change for any developers who might be calling it directly as well. I think diving in the direction you outlined sounds great. The test coverage should let you know if any issues arise. Ping me on the PR whenever you have it open!

My thought is to avoid introducing a breaking change immediately. Instead, I suggest we create a new function that follows the discussed specifications earlier. Alongside this, we can mark the existing function with an @deprecated annotation. This approach should allow for a smoother transition without immediately breaking code for the devs. What do you think? I will give this a try and open up a PR soon.

@nplasterer
Copy link
Contributor Author

@cdhiraj40 That sounds great. Agree deprecating it in advance would give the best experience. 👍

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

No branches or pull requests

3 participants