Overlong message chunk in secret stream push causes "double free or corruption" VM crash

[timmc]

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):

fun repro() {
    val sodium: LazySodiumJava by lazy { LazySodiumJava(SodiumJava()) }
    val STREAM_CHUNK_BYTES = 4000

    val key = (sodium as SecretStream.Lazy).cryptoSecretStreamKeygen()
    val stream = (sodium as SecretStream.Native)
    val state = SecretStream.State()
    val header = ByteArray(SecretStream.HEADERBYTES)
    stream.cryptoSecretStreamInitPush(state, header, key.asBytes)
    || throw IOException("error")
    val cipherChunk = ByteArray(STREAM_CHUNK_BYTES + SecretStream.ABYTES)

    // This triggers the bug
    val msgChunkTooBig = ByteArray(STREAM_CHUNK_BYTES + 2000)

    // Bad call
    stream.cryptoSecretStreamPush(
        state, cipherChunk,
        msgChunkTooBig, msgChunkTooBig.size.toLong(),
        SecretStream.TAG_MESSAGE
    ) || throw IOException("error")
}

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.

[timmc]

I'd be up for adding some additional checks similar to #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.

[gurpreet-]

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 🙂

[timmc]

OK, I'll see if I can work up a PR!

[timmc]

Before I go too far down this road, does this kind of change look like something you'd be happy with? timmc/lazysodium-java@test-fast-random...more-bounds-checks

[gurpreet-]

Yes that's perfect 👍

[timmc]

I created a PR to add checks: #95 However, it's failing on what I think is an existing incorrect test or implementation -- could you check it out?