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.

Sign up for GitHub

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

Jump to bottom

Heap corruption after reusing CURL handle for which buffer size was changed #4143

Closed
felixhaedicke opened this issue Jul 23, 2019 · 1 comment
Closed

Heap corruption after reusing CURL handle for which buffer size was changed #4143

felixhaedicke opened this issue Jul 23, 2019 · 1 comment
Labels
crash

Comments

@felixhaedicke
Copy link
Contributor

felixhaedicke commented Jul 23, 2019 •
edited

I did this

  1. using libcurl-easy to download a file with a buffer size lower than the default (using CURLOPT_BUFFERSIZE)
  2. curl_easy_reset on the CURL handle
  3. reuse the CURL handle (without setting CURLOPT_BUFFERSIZE again)

After this, the heap memory of my application is corrupted.

curl/libcurl version

23c99f6

curl 7.66.0-DEV (x86_64-pc-linux-gnu) libcurl/7.66.0-DEV OpenSSL/1.1.1c zlib/1.2.11 libssh/0.8.7/openssl/zlib
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: AsynchDNS Debug HTTPS-proxy IPv6 Largefile libz NTLM NTLM_WB SSL TLS-SRP TrackMemory UnixSockets

operating system

Debian GNU/Linux 10

example code

The following program shows this issue. Most of the time, it crashes with a segmentation fault in the second curl_easy_perform() call.

See valgrind output for this example:
curl-heap-corruption-valgrind.txt

#include <stdio.h>

#include <curl/curl.h>

#define TEST_URL "ftp://ftp.de.debian.org/debian/pool/main/c/curl/curl_7.64.0-4_amd64.deb"
#define TEST_DL_FILE "/tmp/curl-test-download"

int main()
{
    curl_global_init(CURL_GLOBAL_ALL);
    CURL* curl = curl_easy_init();

    FILE* dl_file = fopen(TEST_DL_FILE, "w");

    curl_easy_setopt(curl, CURLOPT_URL, TEST_URL);
    curl_easy_setopt(curl, CURLOPT_WRITEDATA, dl_file);
    curl_easy_setopt(curl, CURLOPT_BUFFERSIZE, 1024);
    curl_easy_perform(curl);
    curl_easy_reset(curl);

    curl_easy_setopt(curl, CURLOPT_URL, TEST_URL);
    curl_easy_setopt(curl, CURLOPT_WRITEDATA, dl_file);
    curl_easy_perform(curl);

    curl_easy_cleanup(curl);
    curl_global_cleanup();

    return 0;
}
jay added a commit to jay/curl that referenced this issue Jul 23, 2019
@jay
easy: resize receive buffer on easy handle reset
4729aff 
- In curl_easy_reset attempt to resize the receive buffer to its default
  size. If realloc fails then continue using the previous size.

Prior to this change curl_easy_reset did not properly handle resetting
the receive buffer (data->state.buffer). It reset the variable holding
its size (data->set.buffer_size) to the default size (READBUFFER_SIZE)
but then did not actually resize the buffer. If a user resized the
buffer by using CURLOPT_BUFFERSIZE to set the size smaller than the
default, later called curl_easy_reset and attempted to reuse the handle
then a heap overflow would very likely occur during that handle's next
transfer.

Reported-by: Felix Hädicke

Fixes curl#4143
Closes #xxxx
@jay jay mentioned this issue Jul 23, 2019
Closed
@jay
Copy link
Member

jay commented Jul 23, 2019

It's a bug see #4145 for the fix. How it happens:

curl_easy_init (Curl_open) initializes data->state.buffer to READBUFFER_SIZE + 1 and then calls Curl_init_userdefined which sets data->set.buffer_size to READBUFFER_SIZE.

curl/lib/url.c

Lines 593 to 605 in aa73eb4

data->state.buffer = malloc(READBUFFER_SIZE + 1);
if(!data->state.buffer) {
DEBUGF(fprintf(stderr, "Error: malloc of buffer failed\n"));
result = CURLE_OUT_OF_MEMORY;
}
else {
data->state.headerbuff = malloc(HEADERSIZE);
if(!data->state.headerbuff) {
DEBUGF(fprintf(stderr, "Error: malloc of headerbuff failed\n"));
result = CURLE_OUT_OF_MEMORY;
}
else {
result = Curl_init_userdefined(data);

curl/lib/url.c

Line 542 in aa73eb4

set->buffer_size = READBUFFER_SIZE;

CURLOPT_BUFFERSIZE changes the size:

curl/lib/setopt.c

Lines 2006 to 2016 in aa73eb4

/* Resize if new size */
if(arg != data->set.buffer_size) {
char *newbuff = realloc(data->state.buffer, arg + 1);
if(!newbuff) {
DEBUGF(fprintf(stderr, "Error: realloc of buffer failed\n"));
result = CURLE_OUT_OF_MEMORY;
}
else
data->state.buffer = newbuff;
}
data->set.buffer_size = arg;

curl_easy_reset calls Curl_init_userdefined which data->set.buffer_size to READBUFFER_SIZE:

curl/lib/url.c

Line 542 in aa73eb4

set->buffer_size = READBUFFER_SIZE;

curl_easy_reset does not resize the buffer, which is the bug.

@jay jay added the crash label Jul 23, 2019
@jay jay closed this as completed in 78ed3ab Jul 26, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
crash
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

easy: resize receive buffer on easy handle reset jay/curl
2 participants
@jay @felixhaedicke