Lack of length checking causes segfaults

[erikvanzijst]

I ran into issues where an application bug passed an invalid value (-1) for inLen to cryptoGenericHashUpdate(byte[] state, byte[] in, long inLen) and since LazySodium passes this on to libsodium verbatim, it takes out the entire JVM with a segfault.

# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007fff7d789db0, pid=16305, tid=0x0000000000002503
#
# JRE version: OpenJDK Runtime Environment (8.0_232-b09) (build 1.8.0_232-b09)
# Java VM: OpenJDK 64-Bit Server VM (25.232-b09 mixed mode bsd-amd64 compressed oops)
# Problematic frame:
# C  [libsystem_platform.dylib+0x1db0]  _platform_memmove$VARIANT$Haswell+0xf0
#
# Failed to write core dump. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again

Stack: [0x000070000682f000,0x000070000692f000],  sp=0x000070000692daf0,  free space=1018k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
C  [libsystem_platform.dylib+0x1db0]  _platform_memmove$VARIANT$Haswell+0xf0
C  [libsodium.dylib+0xfa65]  crypto_generichash_blake2b__update+0x54
C  [jna6713841504807624253.tmp+0xde74]  ffi_call_unix64+0x4c
C  0x000070000692e6a0

Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
j  com.goterl.lazycode.lazysodium.Sodium.crypto_generichash_update([B[BJ)I+0
j  com.goterl.lazycode.lazysodium.LazySodium.cryptoGenericHashUpdate([B[BJ)Z+8
j  ghost.devops.security.Signing$.main([Ljava/lang/String;)V+492
j  ghost.devops.security.Signing.main([Ljava/lang/String;)V+4
v  ~StubRoutines::call_stub

Would you be open to a patch that adds bounds checking to array offset/length values? It seems an unnecessary risk for a Java level offset bug to crash the entire JVM.

I was think something to the effect of:

    @Override
    public boolean cryptoGenericHashUpdate(byte[] state, byte[] in, long inLen) {
        if (inLen < 0 || inLen > in.length) {
            throw new IllegalArgumentException("inLen out of bounds: " + inLen);
        }
        return successful(getSodium().crypto_generichash_update(state, in, inLen));
    }

Should there be concerns around performance, we could also use assert so it could be disabled in production.

[gurpreet-]

@erikvanzijst you're right, there does need to be some sort of checking going on especially in libraries that bind to C libraries. As you've found out, the result is very dangerous and results in a whole Java runtime going down.

Lazysodium's goals are to be as stress-free, cross-platform and functional as possible. I am not necessarily too concerned with performance, however I have designed it with JNA so it is indeed as performant too 🙂

[erikvanzijst]

Cool. Mind if I take a first stab at that?

[gurpreet-]

Yeah sure, PRs are always welcome :)

[timmc]

Is this also something that should be reported upstream to libsodium? I don't know the details of this bug in particular, but memory corruption issues in general are a huge security risk.