lib: Add explicit_bzero to zero out sensitive buffers #13589
Conversation
|
I would think the secrets are probably still in memory somewhere. I'd want to hear from some security experts to know whether this is really worth it for us and actually does what we expect. |
CMakeLists.txt
Outdated
| @@ -1302,6 +1302,9 @@ check_symbol_exists(getrlimit "${CURL_INCLUDES}" HAVE_GETRLIMIT) | |||
| check_symbol_exists(setlocale "${CURL_INCLUDES}" HAVE_SETLOCALE) | |||
| check_symbol_exists(setmode "${CURL_INCLUDES}" HAVE_SETMODE) | |||
| check_symbol_exists(setrlimit "${CURL_INCLUDES}" HAVE_SETRLIMIT) | |||
| check_symbol_exists(bzero "${CURL_INCLUDES};strings.h" HAVE_BZERO) | |||
| check_symbol_exists(explicit_bzero "${CURL_INCLUDES};strings.h" HAVE_EXPLICIT_BZERO) | |||
| check_symbol_exists(memset_s "${CURL_INCLUDES};string.h" HAVE_MEMSET_S) | |||
There was a problem hiding this comment.
It's safe to enclose these check inside if(NOT WIN32). Saves time.
(libssh2 does this.)
There was a problem hiding this comment.
I think ${CURL_INCLUDES}; is unnecessary for these checks.
There was a problem hiding this comment.
I think so too, but we use ${CURL_INCLUDES} on all the other checks where it's likely not needed so I went for consistency. Maybe a cleanup patch taking all checks into consideration would be in order?
There was a problem hiding this comment.
It's though because each of them needs to be carefully validated. I didn't dare touching it yet, though it's on my TODO list. For these new ones, we can be sure it's the only headers needed.
|
There exists one more variant: check_symbol_exists("explicit_memset" "string.h" HAVE_EXPLICIT_MEMSET)#define _libssh2_explicit_zero(buf, size) (void)explicit_memset(buf, 0, size)https://man.netbsd.org/explicit_memset.3 It's checked after |
|
In libssh2 the wrapper for all the variants is a macro. It has the advantage to inline the native Windows call (and maybe others if they have a similar implementation). Throwing it here as an idea to consider. https://github.com/libssh2/libssh2/blob/d19b61907039554b8261a90898f2f31e1a706f38/src/misc.h#L42-L59 |
Ref: curl/curl#13589 Closes #xxxxx
Interesting, we even use it in the code for the NetBSD bug (which is active for NetBSD < 9 but explicit_memset is only available for 7.0+). Added support for it. |
Absolutely, that's a possibility for sure. This is unlikely to be used in performance critical codepaths but might still be worth doing? |
|
One more TODO is to move clearing of values to when they're no longer needed and not just before the |
You're right. Let's see if this starts to hit something performance critical and deal with it then. |
This is pretty standard operating procedure in code dealing with sensitive information (OpenSSL, OpenSSH, libssh2 et.al), defined as CWE-14, and provides basic code hygiene. I agree that it won't push the needle all that far, but provides more defense in depth. |
lib/strzero.c
Outdated
| /* | ||
| * TODO: Windows 11 ships a new function SecureZeroMemory2 which is a safer | ||
| * method, but is not as widely available as SecureZeroMemory which exist | ||
| * all the way back to Windows XP. | ||
| */ | ||
| (void)SecureZeroMemory(buffer, length); |
There was a problem hiding this comment.
According to this copy of the SDK:
https://github.com/hughbe/windows-sdk-headers/blob/89f151d343587fcd5b854a4851fcb8f7187f3ac4/Microsoft.Windows.SDK.CPP/latest/Include/um/WinBase.h#L123
a simple #ifdef might be enough to detect SecureZeroMemory2, at least with MSVC. We'd need to test it before adding it.
There was a problem hiding this comment.
Hmm, I agree that it seems like an ifdef is all we need. I've added it to the code in this rebase for a test to see if any blows up.
danielgustafsson
added
the
feature-window
This adds cross platform support for zeroing out buffers containing sensitive content without the zeroing being optimized away by the compiler due being a dead-store. In case the platform doesn't have bzero, explicit_bzero or memset_s there is a fallback which use a plain memset via a volatile pointer. A set of buffers are included here, going through and finding all potential buffers is future work. Closes: #xxxx
danielgustafsson
force-pushed
the
dg-explicit_bzero
branch
from
d022576
to
6d55ba9
Compare
Sharing this early as a proof-of-concept to hear others thoughts on this. The CMake and Windows parts are untested so getting them onto the CI is a motivator.
This adds cross platform support for zeroing out buffers containing sensitive content without the zeroing being optimized away by the compiler due being a dead-store. In case the platform doesn't have
bzero,explicit_bzero(available on Archlinux etc) ormemset_sthere is a fallback which use a plainmemsetvia a volatile pointer.A set of buffers are included here, going through and finding all potential buffers is future work.
This is influenced on how OpenSSH and PostgreSQL zeroes out sensitive buffers. It clearly won't move the security needle all that much but every little bit helps.