Protect generichash by a mutex

While sodium is thread-safe, our
resources are not. Furthermore,
we might have an update call going
when someone decides to call
finalize and so on. It is not clever
to do so, but on the other hand
I want to protect against this.

While here, mark the mutexed
calls as dirty CPU. This avoids them
blocking the main scheduler and
only messes with the background
dirty threads, which is somewhat
more safe.

The consequence is that order
access to the resource is now
serialized. I don't think you should
do it, but it is now possible.
This commit is contained in:
Jesper Louis Andersen 2020-01-24 14:48:21 +01:00
parent e4b35a7035
commit 7d8fdf69c0
3 changed files with 21 additions and 3 deletions

View File

@ -18,7 +18,6 @@ Under errors, the current code can leak binaries. I don't want that to happen, s
are going to use a better cleanup scheme in the code base. are going to use a better cleanup scheme in the code base.
- Play the mutex game with: - Play the mutex game with:
- Generic Hash multi-part types
- Sign multi-part types - Sign multi-part types
The resource-variant structs need to be mutex-protected. Otherwise we run the risk of The resource-variant structs need to be mutex-protected. Otherwise we run the risk of
@ -81,6 +80,7 @@ sections applied. The current code is subtly in error!
- Clang static analysis warnings (Thomas Arts). - Clang static analysis warnings (Thomas Arts).
- Replace a constant 31 with a computation from libsodium (Thomas Arts, from a security review). - Replace a constant 31 with a computation from libsodium (Thomas Arts, from a security review).
- Some subtle memory leaks in the error path for kx operations were plugged. - Some subtle memory leaks in the error path for kx operations were plugged.
- The multi-part generichash interface is now properly process/thread safe.
## [0.17.2] ## [0.17.2]

View File

@ -445,8 +445,10 @@ static ErlNifFunc nif_funcs[] = {
enacl_crypto_generichash_KEYBYTES_MAX}, enacl_crypto_generichash_KEYBYTES_MAX},
{"crypto_generichash", 3, enacl_crypto_generichash}, {"crypto_generichash", 3, enacl_crypto_generichash},
{"crypto_generichash_init", 2, enacl_crypto_generichash_init}, {"crypto_generichash_init", 2, enacl_crypto_generichash_init},
{"crypto_generichash_update", 2, enacl_crypto_generichash_update}, erl_nif_dirty_job_cpu_bound_macro("crypto_generichash_update", 2,
{"crypto_generichash_final", 1, enacl_crypto_generichash_final} enacl_crypto_generichash_update),
erl_nif_dirty_job_cpu_bound_macro("crypto_generichash_final", 1,
enacl_crypto_generichash_final)
}; };

View File

@ -6,9 +6,11 @@
#include "generichash.h" #include "generichash.h"
typedef struct enacl_generichash_ctx { typedef struct enacl_generichash_ctx {
ErlNifMutex *mtx;
crypto_generichash_state *ctx; // Underlying hash state from sodium crypto_generichash_state *ctx; // Underlying hash state from sodium
int alive; // Is the context still valid for updates/finalizes? int alive; // Is the context still valid for updates/finalizes?
int outlen; // Final size of the hash int outlen; // Final size of the hash
} enacl_generichash_ctx; } enacl_generichash_ctx;
static ErlNifResourceType *enacl_generic_hash_ctx_rtype; static ErlNifResourceType *enacl_generic_hash_ctx_rtype;
@ -37,6 +39,9 @@ static void enacl_generic_hash_ctx_dtor(ErlNifEnv *env,
if (obj->ctx) if (obj->ctx)
sodium_free(obj->ctx); sodium_free(obj->ctx);
if (obj->mtx != NULL)
enif_mutex_destroy(obj->mtx);
return; return;
} }
@ -175,6 +180,7 @@ ERL_NIF_TERM enacl_crypto_generichash_init(ErlNifEnv *env, int argc,
// Allocate the state context via libsodium // Allocate the state context via libsodium
// Note that this ensures a 64byte alignment for the resource // Note that this ensures a 64byte alignment for the resource
// And also protects the resource via guardpages // And also protects the resource via guardpages
obj->mtx = NULL;
obj->ctx = NULL; obj->ctx = NULL;
obj->alive = 0; obj->alive = 0;
obj->outlen = 0; obj->outlen = 0;
@ -187,6 +193,11 @@ ERL_NIF_TERM enacl_crypto_generichash_init(ErlNifEnv *env, int argc,
obj->alive = 1; obj->alive = 1;
obj->outlen = hash_size; obj->outlen = hash_size;
if ((obj->mtx = enif_mutex_create("enacl.generichash")) == NULL) {
ret = enacl_error_tuple(env, "generichash_mutex_error");
goto err;
}
// Call the library function // Call the library function
if (0 != crypto_generichash_init(obj->ctx, k, key.size, obj->outlen)) { if (0 != crypto_generichash_init(obj->ctx, k, key.size, obj->outlen)) {
ret = enacl_error_tuple(env, "hash_init_error"); ret = enacl_error_tuple(env, "hash_init_error");
@ -230,6 +241,8 @@ ERL_NIF_TERM enacl_crypto_generichash_update(ErlNifEnv *env, int argc,
if (!enif_inspect_binary(env, argv[1], &data)) if (!enif_inspect_binary(env, argv[1], &data))
goto bad_arg; goto bad_arg;
enif_mutex_lock(obj->mtx);
if (!obj->alive) { if (!obj->alive) {
ret = enacl_error_tuple(env, "finalized"); ret = enacl_error_tuple(env, "finalized");
goto done; goto done;
@ -247,6 +260,7 @@ ERL_NIF_TERM enacl_crypto_generichash_update(ErlNifEnv *env, int argc,
bad_arg: bad_arg:
return enif_make_badarg(env); return enif_make_badarg(env);
done: done:
enif_mutex_unlock(obj->mtx);
return ret; return ret;
} }
@ -262,6 +276,7 @@ ERL_NIF_TERM enacl_crypto_generichash_final(ErlNifEnv *env, int argc,
(void **)&obj)) (void **)&obj))
goto bad_arg; goto bad_arg;
enif_mutex_lock(obj->mtx);
if (!obj->alive) { if (!obj->alive) {
ret = enacl_error_tuple(env, "finalized"); ret = enacl_error_tuple(env, "finalized");
goto done; goto done;
@ -293,5 +308,6 @@ bad_arg:
release: release:
enif_release_binary(&hash); enif_release_binary(&hash);
done: done:
enif_mutex_unlock(obj->mtx);
return ret; return ret;
} }