-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Possible memset optimized out in crypto code #10751
Comments
I assigned a number of people who I am aware are working with potentially sensitive data. |
I've implemented a https://github.com/RIOT-OS/RIOT/blob/master/sys/crypto/helper.c#L38-L44 |
sounds to me that we should add something to the wiki on this "sensitive data" matter, maybe even draft a (crypto-related) RDM? |
Did you implement this on your own? Because that's usually a very bad idea and in fact that function looks pretty insufficient to me. As a more sufficient implementation of such a function consider sodium_memzero. See also: https://media.ccc.de/v/35c3-9788-memsad |
That's the talk referenced in OP ;-). The talk also said that even that isn't ideal. |
As stated above, best effort implementation. I think I based this one on the monocypher wipe function. Feel free to submit a PR changing it to a more sufficient implementation though. ;-) |
@gschorcht is this issue on your radar? |
@miri64 Yes, but I'm not sure what to do. Although |
I would first try to fix it upstream and then also apply that patch to the imported version. |
Do we have a general policy in RIOT for this? It's fine
to include a lump of vendor code in RIOT, but there will
always a time that it needs to be modified or updated.
I see at least two problems.
1. you don't want to duplicate upstream work
2. you don't want to make too many changes that complicate
future updates
We want a simple update procedure for upstream changes.
Do we have that? (Not just the wpa-supplicant, but all vendor
code.)
Did we record exactly where the code came from and what version
it was when it was copied?
|
The canonical way to include external code are |
The situation for ESP32 is quite different. It is impossible to use the ESP-IDF (the official SDK) as it is since it is based on FreeRTOS. I had to pick only some parts from ESP-IDF and I had to modify them a lot to integrate them to RIOT and make them compilable at all due to the strict compiler options. Furthermore, the directory structure in |
Just some facts if we would think about to use the ESP-IDF SDK as package. The size of the git repository is about 520 MByte including 3.5 MByte proprietary libraries. It doesn't include the compiler. From these 520 MByte we only use
If we would use it as package, I have just tried to create an according So I don't think that it would be a good idea to integrate ESP-IDF as package. Especially because each compiled application would fetch a complete copy of the package in its build dirctory. Too much to download, too much to be done and too much disk space required. Having ESP-IDF as part of a preconfigured toolchain which exists only once on a system and has not to be prepared during compile time makes much more sense. |
I will try to replace the crypto library of |
@keestux At first, I did not realize that the function you referred to initially came from Ok, I went through all occurences of RIOT/cpu/esp32/vendor/esp-idf/wpa_supplicant/src/crypto/des-internal.c Lines 422 to 423 in 519b9eb
|
@miri64 @keestux After thinking more about the the idea in issue #10787 to replace
Therefore, a more reasonable way might be to realize the |
I had similar problems with |
Back to the problem concerning the |
According to the presentation also
|
@keestux @gschorcht what is the status with this issue? Should it be a tracking of all the occurrences? Or were the problematic ones fixed in #10801. |
After watching the presentation of Ilja van Sprundel at CCC 35C3 [1] I noticed that there is at least one location where a
memset
is used at the end of a function to clear sensitive data. However, as explained in Ilja's talk, there is a high chance that the memset is optimized out.This final memset is clearing
block
. Most compilers however know aboutmemset
, and they know it is clearing local data which is never used again. Thus the compiler can and will remove that code. It's not a bug of the compiler, it's simply allowed. The programmer on the other wants that memory to be cleared to not leave it on the stack after the function finishes.In the same module, there is another occurrence of a memset at the end of a function.
[1] 2018 Chaos Communication Congress talk Memsad
The text was updated successfully, but these errors were encountered: