Avoid overflows in JNI allocations

CHANGELOG.md

@@ -65,6 +65,8 @@ _Code:_
 
   - Kotlin is now officially supported language on Android
     ([#637](https://github.com/cossacklabs/themis/pull/637).
+  - Fixed a crash when decrypting corrupted Secure Cell data
+    ([#639](https://github.com/cossacklabs/themis/pull/639)).
 
   - **Breaking changes**
 

jni/themis_cell.c

@@ -127,6 +127,15 @@ JNIEXPORT jobjectArray JNICALL Java_com_cossacklabs_themis_SecureCell_encrypt(
         goto err;
     }
 
+    /*
+     * Secure Cell can contain up to 4 GB of data but JVM does not support
+     * byte arrays bigger that 2 GB. We just cannot allocate that much.
+     */
+    if (encrypted_data_length > INT32_MAX || additional_data_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        goto err;
+    }
+
     encrypted_data = (*env)->NewByteArray(env, encrypted_data_length);
     if (!encrypted_data) {
         goto err;
@@ -371,6 +380,15 @@ JNIEXPORT jbyteArray JNICALL Java_com_cossacklabs_themis_SecureCell_decrypt(
         goto err;
     }
 
+    /*
+     * Secure Cell can contain up to 4 GB of data but JVM does not support
+     * byte arrays bigger that 2 GB. We just cannot allocate that much.
+     */
+    if (data_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        goto err;
+    }
+
     data = (*env)->NewByteArray(env, data_length);
     if (!data) {
         goto err;

jni/themis_compare.c

@@ -83,6 +83,15 @@ JNIEXPORT jbyteArray JNICALL Java_com_cossacklabs_themis_SecureCompare_jniBegin(
         return NULL;
     }
 
+    /*
+     * Normally the messages should not be this big, but just in case, avoid
+     * integer overflow here. JVM cannot allocate more than 2 GB in one chunk.
+     */
+    if (compare_data_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        return NULL;
+    }
+
     compare_data = (*env)->NewByteArray(env, compare_data_length);
     if (!compare_data) {
         return NULL;
@@ -142,6 +151,15 @@ JNIEXPORT jbyteArray JNICALL Java_com_cossacklabs_themis_SecureCompare_jniProcee
         goto err;
     }
 
+    /*
+     * Normally the messages should not be this big, but just in case, avoid
+     * integer overflow here. JVM cannot allocate more than 2 GB in one chunk.
+     */
+    if (output_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        goto err;
+    }
+
     output = (*env)->NewByteArray(env, output_length);
     if (!output) {
         goto err;

jni/themis_keygen.c

@@ -56,6 +56,15 @@ JNIEXPORT jobjectArray JNICALL Java_com_cossacklabs_themis_KeypairGenerator_gene
         return NULL;
     }
 
+    /*
+     * Normally the keys should not be this big, but just in case, avoid
+     * integer overflow here. JVM cannot allocate more than 2 GB in one chunk.
+     */
+    if (private_key_length > INT32_MAX || public_key_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        return NULL;
+    }
+
     private_key = (*env)->NewByteArray(env, private_key_length);
     if (!private_key) {
         return NULL;
@@ -129,6 +138,15 @@ JNIEXPORT jbyteArray JNICALL Java_com_cossacklabs_themis_SymmetricKey_generateSy
         goto error;
     }
 
+    /*
+     * Normally the key should not be this big, but just in case, avoid
+     * integer overflow here. JVM cannot allocate more than 2 GB in one chunk.
+     */
+    if (key_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        return NULL;
+    }
+
     key = (*env)->NewByteArray(env, key_length);
     if (!key) {
         goto error;

jni/themis_message.c

@@ -118,6 +118,15 @@ JNIEXPORT jbyteArray JNICALL Java_com_cossacklabs_themis_SecureMessage_process(J
         goto err;
     }
 
+    /*
+     * Secure Message data format can store messages up to 4 GB long, but JVM
+     * cannot allocate more than 2 GB in one chunk. Avoid overflows here.
+     */
+    if (output_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        return NULL;
+    }
+
     output = (*env)->NewByteArray(env, output_length);
     if (!output) {
         goto err;

jni/themis_session.c

@@ -132,6 +132,14 @@ static int on_get_public_key_for_id(
         return THEMIS_FAIL;
     }
 
+    /*
+     * Normally peer IDs should not be this big, but just in case, avoid
+     * integer overflow here. JVM cannot allocate more than 2 GB in one chunk.
+     */
+    if (id_length > INT32_MAX) {
+        return THEMIS_NO_MEMORY;
+    }
+
     peer_id = (*(ctx->env))->NewByteArray(ctx->env, id_length);
     if (!peer_id) {
         return THEMIS_FAIL;
@@ -221,6 +229,15 @@ JNIEXPORT jbyteArray JNICALL Java_com_cossacklabs_themis_SecureSession_jniSave(J
         return NULL;
     }
 
+    /*
+     * Normally serialized Secure Session should not be this big, but just in case,
+     * avoid integer overflow here. JVM cannot allocate more than 2 GB in one chunk.
+     */
+    if (state_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        return NULL;
+    }
+
     state = (*env)->NewByteArray(env, state_length);
     if (!state) {
         return NULL;
@@ -291,6 +308,15 @@ JNIEXPORT jbyteArray JNICALL Java_com_cossacklabs_themis_SecureSession_jniWrap(J
         goto err;
     }
 
+    /*
+     * Secure Session protocol can handle messages up to 4 GB, but JVM does not
+     * support byte arrays bigger that 2 GB. We just cannot allocate that much.
+     */
+    if (wrapped_data_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        goto err;
+    }
+
     wrapped_data = (*env)->NewByteArray(env, wrapped_data_length);
     if (!wrapped_data) {
         goto err;
@@ -379,6 +405,15 @@ JNIEXPORT jobjectArray JNICALL Java_com_cossacklabs_themis_SecureSession_jniUnwr
         goto err;
     }
 
+    /*
+     * Secure Session protocol can handle messages up to 4 GB, but JVM does not
+     * support byte arrays bigger that 2 GB. We just cannot allocate that much.
+     */
+    if (data_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        goto err;
+    }
+
     data_array = (*env)->NewByteArray(env, data_length);
     if (!data_array) {
         goto err;
@@ -455,6 +490,15 @@ JNIEXPORT jbyteArray JNICALL Java_com_cossacklabs_themis_SecureSession_jniGenera
         goto err;
     }
 
+    /*
+     * Normally the connection request should not be this big, but just in case,
+     * avoid integer overflow here. JVM cannot allocate more than 2 GB at once.
+     */
+    if (request_length > INT32_MAX) {
+        res = THEMIS_NO_MEMORY;
+        goto err;
+    }
+
     request = (*env)->NewByteArray(env, request_length);
     if (!request) {
         goto err;

tests/themis/wrappers/android/com/cossacklabs/themis/test/SecureCellTokenProtectTest.java

@@ -259,8 +259,6 @@ public void detectExtendedData() {
 
 
     @Test
-    // FIXME(ilammy, 2020-05-05): resolve the bug in JNI code to unblock this test (T1607)
-    @Ignore("crashes on Android due to a bug in JNI code")
     @SuppressWarnings("ResultOfMethodCallIgnored")
     public void detectCorruptedToken() {
         SecureCell.TokenProtect cell = SecureCell.TokenProtectWithKey(new SymmetricKey());
@@ -278,16 +276,11 @@ public void detectCorruptedToken() {
             }
         }
 
-        // FIXME(ilammy, 2020-05-05): improve Themis Core robustness (T1604)
-        // Currently this call throws NegativeArraySizeException instead of SecureCellException
-        // because the Core library fails to detect corruption and returns negative buffer size
-        // which we cannot allocate, unfortunately.
-        // Expect "SecureCellException.class" here once the issue is resolved.
         try {
             cell.decrypt(encrypted, corruptedToken);
             fail("expected SecureCellException");
         }
-        catch (Throwable ignored) {}
+        catch (SecureCellException ignored) {}
     }
 
     @Test
@@ -327,8 +320,6 @@ public void detectExtendedToken() throws SecureCellException {
     }
 
     @Test
-    // FIXME(ilammy, 2020-05-05): resolve the bug in JNI code to unblock this test (T1607)
-    @Ignore("crashes on Android due to a bug in JNI code")
     @SuppressWarnings("ResultOfMethodCallIgnored")
     public void swapTokenAndData() {
         SecureCell.TokenProtect cell = SecureCell.TokenProtectWithKey(new SymmetricKey());
@@ -338,16 +329,13 @@ public void swapTokenAndData() {
         byte[] encrypted = result.getProtectedData();
         byte[] authToken = result.getAdditionalData();
 
-        // FIXME(ilammy, 2020-05-05): improve Themis Core robustness (T1604)
-        // Currently this call throws OutOfMemoryError instead of SecureCellException
-        // because the Core library fails to detect corruption and returns incorrect buffer size
-        // which we cannot allocate, unfortunately.
-        // Expect "SecureCellException.class" here once the issue is resolved.
         try {
             cell.decrypt(authToken, encrypted);
             fail("expected SecureCellException");
         }
-        catch (Throwable ignored) {}
+        // Depending on how lucky you are, Themis might or might not detect the error early enough.
+        // If it does not, it proceeds to allocate some weird buffer which might be too big.
+        catch (SecureCellException | OutOfMemoryError ignored) {}
     }
 
     @Test

tests/themis/wrappers/android/com/cossacklabs/themis/test/SecureCellTokenProtectTestKotlin.kt

@@ -220,8 +220,7 @@ class SecureCellTokenProtectTestKotlin {
         }
     }
 
-    @Test // FIXME(ilammy, 2020-05-05): resolve the bug in JNI code to unblock this test (T1607)
-    @Ignore("crashes on Android due to a bug in JNI code")
+    @Test
     fun detectCorruptedToken() {
         val cell = SecureCell.TokenProtectWithKey(SymmetricKey())
         val message = "All your base are belong to us!".toByteArray(StandardCharsets.UTF_8)
@@ -238,12 +237,7 @@ class SecureCellTokenProtectTestKotlin {
             }
         }
 
-        // FIXME(ilammy, 2020-05-05): improve Themis Core robustness (T1604)
-        // Currently this call throws NegativeArraySizeException instead of SecureCellException
-        // because the Core library fails to detect corruption and returns negative buffer size
-        // which we cannot allocate, unfortunately.
-        // Expect "SecureCellException.class" here once the issue is resolved.
-        assertThrows(Throwable::class.java) {
+        assertThrows(SecureCellException::class.java) {
             cell.decrypt(encrypted, corruptedToken)
         }
     }
@@ -282,8 +276,7 @@ class SecureCellTokenProtectTestKotlin {
         assertArrayEquals(message, decrypted)
     }
 
-    @Test // FIXME(ilammy, 2020-05-05): resolve the bug in JNI code to unblock this test (T1607)
-    @Ignore("crashes on Android due to a bug in JNI code")
+    @Test
     fun swapTokenAndData() {
         val cell = SecureCell.TokenProtectWithKey(SymmetricKey())
         val message = "All your base are belong to us!".toByteArray(StandardCharsets.UTF_8)
@@ -292,14 +285,12 @@ class SecureCellTokenProtectTestKotlin {
         val encrypted = result.protectedData
         val authToken = result.additionalData
 
-        // FIXME(ilammy, 2020-05-05): improve Themis Core robustness (T1604)
-        // Currently this call throws OutOfMemoryError instead of SecureCellException
-        // because the Core library fails to detect corruption and returns incorrect buffer size
-        // which we cannot allocate, unfortunately.
-        // Expect "SecureCellException.class" here once the issue is resolved.
-        assertThrows(Throwable::class.java) {
+        // Depending on how lucky you are, Themis might or might not detect the error early enough.
+        // If it does not, it proceeds to allocate some weird buffer which might be too big.
+        val e = assertThrows(Throwable::class.java) {
             cell.decrypt(authToken, encrypted)
         }
+        assertTrue(e is SecureCellException || e is OutOfMemoryError)
     }
 
     @Test