From edd95498d127ba5b47e32e0cbae6848eaedebee8 Mon Sep 17 00:00:00 2001 From: Jesper Louis Andersen Date: Mon, 21 May 2018 15:21:09 +0200 Subject: [PATCH] Fix pwhash_str* functions. The API for pwhash_str returns a cstring in the output buffer. These are null terminated. However, we return the full buffer as a binary back to Erlang. This means that we have a buffer with 0'es in the end. The tests take this buffer and passes it back in as is. Hence all the tests pass. However, it is conceivable that if we write said buffer to disk somewhere, we are not going to write those 0's out. When we then load the ASCII-armored Argon2 string into memory again, it is not 0-terminated as a cstring should be, and this produces errors all over the place. The fix is twofold: * Return the full buffer to Erlang, but use binary:split/2 to create a subbinary with the relevant part. * Add a 0 in the end of ASCII Argon2 string before passing it to libsodium Since we are looking at pwhashing, and Argon2, we expect the computational problem to be memory bound. Thus, spending a bit more work in memory is not going to have any considerable impact on the speed of this system. --- c_src/enacl_nif.c | 2 +- eqc_test/enacl_eqc.erl | 22 +++++++++++----------- src/enacl.erl | 16 ++++++++++++++-- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/c_src/enacl_nif.c b/c_src/enacl_nif.c index c15b3e8..364a47d 100644 --- a/c_src/enacl_nif.c +++ b/c_src/enacl_nif.c @@ -1327,7 +1327,7 @@ ERL_NIF_TERM enif_crypto_pwhash_str_verify(ErlNifEnv *env, int argc, ERL_NIF_TER // Validate the arguments if( (argc != 2) || - (!enif_inspect_binary(env, argv[0], &h)) || + (!enif_inspect_iolist_as_binary(env, argv[0], &h)) || (!enif_inspect_iolist_as_binary(env, argv[1], &p)) ) { return enif_make_badarg(env); } diff --git a/eqc_test/enacl_eqc.erl b/eqc_test/enacl_eqc.erl index e94229b..e2978dc 100644 --- a/eqc_test/enacl_eqc.erl +++ b/eqc_test/enacl_eqc.erl @@ -721,17 +721,17 @@ pwhash_str_verify(PasswdHash, Passwd) -> prop_pwhash_str_verify() -> ?FORALL({Passwd}, {?FAULT_RATE(1, 40, g_iodata())}, - begin - case v_iodata(Passwd) of - true -> - {ok, Ascii} = enacl:pwhash_str(Passwd), - S = enacl:pwhash_str_verify(Ascii, Passwd), - equals(S, true); - false -> - badargs(fun() -> enacl:pwhash_str(Passwd) end), - badargs(fun() -> enacl:pwhash_str_verify("", Passwd) end) - end - end). + begin + case v_iodata(Passwd) of + true -> + {ok, Ascii} = enacl:pwhash_str(Passwd), + S = enacl:pwhash_str_verify(Ascii, Passwd), + equals(S, true); + false -> + badargs(fun() -> enacl:pwhash_str(Passwd) end), + badargs(fun() -> enacl:pwhash_str_verify("", Passwd) end) + end + end). %% SUBTLE HASHING %% --------------------------- diff --git a/src/enacl.erl b/src/enacl.erl index 8930498..15ea2c2 100644 --- a/src/enacl.erl +++ b/src/enacl.erl @@ -350,7 +350,19 @@ pwhash(Password, Salt) -> %% @end -spec pwhash_str(iodata()) -> {ok, iodata()} | {error, term()}. pwhash_str(Password) -> - enacl_nif:crypto_pwhash_str(Password). + case enacl_nif:crypto_pwhash_str(Password) of + {ok, ASCII} -> + {ok, strip_null_terminate(ASCII)}; + {error, Reason} -> + {error, Reason} + end. + +strip_null_terminate(Binary) -> + [X, _] = binary:split(Binary, <<0>>), + X. + +null_terminate(ASCII) -> + iolist_to_binary([ASCII, 0]). %% @doc pwhash_str_verify/2 compares a password with a hash %% @@ -359,7 +371,7 @@ pwhash_str(Password) -> %% @end -spec pwhash_str_verify(binary(), iodata()) -> boolean(). pwhash_str_verify(HashPassword, Password) -> - enacl_nif:crypto_pwhash_str_verify(HashPassword, Password). + enacl_nif:crypto_pwhash_str_verify(null_terminate(HashPassword), Password). %% Public Key Crypto %% ---------------------