pq-crystals / dilithium

Other
374 stars 136 forks source link

Bad semaphor value in sign.c #65

Open jakemas opened 1 year ago

jakemas commented 1 year ago

A small issue within crypto_sign_open is causing Dilithium to not compile on windows x86. It is a warning treated as an error. The issue is within the function crypto_sign_open in sign.c.

int crypto_sign_open(uint8_t *m,
                     size_t *mlen,
                     const uint8_t *sm,
                     size_t smlen,
                     const uint8_t *pk)
{
  size_t i;

  if(smlen < CRYPTO_BYTES)
    goto badsig;

  *mlen = smlen - CRYPTO_BYTES;
  if(crypto_sign_verify(sm, CRYPTO_BYTES, sm + CRYPTO_BYTES, *mlen, pk))
    goto badsig;
  else {
    /* All good, copy msg, return 0 */
    for(i = 0; i < *mlen; ++i)
      m[i] = sm[CRYPTO_BYTES + i];
    return 0;
  }

badsig:
  /* Signature verification failed */
  *mlen = -1;
  for(i = 0; i < smlen; ++i)
    m[i] = 0;

  return -1;
}

The issue is that *mlen = -1; is attempting to assign -1 to an unsigned integer, giving the warning: warning C4245: '=': conversion from 'int' to 'std::size_t'

Would setting *mlen = 0; be sufficient?

I'm curious as to why *mlen = -1; is even set upon failure? It is not consistent with other verification functions in this file, i.e., it is not set to -1 in the event that crypto_sign_verify fails verification. Is this to distinguish between failure modes? Either way, the fact that the crypto_sign_open returns -1 should be enough of an indication of failure.