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

nmeum · 2020-01-14T11:22:44Z

Contribution description

The optional ssp module implements stack smash protections for RIOT. Stack smash protections are a very useful protection mechanisms against issues like #10739 and #12905 (stack-based buffer overflows). Unfortunately, the ssp module currently uses a hardcode canary value which renders it useless for protecting against malicious attackers.

The changes proposed here randomize the value on each make invocation. Randomization during compile-time was previously mentioned in a comment in ssp.c. On conventional operating systems, random stack canary values are usually generated on program startup. Unfortunately, many constrained devices do not have a hardware random number generator which makes generating cryptographic secure values challenging at run-time.

Generating the value at compile-time has various disadvantages, compared to generating it at run-time. For instance, it offers less protections as the canary value does not change at all for a given firmware image which is an issue if a malicious attacker gains access to it once. Nonetheless, I believe that the changes proposed here are an improvement over the hardcode value used currently. If run-time generation is implemented in the future this implementation may still be suitable as a fallback.

@kaspar030, you committed the initial SSP implementation. Any thoughts on the changes proposed here?

Testing procedure

Run tests/ssp
Verify that the STACK_CHK_GUARD macro in riotbuild.h is different on each build.

kaspar030 · 2020-01-14T11:34:31Z

@kaspar030, you committed the initial SSP implementation. Any thoughts on the changes proposed here?

+1 in general for making this random, even at compile-time. As is, this renders the CI test cache ineffective. We could disable the random value generation if RIOT_CI_BUILD is set. What do you think?

(This also conflicts the long-term goal of reproducible builds.)

dist/tools/randhex/randhex.sh

sys/ssp/ssp.c

This variable contains a new random hexadecimal value on each build. As such, it can be utilized by the ssp module as a canary value which is randomized on each new build. This cannot be implemented in the Makefile.include file of the ssp module as this file is included multiple times and would result in a different random value on each include. The variable is set to a hardcoded value when building software on the CI to not break the CI test cache [1]. [1]: RIOT-OS#13119 (comment)

sys/ssp/Makefile.include

Makefile.include

This implements the randomization of canary values on each build as mentioned in the comment above the STACK_CHK_GUARD macro. The canary value is generated by the buildsystem and passed to the ssp module using a `-D` compiler flag. The ssp object file, using this canary value, is marked as PHONY to make sure it is rebuild on each make invocation, thereby ensuring that each build uses a new random canary value. Implementing this properly would require generating a cryptographically secure random value on each boot of the RIOT operating system. This is not deemed possible on some constrained devices, e.g. due to lack of hardware random number generators. Besides, RIOT only seems to support a PRNG (random module) currently. While this may be implemented in the future for some devices the changes implemented in this commit may still be used as a fallback then. A hardcoded canary value is used when building software on the CI to not break the CI test cache [1]. [1]: RIOT-OS#13119 (comment)

nmeum · 2020-01-14T18:21:28Z

Revised my implementation. The canary value generation has been moved to sys/ssp/Makefile. Besides, a complete rebuild of all modules is no longer necessary. This has been achieved by marking the ssp.o object file, which uses the STACK_CHK_GUARD define, as .PHONY. Meaning, it is rebuild on each make invocation with a new canary value. As this object file is the only one using the canary value it is the only file that needs to be rebuild on each make invocation.

Test procedure now looks as follows:

Build tests/ssp with QUIET= and ensure that the ssp module is rebuild each time.
Flash and use tests/ssp as usual.

This implements the randomization of canary values on each build as mentioned in the comment above the STACK_CHK_GUARD macro. The canary value is generated by the buildsystem and passed to the ssp module using a `-D` compiler flag. The ssp object file, using this canary value, is marked as PHONY to make sure it is rebuild on each make invocation, thereby ensuring that each build uses a new random canary value. Implementing this properly would require generating a cryptographically secure random value on each boot of the RIOT operating system. This is not deemed possible on some constrained devices, e.g. due to lack of hardware random number generators. Besides, RIOT only seems to support a PRNG (random module) currently. While this may be implemented in the future for some devices the changes implemented in this commit may still be used as a fallback then. A hardcoded canary value is used when building software on the CI to not break the CI test cache [1]. [1]: RIOT-OS#13119 (comment)

dist/tools/randhex/randhex.sh

This implements the randomization of canary values on each build as mentioned in the comment above the STACK_CHK_GUARD macro. The canary value is generated by the buildsystem and passed to the ssp module using a `-D` compiler flag. The ssp object file, using this canary value, is marked as PHONY to make sure it is rebuild on each make invocation, thereby ensuring that each build uses a new random canary value. Implementing this properly would require generating a cryptographically secure random value on each boot of the RIOT operating system. This is not deemed possible on some constrained devices, e.g. due to lack of hardware random number generators. Besides, RIOT only seems to support a PRNG (random module) currently. While this may be implemented in the future for some devices the changes implemented in this commit may still be used as a fallback then. A hardcoded canary value is used when building software on the CI to not break the CI test cache [1]. [1]: RIOT-OS#13119 (comment)

nmeum · 2020-01-15T06:41:35Z

Rewrote randhex in python. While the old shell script would always return a hexadecimal value with n characters the new python script might also return less than that, i.e. it should have better entropy. Besides, it is a bit easier to read / understand.

I am currently no longer aware of any remaining issues with the changes proposed here, imho this could be merged as is. Let me know if I missed anything.

This implements the randomization of canary values on each build as mentioned in the comment above the STACK_CHK_GUARD macro. The canary value is generated by the buildsystem and passed to the ssp module using a `-D` compiler flag. The ssp object file, using this canary value, is marked as PHONY to make sure it is rebuild on each make invocation, thereby ensuring that each build uses a new random canary value. Implementing this properly would require generating a cryptographically secure random value on each boot of the RIOT operating system. This is not deemed possible on some constrained devices, e.g. due to lack of hardware random number generators. Besides, RIOT only seems to support a PRNG (random module) currently. While this may be implemented in the future for some devices the changes implemented in this commit may still be used as a fallback then. A hardcoded canary value is used when building software on the CI to not break the CI test cache [1]. [1]: RIOT-OS#13119 (comment)

nmeum · 2020-01-20T10:38:53Z

@kaspar030 I believe I addressed all of your comments. Any chance you could take another look at this? :)

kaspar030

ACK, apart from the missing license.

dist/tools/randhex/randhex.py

kaspar030 · 2020-01-20T13:03:38Z

@kaspar030 I believe I addressed all of your comments. Any chance you could take another look at this? :)

Looking good now. Thanks!

This tool generates a random hexadecimal value of a given maximum size. This is useful for generating random canary values during compile-time for the ssp module which currently uses a constant value.

This implements the randomization of canary values on each build as mentioned in the comment above the STACK_CHK_GUARD macro. The canary value is generated by the buildsystem and passed to the ssp module using a `-D` compiler flag. The ssp object file, using this canary value, is marked as PHONY to make sure it is rebuild on each make invocation, thereby ensuring that each build uses a new random canary value. Implementing this properly would require generating a cryptographically secure random value on each boot of the RIOT operating system. This is not deemed possible on some constrained devices, e.g. due to lack of hardware random number generators. Besides, RIOT only seems to support a PRNG (random module) currently. While this may be implemented in the future for some devices the changes implemented in this commit may still be used as a fallback then. A hardcoded canary value is used when building software on the CI to not break the CI test cache [1]. [1]: RIOT-OS#13119 (comment)

kaspar030 · 2020-01-20T14:26:34Z

Added a LGPL 2.1 license comment.

Thanks!

nmeum · 2020-01-20T16:18:15Z

The examples/suit_update/samr21-xpro:gnu test seems to fail on Murdock:

Triggered [fe80::2bc:49ff:fea6:8b6b%riot0] to update.
handler returned <0
)suit_v4_parse() failed. res=-1
suit: received URL: "coap://[fd00:dead:beef::1]/fw/samr21-xpro/suit_update-riot.suitv4_signed.1579563225.bin"
suit_coap: trigger received
suit_coap: downloading "coap://[fd00:dead:beef::1]/fw/samr21-xpro/suit_update-riot.suitv4_signed.1579563225.bin"
suit_coap: got manifest with size 545
suit: verifying manifest signature...
Unable to validate signature
handler returned <0
)suit_v4_parse() failed. res=-1
Timeout in expect script at "child.expect_exact("seq_nr <= running image")" (examples/suit_update/tests/01-run.py:142)

Is that a known issue? Because it seems unrelated to this PR.

kaspar030 · 2020-01-20T16:22:45Z

Is that a known issue? Because it seems unrelated to this PR.

yes, unrelated. All other tests passes, I'll re-trigger without running tests.

This implements the randomization of canary values on each build as mentioned in the comment above the STACK_CHK_GUARD macro. The canary value is generated by the buildsystem and passed to the ssp module using a `-D` compiler flag. The ssp object file, using this canary value, is marked as PHONY to make sure it is rebuild on each make invocation, thereby ensuring that each build uses a new random canary value. Implementing this properly would require generating a cryptographically secure random value on each boot of the RIOT operating system. This is not deemed possible on some constrained devices, e.g. due to lack of hardware random number generators. Besides, RIOT only seems to support a PRNG (random module) currently. While this may be implemented in the future for some devices the changes implemented in this commit may still be used as a fallback then. A hardcoded canary value is used when building software on the CI to not break the CI test cache [1]. [1]: RIOT-OS#13119 (comment)

nmeum force-pushed the pr/ssp_random_canary branch from 9535ee0 to 384cf0a Compare January 14, 2020 11:28