Skip to content

Commit

Permalink
Kyber symmetric shake: avoid ASAN error
Browse files Browse the repository at this point in the history 
The FIPS202x2 implementation does not play well with ASAN. Specifically,
when handling the trailing blocks that are not divisible by 8, the
implementation uses an 8 byte load followed by a mask to process the
trailing bytes of the input
(https://github.com/open-quantum-safe/liboqs/blob/d93a431aaf9ac929f267901509e968a5727c053c/src/kem/kyber/oldpqclean_kyber512_aarch64/fips202x2.c#L475-L476)

While this may work in practice, performing an 8 byte load on an input
with fewer than 8 bytes remaining leads to UB (and an ASAN error) when
the input size cannot accomodate such a load.

A simple repro, built with `-fsanitize=address` shows this:

```c

static void keypair(const char* alg) {
        OQS_KEM* kem = OQS_KEM_new(alg);
        unsigned char* public = malloc(kem->length_public_key);
        unsigned char* sec = malloc(kem->length_secret_key);

        kem->keypair(public, sec);
        free(sec);
        free(public);
        OQS_KEM_free(kem);
}
int main() {
        keypair(OQS_KEM_alg_kyber_512);
        keypair(OQS_KEM_alg_kyber_768);
        keypair(OQS_KEM_alg_kyber_1024);
        return 0;
}
```

results in the following error:

```
=================================================================
==613880==ERROR: AddressSanitizer: stack-buffer-overflow on address
0xfffff57a5160 at pc 0x0000004e9ef4 bp 0xfffff57a5030 sp 0xfffff57a5020
READ of size 8 at 0xfffff57a5160 thread T0
    #0 0x4e9ef0 in keccakx2_absorb.constprop.1
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4e9ef0)
    #1 0x4a608c in PQCLEAN_KYBER512_AARCH64_neon_kyber_shake128_absorb
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a608c)
    #2 0x4a51a8 in PQCLEAN_KYBER512_AARCH64_gen_matrix
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a51a8)
    #3 0x4a5360 in PQCLEAN_KYBER512_AARCH64_indcpa_keypair
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a5360)
    #4 0x45e460 in PQCLEAN_KYBER512_AARCH64_crypto_kem_keypair
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x45e460)
    #5 0x4075c0 in keypair
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4075c0)
    #6 0x405598 in main
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x405598)
    #7 0xfffcbbd772fc in __libc_start_call_main
(/lib64/libc.so.6+0x272fc)
    #8 0xfffcbbd773d4 in __libc_start_main_impl
(/lib64/libc.so.6+0x273d4)
    #9 0x40746c in _start
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x40746c)

Address 0xfffff57a5160 is located in stack of thread T0 at offset 64 in
frame
    #0 0x4a5efc in PQCLEAN_KYBER512_AARCH64_neon_kyber_shake128_absorb
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4a5efc)

  This frame has 2 object(s):
    [32, 66) 'extseed1' (line 59) <== Memory access at offset 64
partially overflows this variable
    [112, 146) 'extseed2' (line 60)
HINT: this may be a false positive if your program uses some custom
stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow
(/data/users/mingtao/3/oqsrepro/buildopt/oqsrepro+0x4e9ef0) in
keccakx2_absorb.constprop.1
Shadow bytes around the buggy address:
  0x200ffeaf49d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ffeaf49e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ffeaf49f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ffeaf4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ffeaf4a10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x200ffeaf4a20: 00 00 00 00 f1 f1 f1 f1 00 00 00 00[02]f2 f2 f2
  0x200ffeaf4a30: f2 f2 00 00 00 00 02 f3 f3 f3 f3 f3 00 00 00 00
  0x200ffeaf4a40: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  0x200ffeaf4a50: f1 f1 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ffeaf4a60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ffeaf4a70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==613880==ABORTING
```
This PR sidesteps this issue by ensuring that `extseed1` and `extseed2`
are sized to be divisible by 8, which avoids the problematic codepath.

Other alternatives considered for this PR were to modify the
`keccakx2_absorb` implementation (e.g. to memcpy the remaining data
into a local 8 byte buffer first) -- however this approach inhibited
inlining of the absorb implementation and resulted in a call to `memcpy`
which could not be optimized out due to the variable-length `inlen`.

Tested with `ninja run_tests` and ensured that ASAN repro no longer
fires.

Co-Authored-By: Kyle Nekritz <[email protected]>
  • Loading branch information
@mingtaoy
2 people authored and mingtaoy committed Sep 4, 2024
1 parent d93a431 commit b871739
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 9 deletions.
12 changes: 9 additions & 3 deletions src/kem/kyber/oldpqclean_kyber1024_aarch64/neon_symmetric-shake.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

#include <stddef.h>
#include <stdint.h>
#include <assert.h>
#include "params.h"
#include "fips202x2.h"
#include "symmetric.h"
Expand All @@ -56,8 +57,13 @@ void neon_kyber_shake128_absorb(keccakx2_state *state,
uint8_t y1, uint8_t y2)
{
unsigned int i;
uint8_t extseed1[KYBER_SYMBYTES+2];
uint8_t extseed2[KYBER_SYMBYTES+2];

#define ROUNDUP(x,n) ((x+n-1)/n*n)
#define SEEDSIZE (KYBER_SYMBYTES+2)
uint8_t extseed1[ROUNDUP(SEEDSIZE, 8)];
uint8_t extseed2[ROUNDUP(SEEDSIZE, 8)];
assert(sizeof(extseed1) % 8 == 0);
assert(sizeof(extseed2) % 8 == 0);

for(i=0;i<KYBER_SYMBYTES;i++){
extseed1[i] = seed[i];
Expand All @@ -69,7 +75,7 @@ void neon_kyber_shake128_absorb(keccakx2_state *state,
extseed2[KYBER_SYMBYTES ] = x2;
extseed2[KYBER_SYMBYTES+1] = y2;

shake128x2_absorb(state, extseed1, extseed2, sizeof(extseed1));
shake128x2_absorb(state, extseed1, extseed2, SEEDSIZE);
}

/*************************************************
Expand Down
12 changes: 9 additions & 3 deletions src/kem/kyber/oldpqclean_kyber512_aarch64/neon_symmetric-shake.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

#include <stddef.h>
#include <stdint.h>
#include <assert.h>
#include "params.h"
#include "fips202x2.h"
#include "symmetric.h"
Expand All @@ -56,8 +57,13 @@ void neon_kyber_shake128_absorb(keccakx2_state *state,
uint8_t y1, uint8_t y2)
{
unsigned int i;
uint8_t extseed1[KYBER_SYMBYTES+2];
uint8_t extseed2[KYBER_SYMBYTES+2];

#define ROUNDUP(x,n) ((x+n-1)/n*n)
#define SEEDSIZE (KYBER_SYMBYTES+2)
uint8_t extseed1[ROUNDUP(SEEDSIZE, 8)];
uint8_t extseed2[ROUNDUP(SEEDSIZE, 8)];
assert(sizeof(extseed1) % 8 == 0);
assert(sizeof(extseed2) % 8 == 0);

for(i=0;i<KYBER_SYMBYTES;i++){
extseed1[i] = seed[i];
Expand All @@ -69,7 +75,7 @@ void neon_kyber_shake128_absorb(keccakx2_state *state,
extseed2[KYBER_SYMBYTES ] = x2;
extseed2[KYBER_SYMBYTES+1] = y2;

shake128x2_absorb(state, extseed1, extseed2, sizeof(extseed1));
shake128x2_absorb(state, extseed1, extseed2, SEEDSIZE);
}

/*************************************************
Expand Down
12 changes: 9 additions & 3 deletions src/kem/kyber/oldpqclean_kyber768_aarch64/neon_symmetric-shake.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

#include <stddef.h>
#include <stdint.h>
#include <assert.h>
#include "params.h"
#include "fips202x2.h"
#include "symmetric.h"
Expand All @@ -56,8 +57,13 @@ void neon_kyber_shake128_absorb(keccakx2_state *state,
uint8_t y1, uint8_t y2)
{
unsigned int i;
uint8_t extseed1[KYBER_SYMBYTES+2];
uint8_t extseed2[KYBER_SYMBYTES+2];

#define ROUNDUP(x,n) ((x+n-1)/n*n)
#define SEEDSIZE (KYBER_SYMBYTES+2)
uint8_t extseed1[ROUNDUP(SEEDSIZE, 8)];
uint8_t extseed2[ROUNDUP(SEEDSIZE, 8)];
assert(sizeof(extseed1) % 8 == 0);
assert(sizeof(extseed2) % 8 == 0);

for(i=0;i<KYBER_SYMBYTES;i++){
extseed1[i] = seed[i];
Expand All @@ -69,7 +75,7 @@ void neon_kyber_shake128_absorb(keccakx2_state *state,
extseed2[KYBER_SYMBYTES ] = x2;
extseed2[KYBER_SYMBYTES+1] = y2;

shake128x2_absorb(state, extseed1, extseed2, sizeof(extseed1));
shake128x2_absorb(state, extseed1, extseed2, SEEDSIZE);
}

/*************************************************
Expand Down

0 comments on commit b871739

Please sign in to comment.