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

Authentication state is not reset properly when using NTLM #1095

Closed
mkauf opened this Issue Oct 31, 2016 · 19 comments

Comments

Projects
None yet
6 participants
@mkauf
Contributor

mkauf commented Oct 31, 2016

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.

@p1ng0o

This comment has been minimized.

Show comment
Hide comment
@p1ng0o

p1ng0o Jan 26, 2017

Contributor

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

Contributor

p1ng0o commented Jan 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 26, 2017

Member

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.

Member

bagder commented Jan 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@michael-o

michael-o Jan 26, 2017

Member

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

Member

michael-o commented Jan 26, 2017

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

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 26, 2017

Contributor

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

Contributor

frenche commented Jan 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jan 26, 2017

Member

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

Member

jay commented Jan 26, 2017

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

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 26, 2017

Contributor

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):
frenche@459f464

Contributor

frenche commented Jan 26, 2017

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):
frenche@459f464

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 26, 2017

Contributor

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

Contributor

frenche commented Jan 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jan 26, 2017

Member

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.

Member

jay commented Jan 26, 2017

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.

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 27, 2017

Contributor

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).

Contributor

frenche commented Jan 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 1, 2017

Member

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.

Member

jay commented Feb 1, 2017

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.

frenche added a commit to frenche/curl that referenced this issue Feb 1, 2017

authneg: clear auth.multi flag at http_done
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 #1095

Signed-off-by: Isaac Boukris <iboukris@gmail.com>
Reported-by: Michael Kaufmann
@mkauf

This comment has been minimized.

Show comment
Hide comment
@mkauf

mkauf Feb 1, 2017

Contributor

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.

Contributor

mkauf commented Feb 1, 2017

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.

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Feb 1, 2017

Contributor

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

Contributor

frenche commented Feb 1, 2017

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

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Feb 1, 2017

Contributor

@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).

Contributor

frenche commented Feb 1, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mkauf

mkauf Feb 1, 2017

Contributor

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

Contributor

mkauf commented Feb 1, 2017

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

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 1, 2017

Member

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.

Member

jay commented Feb 1, 2017

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.

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Feb 3, 2017

Contributor

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

Contributor

frenche commented Feb 3, 2017

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

@mkauf

This comment has been minimized.

Show comment
Hide comment
@mkauf

mkauf Mar 11, 2017

Contributor

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

Contributor

mkauf commented Mar 11, 2017

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

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Mar 11, 2017

Contributor

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

Contributor

frenche commented Mar 11, 2017

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

@mkauf mkauf closed this in 5278462 Mar 11, 2017

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

@mkauf

This comment has been minimized.

Show comment
Hide comment
@mkauf

mkauf Mar 11, 2017

Contributor

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

Contributor

mkauf commented Mar 11, 2017

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

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

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