Open timmc opened 4 years ago
I'd be up for adding some additional checks similar to https://github.com/terl/lazysodium-java/pull/66 but I wonder if there might be a more terse way of doing it. Maybe calls like assertArrayBounds(fooBytes, fooLength)
that would be implemented like
public void assertArrayBounds(byte[] array, int len) {
if(len < 0) throw new IllegalArgumentException("Out of bounds: Negative length");
if(len > array.length) throw new IllegalArgumentException("Out of bounds: Length greater than array length");
}
It might also make sense to switch some of the Checker stuff over to throw rather than validate, but I haven't looked closely at how that's currently used.
This looks to be an awesome suggestion!
Libsodium does do checks... sometimes... but it can't do everything 😀 Writing C libraries is a difficult endeavour. Porting those C libs to use in another language is even more difficult! I suppose what I'm trying (and failing) to say is that Lazysodium's job is to make up for Libsodium's downsides 🙂
If you could write some functionality in just as you suggested then I can merge it in. Feel free to decline and I'll add it to my to-do list 🙂
OK, I'll see if I can work up a PR!
Before I go too far down this road, does this kind of change look like something you'd be happy with? https://github.com/timmc/lazysodium-java/compare/test-fast-random...more-bounds-checks
Yes that's perfect 👍
I created a PR to add checks: https://github.com/terl/lazysodium-java/pull/95 However, it's failing on what I think is an existing incorrect test or implementation -- could you check it out?
I was getting VM crashes after some code changes where I messed up buffer sizes, and I've narrowed it down to this simplified repro case (in Kotlin):
It could probably be simplified more, but it looks like
cryptoSecretStreamPush
doesn't check byte array sizes before calling libsodium, and libsodium doesn't check either. I think what's going on is that if the message array is too large, libsodium ends up writing past the end of the cipher array and causing memory corruption. I suppose this is similar to issue #64.