decode_string() API is dangerous

[solardiz]

@AlekseyCherepanov reported a bug against JtR bleeding-jumbo that said "$argon2d$m=2048,t=1,p=1$xxxxxxxxxxxxxxxx is cracked by any password". Investigating this, I found that Argon2's decode_string() happily returns ARGON2_OK yet outlen=0 in cases like this. While for password crackers this is just nasty, if a service chooses to decode from ASCII and compare binary hashes (rather than the usual way of encoding and comparing the ASCII hashes), authentication might succeed with any or wrong password against a truncated or otherwise broken hash.

Maybe the decode_string() API should be revised to report error unless the entire string is valid, or/and return a pointer to the first invalid character. Right now, the API is both dangerous and lacks that feature, which makes the risk unjustified.

Besides validating the entire string, maybe decode_string() should also impose a lower bound on outlen (maybe 16 bytes?) and return an error if that is not met. This is to catch strings that are syntactically valid, yet are truncated such that the remnants of the hash are dangerously short.

[sneves]

Doesn't decode_string validate the parameters after decoding? The sample program

#include <stdio.h>
#include "encoding.h"

int main() {
  unsigned char out[1024] = {0};
  unsigned char salt[1024] = {0};
  argon2_context ctx = {0};
  ctx.saltlen = 1024;
  ctx.outlen = 1024;
  ctx.out = out;
  ctx.salt = salt;
  int x = decode_string(&ctx, "$argon2d$m=2048,t=1,p=1$xxxxxxxxxxxxxxxx", Argon2_d);
  printf("%s\n", argon2_error_message(x));
  return 0;
}

results in a Decoding failed error, not ARGON2_OK. How are you coaxing ARGON2_OK out of it?

[solardiz]

Oh, looks like this has already been fixed since we merged the code into JtR -jumbo. In particular, ours is from prior to commit 5d4f755, which includes removal of a couple return ARGON2_OK, including this one:

     BIN(ctx->salt, maxsaltlen, ctx->saltlen);
-    if (*str == 0) {
-        return ARGON2_OK;
-    }

I suspect this could be it. We'll sync to latest code, and I now expect the problem will be gone. If it still exists in the latest, I'll re-open this issue.

Sorry for being too quick to report this. Should have done my homework first.