arekinath / pivy

Tools for using PIV tokens (like Yubikeys) as an SSH agent, for encrypting data at rest, and more
193 stars 26 forks source link

pivy-agent: super weird SIGSEGV on Linux #32

Closed vuori closed 6 months ago

vuori commented 2 years ago

I updated my pivy installation from the arekinath/pivy fork for the first time in the while. After running make distclean followed by make, pivy-tool et al. were working, but pivy-agent would crash as soon as something connected to it.

The reason turns out to be the weirdest in recent memory: prepare_poll() calls recallocarray() (which is linked from libressl compat according to gdb) with *npfdp = 0, which calls calloc(1, 8) (ultimately __libc_calloc) as expected… but the pointer returned is garbage and accessing it leads to SIGSEGV. The pointer value is usually around 0x557afc90 which is far from where one would expect. Calling calloc() from gdb returns pointers around 0x5555557af860, so it feels like there's some kind of stack/register misalignment or prototype mixup going on here.

This happens on Ubuntu 20.04 with the default gcc (9.4.0) and gcc-10 and generally the default system libraries that you get from apt.

git bisect shows that the offending commit is f73d2a7610badec71c3006e7273a3a2d1d13a225: clean up openssh, start on piv-ca module. OpenSSH also has a copy of the OpenBSD compat functions, but the resulting libssh.a doesn't include recallocarray (only xrecallocarray).

I didn't try it, but I suppose the easy fix to pivy-agent would be to use glibc reallocarray, but that might leave problems lurking in the external libraries that try to use recallocarray. Disabling LTO and changing all optimization settings to -O0 had no effect wrt ultimately crashing.

Easy workaround: go back to v0.8.0.

vuori commented 2 years ago

Of course the solution appears right after hitting submit, since I had been running just make distclean && make all the time and the warning scrolled off: pivy-agent.c does not see a prototype for recallocarray and hence thinks the return type is int. There is a declaration for reallocarray though, which is a libc function. Is that a typo?

pivy-agent.c: In function ‘prepare_poll’:
pivy-agent.c:2429:13: warning: implicit declaration of function ‘recallocarray’; did you mean ‘reallocarray’? [-Wimplicit-function-declaration]
 2429 |      (pfd = recallocarray(pfd, *npfdp, npfd, sizeof(struct pollfd))) == NULL)
      |             ^~~~~~~~~~~~~
      |             reallocarray
pivy-agent.c:2429:11: warning: assignment to ‘struct pollfd *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
 2429 |      (pfd = recallocarray(pfd, *npfdp, npfd, sizeof(struct pollfd))) == NULL)
arekinath commented 2 years ago

The libbsd overlay is supposed to declare that in stdlib.h:

$ grep -r recallocarray /usr/include/bsd
/usr/include/bsd/stdlib.h:void *recallocarray(void *ptr, size_t oldnmemb, size_t nmemb, size_t size);

That's on archlinux though, maybe Ubuntu's is missing it for some reason?

arekinath commented 2 years ago

(it's not a typo: recallocarray is to reallocarray as calloc is to malloc: it does the multiplication safely for size and zeros it for you)

vuori commented 2 years ago

/usr/include/bsd is supplied by libbsd-dev which is 0.10.0 on Ubuntu and doesn't have any memory-allocation functions. I guess they appeared in a newer version.

How about having pivy-agent.c include libressl/include/compat/stdlib.h? That looks like it should DTRT when recallocarray is being supplied by libressl's compatibility functions (it also provides the reallocarray prototype).

vuori commented 6 months ago

Finally got around to updating my copy and looks like 666bedd3138ccac62cb36a5186f8f93862282771 fixed this.