Jonathan Nieder wrote: > The series is pretty much as before. Changes since the rough draft:
Thanks again for your help. Here's a new version that uses aprintf (thanks!) and uses Curl_safefree whenever they add a new free() of a pointer that is not assigned some other value on the next line. The patches are also attached here for easy reference. Jonathan Nieder (7): sasl: allow arbitrarily long username and password url: use goto in create_conn() for exception handling url: allocate username, password, and options on the heap netrc: handle longer username and password Curl_setopt: handle arbitrary-length username and password url: handle exceptional cases first in parse_url_login() url: handle arbitrary-length username and password before '@' lib/curl_sasl.c | 25 +++-- lib/netrc.c | 20 ++-- lib/netrc.h | 16 ++-- lib/url.c | 251 ++++++++++++++++++++++++++++---------------------- lib/urldata.h | 1 - tests/unit/unit1304.c | 53 ++++++----- 6 files changed, 205 insertions(+), 161 deletions(-)
From: Jonathan Nieder <[email protected]> Date: Mon, 19 Aug 2013 00:36:53 -0700 Subject: [PATCH 1/7] sasl: allow arbitrarily long username and password Use appropriately sized buffers on the heap instead of fixed-size buffers on the stack, to allow for longer usernames and passwords. Callers never pass anything longer than MAX_CURL_USER_LENGTH (resp. MAX_CURL_PASSWORD_LENGTH), so no functional change inteded yet. --- lib/curl_sasl.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/lib/curl_sasl.c b/lib/curl_sasl.c index fcb00194..924be4bb 100644 --- a/lib/curl_sasl.c +++ b/lib/curl_sasl.c @@ -94,18 +94,18 @@ CURLcode Curl_sasl_create_plain_message(struct SessionHandle *data, const char *passwdp, char **outptr, size_t *outlen) { - char plainauth[2 * MAX_CURL_USER_LENGTH + MAX_CURL_PASSWORD_LENGTH]; + CURLcode result; + char *plainauth; size_t ulen; size_t plen; ulen = strlen(userp); plen = strlen(passwdp); - if(2 * ulen + plen + 2 > sizeof(plainauth)) { + plainauth = malloc(2 * ulen + plen + 2); + if(!plainauth) { *outlen = 0; *outptr = NULL; - - /* Plainauth too small */ return CURLE_OUT_OF_MEMORY; } @@ -117,8 +117,10 @@ CURLcode Curl_sasl_create_plain_message(struct SessionHandle *data, memcpy(plainauth + 2 * ulen + 2, passwdp, plen); /* Base64 encode the reply */ - return Curl_base64_encode(data, plainauth, 2 * ulen + plen + 2, outptr, - outlen); + result = Curl_base64_encode(data, plainauth, 2 * ulen + plen + 2, outptr, + outlen); + Curl_safefree(plainauth); + return result; } /* @@ -190,7 +192,7 @@ CURLcode Curl_sasl_create_cram_md5_message(struct SessionHandle *data, size_t chlglen = 0; HMAC_context *ctxt; unsigned char digest[MD5_DIGEST_LEN]; - char response[MAX_CURL_USER_LENGTH + 2 * MD5_DIGEST_LEN + 1]; + char *response; /* Decode the challenge if necessary */ if(chlg64len && *chlg64 != '=') { @@ -220,14 +222,19 @@ CURLcode Curl_sasl_create_cram_md5_message(struct SessionHandle *data, Curl_HMAC_final(ctxt, digest); /* Prepare the response */ - snprintf(response, sizeof(response), + response = aprintf( "%s %02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x", userp, digest[0], digest[1], digest[2], digest[3], digest[4], digest[5], digest[6], digest[7], digest[8], digest[9], digest[10], digest[11], digest[12], digest[13], digest[14], digest[15]); + if(!response) + return CURLE_OUT_OF_MEMORY; /* Base64 encode the reply */ - return Curl_base64_encode(data, response, 0, outptr, outlen); + result = Curl_base64_encode(data, response, 0, outptr, outlen); + + Curl_safefree(response); + return result; } /* -- 1.8.4.rc4
From: Jonathan Nieder <[email protected]> Date: Mon, 19 Aug 2013 00:38:08 -0700 Subject: [PATCH 2/7] url: use goto in create_conn() for exception handling Instead of remembering before each "return" statement which temporary allocations, if any, need to be freed, take care to set pointers to NULL when no longer needed and use a goto to a common block to exit the function and free all temporaries. No functional change intended. Currently the only temporary buffer in this function is "proxy" which is already correctly freed when appropriate, but there will be more soon. --- lib/url.c | 67 ++++++++++++++++++++++++++++++++++++++------------------------- 1 file changed, 40 insertions(+), 27 deletions(-) diff --git a/lib/url.c b/lib/url.c index 878f3873..64d5add6 100644 --- a/lib/url.c +++ b/lib/url.c @@ -5063,8 +5063,10 @@ static CURLcode create_conn(struct SessionHandle *data, * Check input data *************************************************************/ - if(!data->change.url) - return CURLE_URL_MALFORMAT; + if(!data->change.url) { + result = CURLE_URL_MALFORMAT; + goto out; + } /* First, split up the current URL in parts so that we can use the parts for checking against the already present connections. In order @@ -5072,8 +5074,10 @@ static CURLcode create_conn(struct SessionHandle *data, connection data struct and fill in for comparison purposes. */ conn = allocate_conn(data); - if(!conn) - return CURLE_OUT_OF_MEMORY; + if(!conn) { + result = CURLE_OUT_OF_MEMORY; + goto out; + } /* We must set the return variable as soon as possible, so that our parent can cleanup any possible allocs we may have done before @@ -5103,15 +5107,18 @@ static CURLcode create_conn(struct SessionHandle *data, data->state.path = NULL; data->state.pathbuffer = malloc(urllen+2); - if(NULL == data->state.pathbuffer) - return CURLE_OUT_OF_MEMORY; /* really bad error */ + if(NULL == data->state.pathbuffer) { + result = CURLE_OUT_OF_MEMORY; /* really bad error */ + goto out; + } data->state.path = data->state.pathbuffer; conn->host.rawalloc = malloc(urllen+2); if(NULL == conn->host.rawalloc) { Curl_safefree(data->state.pathbuffer); data->state.path = NULL; - return CURLE_OUT_OF_MEMORY; + result = CURLE_OUT_OF_MEMORY; + goto out; } conn->host.name = conn->host.rawalloc; @@ -5120,7 +5127,7 @@ static CURLcode create_conn(struct SessionHandle *data, result = parseurlandfillconn(data, conn, &prot_missing, user, passwd, options); if(result != CURLE_OK) - return result; + goto out; /************************************************************* * No protocol part in URL was used, add it! @@ -5134,8 +5141,8 @@ static CURLcode create_conn(struct SessionHandle *data, reurl = aprintf("%s://%s", conn->handler->scheme, data->change.url); if(!reurl) { - Curl_safefree(proxy); - return CURLE_OUT_OF_MEMORY; + result = CURLE_OUT_OF_MEMORY; + goto out; } if(data->change.url_alloc) { @@ -5172,7 +5179,7 @@ static CURLcode create_conn(struct SessionHandle *data, if(conn->bits.proxy_user_passwd) { result = parse_proxy_auth(data, conn); if(result != CURLE_OK) - return result; + goto out; } /************************************************************* @@ -5183,7 +5190,8 @@ static CURLcode create_conn(struct SessionHandle *data, /* if global proxy is set, this is it */ if(NULL == proxy) { failf(data, "memory shortage"); - return CURLE_OUT_OF_MEMORY; + result = CURLE_OUT_OF_MEMORY; + goto out; } } @@ -5211,16 +5219,17 @@ static CURLcode create_conn(struct SessionHandle *data, if(proxy) { result = parse_proxy(data, conn, proxy); - free(proxy); /* parse_proxy copies the proxy string */ + Curl_safefree(proxy); /* parse_proxy copies the proxy string */ if(result) - return result; + goto out; if((conn->proxytype == CURLPROXY_HTTP) || (conn->proxytype == CURLPROXY_HTTP_1_0)) { #ifdef CURL_DISABLE_HTTP /* asking for a HTTP proxy is a bit funny when HTTP is disabled... */ - return CURLE_UNSUPPORTED_PROTOCOL; + result = CURLE_UNSUPPORTED_PROTOCOL; + goto out; #else /* force this connection's protocol to become HTTP if not already compatible - if it isn't tunneling through */ @@ -5257,24 +5266,22 @@ static CURLcode create_conn(struct SessionHandle *data, *************************************************************/ result = parse_remote_port(data, conn); if(result != CURLE_OK) - return result; + goto out; /* Check for overridden login details and set them accordingly so they they are known when protocol->setup_connection is called! */ override_login(data, conn, user, passwd, options); result = set_login(conn, user, passwd, options); if(result != CURLE_OK) - return result; + goto out; /************************************************************* * Setup internals depending on protocol. Needs to be done after * we figured out what/if proxy to use. *************************************************************/ result = setup_connection_internals(conn); - if(result != CURLE_OK) { - Curl_safefree(proxy); - return result; - } + if(result != CURLE_OK) + goto out; conn->recv[FIRSTSOCKET] = Curl_recv_plain; conn->send[FIRSTSOCKET] = Curl_send_plain; @@ -5307,7 +5314,7 @@ static CURLcode create_conn(struct SessionHandle *data, DEBUGASSERT(conn->handler->done); /* we ignore the return code for the protocol-specific DONE */ (void)conn->handler->done(conn, result, FALSE); - return result; + goto out; } Curl_setup_transfer(conn, -1, -1, FALSE, NULL, /* no download */ @@ -5317,7 +5324,7 @@ static CURLcode create_conn(struct SessionHandle *data, /* since we skip do_init() */ do_init(conn); - return result; + goto out; } #endif @@ -5342,8 +5349,10 @@ static CURLcode create_conn(struct SessionHandle *data, data->set.ssl.password = data->set.str[STRING_TLSAUTH_PASSWORD]; #endif - if(!Curl_clone_ssl_config(&data->set.ssl, &conn->ssl_config)) - return CURLE_OUT_OF_MEMORY; + if(!Curl_clone_ssl_config(&data->set.ssl, &conn->ssl_config)) { + result = CURLE_OUT_OF_MEMORY; + goto out; + } /************************************************************* * Check the current list of connections to see if we can @@ -5446,7 +5455,8 @@ static CURLcode create_conn(struct SessionHandle *data, conn_free(conn); *in_connect = NULL; - return CURLE_NO_CONNECTION_AVAILABLE; + result = CURLE_NO_CONNECTION_AVAILABLE; + goto out; } else { /* @@ -5468,7 +5478,7 @@ static CURLcode create_conn(struct SessionHandle *data, */ result = setup_range(data); if(result) - return result; + goto out; /* Continue connectdata initialization here. */ @@ -5486,6 +5496,9 @@ static CURLcode create_conn(struct SessionHandle *data, *************************************************************/ result = resolve_server(data, conn, async); + out: + + Curl_safefree(proxy); return result; } -- 1.8.4.rc4
From: Jonathan Nieder <[email protected]> Date: Mon, 19 Aug 2013 00:39:05 -0700 Subject: [PATCH 3/7] url: allocate username, password, and options on the heap This makes it possible to increase the size of the buffers when needed in later patches. No functional change yet. --- lib/url.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/url.c b/lib/url.c index 64d5add6..07555a90 100644 --- a/lib/url.c +++ b/lib/url.c @@ -5046,9 +5046,9 @@ static CURLcode create_conn(struct SessionHandle *data, struct connectdata *conn; struct connectdata *conn_temp = NULL; size_t urllen; - char user[MAX_CURL_USER_LENGTH]; - char passwd[MAX_CURL_PASSWORD_LENGTH]; - char options[MAX_CURL_OPTIONS_LENGTH]; + char *user = NULL; + char *passwd = NULL; + char *options = NULL; bool reuse; char *proxy = NULL; bool prot_missing = FALSE; @@ -5124,6 +5124,14 @@ static CURLcode create_conn(struct SessionHandle *data, conn->host.name = conn->host.rawalloc; conn->host.name[0] = 0; + user = malloc(MAX_CURL_USER_LENGTH); + passwd = malloc(MAX_CURL_PASSWORD_LENGTH); + options = malloc(MAX_CURL_OPTIONS_LENGTH); + if(!user || !passwd || !options) { + result = CURLE_OUT_OF_MEMORY; + goto out; + } + result = parseurlandfillconn(data, conn, &prot_missing, user, passwd, options); if(result != CURLE_OK) @@ -5498,6 +5506,9 @@ static CURLcode create_conn(struct SessionHandle *data, out: + Curl_safefree(options); + Curl_safefree(passwd); + Curl_safefree(user); Curl_safefree(proxy); return result; } -- 1.8.4.rc4
From: Jonathan Nieder <[email protected]> Date: Mon, 19 Aug 2013 00:48:24 -0700 Subject: [PATCH 4/7] netrc: handle longer username and password libcurl truncates usernames and passwords it reads from .netrc to LOGINSIZE and PASSWORDSIZE (64) characters without any indication to the user, to ensure the values returned from Curl_parsenetrc fit in a caller-provided buffer. Fix the interface by passing back dynamically allocated buffers allocated to fit the user's input. The parser still relies on a 256-character buffer to read each line, though. So now you can include an ~246-character password in your .netrc, instead of the previous limit of 63 characters. Reported-by: Colby Ranger --- lib/netrc.c | 20 ++++++++++++------- lib/netrc.h | 16 ++++++---------- lib/url.c | 18 ++++++++--------- tests/unit/unit1304.c | 53 ++++++++++++++++++++++++++++++--------------------- 4 files changed, 59 insertions(+), 48 deletions(-) diff --git a/lib/netrc.c b/lib/netrc.c index 2c5942af..f51fdf34 100644 --- a/lib/netrc.c +++ b/lib/netrc.c @@ -52,13 +52,13 @@ enum host_lookup_state { * @unittest: 1304 */ int Curl_parsenetrc(const char *host, - char *login, - char *password, + char **loginp, + char **passwordp, char *netrcfile) { FILE *file; int retcode=1; - int specific_login = (login[0] != 0); + int specific_login = (**loginp != 0); char *home = NULL; bool home_alloc = FALSE; bool netrc_alloc = FALSE; @@ -109,7 +109,7 @@ int Curl_parsenetrc(const char *host, tok=strtok_r(netrcbuffer, " \t\n", &tok_buf); while(!done && tok) { - if(login[0] && password[0]) { + if(**loginp && **passwordp) { done=TRUE; break; } @@ -138,16 +138,22 @@ int Curl_parsenetrc(const char *host, /* we are now parsing sub-keywords concerning "our" host */ if(state_login) { if(specific_login) { - state_our_login = Curl_raw_equal(login, tok); + state_our_login = Curl_raw_equal(*loginp, tok); } else { - strncpy(login, tok, LOGINSIZE-1); + free(*loginp); + *loginp = strdup(tok); + if(!*loginp) + return -1; /* allocation failed */ } state_login=0; } else if(state_password) { if(state_our_login || !specific_login) { - strncpy(password, tok, PASSWORDSIZE-1); + free(*passwordp); + *passwordp = strdup(tok); + if(!*passwordp) + return -1; /* allocation failed */ } state_password=0; } diff --git a/lib/netrc.h b/lib/netrc.h index 4db764df..a1456011 100644 --- a/lib/netrc.h +++ b/lib/netrc.h @@ -22,19 +22,15 @@ * ***************************************************************************/ -/* Make sure we have room for at least this size: */ -#define LOGINSIZE 64 -#define PASSWORDSIZE 64 - /* returns -1 on failure, 0 if the host is found, 1 is the host isn't found */ int Curl_parsenetrc(const char *host, - char *login, - char *password, + char **loginp, + char **passwordp, char *filename); - /* Assume: password[0]=0, host[0] != 0. - * If login[0] = 0, search for login and password within a machine section - * in the netrc. - * If login[0] != 0, search for password within machine and login. + /* Assume: (*passwordp)[0]=0, host[0] != 0. + * If (*loginp)[0] = 0, search for login and password within a machine + * section in the netrc. + * If (*loginp)[0] != 0, search for password within machine and login. */ #endif /* HEADER_CURL_NETRC_H */ diff --git a/lib/url.c b/lib/url.c index 07555a90..8b628582 100644 --- a/lib/url.c +++ b/lib/url.c @@ -4795,27 +4795,27 @@ static CURLcode parse_remote_port(struct SessionHandle *data, */ static void override_login(struct SessionHandle *data, struct connectdata *conn, - char *user, char *passwd, char *options) + char **userp, char **passwdp, char **optionsp) { if(data->set.str[STRING_USERNAME]) { - strncpy(user, data->set.str[STRING_USERNAME], MAX_CURL_USER_LENGTH); - user[MAX_CURL_USER_LENGTH - 1] = '\0'; /* To be on safe side */ + strncpy(*userp, data->set.str[STRING_USERNAME], MAX_CURL_USER_LENGTH); + (*userp)[MAX_CURL_USER_LENGTH - 1] = '\0'; /* To be on safe side */ } if(data->set.str[STRING_PASSWORD]) { - strncpy(passwd, data->set.str[STRING_PASSWORD], MAX_CURL_PASSWORD_LENGTH); - passwd[MAX_CURL_PASSWORD_LENGTH - 1] = '\0'; /* To be on safe side */ + strncpy(*passwdp, data->set.str[STRING_PASSWORD], MAX_CURL_PASSWORD_LENGTH); + (*passwdp)[MAX_CURL_PASSWORD_LENGTH - 1] = '\0'; /* To be on safe side */ } if(data->set.str[STRING_OPTIONS]) { - strncpy(options, data->set.str[STRING_OPTIONS], MAX_CURL_OPTIONS_LENGTH); - options[MAX_CURL_OPTIONS_LENGTH - 1] = '\0'; /* To be on safe side */ + strncpy(*optionsp, data->set.str[STRING_OPTIONS], MAX_CURL_OPTIONS_LENGTH); + (*optionsp)[MAX_CURL_OPTIONS_LENGTH - 1] = '\0'; /* To be on safe side */ } conn->bits.netrc = FALSE; if(data->set.use_netrc != CURL_NETRC_IGNORED) { if(Curl_parsenetrc(conn->host.name, - user, passwd, + userp, passwdp, data->set.str[STRING_NETRC_FILE])) { infof(data, "Couldn't find host %s in the " DOT_CHAR "netrc file; using defaults\n", @@ -5278,7 +5278,7 @@ static CURLcode create_conn(struct SessionHandle *data, /* Check for overridden login details and set them accordingly so they they are known when protocol->setup_connection is called! */ - override_login(data, conn, user, passwd, options); + override_login(data, conn, &user, &passwd, &options); result = set_login(conn, user, passwd, options); if(result != CURLE_OK) goto out; diff --git a/tests/unit/unit1304.c b/tests/unit/unit1304.c index 8ddd8cae..9242e800 100644 --- a/tests/unit/unit1304.c +++ b/tests/unit/unit1304.c @@ -23,14 +23,14 @@ #include "netrc.h" -static char login[LOGINSIZE]; -static char password[PASSWORDSIZE]; +static char *login; +static char *password; static char filename[64]; static CURLcode unit_setup(void) { - password[0] = 0; - login[0] = 0; + password = strdup(""); + login = strdup(""); return CURLE_OK; } @@ -47,7 +47,7 @@ UNITTEST_START /* * Test a non existent host in our netrc file. */ - result = Curl_parsenetrc("test.example.com", login, password, filename); + result = Curl_parsenetrc("test.example.com", &login, &password, filename); fail_unless(result == 1, "Host not found should return 1"); fail_unless(password[0] == 0, "password should not have been changed"); fail_unless(login[0] == 0, "login should not have been changed"); @@ -55,8 +55,9 @@ UNITTEST_START /* * Test a non existent login in our netrc file. */ - memcpy(login, "me", 2); - result = Curl_parsenetrc("example.com", login, password, filename); + free(login); + login = strdup("me"); + result = Curl_parsenetrc("example.com", &login, &password, filename); fail_unless(result == 0, "Host should be found"); fail_unless(password[0] == 0, "password should not have been changed"); fail_unless(strncmp(login, "me", 2) == 0, "login should not have been changed"); @@ -64,8 +65,9 @@ UNITTEST_START /* * Test a non existent login and host in our netrc file. */ - memcpy(login, "me", 2); - result = Curl_parsenetrc("test.example.com", login, password, filename); + free(login); + login = strdup("me"); + result = Curl_parsenetrc("test.example.com", &login, &password, filename); fail_unless(result == 1, "Host should be found"); fail_unless(password[0] == 0, "password should not have been changed"); fail_unless(strncmp(login, "me", 2) == 0, "login should not have been changed"); @@ -74,8 +76,9 @@ UNITTEST_START * Test a non existent login (substring of an existing one) in our * netrc file. */ - memcpy(login, "admi", 4); - result = Curl_parsenetrc("example.com", login, password, filename); + free(login); + login = strdup("admi"); + result = Curl_parsenetrc("example.com", &login, &password, filename); fail_unless(result == 0, "Host should be found"); fail_unless(password[0] == 0, "password should not have been changed"); fail_unless(strncmp(login, "admi", 4) == 0, "login should not have been changed"); @@ -84,8 +87,9 @@ UNITTEST_START * Test a non existent login (superstring of an existing one) * in our netrc file. */ - memcpy(login, "adminn", 6); - result = Curl_parsenetrc("example.com", login, password, filename); + free(login); + login = strdup("adminn"); + result = Curl_parsenetrc("example.com", &login, &password, filename); fail_unless(result == 0, "Host should be found"); fail_unless(password[0] == 0, "password should not have been changed"); fail_unless(strncmp(login, "adminn", 6) == 0, "login should not have been changed"); @@ -94,8 +98,9 @@ UNITTEST_START * Test for the first existing host in our netrc file * with login[0] = 0. */ - login[0] = 0; - result = Curl_parsenetrc("example.com", login, password, filename); + free(login); + login = strdup(""); + result = Curl_parsenetrc("example.com", &login, &password, filename); fail_unless(result == 0, "Host should have been found"); fail_unless(strncmp(password, "passwd", 6) == 0, "password should be 'passwd'"); @@ -105,8 +110,9 @@ UNITTEST_START * Test for the first existing host in our netrc file * with login[0] != 0. */ - password[0] = 0; - result = Curl_parsenetrc("example.com", login, password, filename); + free(password); + password = strdup(""); + result = Curl_parsenetrc("example.com", &login, &password, filename); fail_unless(result == 0, "Host should have been found"); fail_unless(strncmp(password, "passwd", 6) == 0, "password should be 'passwd'"); @@ -116,9 +122,11 @@ UNITTEST_START * Test for the second existing host in our netrc file * with login[0] = 0. */ - password[0] = 0; - login[0] = 0; - result = Curl_parsenetrc("curl.example.com", login, password, filename); + free(password); + password = strdup(""); + free(login); + login = strdup(""); + result = Curl_parsenetrc("curl.example.com", &login, &password, filename); fail_unless(result == 0, "Host should have been found"); fail_unless(strncmp(password, "none", 4) == 0, "password should be 'none'"); @@ -128,8 +136,9 @@ UNITTEST_START * Test for the second existing host in our netrc file * with login[0] != 0. */ - password[0] = 0; - result = Curl_parsenetrc("curl.example.com", login, password, filename); + free(password); + password = strdup(""); + result = Curl_parsenetrc("curl.example.com", &login, &password, filename); fail_unless(result == 0, "Host should have been found"); fail_unless(strncmp(password, "none", 4) == 0, "password should be 'none'"); -- 1.8.4.rc4
From: Jonathan Nieder <[email protected]> Date: Mon, 19 Aug 2013 00:57:54 -0700 Subject: [PATCH 5/7] Curl_setopt: handle arbitrary-length username and password libcurl truncates usernames, passwords, and options set with curl_easy_setopt to 255 (= MAX_CURL_PASSWORD_LENGTH - 1) characters. This doesn't affect the return value from curl_easy_setopt(), so from the caller's point of view, there is no sign anything strange has happened, except that authentication fails. For example: # Prepare a long (300-char) password. s=0123456789; s=$s$s$s$s$s$s$s$s$s$s; s=$s$s$s; # Start a server. nc -l -p 8888 | tee out & pid=$! # Tell curl to pass the password to the server. curl --user me:$s http://localhost:8888 & sleep 1; kill $pid # Extract the password. userpass=$( awk '/Authorization: Basic/ {print $3}' <out | tr -d '\r' | base64 -d ) password=${userpass#me:} echo ${#password} Expected result: 300 Actual result: 255 The fix is simple: allocate appropriately sized buffers on the heap instead of trying to squeeze the provided values into fixed-size on-stack buffers. Bug: http://bugs.debian.org/719856 Reported-by: Colby Ranger --- lib/url.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/lib/url.c b/lib/url.c index 8b628582..73bee68b 100644 --- a/lib/url.c +++ b/lib/url.c @@ -4793,23 +4793,29 @@ static CURLcode parse_remote_port(struct SessionHandle *data, * Override the login details from the URL with that in the CURLOPT_USERPWD * option or a .netrc file, if applicable. */ -static void override_login(struct SessionHandle *data, - struct connectdata *conn, - char **userp, char **passwdp, char **optionsp) +static int override_login(struct SessionHandle *data, + struct connectdata *conn, + char **userp, char **passwdp, char **optionsp) { if(data->set.str[STRING_USERNAME]) { - strncpy(*userp, data->set.str[STRING_USERNAME], MAX_CURL_USER_LENGTH); - (*userp)[MAX_CURL_USER_LENGTH - 1] = '\0'; /* To be on safe side */ + free(*userp); + *userp = strdup(data->set.str[STRING_USERNAME]); + if(!*userp) + return CURLE_OUT_OF_MEMORY; } if(data->set.str[STRING_PASSWORD]) { - strncpy(*passwdp, data->set.str[STRING_PASSWORD], MAX_CURL_PASSWORD_LENGTH); - (*passwdp)[MAX_CURL_PASSWORD_LENGTH - 1] = '\0'; /* To be on safe side */ + free(*passwdp); + *passwdp = strdup(data->set.str[STRING_PASSWORD]); + if(!*passwdp) + return CURLE_OUT_OF_MEMORY; } if(data->set.str[STRING_OPTIONS]) { - strncpy(*optionsp, data->set.str[STRING_OPTIONS], MAX_CURL_OPTIONS_LENGTH); - (*optionsp)[MAX_CURL_OPTIONS_LENGTH - 1] = '\0'; /* To be on safe side */ + free(*optionsp); + *optionsp = strdup(data->set.str[STRING_OPTIONS]); + if(!*optionsp) + return CURLE_OUT_OF_MEMORY; } conn->bits.netrc = FALSE; @@ -4830,6 +4836,7 @@ static void override_login(struct SessionHandle *data, conn->bits.user_passwd = TRUE; /* enable user+password */ } } + return CURLE_OK; } /* @@ -5278,7 +5285,9 @@ static CURLcode create_conn(struct SessionHandle *data, /* Check for overridden login details and set them accordingly so they they are known when protocol->setup_connection is called! */ - override_login(data, conn, &user, &passwd, &options); + result = override_login(data, conn, &user, &passwd, &options); + if(result != CURLE_OK) + goto out; result = set_login(conn, user, passwd, options); if(result != CURLE_OK) goto out; -- 1.8.4.rc4
From: Jonathan Nieder <[email protected]> Date: Mon, 19 Aug 2013 01:01:26 -0700 Subject: [PATCH 6/7] url: handle exceptional cases first in parse_url_login() Instead of nesting "if(success)" blocks and leaving the reader in suspense about what happens in the !success case, deal with failure cases early, usually with a simple goto to clean up and return from the function. No functional change intended. The main effect is to decrease the indentation of this function slightly. --- lib/url.c | 145 ++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 71 insertions(+), 74 deletions(-) diff --git a/lib/url.c b/lib/url.c index 73bee68b..5f21b4c2 100644 --- a/lib/url.c +++ b/lib/url.c @@ -4460,86 +4460,83 @@ static CURLcode parse_url_login(struct SessionHandle *data, passwd[0] = 0; options[0] = 0; + if(!ptr) + goto out; + /* We will now try to extract the * possible login information in a string like: * ftp://user:[email protected]:8021/README */ - if(ptr) { - /* There's login information to the left of the @ */ - - conn->host.name = ++ptr; - - /* So the hostname is sane. Only bother interpreting the - * results if we could care. It could still be wasted - * work because it might be overtaken by the programmatically - * set user/passwd, but doing that first adds more cases here :-( - */ - - if(data->set.use_netrc != CURL_NETRC_REQUIRED) { - /* We could use the login information in the URL so extract it */ - result = parse_login_details(login, ptr - login - 1, - &userp, &passwdp, &optionsp); - if(!result) { - if(userp) { - char *newname; - - /* We have a user in the URL */ - conn->bits.userpwd_in_url = TRUE; - conn->bits.user_passwd = TRUE; /* enable user+password */ - - /* Decode the user */ - newname = curl_easy_unescape(data, userp, 0, NULL); - if(!newname) { - Curl_safefree(userp); - Curl_safefree(passwdp); - Curl_safefree(optionsp); - return CURLE_OUT_OF_MEMORY; - } - - if(strlen(newname) < MAX_CURL_USER_LENGTH) - strcpy(user, newname); - - free(newname); - } - - if(passwdp) { - /* We have a password in the URL so decode it */ - char *newpasswd = curl_easy_unescape(data, passwdp, 0, NULL); - if(!newpasswd) { - Curl_safefree(userp); - Curl_safefree(passwdp); - Curl_safefree(optionsp); - return CURLE_OUT_OF_MEMORY; - } - - if(strlen(newpasswd) < MAX_CURL_PASSWORD_LENGTH) - strcpy(passwd, newpasswd); - - free(newpasswd); - } - - if(optionsp) { - /* We have an options list in the URL so decode it */ - char *newoptions = curl_easy_unescape(data, optionsp, 0, NULL); - if(!newoptions) { - Curl_safefree(userp); - Curl_safefree(passwdp); - Curl_safefree(optionsp); - return CURLE_OUT_OF_MEMORY; - } - - if(strlen(newoptions) < MAX_CURL_OPTIONS_LENGTH) - strcpy(options, newoptions); - - free(newoptions); - } - } - - Curl_safefree(userp); - Curl_safefree(passwdp); - Curl_safefree(optionsp); + conn->host.name = ++ptr; + + /* So the hostname is sane. Only bother interpreting the + * results if we could care. It could still be wasted + * work because it might be overtaken by the programmatically + * set user/passwd, but doing that first adds more cases here :-( + */ + + if(data->set.use_netrc == CURL_NETRC_REQUIRED) + goto out; + + /* We could use the login information in the URL so extract it */ + result = parse_login_details(login, ptr - login - 1, + &userp, &passwdp, &optionsp); + if(result != CURLE_OK) + goto out; + + if(userp) { + char *newname; + + /* We have a user in the URL */ + conn->bits.userpwd_in_url = TRUE; + conn->bits.user_passwd = TRUE; /* enable user+password */ + + /* Decode the user */ + newname = curl_easy_unescape(data, userp, 0, NULL); + if(!newname) { + result = CURLE_OUT_OF_MEMORY; + goto out; + } + + if(strlen(newname) < MAX_CURL_USER_LENGTH) + strcpy(user, newname); + + free(newname); + } + + if(passwdp) { + /* We have a password in the URL so decode it */ + char *newpasswd = curl_easy_unescape(data, passwdp, 0, NULL); + if(!newpasswd) { + result = CURLE_OUT_OF_MEMORY; + goto out; + } + + if(strlen(newpasswd) < MAX_CURL_PASSWORD_LENGTH) + strcpy(passwd, newpasswd); + + free(newpasswd); + } + + if(optionsp) { + /* We have an options list in the URL so decode it */ + char *newoptions = curl_easy_unescape(data, optionsp, 0, NULL); + if(!newoptions) { + result = CURLE_OUT_OF_MEMORY; + goto out; } + + if(strlen(newoptions) < MAX_CURL_OPTIONS_LENGTH) + strcpy(options, newoptions); + + free(newoptions); } + out: + + Curl_safefree(userp); + Curl_safefree(passwdp); + Curl_safefree(optionsp); + return result; } -- 1.8.4.rc4
From: Jonathan Nieder <[email protected]> Date: Mon, 19 Aug 2013 01:36:46 -0700 Subject: [PATCH 7/7] url: handle arbitrary-length username and password before '@' libcurl quietly truncates usernames, passwords, and options from before an '@' sign in a URL to 255 (= MAX_CURL_PASSWORD_LENGTH - 1) characters to fit in fixed-size buffers on the stack. Allocate a buffer large enough to fit the parsed fields on the fly instead to support longer passwords. After this change, there are no more uses of MAX_CURL_OPTIONS_LENGTH left, so stop defining that constant while at it. The hardcoded max username and password length constants, on the other hand, are still used in HTTP proxy credential handling (which this patch doesn't touch). Reported-by: Colby Ranger --- lib/url.c | 45 +++++++++++++++++++++------------------------ lib/urldata.h | 1 - 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/lib/url.c b/lib/url.c index 5f21b4c2..657681dd 100644 --- a/lib/url.c +++ b/lib/url.c @@ -144,7 +144,8 @@ static void signalPipeClose(struct curl_llist *pipeline, bool pipe_broke); static CURLcode do_init(struct connectdata *conn); static CURLcode parse_url_login(struct SessionHandle *data, struct connectdata *conn, - char *user, char *passwd, char *options); + char **userptr, char **passwdptr, + char **optionsptr); static CURLcode parse_login_details(const char *login, const size_t len, char **userptr, char **passwdptr, char **optionsptr); @@ -3687,7 +3688,8 @@ static CURLcode findprotocol(struct SessionHandle *data, static CURLcode parseurlandfillconn(struct SessionHandle *data, struct connectdata *conn, bool *prot_missing, - char *user, char *passwd, char *options) + char **userp, char **passwdp, + char **optionsp) { char *at; char *fragment; @@ -3931,7 +3933,7 @@ static CURLcode parseurlandfillconn(struct SessionHandle *data, * Parse the login details from the URL and strip them out of * the host name */ - result = parse_url_login(data, conn, user, passwd, options); + result = parse_url_login(data, conn, userp, passwdp, optionsp); if(result != CURLE_OK) return result; @@ -4439,7 +4441,7 @@ static CURLcode parse_proxy_auth(struct SessionHandle *data, */ static CURLcode parse_url_login(struct SessionHandle *data, struct connectdata *conn, - char *user, char *passwd, char *options) + char **user, char **passwd, char **options) { CURLcode result = CURLE_OK; char *userp = NULL; @@ -4456,9 +4458,9 @@ static CURLcode parse_url_login(struct SessionHandle *data, char *ptr = strchr(conn->host.name, '@'); char *login = conn->host.name; - user[0] = 0; /* to make everything well-defined */ - passwd[0] = 0; - options[0] = 0; + DEBUGASSERT(!**user); + DEBUGASSERT(!**passwd); + DEBUGASSERT(!**options); if(!ptr) goto out; @@ -4497,10 +4499,8 @@ static CURLcode parse_url_login(struct SessionHandle *data, goto out; } - if(strlen(newname) < MAX_CURL_USER_LENGTH) - strcpy(user, newname); - - free(newname); + free(*user); + *user = newname; } if(passwdp) { @@ -4511,10 +4511,8 @@ static CURLcode parse_url_login(struct SessionHandle *data, goto out; } - if(strlen(newpasswd) < MAX_CURL_PASSWORD_LENGTH) - strcpy(passwd, newpasswd); - - free(newpasswd); + free(*passwd); + *passwd = newpasswd; } if(optionsp) { @@ -4525,12 +4523,11 @@ static CURLcode parse_url_login(struct SessionHandle *data, goto out; } - if(strlen(newoptions) < MAX_CURL_OPTIONS_LENGTH) - strcpy(options, newoptions); - - free(newoptions); + free(*options); + *options = newoptions; } + out: Curl_safefree(userp); @@ -5128,16 +5125,16 @@ static CURLcode create_conn(struct SessionHandle *data, conn->host.name = conn->host.rawalloc; conn->host.name[0] = 0; - user = malloc(MAX_CURL_USER_LENGTH); - passwd = malloc(MAX_CURL_PASSWORD_LENGTH); - options = malloc(MAX_CURL_OPTIONS_LENGTH); + user = strdup(""); + passwd = strdup(""); + options = strdup(""); if(!user || !passwd || !options) { result = CURLE_OUT_OF_MEMORY; goto out; } - result = parseurlandfillconn(data, conn, &prot_missing, user, passwd, - options); + result = parseurlandfillconn(data, conn, &prot_missing, &user, &passwd, + &options); if(result != CURLE_OK) goto out; diff --git a/lib/urldata.h b/lib/urldata.h index 770afe69..be139ef6 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -1152,7 +1152,6 @@ typedef enum { * Session-data MUST be put in the connectdata struct and here. */ #define MAX_CURL_USER_LENGTH 256 #define MAX_CURL_PASSWORD_LENGTH 256 -#define MAX_CURL_OPTIONS_LENGTH 256 struct auth { unsigned long want; /* Bitmask set to the authentication methods wanted by -- 1.8.4.rc4
------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
