APIs that allow secure handling of secrets
Closed this issue · 5 comments
Right now, many APIs dealing with potentially secret inputs simply use String or byte[]. This is a security issue, for a number of reasons:
- When calling libsodium functions with signatures like
void foo(byte[] ptr);through jnr-ffi, the byte array will be copied. (Appropriate use of@Inand@Outprevents superfluous copying, but copying still happens by default.) - Zeroing a
byte[]requires conscious thought and could be optimized away: by definition, that is the last thing I'll ever do with thatbyte[]. It turns out that you have to do ridiculous things to reliably zero an array in C. Zeroing aStringis impossible, since it is an immutable value. (Unsafemay be able to fix this, but that sounds like a crazy, crazy hack; not something we can expect consumers of kalium to do, and maybe not even something we can reasonably expect kalium itself to do and provide as an API. It's not even publicly documented.) - Even if we could somehow securely and reliably zero both
Stringandbyte[], the JVM is free to (and often does) copy those things all over memory as much as it wants for e.g. garbage collection purposes.
I've contacted Wayne Meissner, the genius behind jnr-ffi about this, and we've discussed ways of fixing this. When you make jnr-ffi calls of type void foo(Pointer ptr); (Pointer from jna) or void foo(ByteBuffer ptr); then it will actually use that pointer's memory location or that buffer's memory location. Normally, ByteBuffer has similar problems to byte[] for our purposes; but if we use ByteBuffer.allocateDirect, we should be fine.
Since this is a serious issue that affects me (and I think everyone who uses kalium with secret data right now, which is probably approximately everyone), I'd be more than happy to:
- Figure out the specifics of how to implement this.
- Actually do the work for implementing this for at least
secretboxandgenerichash. - Carry that work over to caesium.
Some changes I'm suggesting:
- New API that takes a
ByteBuffer, and an API that makes theByteBufferand then passes it to that first API. - Explicitly document the concerns I noted above.
- Make the secure APIs the default, possibly even deprecate the insecure ones.
Does that make any sense? How do you feel about this suggestion?
(To be clear, the main guidance I am looking for here is if it is okay to break APIs or not for security bugs, and, if so, if this counts as a security bug.)
@lvh sorry for the long delay, atm I'm reviewing your PR. I think all the arguments are fair enough to break the compatibility or maybe deprecate some of the methods.
Don't hold your breath on it and go ahead. Thanks for the outstanding help.
Cool! Sorry from the delays from my end as well; I am moving halfway across the world so I have been super busy. We'll have to put this in the fridge until probably at least January. Sorry!