See email thread below.

Briefly:

- Make curl's test for same user and password before re-using a connection more robust.
- Once auth has finished (succeed or fail), close the connection. This stops the connection being re-used for auth
  again later, which confuses curl!

------ EMAIL THREAD STARTS HERE ------

A slight modification. I modified libcurl to do these things:

-	Check user name and password match before re-using a connection.

o	This ensures that if we ever had multiple, concurrent users, there is no chance a user would end up authenticated as someone else.

-	Close the connection once auth has succeeded or failed.

o	This ensures that a user won't end up re-using a connection from a previous auth. Curl's state machine doesn't like this. It's probably safest anyway.

-	Put a 32 byte random string in the password field (while picking up the NT hash from a file).

o	This is to protect against multiple _concurrent_ auth attempts from the _same_ user. Without this, the auth attempts might interfere and fail.

What do you think? I've done these changes outside XenClient so far and will fold them in tomorrow if they seem okay.

David

From: David Halls

Sent: 22 January 2010 18:25

To: Jean-Sebastien Legare

Cc: Ian Pratt; Jean Guyader; Chris Mayers
Subject: libcurl

OK so I found an unpleasant feature of libcurl today: it doesn't quite get NTLM right on its connections.

It doesn't mark NTLM connections as being in use for NTLM so in theory if it decides to re-use a connection doing NTLM, things can fail in weird ways. Here's the bug report I subsequently found:

37. Having more than one connection to the same host when doing NTLM

  authentication (with performs multiple "passes" and authenticates a

  connection rather than a HTTP request), and particularly when using the

  multi interface, there's a risk that libcurl will re-use a wrong connection

  when doing the different passes in the NTLM negotiation and thus fail to

  negotiate (in seemingly mysterious ways).

It also screws up if you happen to re-use a connection that's previously done NTLM, and want to do NTLM again (the existing bed test harness found that - well done whoever wrote that). It says the authentication is in a bad state. This is because it just re-uses the connection as it's to the same host.

What I've done so far for a fix is to modify curl's connection re-use logic like so:

- if a connection has a username and password associated with it, then it can only be re-used if the new request has a username and password; AND

- the username and password on the connection must match those on the request

This ensures that:

- NTLM still works since the challenge-response multiple requests is performed over the same connection

- connections aren't re-used inadvertently, and that when we do re-use them, it's for the same user. [In XenClient, we only do NTLM to login - after that, we use pass headers around to prove the user is logged on. So we don't need to re-use the authenticated connections.]

However, there remains one problem. If a second NTLM authentication (i.e. login) is required for the same user for any reason, and the first connection is still alive, then it fails. This is because the user name and passwords will match and the first connection will be re-used - and curl's state machine gets into a state.

So I have a workaround for this. We don't actually set any password - we pick the hash up from a file. So if I set the password field to a random string, everything works.

What do you think to this solution? Is it safe enough (I guess a long enough random string should be ok?)

David

diff --git a/package/libcurl/libcurl-7.19.7-nthash.patch b/package/libcurl/libcurl-7.19.7-nthash.patch
index 566fd0550aa1b6ae7cac3616505c1e764bfa91d7..5c92885c4d2765cdbc9677ecdfb59eb56388b4b1 100644 (file)
--- a/package/libcurl/libcurl-7.19.7-nthash.patch
+++ b/package/libcurl/libcurl-7.19.7-nthash.patch
@@ -24,7 +24,7 @@ diff -ru curl-7.19.7.orig/include/curl/typecheck-gcc.h curl-7.19.7/include/curl/
  /* evaluates to true if option takes a curl_write_callback argument */
 diff -ru curl-7.19.7.orig/lib/http_ntlm.c curl-7.19.7/lib/http_ntlm.c
 --- curl-7.19.7.orig/lib/http_ntlm.c   2009-04-23 12:51:08.000000000 +0100
-+++ curl-7.19.7/lib/http_ntlm.c        2009-11-18 15:38:40.000000000 +0000
++++ curl-7.19.7/lib/http_ntlm.c        2009-12-08 10:28:26.000000000 +0000
 @@ -483,6 +483,33 @@
                             const char *password,
                             unsigned char *ntbuffer /* 21 bytes */)
@@ -59,9 +59,24 @@ diff -ru curl-7.19.7.orig/lib/http_ntlm.c curl-7.19.7/lib/http_ntlm.c
    size_t len = strlen(password);
    unsigned char *pw = malloc(len*2);
    if(!pw)
+diff -ru curl-7.19.7.orig/lib/transfer.c curl-7.19.7/lib/transfer.c
+--- curl-7.19.7.orig/lib/transfer.c    2009-09-27 22:37:24.000000000 +0100
++++ curl-7.19.7/lib/transfer.c 2010-01-24 21:52:36.000000000 +0000
+@@ -1446,6 +1446,11 @@
+   }
+   while(!*stop_reading && *k->str); /* header line within buffer */
+ 
++  if (conn->bits.user_passwd && !data->req.newurl)
++  {
++    conn->bits.close = TRUE;
++  }
++
+   /* We might have reached the end of the header part here, but
+      there might be a non-header part left in the end of the read
+      buffer. */
 diff -ru curl-7.19.7.orig/lib/url.c curl-7.19.7/lib/url.c
 --- curl-7.19.7.orig/lib/url.c 2009-10-17 19:31:50.000000000 +0100
-+++ curl-7.19.7/lib/url.c      2009-11-18 14:02:36.000000000 +0000
++++ curl-7.19.7/lib/url.c      2010-01-24 21:30:35.000000000 +0000
 @@ -1666,6 +1666,13 @@
      result = setstropt(&data->set.str[STRING_PASSWORD],
                         va_arg(param, char *));
@@ -76,6 +91,31 @@ diff -ru curl-7.19.7.orig/lib/url.c curl-7.19.7/lib/url.c
    case CURLOPT_POSTQUOTE:
      /*
       * List of RAW FTP commands to use after a transfer
+@@ -2729,17 +2736,19 @@
+             continue;
+           }
+         }
++#define EQUALS(x, y) ((x == y) || (x && y && strequal(x, y)))
++
+         if((needle->protocol & PROT_FTP) ||
+            ((needle->protocol & PROT_HTTP) &&
+-            (data->state.authhost.want==CURLAUTH_NTLM))) {
+-          /* 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)) {
++            check->bits.user_passwd)) {
++          /* This has a user name and password, check that they match */ 
++          if((!EQUALS(needle->user, check->user)) ||
++             (!EQUALS(needle->passwd, check->passwd))) {
+             /* one of them was different */
+             continue;
+           }
+         }
++
+         match = TRUE;
+       }
+     }
 diff -ru curl-7.19.7.orig/lib/urldata.h curl-7.19.7/lib/urldata.h
 --- curl-7.19.7.orig/lib/urldata.h     2009-10-29 21:26:30.000000000 +0000
 +++ curl-7.19.7/lib/urldata.h  2009-11-18 14:03:02.000000000 +0000