Improperly sized array in crypto_kem_keypair_derand

[QRCS-CORP]

The cmp array is oversized by symbytes:
uint8_t cmp[KYBER_CIPHERTEXTBYTES+KYBER_SYMBYTES]; should be: uint8_t cmp[KYBER_CIPHERTEXTBYTES];
This is passed as the first parameter to indcpa_enc which is declared:
void indcpa_enc(uint8_t c[KYBER_INDCPA_BYTES],
const uint8_t m[KYBER_INDCPA_MSGBYTES],
const uint8_t pk[KYBER_INDCPA_PUBLICKEYBYTES],
const uint8_t coins[KYBER_SYMBYTES]);
With: #define KYBER_CIPHERTEXTBYTES (KYBER_INDCPA_BYTES)

Looks like you were passing cmp to keccak absorb after copying in the key to the first 32 bytes, but chose instead to create a custom Keccak absorb to avoid the memcpy (you should rename modified Keccak functions for clarity btw).

This issue exists in the ref and avx versions.

I also see that you are no longer hashing coins on keygen and the shared secret, binding the security more closely to the output of the system entropy provider, this assumes the output from the provider is equivalent to the security of this construction, is that really wise?

Cheers,
John

[cryptojedi]

Thanks for pointing out the issue with the size of the cmp array. I just fixed this.
Not sure what you mean by your comment about Keccak absorb.
The decision to not hash the output of the system RNG was not ours, but was made by NIST.

[QRCS-CORP]

Hi,
The Keccak absorb function appears to be a customized version. It splits the message absorb with the state finalization into two functions: keccak_absorb which absorbs the message into the state, and keccak_finalize which adds the block terminator and domain separator. These may be functions from the Keccak library that I have missed, but typically both are combined in a single function named 'absorb'. If they are customized functions, I would suggest renaming them that way as: 'keccak_custom_absorb' and keccak_custom_absorb_finalize', in the off-chance that downstream implementors may adopt them incorrectly in other contexts.

As for the change in hashing input seeding material, you don't sound happy about it either. If the system RNG is compromised (and I remember such a case from a few years ago with Debian and OpenSSL), this could cause a catastrophic failure. Though hashing a seed does not 'add more entropy' obviously, it does negate bias in the seed material that might be attacked directly, and I have to suppose that may have been the intent behind the change made by NIST. There is a simple solution however, one that restores the security potential; change randombytes, make it more secure. I will post the code below to an improved randombytes, one that undoes this change without altering the specification, that can be dropped right into this project. You will have to test the *nix versions, but what I have done is request 2x the entropy from the provider (typically AES or ChaCha based PRNG used in OS implementations) to approximate post-quantum security, then hashed it with SHAKE256. Sorry for the formatting, I tried to fix it, to no avail.
`
#include <stddef.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include "randombytes.h"
#include "fips202.h"

#ifdef _WIN32

include <windows.h>

include <wincrypt.h>

#else

include <fcntl.h>

include <errno.h>

ifdef linux

define _GNU_SOURCE

include <unistd.h>

include <sys/syscall.h>

else

include <unistd.h>

endif

#endif

#define SECURE_ZERO(ptr, len)
do {
/* Use a volatile pointer to memset so that the call /
/ cannot be optimized out. */
void ( volatile memset_v)(void *, int, size_t) = memset;
memset_v((ptr), 0, (len));
} while(0)

int32_t randombytes(uint8_t* out, size_t outlen)
{
uint8_t* buf;
int32_t res;
size_t buflen;
size_t ctr;
size_t pos;

ctr = 0;
pos = 0;
res = -1;

/* allocate 2x the request size for pq equivalent security */
buflen = outlen * 2;
ctr = buflen;
buf = malloc(buflen);

if (buf != NULL)
{
    SECURE_ZERO(buf, buflen);

#ifdef _WIN32

    HCRYPTPROV ctx;

    size_t len;

    if (!CryptAcquireContext(&ctx, NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT))
    {
        abort();
    }

    while (pos < ctr)
    {
        len = (ctr > 1048576) ? 1048576 : ctr;

        if (!CryptGenRandom(ctx, len, (BYTE*)buf + pos))
        {
            abort();
        }

        ctr -= len;
        pos += len;
    }

    if (!CryptReleaseContext(ctx, 0))
    {
        abort();
    }

#elif defined(linux) && defined(SYS_getrandom)

    ssize_t ret;

    while (pos < ctr)
    {
        ret = syscall(SYS_getrandom, buf + pos, ctr, 0);

        if (ret == -1 && errno == EINTR)
        {
            continue;
        }

        else if (ret == -1)
        {
            abort();
        }

        ctr -= ret;
        pos += ret;
    }

#else

    static int fd = -1;
    ssize_t ret;

    while (fd == -1)
    {
        fd = open("/dev/urandom", O_RDONLY);

        if (fd == -1 && errno == EINTR)
        {
            continue;
        }
        else if (fd == -1)
        {
            abort();
        }
    }

    while (pos < ctr)
    {
        ret = read(fd, buf + pos, ctr);

        if (ret == -1 && errno == EINTR)
        {
            continue;
        }
        else if (ret == -1)
        {
            abort();
        }

        ctr -= ret;
        pos += ret;
    }

#endif

    shake256(out, outlen, buf, buflen);
    SECURE_ZERO(buf, buflen);
    free(buf);
    res = outlen;
}
else
{
    abort();
}

return res;

}
`

Time to live up to the moniker my friend ;o)

Cheers,
John