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.
> ...
>
> 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 26 Jun 2014 01:40:28 -0000
> @@ -75,7 +75,8 @@ static int url_get(const char *, const c
> void aborthttp(int);
> void abortfile(int);
> char hextochar(const char *);
> -char *urldecode(const char *);
> +void urldecode(char *);
> +char *recode_credentials(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,27 +537,14 @@ 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");
> - *path = '@'; /* restore @ in proxyurl */
> - /*
> - * This removes the password from proxyurl,
> - * filling with stars
> - */
> - for (host = 1 + strchr(proxyurl + 5, ':'); *host !=
> '@';
> - host++)
> - *host = '*';
> -
> + credentials = recode_credentials(host);
> host = path + 1;
> }
> +
> path = newline;
> }
>
> @@ -697,7 +677,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 +773,7 @@ again:
>
> #ifndef SMALL
> if (proxyenv && sslhost)
> - proxy_connect(s, sslhost, cookie);
> + proxy_connect(s, sslhost, credentials);
> #endif /* !SMALL */
> break;
> }
> @@ -890,10 +870,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 +889,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 +1212,7 @@ cleanup_url_get:
> SSL_free(ssl);
> }
> free(full_host);
> - free(auth);
> + free(credentials);
> #endif /* !SMALL */
> if (fin != NULL)
> fclose(fin);
> @@ -1240,7 +1221,7 @@ cleanup_url_get:
> free(buf);
> free(proxyurl);
> free(newline);
> - free(cookie);
> + free(credentials);
> return (rval);
> }
>
> @@ -1384,8 +1365,8 @@ bad_ftp_url:
> rval = argpos + 1;
> continue;
> }
> - username = urldecode(username);
> - pass = urldecode(pass);
> + urldecode(username);
> + urldecode(pass);
> }
>
> #ifdef INET6
> @@ -1586,36 +1567,47 @@ bad_ftp_url:
> return (rval);
> }
>
> -char *
> -urldecode(const char *str)
> +void
> +urldecode(char *str)
> {
> char *ret, c;
> - int i, reallen;
> + int i, j, reallen;
ret and reallen are unused now.
> if (str == NULL)
> - return NULL;
> - if ((ret = malloc(strlen(str)+1)) == NULL)
> - err(1, "Can't allocate memory for URL decoding");
> - for (i = 0, reallen = 0; str[i] != '\0'; i++, reallen++, ret++) {
> - c = str[i];
> - if (c == '+') {
> - *ret = ' ';
> - continue;
> - }
> -
> - /* Cannot use strtol here because next char
> - * after %xx may be a digit.
> - */
> - if (c == '%' && isxdigit(str[i+1]) && isxdigit(str[i+2])) {
> - *ret = hextochar(&str[i+1]);
> - i+=2;
> - continue;
> + return;
> + for (i = j = 0; (c = str[i]) != '\0'; i++, j++) {
> + if (c == '+')
> + c = ' ';
> + else if (c == '%' && isxdigit(str[i+1]) && isxdigit(str[i+2])) {
> + /*
> + * Cannot use strtol here because next char
> + * after %xx may be a digit.
> + */
> + c = hextochar(&str[i+1]);
> + i += 2;
> }
> - *ret = c;
> + str[j] = c;
> }
> - *ret = '\0';
> + str[j] = c;
> +}
> +
> +char *
> +recode_credentials(char *userinfo)
> +{
> + char *creds;
> + size_t ulen, credsize;
> +
> + /* url-decode the user and pass */
> + urldecode(userinfo);
>
> - return ret-reallen;
> + ulen = strlen(userinfo);
> + credsize = (ulen + 2) / 3 * 4 + 1;
> + creds = malloc(credsize);
> + if (creds == NULL)
> + errx(1, "out of memory");
> + if (b64_ntop(userinfo, ulen, creds, credsize) == -1)
> + errx(1, "error in base64 encoding");
> + return (creds);
> }
>
> char
>
