From 53fbc70fa6928c10c9763e36d7a6a0f2ea5247ed Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Fri, 13 Sep 2024 07:40:38 +0000 Subject: [PATCH] Fix probable causes for crashes in free() While requesting for the next array of random bytes we were cleary malloc'ing memory of the wrong size (sizeof(length) instead of length). This commit includes the bug fix and re-enables SecureRandomTest. --- .github/workflows/maven.yml | 4 +- pom.xml | 3 -- .../com_canonical_openssl_drbg_OpenSSLDrbg.c | 42 +++++++++---------- src/main/native/c/drbg.c | 29 +++++-------- src/main/native/c/signature.c | 1 + src/test/java/SecureRandomTest.java | 4 +- src/test/runner.py | 2 +- 7 files changed, 37 insertions(+), 48 deletions(-) diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index b92abb0..396551e 100644 --- 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 index 8a58c19..3cbbe83 100644 --- a/pom.xml +++ b/pom.xml @@ -172,9 +172,6 @@ **/*Test.java - - **/*SecureRandomTest.java - diff --git a/src/main/native/c/com_canonical_openssl_drbg_OpenSSLDrbg.c b/src/main/native/c/com_canonical_openssl_drbg_OpenSSLDrbg.c index 5edc5ee..b39d019 100644 --- 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 index e2483dc..7f360e2 100644 --- a/src/main/native/c/drbg.c +++ b/src/main/native/c/drbg.c @@ -16,6 +16,7 @@ */ #include #include +#include #include 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 index 9a160cb..bb2750b 100644 --- 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 #include #include diff --git a/src/test/java/SecureRandomTest.java b/src/test/java/SecureRandomTest.java index ed27c61..db835db 100644 --- 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 index a3929be..bbe6201 100755 --- 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]