terl / lazysodium-java

A Java implementation of the Libsodium crypto library. For the lazy dev.
https://github.com/terl/lazysodium-java/wiki
Mozilla Public License 2.0
135 stars 48 forks source link

Kdf addnl checks #74

Closed rmtmckenzie closed 4 years ago

rmtmckenzie commented 4 years ago

I just got caught up in a difficult to find and (would have been very impactful if I didn't find it just before release) bug because if you provide a master key less than MASTER_KEY_LENGTH to kdf it appears to give back random values, so I thought it would be a good idea to add checks for everything being the right length.

My bad on the auto-format, I forgot to turn it off. If you want me to undo it before this is accepted just let me know, but I figure since it's mostly whitespace anyways it should be okay.

gurpreet- commented 4 years ago

This raises a valid follow-up question.. Should I implement checks for all functions like key derivation or shall I leave it up to the end user? 🤔

It would pollute a lot of the code but would make it much more resilient for consumers.

rmtmckenzie commented 4 years ago

That is a good question actually. There's probably a pretty good argument for saying that if you're using the "raw" composition, you should know better. TBH I have no idea why I didn't read the documentation which clearly states there is a length to use - for encryption I wouldn't ever have considered using a shorter key. One thing that would be convenient is if the cryptoKdfKeygen function simply returned a byte[] of the right length instead of having to pass an array in, although I see how that would conflict with the one that returns a Key (and realistically it'd be easy enough to simply use that anyways except that it's not exposed through the same interface).

Maybe a good starting point would be to either figure out a way of replacing any fixed-length key generation functions with a no-argument version (I guess renaming them?) or at least doing a check for the length in them - that way you're not polluting each crypto function with additional checks but people who are probably doing the most common use-case (making a key with the keyGen function) are still protected.