From e9b9bbac22c26cf67316fa8e6c6b9e831af31949 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Fri, 15 Nov 2024 11:06:36 +0100
Subject: [PATCH] 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

Conflict:context adapt
Reference:https://github.com/curl/curl/e9b9bbac22c26cf67316fa8e6c6b9e831af31949
---
 lib/netrc.c            | 113 +++++++++++++++++++++++------------------
 lib/url.c              |  60 +++++++++++++++-------
 tests/data/Makefile.inc |  1 +
 tests/data/test478     |  73 ++++++++++++++++++++++++++
 tests/data/test479     | 107 ++++++++++++++++++++++++++++++++++++++
 tests/data/test480     |  38 ++++++++++++++
 tests/unit/unit1304.c  |  75 ++++++++-------------------
 7 files changed, 345 insertions(+), 122 deletions(-)
 create mode 100644 tests/data/test478
 create mode 100644 tests/data/test479
 create mode 100644 tests/data/test480

diff --git a/lib/netrc.c b/lib/netrc.c
index 034c0307a..e787a6ffc 100644
--- a/lib/netrc.c
+++ b/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;
-  bool specific_login = (login && *login != 0);
-  bool login_alloc = FALSE;
-  bool password_alloc = FALSE;
+  char *password = NULL;
+  bool specific_login = login; /* points to something */
   enum host_lookup_state state = NOTHING;
-  enum found_state found = NONE;
-  bool our_login = TRUE;  /* With specific_login, found *our* login name (or
-                             login-less line) */
+  enum found_state keyword = NONE;
+  unsigned char found = 0; /* login + password found bits, as they can come in
+                              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(specific_login) {
+        if(keyword == LOGIN) {
+          if(specific_login)
             our_login = !Curl_timestrcmp(login, tok);
-          }
-          else if(!login || Curl_timestrcmp(login, tok)) {
-            if(login_alloc)
-              free(login);
+          else {
+            our_login = TRUE;
+            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) {
-          if((our_login || !specific_login) &&
-             (!password || Curl_timestrcmp(password, tok))) {
-            if(password_alloc)
-              free(password);
-            password = strdup(tok);
-            if(!password) {
-              retcode = NETRC_FAILED; /* allocation failed */
-              goto out;
-            }
-            password_alloc = TRUE;
+        else if(keyword == PASSWORD) {
+          free(password);
+          password = strdup(tok);
+          if(!password) {
+            retcode = NETRC_FAILED; /* allocation failed */
+            goto out;
           }
-          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) {
-      free(*loginp);
+    if(!specific_login)
       *loginp = login;
-    }
-    if(password_alloc) {
-      free(*passwordp);
-      *passwordp = password;
-    }
+    *passwordp = password;
   }
   else {
     Curl_dyn_free(filebuf);
-    if(login_alloc)
+    if(!specific_login)
       free(login);
-    if(password_alloc)
-      free(password);
+    free(password);
   }
 
   return retcode;
diff --git a/lib/url.c b/lib/url.c
index f9bb05f79..436edd891 100644
--- a/lib/url.c
+++ b/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 user name 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,
-                          userp, passwdp,
-                          data->set.str[STRING_NETRC_FILE]);
-    if(ret > 0) {
-      infof(data, "Couldn't find host %s in the %s file; using defaults",
-            conn->host.name,
-            (data->set.str[STRING_NETRC_FILE] ?
-             data->set.str[STRING_NETRC_FILE] : ".netrc"));
-    }
-    else if(ret < 0) {
-      failf(data, ".netrc parser error");
-      return CURLE_READ_ERROR;
-    }
-    else {
-      /* 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(!*passwdp) {
+      ret = Curl_parsenetrc(&data->state.netrc, conn->host.name,
+                            userp, passwdp,
+                            data->set.str[STRING_NETRC_FILE]);
+      if(ret > 0) {
+        infof(data, "Couldn't find host %s in the %s file; using defaults",
+              conn->host.name,
+              (data->set.str[STRING_NETRC_FILE] ?
+               data->set.str[STRING_NETRC_FILE] : ".netrc"));
+      }
+      else if(ret < 0) {
+        failf(data, ".netrc parser error");
+        return CURLE_READ_ERROR;
+      }
+      else {
+        if(!(conn->handler->flags&PROTOPT_USERPWDCTRL)) {
+          /* if the protocol can't handle control codes in credentials, make
+             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);
diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc
index ea5221c00..53f62c6e2 100644
--- a/tests/data/Makefile.inc
+++ b/tests/data/Makefile.inc
@@ -77,6 +77,7 @@ test435 test436 test437 test438 test439 test440 test441 test442 test443 \
 test435 test436 test437 test438 test439 test440 test441 test442 test443 \
 test444 test445 test446 test447 test448 test449 test450 test451 test452 \
 test453 test454 test455 test456 test457 test458 \
+test478 test479 test480 \
 \
 test490 test491 test492 test493 test494 test495 test496 test497 test498 \
 \
diff --git a/tests/data/test478 b/tests/data/test478
new file mode 100644
index 000000000..6558363f5
--- /dev/null
+++ b/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>
diff --git a/tests/data/test479 b/tests/data/test479
new file mode 100644
index 000000000..d7ce4652f
--- /dev/null
+++ b/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>
diff --git a/tests/data/test480 b/tests/data/test480
new file mode 100644
index 000000000..aab889f47
--- /dev/null
+++ b/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>
diff --git a/tests/unit/unit1304.c b/tests/unit/unit1304.c
index 238d3c0f7..817887b94 100644
--- a/tests/unit/unit1304.c
+++ b/tests/unit/unit1304.c
@@ -32,13 +32,8 @@ static char *password;
 
 static CURLcode unit_setup(void)
 {
-  password = strdup("");
-  login = strdup("");
-  if(!password || !login) {
-    Curl_safefree(password);
-    Curl_safefree(login);
-    return CURLE_OUT_OF_MEMORY;
-  }
+  password = NULL;
+  login = NULL;
   return CURLE_OK;
 }
 
@@ -60,89 +55,61 @@ UNITTEST_START
   result = Curl_parsenetrc(&store,
                            "test.example.com", &login, &password, arg);
   fail_unless(result == 1, "Host not found should return 1");
-  abort_unless(password != NULL, "returned NULL!");
-  fail_unless(password[0] == 0, "password should not have been changed");
-  abort_unless(login != NULL, "returned NULL!");
-  fail_unless(login[0] == 0, "login should not have been changed");
+  abort_unless(password == NULL, "password did not return NULL!");
+  abort_unless(login == NULL, "user did not return NULL!");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test a non existent login in our netrc file.
    */
-  free(login);
-  login = strdup("me");
-  abort_unless(login != NULL, "returned NULL!");
+  login = (char *)"me";
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &login, &password, arg);
   fail_unless(result == 0, "Host should have been found");
-  abort_unless(password != NULL, "returned NULL!");
-  fail_unless(password[0] == 0, "password should not have been changed");
-  abort_unless(login != NULL, "returned NULL!");
-  fail_unless(strncmp(login, "me", 2) == 0,
-              "login should not have been changed");
+  abort_unless(password == NULL, "password is not NULL!");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test a non existent login and host in our netrc file.
    */
-  free(login);
-  login = strdup("me");
-  abort_unless(login != NULL, "returned NULL!");
+  login = (char *)"me";
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "test.example.com", &login, &password, arg);
   fail_unless(result == 1, "Host not found should return 1");
-  abort_unless(password != NULL, "returned NULL!");
-  fail_unless(password[0] == 0, "password should not have been changed");
-  abort_unless(login != NULL, "returned NULL!");
-  fail_unless(strncmp(login, "me", 2) == 0,
-              "login should not have been changed");
+  abort_unless(password == NULL, "password is not NULL!");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test a non existent login (substring of an existing one) in our
    * netrc file.
    */
-  free(login);
-  login = strdup("admi");
-  abort_unless(login != NULL, "returned NULL!");
+  login = (char *)"admi";
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &login, &password, arg);
   fail_unless(result == 0, "Host should have been found");
-  abort_unless(password != NULL, "returned NULL!");
-  fail_unless(password[0] == 0, "password should not have been changed");
-  abort_unless(login != NULL, "returned NULL!");
-  fail_unless(strncmp(login, "admi", 4) == 0,
-              "login should not have been changed");
+  abort_unless(password == NULL, "password is not NULL!");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test a non existent login (superstring of an existing one)
    * in our netrc file.
    */
-  free(login);
-  login = strdup("adminn");
-  abort_unless(login != NULL, "returned NULL!");
+  login = (char *)"adminn";
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &login, &password, arg);
   fail_unless(result == 0, "Host should have been found");
-  abort_unless(password != NULL, "returned NULL!");
-  fail_unless(password[0] == 0, "password should not have been changed");
-  abort_unless(login != NULL, "returned NULL!");
-  fail_unless(strncmp(login, "adminn", 6) == 0,
-              "login should not have been changed");
+  abort_unless(password == NULL, "password is not NULL!");
   Curl_netrc_cleanup(&store);
 
   /*
    * Test for the first existing host in our netrc file
    * with login[0] = 0.
    */
-  free(login);
-  login = strdup("");
-  abort_unless(login != NULL, "returned NULL!");
+  login = NULL;
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &login, &password, arg);
@@ -159,8 +126,9 @@ UNITTEST_START
    * with login[0] != 0.
    */
   free(password);
-  password = strdup("");
-  abort_unless(password != NULL, "returned NULL!");
+  free(login);
+  password = NULL;
+  login = NULL;
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "example.com", &login, &password, arg);
@@ -177,11 +145,9 @@ UNITTEST_START
    * with login[0] = 0.
    */
   free(password);
-  password = strdup("");
-  abort_unless(password != NULL, "returned NULL!");
+  password = NULL;
   free(login);
-  login = strdup("");
-  abort_unless(login != NULL, "returned NULL!");
+  login = NULL;
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "curl.example.com", &login, &password, arg);
@@ -198,8 +164,9 @@ UNITTEST_START
    * with login[0] != 0.
    */
   free(password);
-  password = strdup("");
-  abort_unless(password != NULL, "returned NULL!");
+  free(login);
+  password = NULL;
+  login = NULL;
   Curl_netrc_init(&store);
   result = Curl_parsenetrc(&store,
                            "curl.example.com", &login, &password, arg);
-- 
2.33.0