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

Empty HTTP headers not thread-safe #3578

Closed
d912e3 opened this issue Feb 18, 2019 · 2 comments

Comments

Projects
None yet
3 participants
@d912e3
Copy link

commented Feb 18, 2019

I did this

client.c sends m HTTP requests with n empty headers named Header (i.e., "Header;") in parallel by cloning a prepared handle and performing it in a thread:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <pthread.h>
#include <curl/curl.h>

static void *
start (void *arg)
{
  CURL *easy = arg;

  CURLcode code = curl_easy_perform (easy);
  if (code)
    fprintf (stderr, "%s\n", curl_easy_strerror (code));
  curl_easy_cleanup (easy);

  return NULL;
}

static void
run (int colon,
     int port,
     int nheaders,
     int nthreads)
{
  CURL *easy = curl_easy_init ();

  curl_easy_setopt (easy, CURLOPT_URL, "http://localhost");
  curl_easy_setopt (easy, CURLOPT_PORT, port);

  struct curl_slist *header = NULL;
  for (int i = 0; i < nheaders; i++)
    header = curl_slist_append (header, colon ? "Header: foo" : "Header;");
  curl_easy_setopt (easy, CURLOPT_HTTPHEADER, header);

  pthread_t threads[nthreads];
  for (int i = 0; i < nthreads; i++)
    pthread_create (&threads[i], NULL, &start, curl_easy_duphandle (easy));
  for (int i = 0; i < nthreads; i++)
    pthread_join (threads[i], NULL);

  curl_slist_free_all (header);
  curl_easy_cleanup (easy);
}

int
main (int   argc,
      char *argv[])
{
  int colon = 0;
  int port = 8000;
  int nheaders = 10;
  int nthreads = 5;

  int c;
  while ((c = getopt (argc, argv, "cp:n:m:")) != -1)
    {
      switch (c)
        {
        case 'c':
          colon = 1;
          break;
        case 'p':
          port = strtol (optarg, NULL, 10);
          break;

        case 'n':
          nheaders = strtol (optarg, NULL, 10);
          break;

        case 'm':
          nthreads = strtol (optarg, NULL, 10);
          break;
        }
    }

  curl_global_init (CURL_GLOBAL_ALL);
  run (colon, port, nheaders, nthreads);
}

server.go returns the number of headers named Header to each request:

package main

import (
    "fmt"
    "flag"
    "net/http"
    "log"
)

func main() {
    var port uint

    flag.UintVar(&port, "p", 8000, "port")
    flag.Parse()

    http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
        n := 0
        if h, ok := r.Header["Header"]; ok {
            n = len(h)
        }
        log.Println(n)
        fmt.Fprintln(w, n)
    })
    http.ListenAndServe(fmt.Sprintf(":%d", port), nil)
}

Run server and execute client repeatedly, e.g.:

$ go run server.go &
$ ./client -n10 -m5
7
7
10
10
10
$ ./client -n10 -m5
10
10
10
10
10
$ ./client -n10 -m5
5
10
6
10
10

I expected the following

The client should always return the same number of lines with the same number of headers received by the server as specified with the flags -m and -n.

When using the -c flag and letting client send non-empty headers (i.e., "Header: foo") instead, the output is as expected.

Thus, empty headers are not thread-safe.

curl/libcurl version

curl 7.64.0 (x86_64-pc-linux-gnu) libcurl/7.64.0 OpenSSL/1.1.1a zlib/1.2.11 libidn2/2.1.1 libpsl/0.20.2 (+libidn2/2.1.1) libssh2/1.8.0 nghttp2/1.36.0
Release-Date: 2019-02-06
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IDN IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL
@bagder

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

Confirmed. The issue comes from the fact that the code finds the semicolon there, puts a temporary colon in its place and then puts the semicolon back again after some checks. This of course doesn't work when done on the same list in multiple parallel threads...

curl/lib/http.c

Lines 1781 to 1852 in e652252

if(ptr) {
optr = ptr;
ptr++; /* pass the semicolon */
while(*ptr && ISSPACE(*ptr))
ptr++;
if(*ptr) {
/* this may be used for something else in the future */
optr = NULL;
}
else {
if(*(--ptr) == ';') {
/* send no-value custom header if terminated by semicolon */
*ptr = ':';
semicolonp = ptr;
}
}
ptr = optr;
}
}
if(ptr) {
/* we require a colon for this to be a true header */
ptr++; /* pass the colon */
while(*ptr && ISSPACE(*ptr))
ptr++;
if(*ptr || semicolonp) {
/* only send this if the contents was non-blank or done special */
CURLcode result = CURLE_OK;
if(conn->allocptr.host &&
/* a Host: header was sent already, don't pass on any custom Host:
header as that will produce *two* in the same request! */
checkprefix("Host:", headers->data))
;
else if(data->set.httpreq == HTTPREQ_POST_FORM &&
/* this header (extended by formdata.c) is sent later */
checkprefix("Content-Type:", headers->data))
;
else if(data->set.httpreq == HTTPREQ_POST_MIME &&
/* this header is sent later */
checkprefix("Content-Type:", headers->data))
;
else if(conn->bits.authneg &&
/* while doing auth neg, don't allow the custom length since
we will force length zero then */
checkprefix("Content-Length:", headers->data))
;
else if(conn->allocptr.te &&
/* when asking for Transfer-Encoding, don't pass on a custom
Connection: */
checkprefix("Connection:", headers->data))
;
else if((conn->httpversion == 20) &&
checkprefix("Transfer-Encoding:", headers->data))
/* HTTP/2 doesn't support chunked requests */
;
else if((checkprefix("Authorization:", headers->data) ||
checkprefix("Cookie:", headers->data)) &&
/* be careful of sending this potentially sensitive header to
other hosts */
(data->state.this_is_a_follow &&
data->state.first_host &&
!data->set.allow_auth_to_other_hosts &&
!strcasecompare(data->state.first_host, conn->host.name)))
;
else {
result = Curl_add_bufferf(&req_buffer, "%s\r\n", headers->data);
}
if(semicolonp)
*semicolonp = ';'; /* put back the semicolon */

@jay

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

Thanks for your report and the clear repro. It takes about n100 for me or n1000 with valgrind (which shows nothing unexpected). I suspect this is caused by Curl_add_custom_headers temporarily changing the semicolon to colon to check against certain headers like Host: etc and then later append the header with the colon.

curl/lib/http.c

Lines 1790 to 1796 in f3294d9

else {
if(*(--ptr) == ';') {
/* send no-value custom header if terminated by semicolon */
*ptr = ':';
semicolonp = ptr;
}
}

I don't think the user's passed in data, of which we are accessing directly without a copy, should be modified even temporarily. The easiest fix would be copy headers->data and then modify our copy. It could even be limited to only the semicolon (like we could have a hdrline and hdrline_is_heap). To do it without a copy would mean changing checkprefix to recognize semicolon. This is just a hunch I haven't tried a fix yet.

@jay jay added the HTTP label Feb 18, 2019

bagder added a commit that referenced this issue Feb 18, 2019

http: make adding a blank header thread-safe
Previously the function would edit the provider header in-place when a
semicolon is used to signify an empty header. This made it impossible to
use the same set of custom headers in multiple threads simultaneously.

This approach now makes a local copy when it needs to edit the string.

Reported-by: d912e3 on github
Fixes #3578
Closes #xxxx

bagder added a commit that referenced this issue Feb 18, 2019

http: make adding a blank header thread-safe
Previously the function would edit the provided header in-place when a
semicolon is used to signify an empty header. This made it impossible to
use the same set of custom headers in multiple threads simultaneously.

This approach now makes a local copy when it needs to edit the string.

Reported-by: d912e3 on github
Fixes #3578
Closes #3579

@bagder bagder closed this in 942eb09 Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.