netrc: address several netrc parser flaws

- make sure that a match that returns a username also returns a
  password, that should be blank if no password is found

- fix handling of multiple logins for same host where the password/login
  order might be reversed.

- reject credentials provided in the .netrc if they contain ASCII control
  codes - if the used protocol does not support such (like HTTP and WS do)

Reported-by: Harry Sintonen

Add test 478, 479 and 480 to verify. Updated unit 1304.

Closes #15586

lib/netrc.c

@@ -54,6 +54,9 @@ enum found_state {
   PASSWORD
 };
 
+#define FOUND_LOGIN    1
+#define FOUND_PASSWORD 2
+
 #define NETRC_FILE_MISSING 1
 #define NETRC_FAILED -1
 #define NETRC_SUCCESS 0
@@ -94,24 +97,24 @@ done:
  */
 static int parsenetrc(struct store_netrc *store,
                       const char *host,
-                      char **loginp,
+                      char **loginp, /* might point to a username */
                       char **passwordp,
                       const char *netrcfile)
 {
   int retcode = NETRC_FILE_MISSING;
   char *login = *loginp;
-  char *password = *passwordp;
+  char *password = NULL;
-  bool specific_login = (login && *login != 0);
+  bool specific_login = login; /* points to something */
-  bool login_alloc = FALSE;
-  bool password_alloc = FALSE;
   enum host_lookup_state state = NOTHING;
-  enum found_state found = NONE;
+  enum found_state keyword = NONE;
-  bool our_login = TRUE;  /* With specific_login, found *our* login name (or
+  unsigned char found = 0; /* login + password found bits, as they can come in
-                             login-less line) */
+                              any order */
+  bool our_login = FALSE;  /* found our login name */
   bool done = FALSE;
   char *netrcbuffer;
   struct dynbuf token;
   struct dynbuf *filebuf = &store->filebuf;
+  DEBUGASSERT(!*passwordp);
   Curl_dyn_init(&token, MAX_NETRC_TOKEN);
 
   if(!store->loaded) {
@@ -124,7 +127,7 @@ static int parsenetrc(struct store_netrc *store,
 
   while(!done) {
     char *tok = netrcbuffer;
-    while(tok) {
+    while(tok && !done) {
       char *tok_end;
       bool quoted;
       Curl_dyn_reset(&token);
@@ -198,11 +201,6 @@ static int parsenetrc(struct store_netrc *store,
         }
       }
 
-      if((login && *login) && (password && *password)) {
-        done = TRUE;
-        break;
-      }
-
       tok = Curl_dyn_ptr(&token);
 
       switch(state) {
@@ -212,11 +210,18 @@ static int parsenetrc(struct store_netrc *store,
              contents begin with the next .netrc line and continue until a
              null line (consecutive new-line characters) is encountered. */
           state = MACDEF;
-        else if(strcasecompare("machine", tok))
+        else if(strcasecompare("machine", tok)) {
           /* the next tok is the machine name, this is in itself the delimiter
              that starts the stuff entered for this machine, after this we
              need to search for 'login' and 'password'. */
           state = HOSTFOUND;
+          keyword = NONE;
+          found = 0;
+          our_login = FALSE;
+          Curl_safefree(password);
+          if(!specific_login)
+            Curl_safefree(login);
+        }
         else if(strcasecompare("default", tok)) {
           state = HOSTVALID;
           retcode = NETRC_SUCCESS; /* we did find our host */
@@ -238,44 +243,54 @@ static int parsenetrc(struct store_netrc *store,
         break;
       case HOSTVALID:
         /* we are now parsing sub-keywords concerning "our" host */
-        if(found == LOGIN) {
+        if(keyword == LOGIN) {
-          if(specific_login) {
+          if(specific_login)
             our_login = !Curl_timestrcmp(login, tok);
-          }
+          else {
-          else if(!login || Curl_timestrcmp(login, tok)) {
+            our_login = TRUE;
-            if(login_alloc)
+            free(login);
-              free(login);
             login = strdup(tok);
             if(!login) {
               retcode = NETRC_FAILED; /* allocation failed */
               goto out;
             }
-            login_alloc = TRUE;
           }
-          found = NONE;
+          found |= FOUND_LOGIN;
+          keyword = NONE;
         }
-        else if(found == PASSWORD) {
+        else if(keyword == PASSWORD) {
-          if((our_login || !specific_login) &&
+          free(password);
-             (!password || Curl_timestrcmp(password, tok))) {
+          password = strdup(tok);
-            if(password_alloc)
+          if(!password) {
-              free(password);
+            retcode = NETRC_FAILED; /* allocation failed */
-            password = strdup(tok);
+            goto out;
-            if(!password) {
-              retcode = NETRC_FAILED; /* allocation failed */
-              goto out;
-            }
-            password_alloc = TRUE;
           }
-          found = NONE;
+          found |= FOUND_PASSWORD;
+          keyword = NONE;
         }
         else if(strcasecompare("login", tok))
-          found = LOGIN;
+          keyword = LOGIN;
         else if(strcasecompare("password", tok))
-          found = PASSWORD;
+          keyword = PASSWORD;
         else if(strcasecompare("machine", tok)) {
-          /* ok, there is machine here go => */
+          /* a new machine here */
           state = HOSTFOUND;
-          found = NONE;
+          keyword = NONE;
+          found = 0;
+          Curl_safefree(password);
+          if(!specific_login)
+            Curl_safefree(login);
+        }
+        else if(strcasecompare("default", tok)) {
+          state = HOSTVALID;
+          retcode = NETRC_SUCCESS; /* we did find our host */
+          Curl_safefree(password);
+          if(!specific_login)
+            Curl_safefree(login);
+        }
+        if((found == (FOUND_PASSWORD|FOUND_LOGIN)) && our_login) {
+          done = TRUE;
+          break;
         }
         break;
       } /* switch (state) */
@@ -294,23 +309,23 @@ static int parsenetrc(struct store_netrc *store,
 
 out:
   Curl_dyn_free(&token);
+  if(!retcode && !password && our_login) {
+    /* success without a password, set a blank one */
+    password = strdup("");
+    if(!password)
+      retcode = 1; /* out of memory */
+  }
   if(!retcode) {
     /* success */
-    if(login_alloc) {
+    if(!specific_login)
-      free(*loginp);
       *loginp = login;
-    }
+    *passwordp = password;
-    if(password_alloc) {
-      free(*passwordp);
-      *passwordp = password;
-    }
   }
   else {
     Curl_dyn_free(filebuf);
-    if(login_alloc)
+    if(!specific_login)
       free(login);
-    if(password_alloc)
+    free(password);
-      free(password);
   }
 
   return retcode;

lib/url.c

@@ -2651,6 +2651,17 @@ static CURLcode parse_remote_port(struct Curl_easy *data,
   return CURLE_OK;
 }
 
+static bool str_has_ctrl(const char *input)
+{
+  const unsigned char *str = (const unsigned char *)input;
+  while(*str) {
+    if(*str < 0x20)
+      return TRUE;
+    str++;
+  }
+  return FALSE;
+}
+
 /*
  * Override the login details from the URL with that in the CURLOPT_USERPWD
  * option or a .netrc file, if applicable.
@@ -2682,29 +2693,40 @@ static CURLcode override_login(struct Curl_easy *data,
 
     if(data->state.aptr.user &&
        (data->state.creds_from != CREDS_NETRC)) {
-      /* there was a username in the URL. Use the URL decoded version */
+      /* there was a username with a length in the URL. Use the URL decoded
+         version */
       userp = &data->state.aptr.user;
       url_provided = TRUE;
     }
 
-    ret = Curl_parsenetrc(&data->state.netrc, conn->host.name,
+    if(!*passwdp) {
-                          userp, passwdp,
+      ret = Curl_parsenetrc(&data->state.netrc, conn->host.name,
-                          data->set.str[STRING_NETRC_FILE]);
+                            userp, passwdp,
-    if(ret > 0) {
+                            data->set.str[STRING_NETRC_FILE]);
-      infof(data, "Couldn't find host %s in the %s file; using defaults",
+      if(ret > 0) {
-            conn->host.name,
+        infof(data, "Couldn't find host %s in the %s file; using defaults",
-            (data->set.str[STRING_NETRC_FILE] ?
+              conn->host.name,
-             data->set.str[STRING_NETRC_FILE] : ".netrc"));
+              (data->set.str[STRING_NETRC_FILE] ?
-    }
+               data->set.str[STRING_NETRC_FILE] : ".netrc"));
-    else if(ret < 0) {
+      }
-      failf(data, ".netrc parser error");
+      else if(ret < 0) {
-      return CURLE_READ_ERROR;
+        failf(data, ".netrc parser error");
-    }
+        return CURLE_READ_ERROR;
-    else {
+      }
-      /* set bits.netrc TRUE to remember that we got the name from a .netrc
+      else {
-         file, so that it is safe to use even if we followed a Location: to a
+        if(!(conn->handler->flags&PROTOPT_USERPWDCTRL)) {
-         different host or similar. */
+          /* if the protocol can't handle control codes in credentials, make
-      conn->bits.netrc = TRUE;
+             sure there are none */
+          if(str_has_ctrl(*userp) || str_has_ctrl(*passwdp)) {
+            failf(data, "control code detected in .netrc credentials");
+            return CURLE_READ_ERROR;
+          }
+        }
+        /* set bits.netrc TRUE to remember that we got the name from a .netrc
+           file, so that it is safe to use even if we followed a Location: to a
+           different host or similar. */
+        conn->bits.netrc = TRUE;
+      }
     }
     if(url_provided) {
       Curl_safefree(conn->user);

tests/data/Makefile.am

@@ -77,7 +77,7 @@ test435 test436 test437 test438 test439 test440 test441 test442 test443 \
 test444 test445 test446 test447 test448 test449 test450 test451 test452 \
 test453 test454 test455 test456 test457 test458 test459 test460 test461 \
 test462 test463 test467 test468 test469 test470 test471 test472 test473 \
-test474 test475 test476 test477 \
+test474 test475 test476 test477 test478 test479 test480 \
 \
 test490 test491 test492 test493 test494 test495 test496 test497 test498 \
 test499 test500 test501 test502 test503 test504 test505 test506 test507 \

tests/data/test478

@@ -0,0 +1,73 @@
+<testcase>
+<info>
+<keywords>
+netrc
+HTTP
+</keywords>
+</info>
+#
+# Server-side
+<reply>
+<data crlf="yes">
+HTTP/1.1 200 OK
+Date: Tue, 09 Nov 2010 14:49:00 GMT
+Server: test-server/fake
+Last-Modified: Tue, 13 Jun 2000 12:10:00 GMT
+ETag: "21025-dc7-39462498"
+Accept-Ranges: bytes
+Content-Length: 6
+Connection: close
+Content-Type: text/html
+Funny-head: yesyes
+
+-foo-
+</data>
+</reply>
+
+#
+# Client-side
+<client>
+<server>
+http
+</server>
+<features>
+proxy
+</features>
+<name>
+.netrc with multiple accounts for same host
+</name>
+<command>
+--netrc --netrc-file %LOGDIR/netrc%TESTNUMBER -x http://%HOSTIP:%HTTPPORT/ http://debbie@github.com/
+</command>
+<file name="%LOGDIR/netrc%TESTNUMBER" >
+
+machine github.com
+password weird
+password firstone
+login daniel
+
+machine github.com
+
+machine github.com
+login debbie
+
+machine github.com
+password weird
+password "second\r"
+login debbie
+
+</file>
+</client>
+
+<verify>
+<protocol>
+GET http://github.com/ HTTP/1.1
+Host: github.com
+Authorization: Basic %b64[debbie:second%0D]b64%
+User-Agent: curl/%VERSION
+Accept: */*
+Proxy-Connection: Keep-Alive
+
+</protocol>
+</verify>
+</testcase>

tests/data/test479

@@ -0,0 +1,107 @@
+<testcase>
+<info>
+<keywords>
+netrc
+HTTP
+</keywords>
+</info>
+#
+# Server-side
+<reply>
+<data crlf="yes">
+HTTP/1.1 301 Follow this you fool
+Date: Tue, 09 Nov 2010 14:49:00 GMT
+Server: test-server/fake
+Last-Modified: Tue, 13 Jun 2000 12:10:00 GMT
+ETag: "21025-dc7-39462498"
+Accept-Ranges: bytes
+Content-Length: 6
+Connection: close
+Location: http://b.com/%TESTNUMBER0002
+
+-foo-
+</data>
+
+<data2 crlf="yes">
+HTTP/1.1 200 OK
+Date: Tue, 09 Nov 2010 14:49:00 GMT
+Server: test-server/fake
+Last-Modified: Tue, 13 Jun 2000 12:10:00 GMT
+ETag: "21025-dc7-39462498"
+Accept-Ranges: bytes
+Content-Length: 7
+Connection: close
+
+target
+</data2>
+
+<datacheck crlf="yes">
+HTTP/1.1 301 Follow this you fool
+Date: Tue, 09 Nov 2010 14:49:00 GMT
+Server: test-server/fake
+Last-Modified: Tue, 13 Jun 2000 12:10:00 GMT
+ETag: "21025-dc7-39462498"
+Accept-Ranges: bytes
+Content-Length: 6
+Connection: close
+Location: http://b.com/%TESTNUMBER0002
+
+HTTP/1.1 200 OK
+Date: Tue, 09 Nov 2010 14:49:00 GMT
+Server: test-server/fake
+Last-Modified: Tue, 13 Jun 2000 12:10:00 GMT
+ETag: "21025-dc7-39462498"
+Accept-Ranges: bytes
+Content-Length: 7
+Connection: close
+
+target
+</datacheck>
+</reply>
+
+#
+# Client-side
+<client>
+<server>
+http
+</server>
+<features>
+proxy
+</features>
+<name>
+.netrc with redirect and default without password
+</name>
+<command>
+--netrc --netrc-file %LOGDIR/netrc%TESTNUMBER -L -x http://%HOSTIP:%HTTPPORT/ http://a.com/
+</command>
+<file name="%LOGDIR/netrc%TESTNUMBER" >
+
+machine a.com
+  login alice
+  password alicespassword
+
+default
+  login bob
+
+</file>
+</client>
+
+<verify>
+<protocol>
+GET http://a.com/ HTTP/1.1
+Host: a.com
+Authorization: Basic %b64[alice:alicespassword]b64%
+User-Agent: curl/%VERSION
+Accept: */*
+Proxy-Connection: Keep-Alive
+
+GET http://b.com/%TESTNUMBER0002 HTTP/1.1
+Host: b.com
+Authorization: Basic %b64[bob:]b64%
+User-Agent: curl/%VERSION
+Accept: */*
+Proxy-Connection: Keep-Alive
+
+</protocol>
+</verify>
+</testcase>

tests/data/test480

@@ -0,0 +1,38 @@
+<testcase>
+<info>
+<keywords>
+netrc
+pop3
+</keywords>
+</info>
+#
+# Server-side
+<reply>
+
+</reply>
+
+#
+# Client-side
+<client>
+<server>
+pop3
+</server>
+<name>
+Reject .netrc with credentials using CRLF for POP3
+</name>
+<command>
+--netrc --netrc-file %LOGDIR/netrc%TESTNUMBER pop3://%HOSTIP:%POP3PORT/%TESTNUMBER
+</command>
+<file name="%LOGDIR/netrc%TESTNUMBER" >
+machine %HOSTIP
+  login alice
+  password "password\r\ncommand"
+</file>
+</client>
+
+<verify>
+<errorcode>
+26
+</errorcode>
+</verify>
+</testcase>

tests/unit/unit1304.c

@@ -32,13 +32,8 @@ static char *s_password;
 
 static CURLcode unit_setup(void)
 {
-  s_password = strdup("");
+  s_password = NULL;
-  s_login = strdup("");
+  s_login = NULL;
-  if(!s_password || !s_login) {
-    Curl_safefree(s_password);
-    Curl_safefree(s_login);
-    return CURLE_OUT_OF_MEMORY;
-  }
   return CURLE_OK;
 }
 
@@ -60,89 +55,61 @@ UNITTEST_START
   result = Curl_parsenetrc(&store,
                            "test.example.com", &s_login, &s_password, arg);
   fail_unless(result == 1, "Host not found should return 1");
-  abort_unless(s_password != NULL, "returned NULL!");
+  abort_unless(s_password == NULL, "password did not return NULL!");
-  fail_unless(s_password[0] == 0, "password should not have been changed");
+  abort_unless(s_login == NULL, "user did not return NULL!");
-  abort_unless(s_login != NULL, "returned NULL!");
-  fail_unless(s_login[0] == 0, "login should not have been changed");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test a non existent login in our netrc file.
    */
-  free(s_login);
+  s_login = (char *)"me";
-  s_login = strdup("me");
-  abort_unless(s_login != NULL, "returned NULL!");
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &s_login, &s_password, arg);
   fail_unless(result == 0, "Host should have been found");
-  abort_unless(s_password != NULL, "returned NULL!");
+  abort_unless(s_password == NULL, "password is not NULL!");
-  fail_unless(s_password[0] == 0, "password should not have been changed");
-  abort_unless(s_login != NULL, "returned NULL!");
-  fail_unless(strncmp(s_login, "me", 2) == 0,
-              "login should not have been changed");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test a non existent login and host in our netrc file.
    */
-  free(s_login);
+  s_login = (char *)"me";
-  s_login = strdup("me");
-  abort_unless(s_login != NULL, "returned NULL!");
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "test.example.com", &s_login, &s_password, arg);
   fail_unless(result == 1, "Host not found should return 1");
-  abort_unless(s_password != NULL, "returned NULL!");
+  abort_unless(s_password == NULL, "password is not NULL!");
-  fail_unless(s_password[0] == 0, "password should not have been changed");
-  abort_unless(s_login != NULL, "returned NULL!");
-  fail_unless(strncmp(s_login, "me", 2) == 0,
-              "login should not have been changed");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test a non existent login (substring of an existing one) in our
    * netrc file.
    */
-  free(s_login);
+  s_login = (char *)"admi";
-  s_login = strdup("admi");
-  abort_unless(s_login != NULL, "returned NULL!");
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &s_login, &s_password, arg);
   fail_unless(result == 0, "Host should have been found");
-  abort_unless(s_password != NULL, "returned NULL!");
+  abort_unless(s_password == NULL, "password is not NULL!");
-  fail_unless(s_password[0] == 0, "password should not have been changed");
-  abort_unless(s_login != NULL, "returned NULL!");
-  fail_unless(strncmp(s_login, "admi", 4) == 0,
-              "login should not have been changed");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test a non existent login (superstring of an existing one)
    * in our netrc file.
    */
-  free(s_login);
+  s_login = (char *)"adminn";
-  s_login = strdup("adminn");
-  abort_unless(s_login != NULL, "returned NULL!");
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &s_login, &s_password, arg);
   fail_unless(result == 0, "Host should have been found");
-  abort_unless(s_password != NULL, "returned NULL!");
+  abort_unless(s_password == NULL, "password is not NULL!");
-  fail_unless(s_password[0] == 0, "password should not have been changed");
-  abort_unless(s_login != NULL, "returned NULL!");
-  fail_unless(strncmp(s_login, "adminn", 6) == 0,
-              "login should not have been changed");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test for the first existing host in our netrc file
    * with s_login[0] = 0.
    */
-  free(s_login);
+  s_login = NULL;
-  s_login = strdup("");
-  abort_unless(s_login != NULL, "returned NULL!");
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &s_login, &s_password, arg);
@@ -159,8 +126,9 @@ UNITTEST_START
    * with s_login[0] != 0.
    */
   free(s_password);
-  s_password = strdup("");
+  free(s_login);
-  abort_unless(s_password != NULL, "returned NULL!");
+  s_password = NULL;
+  s_login = NULL;
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &s_login, &s_password, arg);
@@ -177,11 +145,9 @@ UNITTEST_START
    * with s_login[0] = 0.
    */
   free(s_password);
-  s_password = strdup("");
+  s_password = NULL;
-  abort_unless(s_password != NULL, "returned NULL!");
   free(s_login);
-  s_login = strdup("");
+  s_login = NULL;
-  abort_unless(s_login != NULL, "returned NULL!");
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "curl.example.com", &s_login, &s_password, arg);
@@ -198,8 +164,9 @@ UNITTEST_START
    * with s_login[0] != 0.
    */
   free(s_password);
-  s_password = strdup("");
+  free(s_login);
-  abort_unless(s_password != NULL, "returned NULL!");
+  s_password = NULL;
+  s_login = NULL;
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "curl.example.com", &s_login, &s_password, arg);