ERL-628: Inconsistent behavior when reading binaries in within a NIF

[OTP-Maintainer]

Original reporter: starbelly
Affected versions: OTP-19.0, OTP-20.0, OTP-21.0
Component: compiler
Migrated from: https://bugs.erlang.org/browse/ERL-628

---

I have observed what I believe to be inconsistent behavior when reading in binaries within NIFs (i.e., passed in via erlang function). I stumbled across this while playing around with enacl's pwhash_str_verify/2 (https://github.com/jlouis/enacl/blob/8b8ceff4ef3795f4974cfaa4f077f6e0cc21a30b/c_src/enacl_nif.c#L1335-L1339).

 Naturally, all attention was focused on the NIF itself but this yielded nothing. After fiddling and much needed help from Fred Hebert two conclussions have been reached... 1. That binary is a poor choice for such a case per size oddities that just are what they are. 2 There is a bug in regards to enif_inspect_binary/3 or in the NIF interface in general. 

 It should be noted that I tested this on erlang/OTP 19,20, and 21.0 (rc1) on Mac OS and Linux. 

 Enough talk, let's get to the meat and potatoes! :)

The original behavior observed:

Given a file named 'one.txt' containing an argon2 pw hash and a file named 'two.txt' containing the plain text password used with the following script will sometimes result in true and sometimes false. It should always be true. Storing and reading the values in from the files served to rule out this being limited to literals. 
  You will observe that "sometimes" after the binary is read in the nif, there is some additional cruft tacked on to the end which results in failed hash/pass comparison. 

Test script : 

#!/usr/bin/env escript
%%! -noshell -env ERL_LIBS _build/default/lib

-mode(compile).

test(N) ->
io:format("~n===== Pass #bn", [N]),
{ok, O} = file:read_file("one.txt"),
{ok, T} = file:read_file("two.txt"),
H = string:trim(O, trailing, "\n"),
P = string:trim(T, trailing, "\n"),
io:format("Before NIF => pn", [{H, P}]),
Out = enacl:pwhash_str_verify(H, P),
io:format("After NIF => pn", [{H, P}]),
io:format("NIF RESULT => sn", [Out]),

% Literal pass
HL =
<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
PL = <<"foo">>,
true = (H =:= HL andalso P =:= PL),
io:format("Before NIF (literal) => ~p~n", [{HL, PL}]),
Out2 = enacl:pwhash_str_verify(HL, PL),
io:format("After NIF (literal) => ~p~n", [{HL, PL}]),
io:format("NIF RESULT (literal) => ~s~n", [Out2]).

main([_]) ->
application:ensure_all_started(enacl),
lists:map(fun(N) -> test(N) end, lists:seq(1,3)),
init:stop().

Output:
  Note: We should get true every time.  

==== Pass #1
Before NIF => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
IN NIF => '$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y
��Z�j9��Q�Z�v]@' and 'foo
'
After NIF => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
NIF RESULT => false
Before NIF (literal) => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
IN NIF => '$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y' and 'foo'
After NIF (literal) => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
NIF RESULT (literal) => true

===== Pass #2
Before NIF => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
IN NIF => '$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y
�g���^�]�Jk�{�@' and 'foo
'
After NIF => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
NIF RESULT => false
Before NIF (literal) => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
IN NIF => '$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y' and 'foo'
After NIF (literal) => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
NIF RESULT (literal) => true

===== Pass #3
Before NIF => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
IN NIF => '$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y
�A����2����,@' and 'foo
'
After NIF => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
NIF RESULT => false
Before NIF (literal) => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
IN NIF => '$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y' and 'foo'
After NIF (literal) => {<<"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y">>,
<<"foo">>}
NIF RESULT (literal) => true

After looking at other similar implementations I noticed that where enacl is reading in a binary other implementations read the hash in as charlist, so I figured let's try that... 

diff --git a/c_src/enacl_nif.c b/c_src/enacl_nif.c
index 1046af7..0e6798e 100644
--- a/c_src/enacl_nif.c
+++ b/c_src/enacl_nif.c
@@ -1295,17 +1295,19 @@ ERL_NIF_TERM enif_crypto_pwhash_str(ErlNifEnv *env, int argc, ERL_NIF_TERM const

static
ERL_NIF_TERM enif_crypto_pwhash_str_verify(ErlNifEnv *env, int argc, ERL_NIF_TERM const argv[]) {

• ErlNifBinary h, p;

• char encoded[2024];
• ErlNifBinary p;

// Validate the arguments
if( (argc != 2) ||

•  (!enif_inspect_binary(env, argv[0], &h)) ||
  

•  (!enif_get_string(env, argv[0], encoded, sizeof(encoded), ERL_NIF_LATIN1)) ||
   (!enif_inspect_binary(env, argv[1], &p)) ) {
  

  return enif_make_badarg(env);
  }

  ERL_NIF_TERM retVal = enif_make_atom(env, ATOM_TRUE);

• if( crypto_pwhash_str_verify((char *)h.data, (char *)p.data, p.size) != 0) {

• if( crypto_pwhash_str_verify(encoded, (char )p.data, p.size) != 0) {
  / wrong password */
  retVal = enif_make_atom(env, ATOM_FALSE);
  }

 After applying this patch and modifying the test script to pass in the hash as a charlist we get true every time...

===== Pass #1
Before NIF (literal) => {"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y",
<<"foo">>}
After NIF (literal) => {"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y",
<<"foo">>}
NIF RESULT (literal) => true

===== Pass #2
Before NIF (literal) => {"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y",
<<"foo">>}
After NIF (literal) => {"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y",
<<"foo">>}
NIF RESULT (literal) => true

===== Pass #3
Before NIF (literal) => {"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y",
<<"foo">>}
After NIF (literal) => {"$argon2id$v=19$m=65536,t=2,p=1$39gbxAJq7dxSB8ycRRwZKg$2x5eXfGN0uSh2ywUe7pU9eb6wKAykhY3Ewu4LrSwt7Y",
<<"foo">>}
NIF RESULT (literal) => true

 If there is anything I can do or any other information I can provide please let me know. 

Cheers :) 

P.S. Hopefully markdown is supported ;)

[OTP-Maintainer]

john said:

Hi,

According to [libsodium`s documentation|https://download.libsodium.org/doc/password_hashing/the_argon2i_function.html] the string passed to {{crypto_pwhash_str_verify}} _has to_ be zero-terminated, and I can't see any sign of your code nor {{enacl}} manually zero-terminating the input binary. {{enif_get_string}} on the other hand zero-terminates which would explain why that works.

How did you print the "IN NIF" lines? Did you use a function that expects zero-termination (e.g. {{printf}} without size prefixing) or did you take size into account?

Regards,
John Högberg

[OTP-Maintainer]

jlouis said:

If we don't obey the pwhash_str_verify API, all bets are off. This is definitely an enacl-bug then, which should be fixed inside the API. The _str_* APIs are different in that they expect C strings and not just blobs of binary arrays with a len parameter. So I guess we ought to fix this. Please open an issue for this at `jlouis/enacl` so we can fix it.

[OTP-Maintainer]

jlouis said:

Indeed, the API is abused by enacl here. I have a patch brewing that fixes it in enacl, and I don't expect this to be a bug in Erlang/OTP at all.

[OTP-Maintainer]

starbelly said:

  Hi John,
I did use printf and did not take size into account :( .

[OTP-Maintainer]

john said:

It happens to the best of us, and I doubt I would've spotted it so quick without such a good error description. :o)

Cheers!

[OTP-Maintainer]

jlouis said:

Fixed in https://github.com/jlouis/enacl/commit/edd95498d127ba5b47e32e0cbae6848eaedebee8 in enacl. If you verify this works for you, then this isn't an issue in Erlang/OTP anymore.

[OTP-Maintainer]

starbelly said:

 Jesper,
Confirmed, fixed. Thank you! :)

[OTP-Maintainer]

jlouis said:

For other readers as well: 0.17.1 is cut with the fix and on hex.pm.