build: raise our FORTIFY_SOURCE default to 2

[kevans]

Summary

FORTIFY_SOURCE would have helped slightly mitigate at least some of
the recent SAs and we have no remaining test issues with it raised. I
have received enough signal from folks running FORTIFY_SOURCE == 2 to
conclude that this shouldn't brick most systems, but some caution is
still warranted.

Developers are not expected to require any workflow changes; these shims
don't normally cause any new problems. Most of the build issues fixed
since inception have been in the underlying implementation of fortified
functions, as definitions need to be decorated and some uses need to be
protected from macro expansion.

[emaste]

I have been running with this change on my daily driver laptop since Nov 2024

[kevans]

I'm going to request an exp-run to be safe once bugzilla comes back online, unless I can find someone here with a beefy build server to poudriere bulk -a it a little more quickly

[markj]

How does one verify that a given executable is compiled with FORTIFY_SOURCE > 0?

[khorben]

In D57628#1321573, @markj wrote:

How does one verify that a given executable is compiled with FORTIFY_SOURCE > 0?

AFAICT it works best when combined with -fstack-protector, and it depends on each and every libc call performed by the executable.

I just had a look at a simple program calling gethostname() (on amd64), and it injects a test right before the call.
The magic happens in /usr/include/ssp/*.h.

[kevans]

In D57628#1321586, @khorben wrote:

In D57628#1321573, @markj wrote:

How does one verify that a given executable is compiled with FORTIFY_SOURCE > 0?

AFAICT it works best when combined with -fstack-protector, and it depends on each and every libc call performed by the executable.

I just had a look at a simple program calling gethostname() (on amd64), and it injects a test right before the call.
The magic happens in /usr/include/ssp/*.h.

So, the problem is that I don't think you can rely on that- with the way the __ssp_redirect* are designed. All of them are expected to be inlined and with suitable optimization levels, the intermediate stuff /should/ go away in a lot cases. Specifically, the main added logic is:

#define __ssp_check(buf, len, bos) \                                            
        if (bos(buf) != (size_t)-1 && (size_t)len > bos(buf)) \                 
                __chk_fail()

Where bos is __builtin_object_size, effectively- I'd expect these lines to dissolve completely if it returns -1 (can't deduce the size of the buffer that you're passing in) or if it returns a valid size and len can be determined at compile time.

The flip-side of this is that I didn't override NetBSD's decision to keep the string functions rolling through a stub in libc, for better or worse, so references to at least one of those will almost certainly appear in a fortified program:

void *__memcpy_chk(void *, const void *, size_t, size_t);                       
void *__memmove_chk(void *, const void *, size_t, size_t);                      
void *__memset_chk(void *, int, size_t, size_t);                                
char *__stpcpy_chk(char *, const char *, size_t);                               
char *__stpncpy_chk(char *, const char *, size_t, size_t);                      
char *__strcat_chk(char *, const char *, size_t);                               
char *__strcpy_chk(char *, const char *, size_t);                               
char *__strncat_chk(char *, const char *, size_t, size_t);                      
size_t __strlcat_chk(char *, const char *, size_t, size_t);                     
char *__strncpy_chk(char *, const char *, size_t, size_t);                      
size_t __strlcpy_chk(char *, const char *, size_t, size_t);

(I think many or most of these could be rewritten as __ssp_redirects instead, and maybe that's worth it to avoid an extra layer of indirection.)

[kevans]

I /was/ going to do an exp-run, but I think @netchild has likely already solved all of the problems we'd see with the fortify feature work in ports. Well, one point that we may need to actually address, though: FORTIFY_UNSAFE won't be an effective hammer once the default is lifted, because it won't provide the CFLAGS that override our current bsd.sys.mk logic.

I considered something like the following to make fortify.mk more authoritative, but I'd want to solicit Alxexander's opinion. It flips to a default WITH option at the same time because the =0 logic won't apply without it, and it's probaby not unreasonable to flip ports on?

diff --git a/Mk/Features/fortify.mk b/Mk/Features/fortify.mk
index 2e43ca98242f..24d31faef1f5 100644
--- a/Mk/Features/fortify.mk
+++ b/Mk/Features/fortify.mk
@@ -11,6 +11,10 @@ FORTIFY_Include_MAINTAINER=  [e-mail]
 
 .  if !defined(FORTIFY_UNSAFE)
 FORTIFY_SOURCE?=2
+.  else
+FORTIFY_SOURCE?=0
+.  endif
+
 FORTIFY_CFLAGS?=       -D_FORTIFY_SOURCE=${FORTIFY_SOURCE}
 CFLAGS+=       ${FORTIFY_CFLAGS}
 CXXFLAGS+=     ${FORTIFY_CFLAGS}
diff --git a/Mk/bsd.port.mk b/Mk/bsd.port.mk
index 9e2f1ffb0285..e323825b4c2e 100644
--- a/Mk/bsd.port.mk
+++ b/Mk/bsd.port.mk
@@ -1003,7 +1003,7 @@ LC_ALL=           C
 # by individual Makefiles or local system make configuration.
 _LIST_OF_WITH_FEATURES=        bind_now debug debuginfo fortify lto pie relro \
                        sanitize ssp stack_autoinit testing zeroregs
-_DEFAULT_WITH_FEATURES=        ssp
+_DEFAULT_WITH_FEATURES=        fortify ssp
 PORTSDIR?=             /usr/ports
 LOCALBASE?=            /usr/local
 LINUXBASE?=            /compat/linux

[kevans]

FORTIFY_UNSAFE won't be an effective hammer once the default is lifted, because it won't provide the CFLAGS that override our current bsd.sys.mk logic.

With the caveat noted that most ports probably don't build with bmake, so the blast radius probably isn't that bad?

[netchild]

This is what I have, this is historically grown and may or may not be an issue today:

WITHOUT_FORTIFY_PORTS=misc/mbuffer devel/libepoll-shim lang/gcc* devel/gperf devel/bsddialog textproc/jade math/mpfr security/rhash devel/libudev-devd net/norm devel/glib21 devel/py-libzfs math/mpfi devel/cmake-core devel/re2c net-p2p/libutp textproc/xmlstarlet textproc/groff net/samba419 www/qt6-webengine science/hdf5 graphics/netpbm audio/cdparanoia multimedia/libv4l emulators/wine-proton audio/linux-c7-alsa-plugins-oss
WITHOUT_PIE_PORTS=science/py-scipy security/crowdsec-firewall-bouncer databases/redis security/libsecret
WITHOUT_ZEROREGS_PORTS=math/openblas

The gcc ports are most probably something we need to exclude.

Asking for an exp run is probably not a bad idea. I only build a handfull of ports, about 473 in one instance, 190 in a second, and around 600-ish in a 3rd.
There are overlappings in those 3 lists.

Do all supported releases have fortify support (do we need to make the ports stuff conditional on the release it builds on)?

[netchild]

@kevans would it help if I build a known failing port with your ports-patch applied?

[netchild]

Without doing any build-test, should the UNSAFE part be "=0" instead of "?=0"? We want a hard override in the UNSAFE case, don't we?

[kevans]

In D57628#1321946, @netchild wrote:

Without doing any build-test, should the UNSAFE part be "=0" instead of "?=0"? We want a hard override in the UNSAFE case, don't we?

Ah, yes, I will fix that, thanks!

In D57628#1321940, @netchild wrote:

This is what I have, this is historically grown and may or may not be an issue today:

WITHOUT_FORTIFY_PORTS=misc/mbuffer devel/libepoll-shim lang/gcc* devel/gperf devel/bsddialog textproc/jade math/mpfr security/rhash devel/libudev-devd net/norm devel/glib21 devel/py-libzfs math/mpfi devel/cmake-core devel/re2c net-p2p/libutp textproc/xmlstarlet textproc/groff net/samba419 www/qt6-webengine science/hdf5 graphics/netpbm audio/cdparanoia multimedia/libv4l emulators/wine-proton audio/linux-c7-alsa-plugins-oss
WITHOUT_PIE_PORTS=science/py-scipy security/crowdsec-firewall-bouncer databases/redis security/libsecret
WITHOUT_ZEROREGS_PORTS=math/openblas

The gcc ports are most probably something we need to exclude.

Asking for an exp run is probably not a bad idea. I only build a handfull of ports, about 473 in one instance, 190 in a second, and around 600-ish in a 3rd.
There are overlappings in those 3 lists.

Do all supported releases have fortify support (do we need to make the ports stuff conditional on the release it builds on)?
[...]
@kevans would it help if I build a known failing port with your ports-patch applied?

All supported releases should have it, but it is also harmless to define any of this when it isn't -- I'd probably avoid complicating it too much.

If you don't object to the ports patch as the maintainer, I'll just go ahead and request an exp-run with both patches applied (with the assignment you noted fixed). I had hoped to land this this week, but I also don't want antoine throwing tomatoes at me if we fundamentally break the ports tree. =-)

[netchild]

In D57628#1321971, @kevans wrote:

All supported releases should have it, but it is also harmless to define any of this when it isn't -- I'd probably avoid complicating it too much.

Sounds sensible.

If you don't object to the ports patch as the maintainer, I'll just go ahead and request an exp-run with both patches applied (with the assignment you noted fixed). I had hoped to land this this week, but I also don't want antoine throwing tomatoes at me if we fundamentally break the ports tree. =-)

I don't object for sure. But I suggest to use this instead:

diff --git Mk/Features/fortify.mk Mk/Features/fortify.mk
index 2e43ca98242..86d149ff68e 100644
--- Mk/Features/fortify.mk
+++ Mk/Features/fortify.mk
@@ -14,5 +14,7 @@ FORTIFY_SOURCE?=2
 FORTIFY_CFLAGS?=       -D_FORTIFY_SOURCE=${FORTIFY_SOURCE}
 CFLAGS+=       ${FORTIFY_CFLAGS}
 CXXFLAGS+=     ${FORTIFY_CFLAGS}
+.  else
+FORTIFY_SOURCE=0
 .  endif
 .endif

I do not expect much failures. But I expect some. I expect at least the gcc ports to fail. I start a local build with this now, and I remove my WITHOUT_FORTIFY_PORTS to validate. Sunday I may be able to provide some feedback.

[kevans]

In D57628#1322468, @netchild wrote:

In D57628#1321971, @kevans wrote:

All supported releases should have it, but it is also harmless to define any of this when it isn't -- I'd probably avoid complicating it too much.

Sounds sensible.

If you don't object to the ports patch as the maintainer, I'll just go ahead and request an exp-run with both patches applied (with the assignment you noted fixed). I had hoped to land this this week, but I also don't want antoine throwing tomatoes at me if we fundamentally break the ports tree. =-)

I don't object for sure. But I suggest to use this instead:

diff --git Mk/Features/fortify.mk Mk/Features/fortify.mk
index 2e43ca98242..86d149ff68e 100644
--- Mk/Features/fortify.mk
+++ Mk/Features/fortify.mk
@@ -14,5 +14,7 @@ FORTIFY_SOURCE?=2
 FORTIFY_CFLAGS?=       -D_FORTIFY_SOURCE=${FORTIFY_SOURCE}
 CFLAGS+=       ${FORTIFY_CFLAGS}
 CXXFLAGS+=     ${FORTIFY_CFLAGS}
+.  else
+FORTIFY_SOURCE=0
 .  endif
 .endif

That works- it does need an .export FORTIFY_SOURCE line to work, though, otherwise the build will pick up the default in bsd.sys.mk if we aren't adding to C*FLAGS (since the port build would be running in a submake)

I do not expect much failures. But I expect some. I expect at least the gcc ports to fail. I start a local build with this now, and I remove my WITHOUT_FORTIFY_PORTS to validate. Sunday I may be able to provide some feedback.

Thanks!

[netchild]

In D57628#1322519, @kevans wrote:

In D57628#1322468, @netchild wrote:

In D57628#1321971, @kevans wrote:

All supported releases should have it, but it is also harmless to define any of this when it isn't -- I'd probably avoid complicating it too much.

Sounds sensible.

If you don't object to the ports patch as the maintainer, I'll just go ahead and request an exp-run with both patches applied (with the assignment you noted fixed). I had hoped to land this this week, but I also don't want antoine throwing tomatoes at me if we fundamentally break the ports tree. =-)

I don't object for sure. But I suggest to use this instead:

diff --git Mk/Features/fortify.mk Mk/Features/fortify.mk
index 2e43ca98242..86d149ff68e 100644
--- Mk/Features/fortify.mk
+++ Mk/Features/fortify.mk
@@ -14,5 +14,7 @@ FORTIFY_SOURCE?=2
 FORTIFY_CFLAGS?=       -D_FORTIFY_SOURCE=${FORTIFY_SOURCE}
 CFLAGS+=       ${FORTIFY_CFLAGS}
 CXXFLAGS+=     ${FORTIFY_CFLAGS}
+.  else
+FORTIFY_SOURCE=0
 .  endif
 .endif

That works- it does need an .export FORTIFY_SOURCE line to work, though, otherwise the build will pick up the default in bsd.sys.mk if we aren't adding to C*FLAGS (since the port build would be running in a submake)

.export between the two .endif?
Well, anyway, your version needs one .endif removed, and I do not care much which version is committed, as long as it works.

I do not expect much failures. But I expect some. I expect at least the gcc ports to fail. I start a local build with this now, and I remove my WITHOUT_FORTIFY_PORTS to validate. Sunday I may be able to provide some feedback.

Thanks!

Some failed, I've send the errors to you.

I run now with them disabled from fortification to know if some deps fail too.