Consider using stack canaries via -fstack-protector* flags

[invd]

From what I can see in the Nitrokey Pro source code, the firmware currently does not use or support the ARM GCC provided standard functionality to mitigate certain stack buffer overflows. Having suitable mitigations in place can be essential to prevent attackers from achieving code execution on the device.

I recommend adding -fstack-protector-all or -fstack-protector-strong flags as well as the relevant canary initialization in main() with random values from the TRNG random source.

A weaker form of this stack canary logic with a fixed and well-known canary value is used in the Nitrokey/nitrokey-storage-firmware@51e9ac3 . This approach can be circumvented in case the attacker has sufficient control over the buffer overflow values to re-use the expected values, so I recommend switching to randomized canaries there as well.

Please note that several ARM GCC versions have broken stack canary support, as I (re-)discovered and documented recently. To my knowledge, you are currently using the gcc-arm-none-eabi-8-2018-q4-major for your official builds based on the information in c8e20d4 . gcc-arm-none-eabi-8-2018-q4-major is not affected by this issue, but some later versions are, so I recommend taking this into consideration for the toolchain if you do include stack canaries at some point in the future.

[szszszsz]

Hi!
Thank you for linking this very interesting read! We will take a look into applying the concluded fixes.
Confirming we are using not affected GCC version at the moment - 8.2.1, so only setting the flags, the canary value and testing remains.
I will create a similar ticket for the Nitrokey Storage.

[szszszsz]

Update: currently merged with a static value set on the stack protector constant.
To be done:

• get stack protector constant from the MCU's memory, updated in the previous power-cycle.