Authentication state is not reset properly when using NTLM #1095

mkauf · 2016-10-31T16:04:53Z

I did this

This example program performs two requests. libcurl does not send the request body for the second request - that's the bug.

first request: with NTLM authentication. The request times out (NTLM handshake failure).
second request: without NTLM authentication.

#include <stdio.h>
#include <curl/curl.h>

#define USE_CURL_EASY_RESET

int main(void)
{
  CURL *curl;
  CURLcode res;

  curl_global_init(CURL_GLOBAL_ALL);

  curl = curl_easy_init();
  if(curl) {
    curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);

    curl_easy_setopt(curl, CURLOPT_URL, "http://localhost/sleep.php");
    curl_easy_setopt(curl, CURLOPT_POSTFIELDS, "a=b");

    curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_NTLM);
    curl_easy_setopt(curl, CURLOPT_USERPWD, "domain\\user:password");

    curl_easy_setopt(curl, CURLOPT_TIMEOUT, 3L);

    res = curl_easy_perform(curl);
    if(res != CURLE_OK) {
      fprintf(stderr, "curl_easy_perform() failed: %s\n", curl_easy_strerror(res));
    }

#ifdef USE_CURL_EASY_RESET
    curl_easy_reset(curl);
    curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
#else
    curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_BASIC);
    curl_easy_setopt(curl, CURLOPT_USERPWD, NULL);
#endif

    curl_easy_setopt(curl, CURLOPT_URL, "http://localhost/submit.php");
    curl_easy_setopt(curl, CURLOPT_POSTFIELDS, "a=b");

    res = curl_easy_perform(curl);
    if(res != CURLE_OK) {
      fprintf(stderr, "curl_easy_perform() failed: %s\n", curl_easy_strerror(res));
    }

    curl_easy_cleanup(curl);
  }

  curl_global_cleanup();
  return 0;
}

sleep.php:

<?php
sleep(300);
?>

submit.php:

<?php
if (empty($_POST)) {
    echo "No POST parameters";
}
else {
    echo "POST parameters:";
    print_r($_POST);
}
?>

I expected the following

libcurl sends the request body for the second request.

curl/libcurl version

7.50.3
git master

operating system

Linux x64

hints

The bug appears even if curl_easy_reset() is called between the requests.

For NTLM, the flag state.authhost.multi is set. Because of this flag, conn->bits.authneg is set. And because of this flag, the body is omitted, even for subsequent requests.

state.authhost (and state.authproxy) should probably be reset in curl_easy_reset(). For NTLM, it should also be reset when the connection changes (there is already some code for NTLM in create_conn()). It should also be reset when the URL changes. Probably all members of struct UrlState state should be reset when the URL changes. That's what the name "UrlState" suggests.

All NTLM state should be attached to the current connection. It's wrong to attach it directly to the easy handle. Probably a refactoring is necessary to fix this bug properly.

The text was updated successfully, but these errors were encountered:

p1ng0o · 2017-01-26T12:28:45Z

Reproduced here, with simple digest auth. (curl 7.51.1)

HOWTO:
Curl multi

Attach easy handle
Set CURLOPT_HTTPAUTH to CURLAUTH_DIGEST
Set CURLOPT_USERNAME and CURLOPT_PASSWORD
Doing request -> 200
curl_easy_reset()
Update CURLOPT_USERNAME and CURLOPT_PASSWORD with another allowed user
Doing request -> 401

Only destroy and re-create handle do the trick

bagder · 2017-01-26T16:09:07Z

Reproduced here, with simple digest auth

An NTLM problem cannot be reproduced with Digest. You may have a similar symptom, but it is not this same issue.

michael-o · 2017-01-26T17:49:13Z

I agree here with Daniel, NTLM is strictly connection-bound while Digest ist not.

iboukris · 2017-01-26T20:04:02Z

I think it could be reproduced by setting ntlm on the first request to a server which does not require authentication, then post body is omitted from the subsequent request (even if the connection is not reused and the easy is reset between operations).

Reproduction with '--next' (git-master), against a server on localhost which simply echos back post body:
curl -v -ua:b --ntlm http://127.0.0.1/ --next -F a=b http://localhost/echo.php

jay · 2017-01-26T20:12:12Z

Yes that looks like it. You can also reproduce using http://httpbin.org/post as the URL if you don't have a local server

iboukris · 2017-01-26T22:49:29Z

I think, the digest issue may be related as in digest flow we also may not set authhost.done to true.
This causes authhost.multi to be set to true and later to conn->bits.authneg to be set to true and then for the body to be omitted.

To my understanding, authhost.multi is used when we know that authentication is not complete yet (we expect 401), that's in order to avoid posting the data unnecessarily.

As such, I think it will be safe to clear authhost.multi at http_done() after the flag was used, since even if we'll be still expecting yet another 401 (unlikely), we'll be passing again at output_auth_headers() and it will be set back to true if necessary.

As @mkauf noted, a refactoring of this logic would be great, but in the meanwhile, the below patch seem to work (passes make test):
iboukris@459f464

iboukris · 2017-01-26T23:06:05Z

In fact, the same command can be used to reproduce digest (and is solved by the above commit):
curl -v -ua:b --digest http://httpbin.org/post --next -F a=b http://httpbin.org/post

jay · 2017-01-26T23:42:38Z

That is my understanding as well. The only possible conflict I can think of is if it could mess up when multiple authentication methods are provided but reading the code just now it doesn't look like it. I assume that's covered in the automated tests, anyway.

iboukris · 2017-01-27T11:24:00Z

I don't think it will conflict with multiple auth-methods, in that case the local 'auth' variable in output_auth_headers() does not get set, and so auth.multi gets set to false (then the request gets sent without any auth header and we get the list of supported auth methods from the server and we pick one auth from the intersection of wanted and supported auth-methods).

jay · 2017-02-01T07:40:02Z

yup. my concern was multi might also be used for starting the auth, for example imagine if there was some code like if(!someauth->multi) // try first multipass auth and then there could be some scenario where it doesn't function correctly, for example doesn't actually cycle through all the authentication methods. unfortunately trying to flesh out all the scenarios is a little difficult because the variable is named 'multi' and when I do a find symbols in Visual Studio or a git grep multi I of course get back like 99% multi pointers for multi handles. manually following the flow I did not find any conflicts.

I am going to add a test and I will land this tomorrow/Wed, if there are no objections.

This flag is meant for the current request based on authentication state, once the request is done we can clear the flag. Also change auth.multi to auth.multipass for better readability. See discussion at curl#1095 Signed-off-by: Isaac Boukris <iboukris@gmail.com> Reported-by: Michael Kaufmann

mkauf · 2017-02-01T19:53:46Z

We should also clear the whole auth state at the bottom of curl_easy_reset() with:

/* reset authentication flags */
memset(&data->state.authhost, 0, sizeof(struct auth));
memset(&data->state.authproxy, 0, sizeof(struct auth));

I don't understand why curl_easy_reset() keeps some state. The documentation says:

This puts back the handle to the same state as it was in when it was just created with curl_easy_init.

... but obviously that's not what curl_easy_reset() does.

iboukris · 2017-02-01T19:54:32Z

Yea, the multi name doesn't help much, let's change it to multipass: iboukris@57ce2ec

iboukris · 2017-02-01T20:00:38Z

@mkauf we probably should reset auth-struct in easy reset, but that by itself will not address the case of easy reuse without reset (i think).

mkauf · 2017-02-01T20:16:13Z

@Frenche yes, your patch is needed too! we should commit them both.

jay · 2017-02-01T21:59:24Z

Maybe we should have a better fix for curl_easy_reset, I've seen similar. Perhaps we should make a new handle, move over pointers to what is documented to still exist, cleanup the original handle and then memcpy that whole new handle back into the original handle. I'm still in the thought stage but how does that sound. The only problem that could stick is if anything in the new handle is a pointer to itself, though tests would catch it if that happened.

iboukris · 2017-02-03T09:57:49Z

@jay yea, that's probably how I'd picture easy_reset to look like.

mkauf · 2017-03-11T11:11:47Z

I think we should go forward and push the two bugfixes that we have developed.

iboukris · 2017-03-11T11:57:59Z

I opened #1326 to clear the multi(pass) flag at http_done().

Follow-up to 5278462 See #1095

mkauf · 2017-03-11T17:46:13Z

I have pushed the two bugfixes, thanks to everyone who has worked on this issue!

p1ng0o mentioned this issue Jan 26, 2017

Using forced Negotiate auth for reused connections #316

Closed

iboukris mentioned this issue Mar 11, 2017

authneg: clear auth.multi flag at http_done #1326

Closed

mkauf closed this as completed in 5278462 Mar 11, 2017

mkauf added a commit that referenced this issue Mar 11, 2017

curl_easy_reset: Also reset the authentication state

0afbcfd

Follow-up to 5278462 See #1095

lock bot locked as resolved and limited conversation to collaborators May 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authentication state is not reset properly when using NTLM #1095

Authentication state is not reset properly when using NTLM #1095

mkauf commented Oct 31, 2016

p1ng0o commented Jan 26, 2017

bagder commented Jan 26, 2017

michael-o commented Jan 26, 2017

iboukris commented Jan 26, 2017

jay commented Jan 26, 2017

iboukris commented Jan 26, 2017 •

edited

iboukris commented Jan 26, 2017

jay commented Jan 26, 2017

iboukris commented Jan 27, 2017 •

edited

jay commented Feb 1, 2017

mkauf commented Feb 1, 2017

iboukris commented Feb 1, 2017

iboukris commented Feb 1, 2017 •

edited

mkauf commented Feb 1, 2017

jay commented Feb 1, 2017

iboukris commented Feb 3, 2017

mkauf commented Mar 11, 2017

iboukris commented Mar 11, 2017

mkauf commented Mar 11, 2017

Authentication state is not reset properly when using NTLM #1095

Authentication state is not reset properly when using NTLM #1095

Comments

mkauf commented Oct 31, 2016

I did this

I expected the following

curl/libcurl version

operating system

hints

p1ng0o commented Jan 26, 2017

bagder commented Jan 26, 2017

michael-o commented Jan 26, 2017

iboukris commented Jan 26, 2017

jay commented Jan 26, 2017

iboukris commented Jan 26, 2017 • edited

iboukris commented Jan 26, 2017

jay commented Jan 26, 2017

iboukris commented Jan 27, 2017 • edited

jay commented Feb 1, 2017

mkauf commented Feb 1, 2017

iboukris commented Feb 1, 2017

iboukris commented Feb 1, 2017 • edited

mkauf commented Feb 1, 2017

jay commented Feb 1, 2017

iboukris commented Feb 3, 2017

mkauf commented Mar 11, 2017

iboukris commented Mar 11, 2017

mkauf commented Mar 11, 2017

iboukris commented Jan 26, 2017 •

edited

iboukris commented Jan 27, 2017 •

edited

iboukris commented Feb 1, 2017 •

edited