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 <
> > [email protected]> wrote:
...
> > Sorry, the patch is incorrect; per RFC 3986, the parser is correct to
> > split the authority on the first '@'. The correct method to include
> > an '@' in the userinfo part is to percent-encode it as %40.
>
> Thanks for pointing me the good RFC for the format of userinfo in URI.
>
> But, I am not sure to understand *where* is located my problem: if it is
> a problem in ftp(1), or a problem in specific service I try to used with
> ftp (dyndns server, the http reply come from ngnix server).
>
> Currently ftp(1) manage http, https with url_get function. The basic
> auth implemented here do *not* try to decode any urlencoded information.
>
> So pass "user%40example.com:password" as userinfo in URI will result to
> base64 "user%40example.com:password" (and not
> "[email protected]:password").
...
> 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.
Here's a patch to do that.
Also:
- eliminates the COOKIE_MAX_LEN constant (if they can fit it on the
command line or in their environment, surely we can malloc the base64
version)
- renames the variable with user:pass from "cookie" to "credentials"
- empty password isn't an error
- add a boolean ishttpurl so that we don't have to do strcmps on the schema
that we just set
- when looping across multiple ftp:// urls on the command line, don't
leak the username/password memory
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;
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
