Merge pull request #33 from canonical/fix-free-crashes

Fix a possible cause of crashes in free()
canonical · Sep 18, 2024 · 6e78a57 · 6e78a57
2 parents 38946a1 + 53fbc70
commit 6e78a57
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 48 deletions.
diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml
@@ -47,4 +47,6 @@ jobs:
       if: always()
       with:
         name: maven-surefire-reports
-        path: ${{ github.workspace }}/target/surefire-reports
+        path: |
+          ${{ github.workspace }}/target/surefire-reports
+          ${{ github.workspace }}/build/test/test.out
diff --git a/pom.xml b/pom.xml
@@ -172,9 +172,6 @@
                     <includes>
                         <include>**/*Test.java</include>
                     </includes>
-                    <excludes>
-                       <exclude>**/*SecureRandomTest.java</exclude>
-                    </excludes>
                 </configuration>
             </plugin>           
         </plugins>

diff --git a/src/main/native/c/com_canonical_openssl_drbg_OpenSSLDrbg.c b/src/main/native/c/com_canonical_openssl_drbg_OpenSSLDrbg.c
@@ -45,23 +45,23 @@ void populate_params(DRBGParams *params, int strength, int prediction_resistance
 JNIEXPORT jlong JNICALL Java_com_canonical_openssl_drbg_OpenSSLDrbg_init
   (JNIEnv *env, jobject this, jstring name, jint strength, jboolean prediction_resistance, jboolean reseeding, jbyteArray personalization_string) {
     const char *name_string = (*env)->GetStringUTFChars(env, name, 0);
-    byte *pstr_bytes = NULL;
+    byte *ps_bytes_native = NULL;
     jsize pstr_length = 0;
 
     if (personalization_string != NULL) { 
         pstr_length = (*env)->GetArrayLength(env, personalization_string);
-        pstr_bytes = (*env)->GetByteArrayElements(env, personalization_string, NULL);
+        byte *pstr_bytes = (*env)->GetByteArrayElements(env, personalization_string, NULL);
+        ps_bytes_native = (byte *)malloc(pstr_length);
+        memcpy(ps_bytes_native, pstr_bytes, pstr_length);
+        (*env)->ReleaseByteArrayElements(env, personalization_string, pstr_bytes, JNI_ABORT);
     }
 
     DRBGParams *params = (DRBGParams *)malloc(sizeof(DRBGParams));
-    populate_params(params, strength, prediction_resistance, reseeding, pstr_bytes, pstr_length, NULL, 0);
+    populate_params(params, strength, prediction_resistance, reseeding, ps_bytes_native, pstr_length, NULL, 0);
 
     DRBG* drbg = create_DRBG_with_params(name_string, NULL, params);
     (*env)->ReleaseStringUTFChars(env, name, name_string);
 
-    if (personalization_string != NULL) {
-        (*env)->ReleaseByteArrayElements(env, personalization_string, pstr_bytes, JNI_ABORT);
-    }
     return (jlong)drbg; 
 }
 
@@ -72,31 +72,29 @@ JNIEXPORT jlong JNICALL Java_com_canonical_openssl_drbg_OpenSSLDrbg_init
  */
 JNIEXPORT void JNICALL Java_com_canonical_openssl_drbg_OpenSSLDrbg_nextBytes0
   (JNIEnv *env, jobject this, jbyteArray out_bytes, jint strength, jboolean prediction_resistance , jbyteArray additional_input) {
-
-    int additional_input_length = 0;
-    jbyte *additional_input_bytes = NULL;
-
+    byte *ai_bytes_native = NULL;
+    jsize additional_input_length = 0;
     jclass clazz = (*env)->GetObjectClass(env, this);
     jfieldID drbg_id = (*env)->GetFieldID(env, clazz, "drbgContext", "J");
     jlong drbg_handle = (*env)->GetLongField(env, this, drbg_id);
 
     int output_bytes_length = (*env)->GetArrayLength(env, out_bytes);
-    byte *output_bytes = (byte *)malloc(sizeof(output_bytes_length));
+    byte *output_bytes = (byte *)malloc(output_bytes_length);
 
     if (additional_input != NULL) {
         additional_input_length = (*env)->GetArrayLength(env, additional_input);
-        additional_input_bytes = (*env)->GetByteArrayElements(env, additional_input, NULL);
+        jbyte *additional_input_bytes = (*env)->GetByteArrayElements(env, additional_input, NULL);
+        ai_bytes_native = (byte*)malloc(additional_input_length);
+        memcpy(ai_bytes_native, additional_input_bytes, additional_input_length);
+        (*env)->ReleaseByteArrayElements(env, additional_input, additional_input_bytes, JNI_ABORT);
     }
 
     DRBGParams *params = (DRBGParams *)malloc(sizeof(DRBGParams));
-    populate_params(params, strength, prediction_resistance, 0, NULL, 0, (byte *)additional_input_bytes, additional_input_length);
+    populate_params(params, strength, prediction_resistance, 0, NULL, 0, ai_bytes_native, additional_input_length);
 
     next_rand_with_params((DRBG *)drbg_handle, output_bytes, output_bytes_length, params);
 
     (*env)->SetByteArrayRegion(env, out_bytes, 0, output_bytes_length, output_bytes);
-    if (additional_input != NULL) {
-        (*env)->ReleaseByteArrayElements(env, additional_input, additional_input_bytes, JNI_ABORT);
-    }
 }
 
 /*
@@ -139,6 +137,7 @@ JNIEXPORT void JNICALL Java_com_canonical_openssl_drbg_OpenSSLDrbg_reseed0
     }
 }
 
+#define MAX_SEED_BYTES 256
 /*
  * Class:     com_canonical_openssl_OpenSSLDrbg
  * Method:    generateSeed0
@@ -151,18 +150,17 @@ JNIEXPORT jbyteArray JNICALL Java_com_canonical_openssl_drbg_OpenSSLDrbg_generat
     jfieldID drbg_id = (*env)->GetFieldID(env, clazz, "drbgContext", "J");
     jlong drbg_handle = (*env)->GetLongField(env, this, drbg_id);
 
-    byte *output = (byte *)malloc(num_bytes);
-
-    if (output == NULL) {
-        return NULL;
+    if (num_bytes > 256) {
+        num_bytes = 256;
     }
 
+    byte output[MAX_SEED_BYTES];
+
     generate_seed((DRBG*)drbg_handle, output, num_bytes);
 
     jbyteArray ret_array = (*env)->NewByteArray(env, num_bytes);
-    (*env)->SetBooleanArrayRegion(env, ret_array, 0, num_bytes, output);
+    (*env)->SetByteArrayRegion(env, ret_array, 0, num_bytes, output);
 
-    free(output);
     return ret_array;
 }
 

diff --git a/src/main/native/c/drbg.c b/src/main/native/c/drbg.c
@@ -16,6 +16,7 @@
  */
 #include <drbg.h>
 #include <stdio.h>
+#include <sys/random.h>
 #include <unistd.h>
 
 DRBGParams NO_PARAMS = { DEFAULT_STRENGTH, 0, 0, NULL, 0, NULL, 0 };
@@ -57,7 +58,7 @@ DRBG* create_DRBG(const char* name, DRBG* parent) {
 DRBG* create_DRBG_with_params(const char* name, DRBG* parent, DRBGParams *drbg_params) {
     EVP_RAND *rand = EVP_RAND_fetch(NULL, name, NULL);
     if (NULL == rand) {
-        fprintf(stderr, "Couldn't allocate EVP_RAND\n");
+        fprintf(stderr, "Couldn't allocate EVP_RAND: %s\n", name);
         return NULL;
     }
 
@@ -93,6 +94,9 @@ DRBG* create_DRBG_with_params(const char* name, DRBG* parent, DRBGParams *drbg_p
 }
 
 int free_DRBGParams(DRBGParams *params) {
+    if (params == NULL) {
+        return 0;
+    }
     FREE_IF_NON_NULL(params->additional_data);
     FREE_IF_NON_NULL(params->personalization_str);
     FREE_IF_NON_NULL(params);
@@ -103,23 +107,10 @@ int free_DRBG(DRBG *generator) {
     if (generator == NULL) {
         return 0;
     }
-
+    free_DRBGParams(generator->params);
+    free_DRBG(generator->parent);
     FREE_IF_NON_NULL(generator->seed);
-    if (generator->context != NULL) {
-        EVP_RAND_CTX_free(generator->context);
-        generator->context = NULL;
-    }
-
-    if (generator->params != NULL) {
-        free_DRBGParams(generator->params);
-        generator->params = NULL;
-    }
-
-    if (generator->parent != NULL) {
-        free_DRBG(generator->parent);
-        generator->parent = NULL;
-    }
-
+    EVP_RAND_CTX_free(generator->context);
     free(generator);
     return 1;
 }
@@ -157,7 +148,7 @@ int generate_seed(DRBG* generator, byte output[], int n_bytes) {
     if (parent != NULL) {
         return next_rand(parent, output, n_bytes);
     } else {
-        return getentropy(output, n_bytes);
+        return getrandom(output, n_bytes, 0);
     }
 }
 
@@ -168,7 +159,7 @@ void reseed(DRBG* generator) {
 void reseed_with_params(DRBG *generator, DRBGParams *params) {
     byte seed[128]; // TODO: what should the default seed size be?
     size_t length = 128;
-    getentropy(seed, length);
+    getrandom(seed, length, 0);
     EVP_RAND_reseed(generator->context, params->prediction_resistance, seed, length, params->additional_data, params->additional_data_length);
 }
 

diff --git a/src/main/native/c/signature.c b/src/main/native/c/signature.c
@@ -16,6 +16,7 @@
  */
 #include "signature.h"
 #include "jssl.h"
+#include <openssl/err.h>
 #include <openssl/rsa.h>
 #include <openssl/core_names.h>
 

diff --git a/src/test/java/SecureRandomTest.java b/src/test/java/SecureRandomTest.java
@@ -169,8 +169,8 @@ public void testDRBGCreationNextBytesWithParams() throws NoSuchAlgorithmExceptio
         testArrayInequality(hash1, hash2);
     }
 
-    /* disabled - needs a "double-free" investigation" */
-    private void testDRBGCreationWithParamsNextBytesWithParams() throws NoSuchAlgorithmException, NoSuchProviderException {
+    @Test
+    public void testDRBGCreationWithParamsNextBytesWithParams() throws NoSuchAlgorithmException, NoSuchProviderException {
         SecureRandomParameters params = DrbgParameters.instantiation(128, Capability.PR_AND_RESEED, "FIPSPROTOTYPE".getBytes());
         SecureRandomParameters nbParams = DrbgParameters.nextBytes(128, true, "ADDITIONALINPUT".getBytes());
 

diff --git a/src/test/runner.py b/src/test/runner.py
@@ -34,7 +34,7 @@
 }
 
 def run_native_test(name):
-  return os.system(f"build/test/bin/{name} > /dev/null 2>&1")
+  return os.system(f"build/test/bin/{name} >> build/test/test.out 2>&1") 
 
 for test in tests.keys():
   name = tests[test]