Consider allowing the output of Argon2i to overlap with its work area

[LoupVaillant]

I misremembered the Argon2i documentation, and put the output hash in the work area. This allowed me to avoid a couple allocations, and seemed convenient. Problem is, the work area is wiped at the end of crypto_argon2i_general(), and that wiped the hash along with it. The result was a password hash that always gave the same result. For every password & salt.

Oops.

This behaviour is documented. Yet I, the goddamn maintainer, managed to get it wrong. I'm willing to bet others will too. A brief inspection at the source code suggest we can allow the final hash to overlap with the work area at no cost. I'm just wondering whether we should.

@fscoto, @tankf33der, thoughts?

[tankf33der]

Saving C coder from himself is pseudo problem for me. Starting using library should be started from reading documentation, not RFCs or specs. Lets keep consider: chaos input - chaos output.

I vote, leave it as is and-or add text:

... otherwise your output hash will be all zeroes and this is mitigation of future bugs and security violations.

[LoupVaillant]

Note that I do not propose that we do not wipe the work area. We should definitely wipe it. Here's what I propose instead:

diff --git a/src/monocypher.c b/src/monocypher.c
index c37482e..dd488e0 100644
--- a/src/monocypher.c
+++ b/src/monocypher.c
@@ -1023,15 +1023,16 @@ void crypto_argon2i_general(u8       *hash,      u32 hash_size,
         }
     }
     wipe_block(&tmp);
-    // hash the very last block with H' into the output hash
     u8 final_block[1024];
     store_block(final_block, blocks + (nb_blocks - 1));
-    extended_hash(hash, hash_size, final_block, 1024);
-    WIPE_BUFFER(final_block);

     // wipe work area
     volatile u64 *p = (u64*)work_area;
     ZERO(p, 128 * nb_blocks);
+
+    // hash the very last block with H' into the output hash
+    extended_hash(hash, hash_size, final_block, 1024);
+    WIPE_BUFFER(final_block);
 }

The idea is to wipe the work area before we write the output hash. That way the only part of the work area that may not be wiped is the actual output we were looking for.

Saving C coder from himself is pseudo problem for me.

I agree, to a point. Here though, I feel the API is being unnecessarily inconsistent. See, Monocypher almost systematically allows buffers to overlap, including in many cases where it does not make sense. The only exceptions I recall are Chacha20/crypto_lock() (the ciphertext must overlap perfectly with the plaintext, or not at all), and Argon2i.

I also like consistency.

[fscoto]

Personally, I didn't realize this was even possible. The work area and output hash are different “kinded” memory to me, so to speak.

I agree with @tankf33der in principle. However, I'm on Loup's side on this one for the consistency reasons he brought up. Monocypher generally goes out of its way to allow overlapping or at least identical pointers to share memory. This is generally a nice convenience and just a really stupid, avoidable footgun to trip over.

+1 for making the change.