search

xmtp / xmtp-android

XMTP client SDK for Android applications written in Kotlin.
https://xmtp.github.io/xmtp-android/
MIT License
32 stars 12 forks source link

Payloads MUST be hashed using a cryptographic hash function #120

Open nplasterer opened 12 months ago

nplasterer commented 12 months ago

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.

codingtosh commented 9 months ago

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 commented 9 months ago

@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 commented 8 months ago

@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 commented 8 months ago

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