Argon2i memory bugs

[guidovranken]

I'm fuzzing Monocypher using https://github.com/guidovranken/cryptofuzz

The following examples may be using invalid parameters, but that shouldn't lead to memory issues IMO.

Segfault if blocks < 8

#include <stdint.h>
#include <stdlib.h>
#include <monocypher.h>

int main(void)
{
    const uint8_t password[] = { 0x00 };
    const uint8_t salt[] = { 0x00 };

    uint8_t* out = malloc(4);
    uint8_t* work_area = malloc(4 * 1024);

    /* noret */ crypto_argon2i(
            out, 4,
            work_area, 4,
            1,
            password, sizeof(password),
            salt, sizeof(salt));

    free(out);
    free(work_area);
}

Outputs uninitialized memory if iterations is 0

Use valgrind to verify this.

#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <monocypher.h>

int main(void)
{
    const uint8_t password[] = { 0x00 };
    const uint8_t salt[] = { 0x00 };

    uint8_t* out = malloc(1);
    uint8_t* work_area = malloc(8 * 1024);

    /* noret */ crypto_argon2i(
            out, 1,
            work_area, 8,
            0,
            password, sizeof(password),
            salt, sizeof(salt));
    for (size_t i = 0; i < 1; i++) {
        printf("%02X ", out[i]);
    }
    printf("\n");

    free(out);
    free(work_area);
}

[fscoto]

This behavior is not only known, but also documented:

nb_blocks
The number of blocks for the work area. Must be at least 8. A value of 100000 (one hundred megabytes) is a good starting point. If the computation takes too long, reduce this number. If it is too fast, increase this number. If it is still too fast with all available memory, increase nb_iterations.

nb_iterations
The number of iterations. It must be at least 1. A value of 3 is strongly recommended; any value lower than 3 enables significantly more efficient attacks.

[...]

CAVEATS

Any deviation from the specified input and output length ranges results in undefined behaviour. Make sure your inputs are correct.

There are multiple facets as to what led to this decision (without wanting to say whether they are good or bad reasons right now):

1. There is no meaningful action to take. The spec doesn't define what happens when performing no passes. The only reasonable action that could be taken would be either intentionally segfaulting or a return code after writing a dummy value in the output buffer.
2. It is easier to ignore a return code than it is to ignore non-deterministic nonsense results or crashes. The current behavior angers valgrind and gdb, which is intentional and by design, precisely so people notice that something breaks.
3. In many cases, these parameters are hardcoded. At runtime, range checks for size/iteration parameters add needless branching for something you only get wrong once during development and then freeze forever and ever until the end of time (or at least until that part is rewritten).

The above rationale applies for the crypto_blake2b*() family as well, which will do silly things if you break the documented rules for key and output sizes.

[LoupVaillant]

@guidovranken, thanks for the report.
@fscoto, thanks for your spot-on reply.

This is indeed "not a bug". In general, Monocypher does not check trusted input. When you give a pointer no NULL check is performed. When you give a size, it is assumed valid. Monocypher is neither foolproof nor misuse resistant —it doesn't even aim to be.

That said, your bug report suggests you may disagree with those design choices. If so, I would be very interested to know why. You might have arguments we haven't thought of yet.

[guidovranken]

Ok