Potential buffer overread in ctap_encode_der_sig()

[szszszsz]

Description

It looks like memmove reads (writes?) one byte too much, while writing final result of the ctap_encode_der_sig() function. It does not occur always - probably only in case for maximum signature length.
Found, while running simulation binary compiled with -lasan -fsanitize=address -O1 -g -fno-omit-frame-pointer. I think it was confirmed by Clang's scan_build tool as well (to add later).

Frequency: sometimes (1/10)

Environment

• Fedora 29
• gcc (GCC) 8.2.1 20181215 (Red Hat 8.2.1-6)
• libasan.so.5 => /lib64/libasan.so.5

Reproduction route

1. Start simulation, built with memory sanitizer
2. Run Python test: tools/ctap_test.py 10 times
3. Simulation should crash, with similar report as below

With WIP code, from https://github.com/Nitrokey/nitrokey-fido2-firmware/tree/testing:

1. make clean
2. make test_simulation

Logs

ASAN report (click)

==20405==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffcb8932510 
at pc 0x7f31031e5ef7 bp 0x7ffcb8932390 sp 0x7ffcb8931b38 
READ of size 32 at 0x7ffcb8932510 thread T0
    #0 0x7f31031e5ef6 in __interceptor_memmove (/lib64/libasan.so.5+0x38ef6)
    #1 0x40e54c in ctap_encode_der_sig fido2/ctap.c:474
    #2 0x40e654 in ctap_calculate_signature fido2/ctap.c:496
    #3 0x41031e in ctap_end_get_assertion fido2/ctap.c:875
    #4 0x4110a0 in ctap_get_assertion fido2/ctap.c:1048
    #5 0x412c2d in ctap_request fido2/ctap.c:1412
    #6 0x40b817 in ctaphid_handle_packet fido2/ctaphid.c:638
    #7 0x413325 in main fido2/main.c:91
    #8 0x7f310300b412 in __libc_start_main (/lib64/libc.so.6+0x24412)
    #9 0x40241d in _start (/home/sz/work/conor-solo/solo-simulation-main+0x40241d)

Address 0x7ffcb8932510 is located in stack of thread T0 at offset 160 in frame
    #0 0x410065 in ctap_end_get_assertion fido2/ctap.c:858

  This frame has 3 object(s):
    [32, 64) 'desc'
    [96, 160) 'sigbuf' <== Memory access at offset 160 overflows this variable
    [192, 264) 'sigder' <== Memory access at offset 160 underflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (/lib64/libasan.so.5+0x38ef6) in __interceptor_memmove
Shadow bytes around the buggy address:
  0x10001711e450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001711e460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001711e470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001711e480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1
  0x10001711e490: f1 f1 f8 f8 f8 f8 f2 f2 f2 f2 00 00 00 00 00 00
=>0x10001711e4a0: 00 00[f2]f2 f2 f2 00 00 00 00 00 00 00 00 00 f2
  0x10001711e4b0: f2 f2 f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00
  0x10001711e4c0: 00 00 00 00 f1 f1 f1 f1 00 00 00 00 f2 f2 f2 f2
  0x10001711e4d0: 00 00 00 00 05 f2 f2 f2 f2 f2 f2 f2 00 00 00 00
  0x10001711e4e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001711e4f0: 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
==20405==ABORTING

References

• https://github.com/solokeys/solo/blob/4c36a752cb53e08a5a1202d99dc944c797de56b5/fido2/ctap.c#L475
• Nitrokey@0a8a40f#diff-0f34b62f55a3dab9eb4fa37e853851b6R481

Edit: add env info
Edit: remove whitespace
Edit: add scenario

[szszszsz]

Scan build complains about it as well:

fido2/ctap.c:481:5: warning: Memory copy function accesses out-of-bound array element 
    memmove(sigder + 6 + 32 + pad_r + pad_s - lead_r, sigbuf + 32 + lead_s, 32); 
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 

clang version 7.0.1 (Fedora 7.0.1-1.fc29)

[szszszsz]

It seems that I have found the fix (it is not triggering ASan at least; see linked commit). Tested on the simulated device, later will check on hardware.

[conorpp]

Great find! Thank you