crypto_argon2i_general() should abort on bad input

[Sc00bz]

https://github.com/LoupVaillant/Monocypher/blob/2e9a54b99534584f887de247c068832f146c435f/src/monocypher.c#L929-L935 nb_blocks must be at least 8 because anything lower is invalid (well 8*p but p=1). Since this is Argon2i, nb_iterations must be at least 3.

[fscoto]

@LoupVaillant may override me of course, but my understanding of the input validity situation is as follows:

nb_blocks ust indeed be at least 8. As much is documented in the manual.

nb_iterations doesn't need to be at least 3, but rather 1, with 3 being the default—or so the spec says on p. 8 (“With default number of passes over memory ([...] 3 for Argon2i). The minimum of 1 is documented in the Monocypher manual.

---

Now as for why Monocypher doesn't abort on invalid invariants (bad lengths in BLAKE2b and Argon2i in particular come to mind), my understanding is that aborting (i.e. calling the abort function) would imply relying on the C standard library for anything other than providing types. Monocypher is very explicitly designed not to require any kind of external support from the C standard library.

The alternative would have been a return value whose only purpose is an invalid-input check for static invariants that are well-documented. This felt like a cumbersome API because then people would be tempted to check the return value that in every case is going to be correct.

Again, @LoupVaillant may correct me since I may have misconceptions, but I recall that's how the design decisions went. Though if you've got a compelling use case where it clearly makes sense to have some kind of invalid-input escape hatch that doesn't boil down to "hardcoded constants by someone who skimmed the manual", you'll probably at least get a return value in.

I agree that the situation is suboptimal, but it's a series of unfortunate trade-offs.

[LoupVaillant]

@fscoto is correct: Monocypher does not validate inputs that are under the caller's control. It is the caller's responsibility to make sure the pre-conditions are respected.

The advantage of that approach is that we can avoid returning a value, which the caller wouldn't even have to check if they make sure the inputs are correct. The drawback is that some errors don't have the loud consequences we'd like them to have.

Adding runtime asserts would be tempting, but I'm afraid this would indeed introduce yet another dependency to libc, while the lack of such dependency is one of the major selling points of Monocypher (no dependency, no hassle, greater portability).

Not ideal, I agree. Had I implemented Monocypher in Rust, I would definitely have an assert. I even think language bindings should add those asserts.

Practically speaking though: this is a memory hard function we're talking about: who in their right mind would select less than 8KB of memory hardness? Even if I let end users control that hardness, I would put the minimum at something like 8 megabytes, and I'd display it in red to convey how dangerously low that parameter is.

For now I consider this "not a bug". It was a deliberate design decision. One we could perhaps revisit later. Just not yet.

[Sc00bz]

Note that Argon2i with 1 iteration an attacker can do a 1/4 memory attack at no cost and 1/2 memory attack at no cost with 2 iterations. Also there are other attacks. This is why 3 iterations is the minimum for Argon2i.

[LoupVaillant]

@fscoto Perhaps we should word the manual a bit more strongly then? Mandate 3 iterations instead of just recommending them?

[fscoto]

This mixes two things: Valid inputs and good inputs. 1 iteration is a valid input, by spec § 3.1. But it's not a good input (see § 5.7 of the spec noting the attacks on 1-iteration and 2-iteration Argon2i). Similarly, we allow people to use 1-byte keys with BLAKE2b, which is completely meaningless because it's devoid of any meaningful security, but possible.

@LoupVaillant @Sc00bz Thoughts on doing this in the documentation as a compromise (plus mdoc date bump)?

@@ -134,7 +134,10 @@ If it is still too fast with all available memory, increase
 .It Fa nb_iterations
 The number of iterations.
 It must be at least 1.
-A value of 3 is recommended.
+A value of 3 is
+.Em strongly
+recommended;
+any value lower than 3 enables significantly more efficient attacks.
 .It Fa password
 The password to hash.
 It should be wiped with

[LoupVaillant]

That works for me.