szszszsz
commented
3 years ago 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.
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-allor-fstack-protector-strongflags as well as the relevant canary initialization inmain()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 https://github.com/Nitrokey/nitrokey-storage-firmware/commit/51e9ac3fa05d557a8e239b96d789f31897ac2493 . 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-majorfor your official builds based on the information in https://github.com/Nitrokey/nitrokey-pro-firmware/commit/c8e20d446eea7eccc1d195952ff7514617b3e3d6 .gcc-arm-none-eabi-8-2018-q4-majoris 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.