Enable stack smash protections by default

[nmeum]

Contribution description

The RIOT website describes RIOT as the "friendly Operating System for
the Internet of Things" and further claims that it is "Developer
Friendly" [1]. Scientific publications, linked on the
website, acknowledge a "raising high demand for security and privacy of
systems and applications" in the Internet of Things. Additionally, the
cited publication claims that "the sheer number of deployed IoT devices
poses a severe threat to the general Internet infrastructure" [2].

Regarding this emphasize on security it seems surprising that RIOT does
not come with any system security features enabled by default. This is
also in contrast to the developer friendlyness mentioned above as it
requires novice users to become security experts, investigate on their
own which security features are available, and manually enable them.

To improve this situation this PR enables a central system security
feature (stack smash protections) by default. While the current
implementation isn't perfect (see #13119) it is still better than having
no stack smash protections at all. Since the current implementation
generates the canary value at compile time the introduced overhead is
minimal. I did some experiments with BOARD=pba-d-01-kw2x and changes in
binary size are minimal, e.g. ~1.5KiB with examples/dtls-echo.
The change has an additional impact on
runtime memory consumption, however, currently stack
canaries are only added to functions "with buffers larger than 8 bytes".
As such, the overhead is deemed negligible. If you have any tooling to
investigate runtime memory consumption further let me know.

Since the current SSP implementation does have an impact on reproducible
builds it can be disabled explicitly using the RIOTINSECURE environment variable.

Testing procedure

• Build an example application without any further configuration, check that sys/ssp is included in the output.
• Build an example application with RIOTINSECURE=1 check that sys/ssp is not included in the output.

Issues/PRs references

• sys/spp: randomize canary value on each build #13119

---

CC: @kaspar030

[kaspar030]

+1 for maybe enabling ssp by default, but please let's not introduce polemic variables like RIOTINSECURE.

I suggest adding a per-CPU feature for ssp and enabling the module through that, or maybe let's just add DEFAULT_MODULES += ssp for those CPUs.

I did some experiments with BOARD=pba-d-01-kw2x and changes in
binary size are minimal, e.g. ~1.5KiB with examples/dtls-echo.

That's not minimal.

[nmeum]

+1 for maybe enabling ssp by default, but please let's not introduce polemic variables like RIOTINSECURE.

I just wanted to make sure people setting this variable are aware of the consequences. I don't have any strong opinion on the name of the environment variable, what would you prefer instead?

I suggest adding a per-CPU feature for ssp and enabling the module through that, or maybe let's just add DEFAULT_MODULES += ssp for those CPUs.

What's the advantage of doing it this way? Are there any CPUs for which it shouldn't be enabled?

That's not minimal.

To avoid vaguely defined terms such as "minimal" and to be more exact: It's 1.4% overhead in the text segment and 0.9% in the data segment for examples/dtls-echo.

[kaspar030]

What's the advantage of doing it this way? Are there any CPUs for which it shouldn't be enabled?

those that don't support it:

https://github.com/kaspar030/RIOT/blob/cf5356df244e47c70ade05444abebffe50e15b81/tests/ssp/Makefile#L3-L4

[nmeum]

Those that don't support it: …

Do you have further information on why these architectures do not support SSP? Do they use an old GCC version which does not have -fstack-protector?

Is it possible to implement a backlisting approach with DEFAULT_MODULES, i.e. enable it by default and only disable it for those few architectures? Otherwise I would suggest simply setting the RIOTINSECURE (or whatever we end up calling it) variable in the Makefile.include of those blacklisted architectures.

[benpicco]

According to Murdock it seems to be ATmega, MSP430 and pic32.
Although the msp430 part might be resolved by RIOT-OS/riotdocker#82
(ATmega maybe if Debian ever packages a newer version of avr-gcc)

[nmeum]

Renamed the environment variable to RIOTNOSSP. Additionally, I implemented the approach laid out in #13175 (comment) to disable it on platforms with unsupported compilers. Let see if it passes on the CI now…

[kaspar030]

Renamed the environment variable to RIOTNOSSP.

Please get rid of the variable. The way in RIOT to select use of something (including ssp) is by using a module. If something is only available on some architectures / toolchains, we use a feature in addition to the module. If something should be default, it goes into DEFAULT_MODULES.

[kaspar030]

If something is only available on some architectures / toolchains, we use a feature in addition to the module. If something should be default, it goes into DEFAULT_MODULES.

So concretely:

• add FEATURES_PROVIDED += ssp to the lowest-level Makefile.features (e.g., cpu/cortexm_common/Makefile.features, ...). (corresponding to where you currently set RIOTNOSSP)
  This could be a seperate PR, combined with it's use in tests/ssp instead of the blacklist. I'd easily ACK it. So maybe split that change out.

• add ifneq (, $(filter ssp, $(FEATURES_PROVIDED)) \ DEFAULT_MODULES += ssp \ endif to Makefile.dep

But we will need more code size / bss / runtime cost evaluations before making this default.

[kaspar030]

So do we want this by default?

[benpicco]

For examples/gnrc_networking and samr21-xpro I get +1300 bytes .text, no change in RAM use.

Maybe with the efforts on integrating KConfig we can have a make tinyconfig in the future that does not include this?

Traditionally we did put such features under DEVELHELP=1

[nmeum]

For examples/gnrc_networking and samr21-xpro I get +1300 bytes .text, no change in RAM use.

Here is my size(1) output for this application and this board. With SSP:

 text    data     bss     dec     hex filename
94628     196   19228  114052   1bd84 gnrc_networking.elf

Without SSP:

 text    data     bss     dec     hex filename
93324     192   19228  112744   1b868 gnrc_networking.elf

I also get ~+1300 bytes but just to put this in perspective: This is an increase of ~1.4% in the text segment.

Traditionally we did put such features under DEVELHELP=1.

Again: This is not a development feature. This is an essential protection mechanism for system security. As such, it helps protecting against exploitation of issue like #10739 and #12905. Please read the original PR text on why it is important to enable by default.

From the mechanisms usually employed on constrained operating systems this is the one with the most minimal overhead in terms of binary size. If you are not even willing to enable this one by default I don't see how you want to fulfill the "raising high demand for security and privacy of systems and applications" in the Internet of Things [1, p. 3].

[kaspar030]

ssp breaks LTO on cortexm:

/usr/lib/gcc/arm-none-eabi/9.2.0/../../../../arm-none-eabi/bin/ld: /usr/lib/gcc/arm-none-eabi/9.2.0/../../../../arm-none-eabi/lib/thumb/v6-m/nofp/libc_nano.a(lib_a-stack_protector.o): in function `__stack_chk_fail':
stack_protector.c:(.text.__stack_chk_fail+0x26): undefined reference to `_exit'
/usr/lib/gcc/arm-none-eabi/9.2.0/../../../../arm-none-eabi/bin/ld: /usr/lib/gcc/arm-none-eabi/9.2.0/../../../../arm-none-eabi/lib
/thumb/v6-m/nofp/libc_nano.a(lib_a-signal.o): in function `_raise_r':
signal.c:(.text._raise_r+0x26): undefined reference to `_getpid_r'
/usr/lib/gcc/arm-none-eabi/9.2.0/../../../../arm-none-eabi/bin/ld: signal.c:(.text._raise_r+0x30): undefined reference to `_kill_r'
collect2: error: ld returned 1 exit status

[nmeum]

ssp breaks LTO on cortexm:

How do I reproduce this? Doesn't seem to cause an issue on the builders, builders only fails due to some makefile sanitycheck. Do you want me to adjust that check script accordingly by the way?

Also: This doesn't seem to be the only open issue with LTO and cortexm (#5775, #5774, …). Without having investigated this further, it would of cause be possible to (temporarily) disable SSP when using LTO on cortexm. However, if you are not interested in enabling this by default anyhow it doesn't make much sense for me to invest more time in this PR.

[fjmolinas]

So do we want this by default?

IMO ssp is a necessary feature to have in production since as @nmeum said it is "an essential protection mechanism for system security".

But this raises for me the question, if we build application/foo should it be built by default with the production configuration or development configuration?. How do we distinguish both environments?

An alternative we could have is enable it as default except for all applications furnished in RIOT, meaning tests and examples would deactivate it. Non of these are meant as production code , but if a user develops its own application this would be enabled.

Regarding this emphasize on security it seems surprising that RIOT does
not come with any system security features enabled by default. This is
also in contrast to the developer friendlyness mentioned above as it
requires novice users to become security experts, investigate on their
own which security features are available, and manually enable them.

This for me raises a different very important question, how do we distinguish FEATURES (as well as crypto libraries for example) that should or should not be used in production?

We can certainly improve this part, there is no clear guide on what is safe to use, and what is just OK for local experimentation, but AFAIK it is something that is being worked on. But we could non the less start documenting this, and make it easy for a novice to grasp what he can easily do. But I agree this being necessarily the concerns of a novice.

[benpicco]

This is not a development feature. This is an essential protection mechanism for system security. As such, it helps protecting against exploitation of issue like #10739 and #12905. Please read the original PR text on why this is important to enable by default.

RIOT applications are not necessarily networked.
You can use RIOT just as a HAL for small MCUs that just expose a few buttons to the outside world and control a motor or similar - not much potential for exploitation there.

Enabling it by default for the network examples makes sense - the if networking then use ssp requirement might be easier to fulfill with the upcoming KConfig though.

[nmeum]

RIOT applications are not necessarily networked.
You can use RIOT just as a HAL for small MCUs that just expose a few buttons to the outside world and control a motor or similar - not much potential for exploitation there.

There is nothing prevent application developers from removing SSP from DEFAULT_MODULE if desired. Just because it's enabled by default doesn't mean that it is impossible to disable. However, I believe that this is a decision which should be explicitly made by the application developers. Even applications without networking may receive user input through some other means, i.e. offer a different attack vector.

[kaspar030]

ssp breaks LTO on cortexm:

How do I reproduce this?

LTO=1 USEMODULE=ssp BOARD=samr21-xpro make -Cexamples/gnrc_networking clean all -j8

However, if you are not interested in enabling this by default anyhow it doesn't make much sense for me to invest more time in this PR.

Easy, just collecting information / side effects.

[nmeum]

To give some perspective on the size overhead of this change. The following table compares the size of the text segment, with and without the SSP module, of all example applications available on BOARD=pba-d-01-kw2x. Some applications show no increase in text segment size as currently only function with a buffer larger than 8 bytes are instrumented. Application which show a text segment increase also have an increase of 4 Byte in the data segment to store the __stack_chk_guard variable. To be honest, the worst increase seen is 2% I consider this negligible.

The change, of cause, also has a potential impact on stack space as instrumented function will require additional stack space for the canary value + potential alignment. However, RIOT currently does not seem to employ any technique for determining the current maximum stack usage. Since most threads seem to use macro such as THREAD_STACKSIZE_DEFAULT anyhow, the stack of most threads should be sufficiently overprovisioned for storing an additional word size canary value in a few functions.

[maribu]

Why not use FEATURES_OPTIONAL += ssp?

[benpicco]

DEFAULT_MODULEs can be disabled via DISABLE_MODULE, I think that's not possible with features.

[maribu]

You can, using FEATURES_BLACKLIST += <FEATURE_TO_BLACKLIST> ;-)