Add a function for securely erasing a buffer
arnavyc opened this issue · 3 comments
After using any sensitive data such as passwords, the buffers which store them must be cleared by the code to prevent it from being misused by a malicious actor by performing core dumps or using other means to access the memory of the application. However, most compilers optimize away the memset call as they interpret the call as dead code because the memory being written to is not being used afterwards.
I have already added a psnip_memset_explicit function in https://github.com/arnavyc/portable-snippets/tree/arnavyc/memset-explicit/memset-explicit that can erase a buffer without fear of 'dead-store removal'
I'm willing to add this, but the implementation needs some work first.
First, I think the API should just zero the memory instead of set it to an arbitrary value. That lets us implement it using either APIs which zero the memory or set it to an arbitrary value (in which case we just pass 0 as the value).
This should probably be header-only instead of a separate C file. That simplifies things for people integrating this into their code since all they have to do is #include the header.
The down side of that is that we don't really want to include Windows.h in a header since it would be included either with or without WIN32_LEAN_AND_MEAN, and the user may want the opposite. The workaround would probably be to use RtlSecureZeroMemory instead since it's defined at a lower level; I'd still keep a SecureZeroMemory-based implementation around, just guard it with an ifdef on _WINDOWS_ (IIRC that macro is set when you include <Windows.h>).
explicit_bzero actually has nothing to do with Linux; you should be looking for glibc not __linux__, otherwise this code is likely to fail when compiled using a different libc.
In general, all the version checks are missing. For example, explicit_bzero was added in glibc 2.25, musl 1.2.2, OpenBSD 5.5, and FreeBSD 11.0.
An explicit_memset-based implementation would cover more ground. Solaris includes it since 11.4, and NetBSD since version 7.
A test would be very good. https://gist.github.com/jiixyj/3e3389649c866f7ff7bd turned up in my searches, it should be possible to modify that (it's just a test case, so the license shouldn't be a problem; the actual implementation would still be public domain). Speaking of licenses, a public domain dedication is needed in a comment in the implementation.
Instead of falling back on a normal memset, there should be an #error. This is security-sensitive code, so silently falling back on insecure code is not an acceptable option. That said, a normal memset-based implementation could be hidden behind an #if defined(PSNIP_EXPLICIT_BZERO_ALLOW_INSECURE_IMPLEMENTATION) macro for people who want to opt-in to it.
I have incorporated some of the changes you suggested at commit arnavyc/portable-snippets@d98a23a8d4406ca16a3f8d7617647f130b2f51fb and arnavyc/portable-snippets@792825f533ae50772524aaca3201232ffb46d8ba.
See #40.