Re: PATCH: ftp: allow @ in username for Basic Auth
On Tue, Jul 1, 2014 at 7:09 AM, Sébastien Marie semarie-open...@latrappe.fr wrote: On Sun, Jun 29, 2014 at 09:52:28PM -0700, Philip Guenther wrote: 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. It works also for my use-case. Please note I haven't tested proxy setting (by lake of server to test). Just committed it. Thank you for pointing out the problem and confirming that it works for you! Philip Guenther
Re: PATCH: ftp: allow @ in username for Basic Auth
On Sun, Jun 29, 2014 at 09:52:28PM -0700, Philip Guenther wrote: 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. It works also for my use-case. Please note I haven't tested proxy setting (by lake of server to test). Thanks for your help. -- Sébastien Marie
Re: PATCH: ftp: allow @ in username for Basic Auth
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 - 1.122
+++ fetch.c 30 Jun 2014 04:47:25 -
@@ -76,6 +76,7 @@ void aborthttp(int);
void abortfile(int);
char hextochar(const char *);
char *urldecode(const char *);
+char *recode_credentials(const char *_userinfo);
intftp_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, ':');
-
Re: PATCH: ftp: allow @ in username for Basic Auth
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:
As I see not activity or feedback for this one line patch, I think it
need more explain ?
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
u...@example.com:password).
The dyndns service reply with 401 Unauthorized.
Comparing with curl(1) (in ports), a user:password information passed as
argument is passed to base64 as it, but a user:password passed in the
URI is firstly urldecoded before base64.
$ curl -v -u 'n...@example.com:password' 'http://localhost/'
Authorization: Basic bmFtZUBleGFtcGxlLmNvbTpwYXNzd29yZA==
$ curl -v 'http://name%40example.com:password@localhost/'
Authorization: Basic bmFtZUBleGFtcGxlLmNvbTpwYXNzd29yZA==
bmFtZUBleGFtcGxlLmNvbTpwYXNzd29yZA== is n...@example.com:password
So, I think ftp(1) should urldecode userinfo before base64. I propose
another patch that implement urldecoding of userinfo (as it come from
URI).
Some comments about the patch:
- I stop use p variable for userinfo parse: I declare and use two new
variables, userinfo and userinfoend. I hope the code is more
readable.
- for urldecoding, I use an already defined function. Memory is
allocated and checked by this function.
- I didn't try to parse userinfo as user:password. The userinfo
string is passed as it to urldecode.
Thanks.
--
Sébastien Marie
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 - 1.122
+++ fetch.c 25 Jun 2014 06:49:11 -
@@ -474,16 +474,30 @@ noslash:
*/
if (proxyenv == NULL
(!strcmp(scheme, HTTP_URL) || !strcmp(scheme, HTTPS_URL))) {
- if ((p = strchr(host, '@')) != NULL) {
- size_t authlen = (strlen(host) + 5) * 4 / 3;
- *p = 0; /* Kill @ */
+ char *userinfo, *userinfoend;
+
+ /* extract userinfo part */
+ if ((userinfoend = strchr(host, '@')) != NULL) {
+ size_t authlen;
+
+ /* separate userinfo and host components */
+ userinfo = host;
+ host = userinfoend + 1;
+ *userinfoend = '\0';
+
+ /* urldecode userinfo */
+ userinfo = urldecode(userinfo);
+
+ /* build Basic auth */
+ authlen = (strlen(userinfo) + 5) * 4 / 3;
if ((auth = malloc(authlen)) == NULL)
err(1, Can't allocate memory for
authorization);
- if (b64_ntop(host, strlen(host),
+ if (b64_ntop(userinfo, strlen(userinfo),
auth, authlen) == -1)
errx(1, error in base64 encoding);
- host = p + 1;
+
+ free(userinfo);
}
}
#endif /* SMALL */
Re: PATCH: ftp: allow @ in username for Basic Auth
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:
...
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
u...@example.com: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 - 1.122
+++ fetch.c 26 Jun 2014 01:40:28 -
@@ -75,7 +75,8 @@ static inturl_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);
intftp_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
Re: PATCH: ftp: allow @ in username for Basic Auth
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 - 1.122
+++ fetch.c 26 Jun 2014 01:40:28 -
@@ -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_PROXYftp_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
Re: PATCH: ftp: allow @ in username for Basic Auth
Hi,
As I see not activity or feedback for this one line patch, I think it
need more explain ?
Currently, when you pass an URL with user/pass embed, the code parse it
badly.
For example:
https://mym...@example.com:my-passw...@another-domain.example.com/blabla
Just before the code search if the supplied URL contains a user/pass,
the variables are:
scheme = https://;
host = mym...@example.com:my-passw...@another-domain.example.com
The code use strchr(3) on host in order to find '@' in host variable,
and separate the user/pass component and the host component.
But, with strchr the result is:
p = mymail
host = example.com:my-passw...@another-domain.example.com
The patch replace strchr(3) by strrchr(3) to obtain:
p = mym...@example.com:my-password
host = another-domain.example.com
As the hostname should not contains '@' char, and user/pass may contains
it, (as defined by rfc1738), this patch make ftp(1) to more respect
standard.
Thanks.
--
Sébastien Marie
On Mon, Jun 23, 2014 at 10:15:25AM +0200, Sébastien Marie wrote:
Hi,
Using ftp(1) with HTTP(S) scheme and Basic auth, it is currently not
possible to have username (or password) with a '@' inner.
For example, this URI is badly parsed:
ftp https://mym...@example.com:my-passw...@another-domain.example.com/blabla
According to RFC2617, '@' is a valid character in user-id or password:
user-pass = userid : password
userid = *TEXT excluding :
password= *TEXT
Here a patch to search the last '@' in the string (which don't contains
the path at this time).
--
Sébastien Marie
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 - 1.122
+++ fetch.c 23 Jun 2014 07:46:33 -
@@ -474,7 +474,7 @@ noslash:
*/
if (proxyenv == NULL
(!strcmp(scheme, HTTP_URL) || !strcmp(scheme, HTTPS_URL))) {
- if ((p = strchr(host, '@')) != NULL) {
+ if ((p = strrchr(host, '@')) != NULL) {
size_t authlen = (strlen(host) + 5) * 4 / 3;
*p = 0; /* Kill @ */
if ((auth = malloc(authlen)) == NULL)
Re: PATCH: ftp: allow @ in username for Basic Auth
On Tue, Jun 24, 2014 at 9:01 AM, Sébastien Marie semarie-open...@latrappe.fr wrote: As I see not activity or feedback for this one line patch, I think it need more explain ? 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. Philip Guenther
PATCH: ftp: allow @ in username for Basic Auth
Hi,
Using ftp(1) with HTTP(S) scheme and Basic auth, it is currently not
possible to have username (or password) with a '@' inner.
For example, this URI is badly parsed:
ftp https://mym...@example.com:my-passw...@another-domain.example.com/blabla
According to RFC2617, '@' is a valid character in user-id or password:
user-pass = userid : password
userid = *TEXT excluding :
password= *TEXT
Here a patch to search the last '@' in the string (which don't contains
the path at this time).
--
Sébastien Marie
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 - 1.122
+++ fetch.c 23 Jun 2014 07:46:33 -
@@ -474,7 +474,7 @@ noslash:
*/
if (proxyenv == NULL
(!strcmp(scheme, HTTP_URL) || !strcmp(scheme, HTTPS_URL))) {
- if ((p = strchr(host, '@')) != NULL) {
+ if ((p = strrchr(host, '@')) != NULL) {
size_t authlen = (strlen(host) + 5) * 4 / 3;
*p = 0; /* Kill @ */
if ((auth = malloc(authlen)) == NULL)
