Menu
▾
▴
#1127 auth status not being cleared when handles are reset
Using libcurl 7.24.0:
I'm trying to set up an app to connect to a server that supports GSS-Negotiate and NTLM auth. If the client-side auth setup is incorrect I want to fall back to using NTLM.
So this is pretty simple:
void doRequest(const char* url, const char* username, const char* password, bool allowNegotiate)
{
int numErrors = 0;
CURL* handle = curl_easy_init();
curl_easy_setopt(handle, CURLOPT_URL, url);
curl_easy_setopt(handle, CURLOPT_HTTPGET, true);
curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, writeCallback);
curl_easy_setopt(handle, CURLOPT_HEADERFUNCTION, headerCallback); // increments numErrors on every 401
curl_easy_setopt(handle, CURLOPT_HEADERDATA, &numErrors);
curl_easy_setopt(handle, CURLOPT_USERNAME, username);
curl_easy_setopt(handle, CURLOPT_PASSWORD, password);
long allowedAuthSchemes = CURLAUTH_NTLM;
if (allowNegotiate) {
allowedAuthSchemes |= CURLAUTH_GSSNEGOTIATE;
}
curl_easy_setopt(handle, CURLOPT_HTTPAUTH, allowedAuthSchemes);
curl_easy_perform(handle);
curl_easy_cleanup(handle);
// expect one 401 to be sent during auth setup; anything more than that means auth failed
if (numErrors > 1 && allowNegotiate) {
// try again without Negotiate
doRequest(url, username, password, false);
}
}
We set up a request allowing CURLAUTH_NTLM | CURLAUTH_GSSNEGOTIATE and, if it receives too many 401 errors, send it again with just CURLAUTH_NTLM.
To both requests, the server will send back:
WWW-Authenticate: Negotiate
WWW-Authenticate: NTLM
The problem comes in Curl_http_input_auth:
if(checkprefix("GSS-Negotiate", start) ||
checkprefix("Negotiate", start)) {
int neg;
*availp |= CURLAUTH_GSSNEGOTIATE;
authp->avail |= CURLAUTH_GSSNEGOTIATE;
if(authp->picked == CURLAUTH_GSSNEGOTIATE) {
if(data->state.negotiate.state == GSS_AUTHSENT) {
/* if we sent GSS authentication in the outgoing request and we get
this back, we're in trouble */
infof(data, "Authentication problem. Ignoring this.\n");
data->state.authproblem = TRUE;
}
else {
neg = Curl_input_negotiate(conn, (bool)(httpcode == 407), start);
if(neg == 0) {
DEBUGASSERT(!data->req.newurl);
data->req.newurl = strdup(data->change.url);
if(!data->req.newurl)
return CURLE_OUT_OF_MEMORY;
data->state.authproblem = FALSE;
/* we received GSS auth info and we dealt with it fine */
data->state.negotiate.state = GSS_AUTHRECV;
}
else
data->state.authproblem = TRUE;
}
}
}
The first time through, it reads the Negotiate header, updates "avail", and then does nothing since "picked" is 0. After reading all the headers, it will set set "picked" to CURLAUTH_GSSNEGOTIATE because that's the highest priority, and attempt to output a negotiate header. (Which will fail because, for this test, GSS auth is set up wrong.)
The second time through, it reads the Negotiate header, updates "avail", and then goes into the "if" statement because "picked" is still CURLAUTH_NEGOTIATE from the first pass. Even though we're using a different handle! It then calls Curl_input_negotiate, which will call some gss functions and return an error code when they fail. It then sets authproblem to TRUE, which means that even though it reads the NTLM header it will not continue to send the NTLM auth as we want. It seems to me that it should be ignoring this Negotiate header entirely, since in this second pass CURLAUTH_GSSNEGOTIATE isn't even included in CURLOPT_HTTPAUTH.
(I've also tried adding CURLOPT_FRESH_CONNECT when allowNegotiate is false, to force it to use a new connection, but "picked" is still set.)
So it looks like "picked" should be cleared at some point when Negotiate auth fails, so that it doesn't keep trying to use it for further requests. But I'm not sure where this is should be done.
Another option would be to change that if statement to "if(authp->picked == CURLAUTH_GSSNEGOTIATE && authp->want & CURLAUTH_GSSNEGOTIATE)", so that if we don't want negotiate it just ignores the value of picked here, but I think that would just be hiding the problem.
Actually I think picked needs to be cleared even on a successful auth. I have a server that serves most resources with "WWW-Authenticate: Negotiate" and "WWW-Authenticate: Basic" headers, but one resource (call it B) that only has Basic for some reason. When fetching a page that includes subresources A, B, C, curl chooses Negotiate for A and Basic for B (since there's no Negotiate header to trigger the code above). I would expect curl to choose Negotiate again for C since it's most secure, but actually it chooses Basic again because picked is still set from the last request.
Thanks for the report.
Do you have any recipe to repeat/see this problem in a decent way?
Better: I have a test suite! See the reset_test branch at https://github.com/JoeNotCharles/curl.git. (Also attached as a patch.)
I added 9 tests (starting from test2023) which are each driven by a tool called "libauthretry" - I chose to name it this rather than following the pattern of naming it lib2023 because it's used from so many tests, I thought a descriptive name was better.
The tool takes two auth types (for example Digest and NTLM) and sends the following requests:
create new curl handle with curl_easy_init
(A) get url using testuser:wrongpass enabling only auth type 1 - expect 401
curl_easy_reset
(B) get url using testuser:testpass enabling only auth type 2 - expect 200
create new handle with curl_easy_cleanup / curl_easy_init
(C) get url using testuser:wrongpass enabling only auth type 1 - expect 401
curl_easy_reset
(D) get url using testuser:wrongpass enabling only auth type 2 - expect 401
curl_easy_reset
(E) get url using testuser:testpass enabling only auth type 2 - expect 200
curl_easy_cleanup
So for a given pair of auth types, this tests whether data persists across a handle reuse (B), whether it persists with a connection even with a new handle (C), whether an authproblem messes up a subsequent successful auth (A -> B), whether an authproblem messes up a subsequenct failed auth (C -> D), and whether an authproblem still messes up if the following auth is of the same type as the original (D -> E).
The 9 tests run this tool on each pair of auth types, including having both the "main" and "fallback" auth types be the same. For example, test2023 is "basic basic", and tests whether retrying an auth with the same handle using only Basic auth works; test2024 is "basic digest", and tests whether retrying a failed Basic auth using Digest auth works; test2026 is "digest basic", which is the opposite - start with digest and fall back to basic.
I made all these tests because in my quick attempts to fix this I found that by fixing NTLM I caused new failures in Digest, so it seems like the code paths for each auth type use different information. So it's important to test each different auth type as both the first auth (which leaves state behind) and the second auth (which reads the state).
I doubt the actual contents of the Digest and NTLM headers are correct - the important thing right now is whether requests with Basic, Digest and NTLM auth are sent at the right times. While fixing, if we get a test to the point where it succeeds except that the hashes aren't correct, we can fix the tests to use the hashes we see curl sending.
Running these I see:
test2023 - basic only - succeeds
test2024 - basic -> digest - curl continues to use Basic auth for subsequent requests even though only Digest is enabled ("picked" is not reset). Also it prints "Authentication problem. Ignoring this." before many but not all WWW-Authenticate headers, suggesting authproblem is set incorrectly.
test2025 - basic -> ntlm - gets in an infinite loop printing "additional stuff not fine transfer.c:1037: 0 0"
test2026 - digest -> basic - the handle receives a 401 with digest nonce "2" before the reset. After the reset, the first request sent out by curl includes a digest with nonce "2", even though only Basic is enabled. (Using that nonce would actually be correct for Digest, but I believe the reset should have cleared curl's knowledge of auth schemes the site supports, so there should be no way for curl to know that - I expect it to send a new request with no authorization)
test2027 - digest only - as above, after the reset sends a request with the nonce from before the reset (which succeeds because this is an acceptable thing to do). However, when sending multiple failed digests in a with a reset between, curl prints "Ignoring duplicate digest auth header." and continues to use the same nonce over and over.
test2028 - digest -> ntlm - gets in an infinite loop printing "additional stuff not fine transfer.c:1037: 0 0"
test2029 - ntlm -> basic - whenever switching to Basic auth after using NTLM, curl prints "NTLM handshake failure (internal error)", showing that it has saved the NTLM transaction state and is expecting this new request to continue the previous NTLM auth transaction. Also it continues to choose NTLM over Basic even when NTLM is not included in CURLOPT_AUTH
test2030 - ntlm -> digest - pretty much the same as 2029, except now I also see "Authentication problem. Ignoring this." and "Ignoring duplicate digest auth header." from Digest, although curl never chooses to actually use digest.
test2031 - ntlm only - gets in an infinite loop printing "additional stuff not fine transfer.c:1037: 0 0"
test suite
The infinite loop in the NTLM tests seems to be a limitation of the test suite:
When sending a new NTLM request with a different password, this code forces it to use a new connection:
if((needle->handler->protocol & CURLPROTO_FTP) ||
((needle->handler->protocol & CURLPROTO_HTTP) &&
((data->state.authhost.want==CURLAUTH_NTLM) ||
(data->state.authhost.want==CURLAUTH_NTLM_WB)))) {
/* This is FTP or HTTP+NTLM, verify that we're using the same name
and password as well */
if(!strequal(needle->user, check->user) ||
!strequal(needle->passwd, check->passwd)) {
/* one of them was different */
continue;
}
But when that happens the old connection is never closed (I think this is correct; it will be closed if the connection cache fills up, or by the server on a timeout).
However the sws test server can't deal with requests coming on more than one connection at once. It ignores the second request which is coming on a second connection, and so the server is waiting forever for the client to either close the first connection or send another request on it, and the client is waiting forever for the server to respond to its second request.
I can cause the test to not infinite loop by adding "swsclose" to data100 and data1402 (the two requests which came right before an NTLM request that has a different password) to close the socket from the server side. But I'd prefer to fix the server so that this test doesn't depend so closely on the internals of when connections can be reused.
(After working around the infinite loop, the test still fails: "NTLM handshake failure (internal error)" in the logs.)
As an aside, I think the above check is too strict: if a connection has never had an NTLM request (or another connection-oriented auth), it can be reused for NTLM no matter what the password is. Currently it forces a new connection if you send a Basic auth request with password "pass1" and then an NTLM request with password "pass2". I think this should be ok. But what it's doing is correct, just not efficient.
Using the -f option to runtest.pl (to make the server fork a new process to handle each connection) also works around the NTLM loop, although it leaves a bunch of copies of sws running when the test finishes. (Which prevents it from running again until you kill them, because they're still using the port.)
I now have a fix in https://github.com/JoeNotCharles/curl/commits/reset_fix. There are 2 patches with separate independent fixes, 1 with new tests, and 5 refactoring the test suite:
24c43e9d34615236489bde4797ce50de4bb56a84 - add 9 new tests covering this bug
50b87c4e689088fc3ddcf2fac163b75f839ef69a - if receiving a bare "NTLM" header after NTLM auth has started, curl always considers this an "internal error" and aborts. But if NTLM has reached NTLMSTATE_TYPE3, a bare "NTLM" is expected - it just means the password has been rejected and the server is saying to try again. In this case, we need to clear the NTLM state so it will be restarted.
ce8311c7e49eca93c136b58efa6763853541ec97 - zero our state.authhost and state.authproxy in Curl_pretransfer, to ensure that auth->picked is not incorrectly inherited from the last transfer.
5 patches from Aug 5: refactor sws to allow multiple sockets and poll (this still has an intermittent bug where sometimes sockets that haven't been used for a while are closed, and then sws goes into an infinite loop. But I've been able to run the test suite through several times, and without the refactor it goes into this loop with the new tests 100% of the time.)
Thanks for the report, this problem is now fixed in the git repository.
To try it out, you either checkout/update your git clone: http://curl.haxx.se/source.html
or you try tomorrow's daily snapshot: http://curl.haxx.se/snapshots/