Skip to content

Commit

Permalink
Removed "/KYBER_Q" in poly_compress and polyvec_compress; thanks to P…
Browse files Browse the repository at this point in the history 
…rasanna Ravi and Matthias Kannwischer for pointing out that a DIV instruction could turn into a plaintext-checking oracle
  • Loading branch information
@cryptojedi
cryptojedi committed Dec 30, 2023
1 parent dda29cc commit 272125f
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 10 deletions.
15 changes: 13 additions & 2 deletions ref/poly.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ void poly_compress(uint8_t r[KYBER_POLYCOMPRESSEDBYTES], const poly *a)
{
unsigned int i,j;
int16_t u;
uint32_t d0;
uint8_t t[8];

#if (KYBER_POLYCOMPRESSEDBYTES == 128)
Expand All @@ -27,7 +28,12 @@ void poly_compress(uint8_t r[KYBER_POLYCOMPRESSEDBYTES], const poly *a)
// map to positive standard representatives
u = a->coeffs[8*i+j];
u += (u >> 15) & KYBER_Q;
t[j] = ((((uint16_t)u << 4) + KYBER_Q/2)/KYBER_Q) & 15;
/* t[j] = ((((uint16_t)u << 4) + KYBER_Q/2)/KYBER_Q) & 15; */
d0 = u << 4;
d0 += 1665;
d0 *= 80635;
d0 >>= 28;
t[j] = d0 & 0xf;
}

r[0] = t[0] | (t[1] << 4);
Expand All @@ -42,7 +48,12 @@ void poly_compress(uint8_t r[KYBER_POLYCOMPRESSEDBYTES], const poly *a)
// map to positive standard representatives
u = a->coeffs[8*i+j];
u += (u >> 15) & KYBER_Q;
t[j] = ((((uint32_t)u << 5) + KYBER_Q/2)/KYBER_Q) & 31;
/* t[j] = ((((uint32_t)u << 5) + KYBER_Q/2)/KYBER_Q) & 31; */
d0 = u << 5;
d0 += 1664;
d0 *= 40318;
d0 >>= 27;
t[j] = d0 & 0x1f;
}

r[0] = (t[0] >> 0) | (t[1] << 5);
Expand Down
17 changes: 15 additions & 2 deletions ref/polyvec.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
void polyvec_compress(uint8_t r[KYBER_POLYVECCOMPRESSEDBYTES], const polyvec *a)
{
unsigned int i,j,k;
uint64_t d0;

#if (KYBER_POLYVECCOMPRESSEDBYTES == (KYBER_K * 352))
uint16_t t[8];
Expand All @@ -23,7 +24,13 @@ void polyvec_compress(uint8_t r[KYBER_POLYVECCOMPRESSEDBYTES], const polyvec *a)
for(k=0;k<8;k++) {
t[k] = a->vec[i].coeffs[8*j+k];
t[k] += ((int16_t)t[k] >> 15) & KYBER_Q;
t[k] = ((((uint32_t)t[k] << 11) + KYBER_Q/2)/KYBER_Q) & 0x7ff;
/* t[k] = ((((uint32_t)t[k] << 11) + KYBER_Q/2)/KYBER_Q) & 0x7ff; */
d0 = t[k];
d0 <<= 11;
d0 += 1664;
d0 *= 645084;
d0 >>= 31;
t[k] = d0 & 0x7ff;
}

r[ 0] = (t[0] >> 0);
Expand All @@ -47,7 +54,13 @@ void polyvec_compress(uint8_t r[KYBER_POLYVECCOMPRESSEDBYTES], const polyvec *a)
for(k=0;k<4;k++) {
t[k] = a->vec[i].coeffs[4*j+k];
t[k] += ((int16_t)t[k] >> 15) & KYBER_Q;
t[k] = ((((uint32_t)t[k] << 10) + KYBER_Q/2)/ KYBER_Q) & 0x3ff;
/* t[k] = ((((uint32_t)t[k] << 10) + KYBER_Q/2)/ KYBER_Q) & 0x3ff; */
d0 = t[k];
d0 <<= 10;
d0 += 1665;
d0 *= 1290167;
d0 >>= 32;
t[k] = d0 & 0x3ff;
}

r[0] = (t[0] >> 0);
Expand Down
6 changes: 3 additions & 3 deletions ref/test_kyber.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

#define NTESTS 1000

static int test_keys()
static int test_keys(void)
{
uint8_t pk[CRYPTO_PUBLICKEYBYTES];
uint8_t sk[CRYPTO_SECRETKEYBYTES];
Expand All @@ -31,7 +31,7 @@ static int test_keys()
return 0;
}

static int test_invalid_sk_a()
static int test_invalid_sk_a(void)
{
uint8_t pk[CRYPTO_PUBLICKEYBYTES];
uint8_t sk[CRYPTO_SECRETKEYBYTES];
Expand Down Expand Up @@ -59,7 +59,7 @@ static int test_invalid_sk_a()
return 0;
}

static int test_invalid_ciphertext()
static int test_invalid_ciphertext(void)
{
uint8_t pk[CRYPTO_PUBLICKEYBYTES];
uint8_t sk[CRYPTO_SECRETKEYBYTES];
Expand Down
2 changes: 1 addition & 1 deletion ref/test_speed.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
uint64_t t[NTESTS];
uint8_t seed[KYBER_SYMBYTES] = {0};

int main()
int main(void)
{
unsigned int i;
uint8_t pk[CRYPTO_PUBLICKEYBYTES];
Expand Down
4 changes: 2 additions & 2 deletions runtests.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ fi

if [ "$ARCH" = "amd64" -o "$ARCH" = "arm64" ]; then
export CC="clang"
export CFLAGS="-fsanitize=address,undefined ${CFLAGS}"
# export CFLAGS="-fsanitize=address,undefined ${CFLAGS}"
fi

for dir in $DIRS; do
make -j$(nproc) -C $dir
for alg in 512 768 1024 512-90s 768-90s 1024-90s; do
#valgrind --vex-guest-max-insns=25 ./$dir/test_kyber$alg
valgrind --vex-guest-max-insns=25 ./$dir/test_kyber$alg
./$dir/test_kyber$alg &
PID1=$!
./$dir/test_kex$alg &
Expand Down

3 comments on commit 272125f

@DavidBuchanan314
Copy link

@DavidBuchanan314 DavidBuchanan314 commented on 272125f Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have some instances of KYBER_Q/2 been replaced with 1664, and others 1665? Isn't the correct value 1664? Perhaps I don't understand the barrett reduction properly.

(edit: ok I tested it against my own version and the numbers all match, so I guess I'm indeed not understanding the specifics of the barrett reduction)

@cryptojedi
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to rounding. The reductions are exhaustively tested to produce the correct output for all inputs.

@Alikhosravi22
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"In the poly_compress and polyvec_compress functions, different multiplication factors (such as 80635, 40318, 645084, and 1290167) are used. Why are these factors different, and how were they computed? Additionally, in the poly_tomsg function, the line t += ((int16_t)t >> 15) & KYBER_Q is commented out, while it remains active in the poly_compress and polyvec_compress functions. Why was this change made in poly_tomsg?"

Please sign in to comment.