NetBSD-Bugs archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

lib/60858: fortuitous embarrassment: fortify is all kinds of busted



>Number:         60858
>Category:       lib
>Synopsis:       fortuitous embarrassment: fortify is all kinds of busted
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Oct 06 14:50:00 +0000 2026
>Originator:     Taylor R Campbell
>Release:        current, 11, 10
>Organization:
__ssp_protected_bsd, inc.
>Environment:
>Description:

	tl;dr:

	1. NetBSD 10 ssp/fortify wrappers are broken by `static inline'
	   instead of `extern inline', causing function pointer
	   equality in library symbols to break (PR bin/45200: Scp
	   hangs after sending <https://gnats.NetBSD.org/45200>, worked
	   around locally and upstream but never properly fixed, and
	   recently rediscovered, when openssh-portable upstream
	   removed the workaround, as PR pkg/60563: exposed sshd w/o
	   password-login causes sshd-auth looping
	   <https://gnats.NetBSD.org/60563>).

	2. NetBSD 11/current ssp/fortify wrappers may be broken by
	   creating symbol references that should not exist, and we are
	   now shipping symbols __ssp_protected_getcwd,
	   __ssp_protected_read, and __ssp_protected_readlink (which
	   are just trivial jump wrappers for getcwd/read/readlink) in
	   libc that should never have existed but can't be removed.

	3. We are not actually building many things in base with
	   fortify despite having USE_FORT=yes in their Makefiles,
	   because putting that _after_ including bsd.own.mk has no
	   effect.


	* Background

	Under -D_FORTIFY_SOURCE=2, various libc functions that store
	into a caller-provided buffer with caller-provided length are
	wrapped with inline functions that check the caller-provided
	length against any compile-time-known length.  This way, for
	instance, code like

		uint8_t x[2];
		read(fd, x, N);

	gets runtime bounds checks as if it had been written like

		uint8_t x[2];
		if (N > sizeof(x))
			__chk_fail();
		read(fd, x, N);

	If the size of x is not known, there is no impact.  And taking
	the address of the function read to pass a function pointer
	around shouldn't be any different with or without
	-D_FORTIFY_SOURCE=2.


	* Issues

	The mechanism of the wrappers, in src/include/ssp/ssp.h, has
	changed over the years a little bit, but from NetBSD 6 through
	NetBSD 10, it was essentially for <unistd.h> to add the
	following declarations to declare an external C function
	`__ssp_real_read' whose ELF symbol is `read', and an inline C
	function `read' that does the check and calls the ELF symbol:

ssize_t __ssp_real_read(int, void *, size_t)
    __RENAME(read);

static inline ssize_t read(int, void *, size_t)
    __RENAME(__ssp_protected_read);

static inline ssize_t
read(int fd, void *buf, size_t len)
{
        if (__builtin_object_size(buf) != (size_t)-1 &&
            len > __builtin_object_size(buf))
                __chk_fail();
        return __ssp_real_read(fd, buf, len)
}

	https://nxr.netbsd.org/xref/src/include/ssp/ssp.h?r=1.9#63

	** Issue 1: static inline is wrong (applicable to netbsd-10)

	Unfortunately, using `static inline' means that if you take the
	address of `read' after the definition, _this definition_ gets
	materialized as a distinct function from the library function
	`read' -- and each .o file has its own distinct definition,
	under symbols named __ssp_protected_read because of the middle
	declaration above:

$ head a.c b.c main.c
==> a.c <==
#include <unistd.h>
void *read_a = read;

==> b.c <==
#include <unistd.h>
void *read_b = read;

==> main.c <==
#include <stdio.h>
extern void *read_a, *read_b;
int main(void) { return read_a == read_b; }
$ rm -f *.o && make a.o b.o main.o
cc -O2   -c a.c
cc -O2   -c b.c
cc -O2   -c main.c
$ cc -o test a.o b.o main.o && ./test; echo $?
1
$ rm -f *.o && make a.o b.o main.o CPPFLAGS=-D_FORTIFY_SOURCE=2
cc -O2  -D_FORTIFY_SOURCE=2 -c a.c
cc -O2  -D_FORTIFY_SOURCE=2 -c b.c
cc -O2  -D_FORTIFY_SOURCE=2 -c main.c
$ cc -o test a.o b.o main.o && ./test; echo $?
0
$ nm test | grep __ssp_protected_
000000000040096a t __ssp_protected_read
000000000040096f t __ssp_protected_read

	Fortunately, it is relatively easy to search for _possible_
	cases of the library function pointer equality problem by just
	searching for the string `__ssp_protected_' in objdirs.

	In 2023, `static inline' was replaced by `extern inline' in
	response to PR lib/57689 (getcwd() not overridable with
	-D_FORTIFY_SOURCE <https://gnats.NetBSD.org/57689>).  That was
	good: it meant that the inline wrapper would not be
	materialized as a distinct function pointer when taking the
	address of a library function like read().

	** Issue 2: extern inline with __RENAME is wrong

	However, we are still generating this declaration with a symbol
	rename:

extern inline ssize_t read(int, void *, size_t)
    __RENAME(__ssp_protected_read);

	This causes the .o file to have a reference to an external
	symbol __ssp_protected_read.  It's not used for anything!  It's
	just required to be defined or else the program won't link, as
	observed by prlw1@ in 2023:

=> Bootstrap dependency digest>=20211023: found digest-20220214
===> Checking for vulnerabilities in ffmpeg6-6.0nb6
===> Building for ffmpeg6-6.0nb6
LD      ffmpeg6_g
LD      ffprobe6_g
ld: /usr/lib/crt0.o and /usr/lib/crt0.o: warning: multiple common of `environ'
ld: /usr/lib/crt0.o and /usr/lib/crt0.o: warning: multiple common of `environ'
ld: libavdevice/libavdevice.so: undefined reference to `__ssp_protected_read'
ld: libavdevice/libavdevice.so: undefined reference to `__ssp_protected_read'
gmake: *** [Makefile:131: ffprobe6_g] Error 1
gmake: *** Waiting for unfinished jobs....
gmake: *** [Makefile:131: ffmpeg6_g] Error 1
*** Error code 2

	https://mail-index.netbsd.org/pkgsrc-users/2023/11/13/msg038461.html

	readelf(1) shows that the file has an undefined
	__ssp_protected_read, even though it's not used by anything:

$ readelf -s a.o

Symbol table '.symtab' contains 4 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS a.c
     2: 0000000000000000     8 OBJECT  GLOBAL DEFAULT    4 read_a
     3: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND __ssp_protected_read

	This happens only because of the spurious declaration of read()
	with a symbol rename.

	** Issue 3: fortify 

	Both fortuitously and embarrassingly, this library symbol
	equality bug hasn't affected many programs in base by default
	because although we explicitly enable USE_FORT=yes in many
	Makefiles, it only takes effect if it happens _before_
	bsd.own.mk -- because bsd.own.mk uses a `.if' conditional,
	which does eager expansion, to define USE_SSP which is used to
	decide whether to add -D_FORTIFY_SOURCE=2:

    274 .if !defined(NOFORT) && ${USE_FORT:Uno} != "no"
    275 USE_SSP?=	yes
    276 .endif

	https://nxr.netbsd.org/xref/src/share/mk/bsd.own.mk?r=1.1487#274

    181 .if !defined(NOSSP) && (${USE_SSP:Uno} != "no") && (${BINDIR:Ux} != "/usr/mdec")
    182 .   if !defined(KERNSRCDIR) && !defined(KERN) # not for kernels / kern modules
    183 CPPFLAGS+=	-D_FORTIFY_SOURCE=2
    184 .   endif

	https://nxr.netbsd.org/xref/src/share/mk/bsd.sys.mk?r=1.319#181

	So use cases like usr.bin/ftp/Makefile don't do anything:

      4 .include <bsd.own.mk>
      5 
      6 USE_FORT?= yes	# network client

	https://nxr.netbsd.org/xref/src/usr.bin/ftp/Makefile?r=1.46

	You can verify this by showing all the variables:

$ nbmake-amd64 -v USE_FORT -v USE_SSP -v CPPFLAGS
yes

 --sysroot=/home/riastradh/netbsd/current/src/../obj.amd64/destdir.amd64 -DWITH_SSL -DINET6 

	It's not because of the `?=' -- if USE_FORT?=yes is moved above
	.include <bsd.own.mk>, it works:

$ nbmake-amd64 -v USE_FORT -v USE_SSP -v CPPFLAGS
yes
yes
 --sysroot=/home/riastradh/netbsd/current/src/../obj.amd64/destdir.amd64 -DWITH_SSL -DINET6  -D_FORTIFY_SOURCE=2

>How-To-Repeat:

	Please don't; this problem is pretty embarrassing!

>Fix:

	1. In NetBSD 10: change `static inline' to `extern inline'.

	   (Won't help package builds against 10.0 -- unless the
	   builders patch their `10.0' trees, which won't create new
	   ABI issue (as long as the patch covers (2) below -- but will
	   help the base system...once (3) below is done too.)

	2. In NetBSD 10/11/current: nix the
	   __RENAME(__ssp_protected_ ## fun) declaration.

	3. In bsd.own.mk, use CFLAGS+= ${...:?...:...} for lazy
	   expansion instead of `.if ... CFLAGS+= ... .endif' for eager
	   expansion.




Home | Main Index | Thread Index | Old Index