From bb202352cb65102f934e724c1cff0e416a98dd3e Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Fri, 15 Nov 2024 11:06:36 +0100 Subject: [PATCH] [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 Backported by: Matheus Polkorny Based on the work of Marc Deslauriers for curl 7.81.0-1ubuntu1.20. Changes: - Refresh patch context. - Adjust `%LOGDIR/` to 'log/' due to its absence in bookworm. - Replaces the previous usage of the state_login, state_password, and state_our_login variables with the found_state enum, which includes the values NONE, LOGIN, and PASSWORD. As a result, all conditionals and memory management logic associated with these variables were updated. - Updates to use password and login instead of s_password and s_login, which do not exist in the Bookworm version. This change preserves the same logic while adapting the code to the current structure. - test478 is disabled as this version of curl does not support searching for a specific login in the netrc file. (see https://github.com/curl/curl/issues/8241) - test480 is disabled as this version of curl does not support quoted or escaped strings in the netrc file. (see https://github.com/curl/curl/issues/8908) - Small change in the Makefile to add a new test. Gbp-Pq: Name CVE-2024-11053.patch --- lib/netrc.c | 123 ++++++++++++++++++++++------------------ lib/url.c | 78 ++++++++++++++++--------- tests/data/DISABLED | 3 + tests/data/Makefile.inc | 2 + tests/data/test478 | 73 ++++++++++++++++++++++++ tests/data/test479 | 107 ++++++++++++++++++++++++++++++++++ tests/data/test480 | 38 +++++++++++++ tests/unit/unit1304.c | 75 +++++++----------------- 8 files changed, 362 insertions(+), 137 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 aa1b80ab..fb1f4955 100644 --- a/lib/netrc.c +++ b/lib/netrc.c @@ -49,6 +49,15 @@ enum host_lookup_state { MACDEF }; +enum found_state { + NONE, + LOGIN, + PASSWORD +}; + +#define FOUND_LOGIN 1 +#define FOUND_PASSWORD 2 + #define NETRC_FILE_MISSING 1 #define NETRC_FAILED -1 #define NETRC_SUCCESS 0 @@ -57,25 +66,23 @@ enum host_lookup_state { * Returns zero on success. */ static int parsenetrc(const char *host, - char **loginp, + char **loginp, /* might point to a username */ char **passwordp, char *netrcfile) { FILE *file; 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; - - char state_login = 0; /* Found a login keyword */ - char state_password = 0; /* Found a password keyword */ - int state_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 */ DEBUGASSERT(netrcfile); + DEBUGASSERT(!*passwordp); file = fopen(netrcfile, FOPEN_READTEXT); if(file) { @@ -94,7 +101,7 @@ static int parsenetrc(const char *host, continue; } tok = netrcbuffer; - while(tok) { + while(tok && !done) { while(ISBLANK(*tok)) tok++; /* tok is first non-space letter */ @@ -153,11 +160,6 @@ static int parsenetrc(const char *host, } } - if((login && *login) && (password && *password)) { - done = TRUE; - break; - } - switch(state) { case NOTHING: if(strcasecompare("macdef", tok)) { @@ -172,6 +174,12 @@ static int parsenetrc(const char *host, 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; @@ -195,48 +203,55 @@ static int parsenetrc(const char *host, break; case HOSTVALID: /* we are now parsing sub-keywords concerning "our" host */ - if(state_login) { + if(keyword == LOGIN) { if(specific_login) { - state_our_login = !Curl_timestrcmp(login, tok); + our_login = !Curl_timestrcmp(login, tok); } - else if(!login || Curl_timestrcmp(login, tok)) { - if(login_alloc) { - free(login); - login_alloc = FALSE; - } + else { + our_login = TRUE; + free(login); login = strdup(tok); if(!login) { retcode = NETRC_FAILED; /* allocation failed */ goto out; } - login_alloc = TRUE; } - state_login = 0; + found |= FOUND_LOGIN; + keyword = NONE; } - else if(state_password) { - if((state_our_login || !specific_login) - && (!password || Curl_timestrcmp(password, tok))) { - if(password_alloc) { - free(password); - password_alloc = FALSE; - } - 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; } - state_password = 0; + found |= FOUND_PASSWORD; + keyword = NONE; } else if(strcasecompare("login", tok)) - state_login = 1; + keyword = LOGIN; else if(strcasecompare("password", tok)) - state_password = 1; + keyword = PASSWORD; else if(strcasecompare("machine", tok)) { - /* ok, there's machine here go => */ + /* a new machine here */ state = HOSTFOUND; - state_our_login = FALSE; + 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) */ @@ -245,24 +260,22 @@ static int parsenetrc(const char *host, } /* while Curl_get_line() */ out: + 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(*loginp) - free(*loginp); + if(!specific_login) *loginp = login; - } - if(password_alloc) { - if(*passwordp) - free(*passwordp); - *passwordp = password; - } + *passwordp = password; } else { - if(login_alloc) + if(!specific_login) free(login); - if(password_alloc) - free(password); + free(password); } fclose(file); } diff --git a/lib/url.c b/lib/url.c index e6155dc0..cf0ad078 100644 --- a/lib/url.c +++ b/lib/url.c @@ -2702,6 +2702,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. @@ -2733,39 +2744,50 @@ 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(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", + if(!*passwdp) { + ret = Curl_parsenetrc(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]); - } - 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(url_provided) { - Curl_safefree(conn->user); - conn->user = strdup(*userp); - if(!conn->user) - return CURLE_OUT_OF_MEMORY; - } - /* no user was set but a password, set a blank user */ - if(!*userp && *passwdp) { - *userp = strdup(""); - if(!*userp) - return CURLE_OUT_OF_MEMORY; + } + 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); + conn->user = strdup(*userp); + if(!conn->user) + return CURLE_OUT_OF_MEMORY; + } + /* no user was set but a password, set a blank user */ + if(!*userp && *passwdp) { + *userp = strdup(""); + if(!*userp) + return CURLE_OUT_OF_MEMORY; + } } } #endif diff --git a/tests/data/DISABLED b/tests/data/DISABLED index 6edc0b44..21634abf 100644 --- a/tests/data/DISABLED +++ b/tests/data/DISABLED @@ -106,3 +106,6 @@ %if bearssl 313 %endif +# 478 and 480 are backported and do not work with this version of curl +478 +480 diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc index d69262e3..ceedd387 100644 --- a/tests/data/Makefile.inc +++ b/tests/data/Makefile.inc @@ -74,6 +74,8 @@ test430 test431 test432 test433 test434 test435 test436 \ \ test440 test441 test442 test443 test444 test445 test446 \ \ +test478 test479 test480 \ +\ test490 test491 test492 test493 test494 test495 test496 \ \ test500 test501 test502 test503 test504 test505 test506 test507 test508 \ diff --git a/tests/data/test478 b/tests/data/test478 new file mode 100644 index 00000000..2e97ca7b --- /dev/null +++ b/tests/data/test478 @@ -0,0 +1,73 @@ + + + +netrc +HTTP + + +# +# Server-side + + +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- + + + +# +# Client-side + + +http + + +proxy + + +.netrc with multiple accounts for same host + + +--netrc --netrc-file log/netrc%TESTNUMBER -x http://%HOSTIP:%HTTPPORT/ http://debbie@github.com/ + + + +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 + + + + + + +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 + + + + diff --git a/tests/data/test479 b/tests/data/test479 new file mode 100644 index 00000000..55eba8f0 --- /dev/null +++ b/tests/data/test479 @@ -0,0 +1,107 @@ + + + +netrc +HTTP + + +# +# Server-side + + +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- + + + +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 + + + +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 + + + +# +# Client-side + + +http + + +proxy + + +.netrc with redirect and default without password + + +--netrc --netrc-file log/netrc%TESTNUMBER -L -x http://%HOSTIP:%HTTPPORT/ http://a.com/ + + + +machine a.com + login alice + password alicespassword + +default + login bob + + + + + + +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 + + + + diff --git a/tests/data/test480 b/tests/data/test480 new file mode 100644 index 00000000..f097f819 --- /dev/null +++ b/tests/data/test480 @@ -0,0 +1,38 @@ + + + +netrc +pop3 + + +# +# Server-side + + + + +# +# Client-side + + +pop3 + + +Reject .netrc with credentials using CRLF for POP3 + + +--netrc --netrc-file log/netrc%TESTNUMBER pop3://%HOSTIP:%POP3PORT/%TESTNUMBER + + +machine %HOSTIP + login alice + password "password\r\ncommand" + + + + + +26 + + + diff --git a/tests/unit/unit1304.c b/tests/unit/unit1304.c index 2e7e97a7..4c7a9b8a 100644 --- a/tests/unit/unit1304.c +++ b/tests/unit/unit1304.c @@ -31,13 +31,8 @@ static char filename[64]; 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; } @@ -59,80 +54,52 @@ UNITTEST_START result = Curl_parsenetrc("test.example.com", &login, &password, filename); 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!"); /* * Test a non existent login in our netrc file. */ - free(login); - login = strdup("me"); - abort_unless(login != NULL, "returned NULL!"); + login = (char *)"me"; result = Curl_parsenetrc("example.com", &login, &password, filename); 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!"); /* * 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"; result = Curl_parsenetrc("test.example.com", &login, &password, filename); 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!"); /* * 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"; result = Curl_parsenetrc("example.com", &login, &password, filename); 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!"); /* * 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"; result = Curl_parsenetrc("example.com", &login, &password, filename); 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!"); /* * 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; result = Curl_parsenetrc("example.com", &login, &password, filename); fail_unless(result == 0, "Host should have been found"); @@ -147,8 +114,9 @@ UNITTEST_START * with login[0] != 0. */ free(password); - password = strdup(""); - abort_unless(password != NULL, "returned NULL!"); + free(login); + password = NULL; + login = NULL; result = Curl_parsenetrc("example.com", &login, &password, filename); fail_unless(result == 0, "Host should have been found"); abort_unless(password != NULL, "returned NULL!"); @@ -162,11 +130,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; result = Curl_parsenetrc("curl.example.com", &login, &password, filename); fail_unless(result == 0, "Host should have been found"); @@ -181,8 +147,9 @@ UNITTEST_START * with login[0] != 0. */ free(password); - password = strdup(""); - abort_unless(password != NULL, "returned NULL!"); + free(login); + password = NULL; + login = NULL; result = Curl_parsenetrc("curl.example.com", &login, &password, filename); fail_unless(result == 0, "Host should have been found"); -- 2.30.2