stack smashing error with Yubikey recording

[spoore1]

I'm trying to record some basic interactions with a Yubikey for FIDO2 token testing. To do this, I'm using the basic "ykman" command and record it with umockdev. When I run the recording through, I get an error about stack smashing:

[root@yubikey umockdev_test]# umockdev-record /dev/hidraw3 > yk.umockdev

[root@yubikey umockdev_test]# umockdev-record --ioctl /dev/hidraw3=yk.ioctl --script /dev/hidraw3=yk.script -- ykman info

Device type: YubiKey 5 NFC
Serial number: 14973515
Firmware version: 5.4.3
Form factor: Keychain (USB-A)
Enabled USB interfaces: OTP, FIDO, CCID
NFC transport is enabled.

Applications    USB     NFC    
FIDO2           Enabled Enabled 
OTP             Enabled Enabled 
FIDO U2F        Enabled Enabled 
OATH            Enabled Enabled 
YubiHSM Auth    Enabled Enabled 
OpenPGP         Enabled Enabled 
PIV             Enabled Enabled 

[root@yubikey umockdev_test]# umockdev-run --device yk.umockdev --ioctl /dev/hidraw3=yk.ioctl --script /dev/hidraw3=yk.script -- ykman info

*** stack smashing detected ***: terminated

And after help from @ueno I see this from valgrand:

[root@yubikey umockdev_test]# cat random.c
#include <stdlib.h>
#include <string.h>

ssize_t __attribute__ ((visibility ("protected")))
getrandom (void *buf, size_t buflen, unsigned int flags)
{
  memset (buf, 0x1, buflen);
  return (ssize_t)buflen;
}

[root@yubikey umockdev_test]# gcc -fPIC -shared -o random.so random.c

[root@yubikey umockdev_test]# export LD_PRELOAD=$PWD/random.so

[root@yubikey umockdev_test]# umockdev-record /dev/hidraw2 > fido2.umockdev

[root@yubikey umockdev_test]# umockdev-record --ioctl /dev/hidraw2=fido2.ioctl --script /dev/hidraw2=fido2.script -- fido2-token -I /dev/hidraw2
proto: 0x02
major: 0x05
minor: 0x04
build: 0x03
caps: 0x05 (wink, cbor, msg)
version strings: U2F_V2, FIDO_2_0, FIDO_2_1_PRE
extension strings: credProtect, hmac-secret
transport strings: nfc, usb
algorithms: es256 (public-key), eddsa (public-key)
aaguid: 2fc0579f811347eab116bb5a8db9202a
options: rk, up, noplat, clientPin, credentialMgmtPreview
maxmsgsiz: 1200
maxcredcntlst: 8
maxcredlen: 128
fwversion: 0x50403
pin protocols: 2, 1
pin retries: 8
uv retries: undefined

[root@yubikey umockdev_test]# umockdev-run --device fido2.umockdev --ioctl /dev/hidraw2=fido2.ioctl --script /dev/hidraw2=fido2.script -- valgrind fido2-token -I /dev/hidraw2
==1841== Memcheck, a memory error detector
==1841== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==1841== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==1841== Command: fido2-token -I /dev/hidraw2
==1841== 
==1841== Syscall param socketcall.sendto(msg) points to uninitialised byte(s)
==1841==    at 0x4E31670: send (send.c:28)
==1841==    by 0x48719C0: remote_emulate (libumockdev-preload.c:634)
==1841==    by 0x4876CFF: ioctl (libumockdev-preload.c:1794)
==1841==    by 0x4CD8976: get_report_descriptor (hid_linux.c:46)
==1841==    by 0x4CD9524: fido_hid_open (hid_linux.c:274)
==1841==    by 0x4CC3EDB: fido_dev_open_tx (dev.c:133)
==1841==    by 0x4CCF4D8: UnknownInlinedFun (dev.c:253)
==1841==    by 0x4CCF4D8: fido_dev_open (dev.c:364)
==1841==    by 0x10FBB4: open_dev (util.c:169)
==1841==    by 0x112B8B: token_info (token.c:211)
==1841==    by 0x10EBE7: main (fido2-token.c:93)
==1841==  Address 0x1ffeffee74 is on thread 1's stack
==1841==  in frame #4, created by fido_hid_open (hid_linux.c:241)
==1841== 
proto: 0x02
major: 0x05
minor: 0x04
build: 0x03
caps: 0x05 (wink, cbor, msg)
version strings: U2F_V2, FIDO_2_0, FIDO_2_1_PRE
extension strings: credProtect, hmac-secret
transport strings: nfc, usb
algorithms: es256 (public-key), eddsa (public-key)
aaguid: 2fc0579f811347eab116bb5a8db9202a
options: rk, up, noplat, clientPin, credentialMgmtPreview
maxmsgsiz: 1200
maxcredcntlst: 8
maxcredlen: 128
fwversion: 0x50403
pin protocols: 2, 1
pin retries: 8
uv retries: undefined
==1841== 
==1841== HEAP SUMMARY:
==1841==     in use at exit: 0 bytes in 0 blocks
==1841==   total heap usage: 272 allocs, 272 frees, 12,627 bytes allocated
==1841== 
==1841== All heap blocks were freed -- no leaks are possible
==1841== 
==1841== Use --track-origins=yes to see where uninitialised values come from
==1841== For lists of detected and suppressed errors, rerun with: -s
==1841== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

[spoore1]

Attaching the umockdev files for both examples above.

umockdev_files.tar.gz

[benzea]

==1841==  Address 0x1ffeffee74 is on thread 1's stack
==1841==  in frame #4, created by fido_hid_open (hid_linux.c:241)

Well, yes and no. It is either a libfido bug or a false positive. i.e. the issue is that we are sending the hdr structure over to the parent process for emulation, and this reads uninitialized memory.

If that memory doesn't need to be initialized (for the kernel), then this can be safely ignored. Thing is here, that umockdev always does a read, even if a write would be sufficient. As such, false positives like this are likely to happen.

EDIT: Said differently, I see the following possibilities:

1. It is an error, initialize add = {0,} to initialize the stack structure.
2. It is a false positive, work around it in the code by initializing it
3. It is a false positive, add it to the suppression file to be ignored.

One could improve it in umockdev, but not easily. i.e. it would require adding API to only do partial memory reads/writes making the emulation layer more complicated. I don't expect that this happens often enough to make it worthwhile, i.e. it is easier to just deal with the false positive in the test.

[benzea]

OK, talked a bit too early. So, the above is true for fido2-token and the valgrind error has nothing to do with the stack smash error in the python code (i.e. false positive). The problem is the following code combined with umockdev's emulation:

        # Read report descriptor
        buf = array("B", [0] * 4)
        fcntl.ioctl(f, HIDIOCGRDESCSIZE, buf, True)
        size = struct.unpack("<I", buf)[0]
        buf += array("B", [0] * size)
        fcntl.ioctl(f, HIDIOCGRDESC, buf, True)

i.e. it first reads the size into the header of the structure. Then it allocates enough memory and fetches it. The kernel will only write min(size in struct, 4kiB) of the memory. umockdev however writes the whole 4kiB + 4B (even overwriting the size).

This can be fixed in umockdev by adding a custom implementation for the IOCTL that will only update the correct memory areas. It isn't overly hard, but it is a bit of a pain (unless one accepts reading/writing the full 4kiB + 4B but not actually modifying the tail, which wouldn't be thread safe).

[benzea]

It isn't overly hard, but it is a bit of a pain

I should have added what this means:

1. In umockdev-ioctl.vala, update handle_ioctl to resolve only the amount of memory needed. This is a bit of a pain, as it means first resolving the first 4 bytes, then doing the same for the correct length.
2. Fix IoctlData.resolve to update the length of the data object and reload the rest of the data (so the data object remains valid for the pointer).
3. Move the ioctl to use I_VARLEN_STRUCT_IN, though the code is not compiled currently.

Note that in 2. and 3. we duplicate the size detection logic in umockdev-ioctl.vala and ioctl_tree.c. Not elegant, but everything else would require more code refactorings.