Riot: Possible memset optimized out in crypto code

[keestux]

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.

void
SHA1Transform(u32 state[5], const unsigned char buffer[64])
{
    u32 a, b, c, d, e;
    typedef union {
        unsigned char c[64];
        u32 l[16];
    } CHAR64LONG16;
    CHAR64LONG16* block;
...
    /* Wipe variables */
    a = b = c = d = e = 0;
#ifdef SHA1HANDSOFF
    os_memset(block, 0, 64);
#endif
}

This final memset is clearing block. Most compilers however know about memset, 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](https://media.ccc.de/v/35c3-9788-memsad)

[miri64]

I assigned a number of people who I am aware are working with potentially sensitive data.

[bergzand]

I've implemented a crypto_wipe a while ago with a best effort implementation:

https://github.com/RIOT-OS/RIOT/blob/master/sys/crypto/helper.c#L38-L44

[smlng]

sounds to me that we should add something to the wiki on this "sensitive data" matter, maybe even draft a (crypto-related) RDM?

[nmeum]

I've implemented a crypto_wipe a while ago with a best effort implementation:

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

[miri64]

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.

[bergzand]

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 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. ;-)

[miri64]

@gschorcht is this issue on your radar?

[gschorcht]

@miri64 Yes, but I'm not sure what to do. Although cpu/esp32/vendor/esp-idf/wpa-supplicant/src is part of the RIOT repository, it is vendor code that has been extracted one by one from the original ESP-IDF code, which in turn is extracted from the original source code wpa_supplicant source. Should we change it anyway?

[miri64]

@miri64 Yes, but I'm not sure what to do. Although cpu/esp32/vendor/esp-idf/wpa-supplicant/src is part of the RIOT repository, it is vendor code that has been extracted one by one from the original ESP-IDF code, which in turn is extracted from the original source code wpa_supplicant source. Should we change it anyway?

I would first try to fix it upstream and then also apply that patch to the imported version.

[keestux]

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?

[miri64]

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 pkgs. But I understand that for vendor files this might not possible (usually this were just headers though in the past). On the other hand we do this for efm32-based boards to a degree already with gecko_sdk.

[gschorcht]

On the other hand we do this for efm32-based boards to a degree already with gecko_sdk.

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 cpu/esp32/vendor/esp-idf is completely different from that in the original version.

[gschorcht]

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

• the header files
• the proprietary libraries with a size of 3.5 MByte and
• about 1.7 MByte source files extracted from the ESP-IDF and modified to be compatible with RIOT

If we would use it as package, I have just tried to create an according pkg/esp-idf, the repository could be reduced to 220 MByte by deleting unnecessary parts, but the generated patch files would also have 45 MByte.

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.

[gschorcht]

I will try to replace the crypto library of wpa_supplicant by RIOT's sys/crypto and sys/hashes modules, but there will be no direct mapping, see #10787

[gschorcht]

@keestux At first, I did not realize that the function you referred to initially came from cpu/esp32/vendor/esp-idf/wpa-supplicant. Specifying a reference to the file would have helped :smile:

Ok, I went through all occurences of os_memset and figured out these further locations:
https://github.com/RIOT-OS/RIOT/blob/519b9ebc33e98bb1066e3a2b48050f846ea60b6f/cpu/esp32/vendor/esp-idf/wpa_supplicant/src/crypto/des-internal.c#L422-L423 https://github.com/RIOT-OS/RIOT/blob/519b9ebc33e98bb1066e3a2b48050f846ea60b6f/cpu/esp32/vendor/esp-idf/wpa_supplicant/src/crypto/sha1-internal.c#L310

[gschorcht]

@miri64 @keestux After thinking more about the the idea in issue #10787 to replace wpa_supplicant by RIOT's sys/crypto and sys/hashes modules, I'm wondering whether it would be really a good idea for the following reasons:

• wpa_supplicant is wide-spreaded and accepted, it is part of all Linux systems and a lot of wireless routers.
• If ESP Wifi should support WPA2 Enterprise Mode in future, we will need the whole wpa_supplicant and not only its crypto functions.

Therefore, a more reasonable way might be to realize the wpa_supplicant as package imported directly from the original source and not the adopted version from ESP-IDF. What do you think?

[miri64]

On the other hand we do this for efm32-based boards to a degree already with gecko_sdk.

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 cpu/esp32/vendor/esp-idf is completely different from that in the original version.

I had similar problems with emb6 which is a "os independent" port of Contiki's network stack uip (still gotten around updating that...). But it worked out as a package after all. But since ESP-IDF seems to contain binaries I understand that this wouldn't be as simple as just changing names around ;-).

[gschorcht]

Back to the problem concerning the os_memset in wpa_supplicant. I have checked produced code. Indeed, the os_memset call is optimized out :worried:

[gschorcht]

According to the presentation also memset calls for pointers that are freed immediately after memset are optimized out. This is indeed the case for example here:
https://github.com/RIOT-OS/RIOT/blob/b6eb12c6d4a3ee85ac7c463f555a68fe6932d47b/cpu/esp32/vendor/esp-idf/wpa_supplicant/src/crypto/aes-internal-dec.c#L170

[gschorcht]

@miri64 @keestux PR #10801 fixes the problems with wpa_supplicant in ESP32 port. os_memset points now to a function that cannot be optimized out. This function uses the libsodium approach of weak symbols.

[fjmolinas]

@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.