On Thu, 26 Jun 2014, S?bastien Marie wrote: > On Wed, Jun 25, 2014 at 07:07:30PM -0700, Philip Guenther wrote: > > On Wed, 25 Jun 2014, S?bastien Marie wrote: > > > On Tue, Jun 24, 2014 at 10:55:44AM -0700, Philip Guenther wrote: > > > > On Tue, Jun 24, 2014 at 9:01 AM, S?bastien Marie < > > > > semarie-open...@latrappe.fr> wrote: > > ... > > > So, I think ftp(1) should urldecode userinfo before base64. > > > > I agree. > > > > > I propose another patch that implement urldecoding of userinfo (as it > > > come from URI). > > > > It also needs to be fixed for proxy authentication. I think the code to > > urldecode and reencode in base64 should be factored out. > > If urldecode is refactored, it may return the length of decoded string > (as size_t* parameter: NULL for no result, output variable else). > > The purpose is to properly managed the case where %00 is include in the > userinfo. Else it results truncation (as string are \0 terminate). > > But it would need more work: if in recode_credentials there is no > problem (just need to pass ulen to urldecode, and don't need strlen), > urldecode is used also for FTP URL parsing. > > But note that I don't sure that manage %00 in userinfo make sense. > > > Here's a patch to do that. > > Just a comment in code (unused variables in urldecode). > Else it seems ok. And my use-case works.
Dang it, I just noticed that I had sent an earlier version of my diff, which had problems with some proxy setting, IIRC. Here's the diff that I settled on after testing. <sigh> Philip Guenther Index: fetch.c =================================================================== RCS file: /cvs/src/usr.bin/ftp/fetch.c,v retrieving revision 1.122 diff -u -p -r1.122 fetch.c --- fetch.c 20 May 2014 01:25:23 -0000 1.122 +++ fetch.c 30 Jun 2014 04:47:25 -0000 @@ -76,6 +76,7 @@ void aborthttp(int); void abortfile(int); char hextochar(const char *); char *urldecode(const char *); +char *recode_credentials(const char *_userinfo); int ftp_printf(FILE *, SSL *, const char *, ...) __attribute__((format(printf, 3, 4))); char *ftp_readline(FILE *, SSL *, size_t *); size_t ftp_read(FILE *, SSL *, char *, size_t); @@ -97,8 +98,6 @@ SSL_CTX *ssl_get_ssl_ctx(void); #define FTP_PROXY "ftp_proxy" /* env var with ftp proxy location */ #define HTTP_PROXY "http_proxy" /* env var with http proxy location */ -#define COOKIE_MAX_LEN 42 - #define EMPTYSTRING(x) ((x) == NULL || (*(x) == '\0')) static const char at_encoding_warning[] = @@ -393,7 +392,7 @@ url_get(const char *origline, const char struct addrinfo hints, *res0, *res, *ares = NULL; const char * volatile savefile; char * volatile proxyurl = NULL; - char *cookie = NULL; + char *credentials = NULL; volatile int s = -1, out; volatile sig_t oldintr, oldinti; FILE *fin = NULL; @@ -402,9 +401,9 @@ url_get(const char *origline, const char ssize_t len, wlen; #ifndef SMALL char *sslpath = NULL, *sslhost = NULL; - char *locbase, *full_host = NULL, *auth = NULL; + char *locbase, *full_host = NULL; const char *scheme; - int ishttpsurl = 0; + int ishttpurl = 0, ishttpsurl = 0; SSL_CTX *ssl_ctx = NULL; #endif /* !SMALL */ SSL *ssl = NULL; @@ -420,6 +419,7 @@ url_get(const char *origline, const char if (strncasecmp(newline, HTTP_URL, sizeof(HTTP_URL) - 1) == 0) { host = newline + sizeof(HTTP_URL) - 1; #ifndef SMALL + ishttpurl = 1; scheme = HTTP_URL; #endif /* !SMALL */ } else if (strncasecmp(newline, FTP_URL, sizeof(FTP_URL) - 1) == 0) { @@ -472,17 +472,10 @@ noslash: * contain the path. Basic auth from RFC 2617, valid * characters for path are in RFC 3986 section 3.3. */ - if (proxyenv == NULL && - (!strcmp(scheme, HTTP_URL) || !strcmp(scheme, HTTPS_URL))) { + if (proxyenv == NULL && (ishttpurl || ishttpsurl)) { if ((p = strchr(host, '@')) != NULL) { - size_t authlen = (strlen(host) + 5) * 4 / 3; - *p = 0; /* Kill @ */ - if ((auth = malloc(authlen)) == NULL) - err(1, "Can't allocate memory for " - "authorization"); - if (b64_ntop(host, strlen(host), - auth, authlen) == -1) - errx(1, "error in base64 encoding"); + *p = '\0'; + credentials = recode_credentials(host); host = p + 1; } } @@ -544,17 +537,13 @@ noslash: path = strchr(host, '@'); /* look for credentials in proxy */ if (!EMPTYSTRING(path)) { *path = '\0'; - cookie = strchr(host, ':'); - if (EMPTYSTRING(cookie)) { + if (strchr(host, ':') == NULL) { warnx("Malformed proxy URL: %s", proxyenv); goto cleanup_url_get; } - cookie = malloc(COOKIE_MAX_LEN); - if (cookie == NULL) - errx(1, "out of memory"); - if (b64_ntop(host, strlen(host), cookie, COOKIE_MAX_LEN) == -1) - errx(1, "error in base64 encoding"); + credentials = recode_credentials(host); *path = '@'; /* restore @ in proxyurl */ + /* * This removes the password from proxyurl, * filling with stars @@ -565,6 +554,7 @@ noslash: host = path + 1; } + path = newline; } @@ -697,7 +687,7 @@ noslash: if (debug) fprintf(ttyout, "host %s, port %s, path %s, " "save as %s, auth %s.\n", - host, portnum, path, savefile, auth); + host, portnum, path, savefile, credentials); #endif /* !SMALL */ memset(&hints, 0, sizeof(hints)); @@ -793,7 +783,7 @@ again: #ifndef SMALL if (proxyenv && sslhost) - proxy_connect(s, sslhost, cookie); + proxy_connect(s, sslhost, credentials); #endif /* !SMALL */ break; } @@ -890,10 +880,11 @@ again: * Host: directive must use the destination host address for * the original URI (path). We do not attach it at this moment. */ - if (cookie) + if (credentials) ftp_printf(fin, ssl, "GET %s HTTP/1.0\r\n" "Proxy-Authorization: Basic %s%s\r\n%s\r\n\r\n", - epath, cookie, buf ? buf : "", HTTP_USER_AGENT); + epath, credentials, buf ? buf : "", + HTTP_USER_AGENT); else ftp_printf(fin, ssl, "GET %s HTTP/1.0\r\n%s%s\r\n\r\n", epath, buf ? buf : "", HTTP_USER_AGENT); @@ -908,14 +899,14 @@ again: else restart_point = 0; } - if (auth) { + if (credentials) { ftp_printf(fin, ssl, "GET /%s %s\r\nAuthorization: Basic %s\r\nHost: ", epath, restart_point ? "HTTP/1.1\r\nConnection: close" : "HTTP/1.0", - auth); - free(auth); - auth = NULL; + credentials); + free(credentials); + credentials = NULL; } else #endif /* SMALL */ ftp_printf(fin, ssl, "GET /%s %s\r\nHost: ", epath, @@ -1231,7 +1222,7 @@ cleanup_url_get: SSL_free(ssl); } free(full_host); - free(auth); + free(credentials); #endif /* !SMALL */ if (fin != NULL) fclose(fin); @@ -1240,7 +1231,7 @@ cleanup_url_get: free(buf); free(proxyurl); free(newline); - free(cookie); + free(credentials); return (rval); } @@ -1316,9 +1307,13 @@ auto_fetch(int argc, char *argv[], char /* * Loop through as long as there's files to fetch. */ + username = pass = NULL; for (rval = 0; (rval == 0) && (argpos < argc); free(url), argpos++) { if (strchr(argv[argpos], ':') == NULL) break; + + free(username); + free(pass); host = dir = file = portnum = username = pass = NULL; /* @@ -1375,6 +1370,7 @@ auto_fetch(int argc, char *argv[], char if (strchr(pass, '@') != NULL || (passagain != NULL && passagain < dir)) { warnx(at_encoding_warning); + username = pass = NULL; goto bad_ftp_url; } @@ -1382,6 +1378,7 @@ auto_fetch(int argc, char *argv[], char bad_ftp_url: warnx("Invalid URL: %s", argv[argpos]); rval = argpos + 1; + username = pass = NULL; continue; } username = urldecode(username); @@ -1616,6 +1613,26 @@ urldecode(const char *str) *ret = '\0'; return ret-reallen; +} + +char * +recode_credentials(const char *userinfo) +{ + char *ui, *creds; + size_t ulen, credsize; + + /* url-decode the user and pass */ + ui = urldecode(userinfo); + + ulen = strlen(ui); + credsize = (ulen + 2) / 3 * 4 + 1; + creds = malloc(credsize); + if (creds == NULL) + errx(1, "out of memory"); + if (b64_ntop(ui, ulen, creds, credsize) == -1) + errx(1, "error in base64 encoding"); + free(ui); + return (creds); } char