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