[MPlayer-dev-eng] http stream basic auth base64_encode() fix

[Jeff D'Angelo]

On Tue, 28 Feb 2006, Jeff D'Angelo wrote:

> On Tue, 28 Feb 2006, Dominik 'Rathann' Mierzejewski wrote:
>
> > On Tuesday, 28 February 2006 at 19:15, Jeff D'Angelo wrote:
> > > On Tue, 28 Feb 2006, Dominik 'Rathann' Mierzejewski wrote:
> > >
> > > > On Tuesday, 28 February 2006 at 18:15, Jeff D'Angelo wrote:
> > > > > So that it conforms to RFC 2045:
> > > > >
> > > > > <http://jeffdangelo.org/patches/>
> > > > > MPlayer-1.0pre7try2.http_basic_auth_base64_encode.patch
> > > > > MPlayer-cvs-main-20060228.http_basic_auth_base64_encode.patch
> > > > >
> > > > > The two patch files are the same patch, but diffed against each of
> > > > > MPlayer-1.0pre7try2 and today's main CVS copy, libmpdemux/http.c version
> > > > > 1.34; both work equally well with the line offsets.  These were done diff
> > > > > -r -u, let me know if you prefer -c format.
> > > >
> > > > We prefer -u, thanks.
> > > >
> > [...]
> > > Password was so chosen, so that:
> > >
> > > a) total number of characters (username, ':', password) % 3 = 1, thus
> > >    the basic auth base64 encoding would end in two '=' chars
> > >
> > > b) '?' and '>' chars were placed at end of base64 encode input
> > >    octet tripplets so that I could demonstrate the '+' and '/'
> > >    characters in the encoding
> > >
> > > Let me know when you are done testing.
> >
> > I can confirm that with this patch, authentication does indeed work with
> > that server, while it doesn't without it.
> >
> > However, using http://user:pass@url/ syntax still doesn't work. Could you
> > look at that, while you're at it?
>
> It seems the '>' is being mangled, perhaps by something doing a URL
> encode.  I'll take a look tonight.

Yes, this is actually a different bug.  I believe the patches I and Reimar
suggested fix the base64_encode() bug for all cases.

It was the url_new() function in libmpdemux/url.c as called by open_s1()
in libmpdemux/http.c, which converts the url string (be it passed on the
command line or in a .pls file) into a URL_t struct that also passes the
username and password through url_escape_string() while they are still in
the URL.  Chars such as '>' are URL encoded to chars like '%3E'.  No
attempt is made to decode them.

My first instinct was to pull them out of the URL before the encoding
process.  I figure only the part of the URL after the first slash
(filepath, query, etc) need the encoding in any sane case; can't imagine
any scheme (protocol) that would need it, nor DNS or IP (v4, v6) that
would need it either.  However, after reviewing RFC 2396 section 2.4.2, it
sounds as it may be best to leave them encoded in the URL and then decode
them as they are pulled out to keep to the spirit of the RFC.

The enclosed patch file decodes the username and password before adding
them to the respective members of the URL_t type struct that is returned
from url_new().

Certainly there may be more efficient ways of doing this, such as passing
the length of the part of the url string (e.g. the int len and len2 vars
on lines 87 and 102 after patch) into url_unescape_string() to avoid the
temp vars.  I can provide a patch for that too if that or another way is
preferred.

The "yortwer" account mentioned earlier is still active and you are still
welcome to test against it.

With this patch, the following cases appear to work fine (tested against
the Icecast server and while retrieving "static" media from an Apache
server):

1) when either, both or neither of the username and/or password contain
   characters that would be encoded in a URL, e.g. "?a0>Belo"

2) when either, both or neither of the username and/or password contain
   characters already encoded in a URL, e.g. username "%79ortwer"

3) when the password is a null string:
	a) http://username:@host/
	b) -playlist playlistfile.pls
		containing http://username:@host/
	c) -user username -passwd ''

4) when the password is not present:
	a) http://username@host/
	b) -playlist playlistfile.pls
		containing http://username@host/
	c) -user username

A packet sniff revealed the proper username:password pair encoding each
time and the servers responded favorably.

Let me know if there's anything else you'd like from me.  Thanks.

-- 
Jeff

> > I've checked the relevant RFC and it looks like your patch is correct.
> >
> > I'd say it's good to commit as long as you remove your name from the
> > comments. Don't worry about attribution, it'll go into the CVS log.
>
> No prob.  Just new to the protocol :).
>
> <http://jeffdangelo.org/patches/MPlayer-cvs-main-20060228.http_basic_auth_base64_encode.patch>
> has been updated.
>
> > And since this fixes half of http://bugzilla.mplayerhq.hu/show_bug.cgi?id=390,
> > it will probably be mentioned in the ChangeLog as well.
> >
> > > Thanks for the quick response!
> >
> > You're welcome.
> >
> > R.
> >
> > --
> > MPlayer developer and RPMs maintainer: http://rpm.greysector.net/mplayer/
> > There should be a science of discontent. People need hard times and oppression
> > to develop psychic muscles.
> > 	-- from "Collected Sayings of Muad'Dib" by the Princess Irulan
> >
> > _______________________________________________
> > MPlayer-dev-eng mailing list
> > MPlayer-dev-eng at mplayerhq.hu
> > http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
> >
>
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
>
-------------- next part --------------
Index: url.c
===================================================================
RCS file: /cvsroot/mplayer/main/libmpdemux/url.c,v
retrieving revision 1.30
diff -u -r1.30 url.c
--- url.c	26 Nov 2005 15:57:39 -0000	1.30
+++ url.c	6 Mar 2006 04:01:46 -0000
@@ -86,12 +86,15 @@
 		// We got something, at least a username...
 		int len = ptr2-ptr1;
 		Curl->username = (char*)malloc(len+1);
-		if( Curl->username==NULL ) {
+		char *usernametmp = (char*)malloc(len+1);
+		if( Curl->username==NULL || usernametmp==NULL ) {
 			mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed);
 			goto err_out;
 		}
-		strncpy(Curl->username, ptr1, len);
-		Curl->username[len] = '\0';
+		strncpy(usernametmp, ptr1, len);
+		usernametmp[len] = '\0';
+		url_unescape_string( Curl->username, usernametmp );
+		free(usernametmp);
 
 		ptr3 = strstr(ptr1, ":");
 		if( ptr3!=NULL && ptr3<ptr2 ) {
@@ -99,12 +102,15 @@
 			int len2 = ptr2-ptr3-1;
 			Curl->username[ptr3-ptr1]='\0';
 			Curl->password = (char*)malloc(len2+1);
-			if( Curl->password==NULL ) {
+			char *passwordtmp = (char*)malloc(len2+1);
+			if( Curl->password==NULL || passwordtmp==NULL ) {
 				mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed);
 				goto err_out;
 			}
-			strncpy( Curl->password, ptr3+1, len2);
-			Curl->password[len2]='\0';
+			strncpy(passwordtmp, ptr3+1, len2);
+			passwordtmp[len2]='\0';
+			url_unescape_string( Curl->password, passwordtmp );
+			free(passwordtmp);
 		}
 		ptr1 = ptr2+1;
 		pos1 = ptr1-escfilename;