Skip to content
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

Not erasing secrets from heap/stack after use #7268

Closed
piru opened this issue Jun 16, 2021 · 4 comments
Closed

Not erasing secrets from heap/stack after use #7268

piru opened this issue Jun 16, 2021 · 4 comments

Comments

@piru
Copy link

piru commented Jun 16, 2021

In number of locations libcurl omits zeroing memory after it has been used to store a copy of a clear text password or derived key material. This doesn't have a direct security impact in normal circumstances, but may become one if application using libcurl has some flaw that allows heap or stack disclosure. One historical example of such issue is the infamous openssl heartbleed vulnerability.

I did this

  1. asked user for a password
  2. passed the password to libcurl
  3. afterwards securely memset_s the password to all 0
  4. dumped process memory
  5. looked for the password in memory and found copy of it

I expected the following

libcurl itself to wipe the passwords / key material after use.

curl/libcurl version

curl 7.77.1-DEV (x86_64-pc-linux-gnu) libcurl/7.77.1-DEV OpenSSL/1.1.1k zlib/1.2.11 brotli/1.0.9 libidn2/2.3.0 libpsl/0.21.0 (+libidn2/2.3.0) libssh2/1.9.0 nghttp2/1.43.0 librtmp/2.3 OpenLDAP/2.4.57
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS brotli HSTS HTTP2 HTTPS-proxy IDN IPv6 Largefile libz NTLM NTLM_WB PSL SSL TLS-SRP UnixSockets

operating system

Linux hostname 5.10.0-6-amd64 #1 SMP Debian 5.10.28-1 (2021-04-09) x86_64 GNU/Linux

references

@bagder
Copy link
Member

bagder commented Jun 16, 2021

This has been touched many times before. It isn't very easy to do this so that it works and is portable, and in most cases the application already has the sensitive data in memory to be able to pass it to libcurl so it won't help a lot that libcurl clears it since it will still remain in the heap or on the stack somewhere else.

But I won't object to someone trying to fix it.

@piru
Copy link
Author

piru commented Jun 16, 2021

in most cases the application already has the sensitive data in memory to be able to pass it to libcurl so it won't help a lot that libcurl clears it since it will still remain in the heap or on the stack somewhere else.

That indeed likely is the case in most of the time. However, as pointed out currently libcurl is the weak point in otherwise "memory secure" application. This could be improved.

But I won't object to someone trying to fix it.

Good to hear. I might (time permitting) take a stab at this.

@nico-abram
Copy link
Contributor

I wrote a simple test program (For windows) which tells curl to use a custom bump allocator and does not free anything, it performs a simple POST HTTPS request against a postman echo server, and then scans the memory block for sent/received data: https://github.com/nico-abram/curl-test-zeromem . This is obviously not perfect since, for example, it doesn't look at the stack, but it's something.

From this paper https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/yang and this liboqs implementation https://github.com/open-quantum-safe/liboqs/blob/636d9725b255442e0b1e8a1ba9fabc5feca600a9/src/common/common.c#L176-L188 (MIT) I got this:

static volatile void*(*memset_func)(void*, int, size_t) = &memset;
static void Curl_cleanse(void* data, size_t size) {
	if(!data) return;
#ifdef defined(WIN32)
	SecureZeroMemory(data, size);
#elif defined(HAVE_MEMSET_S)
	memset_s(data, 0, size);
#else	
	memset_func(data, 0, size);
#endif
}

Which works under the latest MSVC version on windows
Is it ok to put this in curl_memory.h ?

I've also got a question, I started trying to apply it in some places, and I'm not sure how realloc should be handled. Do we need/want to clear the data from the pre-reallocation buffer? Is there a legal way to do that without changing it into a manual malloc+memcpy+cleanse+free ?

Here's what I've got so far (Very far from done): nico-abram@30a61f3

@bagder bagder closed this as completed in cfe3667 Aug 10, 2021
@piru
Copy link
Author

piru commented Jan 9, 2023

If such secure library version is planned, SSL_CTX_set_options SSL_OP_CLEANSE_PLAINTEXT would instruct openssl to wipe internal plaintext buffers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants