Bug in rfc1783_decode() in 3.4 and master

[Andrew Bartlett]

I noticed doing some work in master that the current rfc1783_decode()
has:

        while ((p=strchr(p,'+')))
                *p = ' ';

As far as I can see, this was merged incorrectly from Samba4 when the
string util code was brought back in common in Nov last year.  The
Samba3 code is deliberately missing this loop, with this loop move to
SWAT specificity.

Anyway, the long and the short of it is that ntlm_auth uses
rfc1783_decode(), and when Squid calls the plaintext ntlm_auth helpers,
it does not replace space characters with +, so this merge (I suspect)
breaks squid.

I'm importing a full rfc1783 decode and encode impelementation from
squid, but a patch to 3.4 may wish to just remove those lines.

(Anyway, I'm tired, but I wanted to mention this before I merged in the
replacement code)

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[Jeremy Allison]

On Sat, Oct 31, 2009 at 12:13:01AM +1100, Andrew Bartlett wrote:

> I noticed doing some work in master that the current rfc1783_decode()
> has:
>
> while ((p=strchr(p,'+')))
> *p = ' ';
>
> As far as I can see, this was merged incorrectly from Samba4 when the
> string util code was brought back in common in Nov last year.  The
> Samba3 code is deliberately missing this loop, with this loop move to
> SWAT specificity.
>
> Anyway, the long and the short of it is that ntlm_auth uses
> rfc1783_decode(), and when Squid calls the plaintext ntlm_auth helpers,
> it does not replace space characters with +, so this merge (I suspect)
> breaks squid.

I can't find the spec that requires ' ' characters to be
replaced with '+' on encode. Which spec is this ? Why was this added ?
Shouldn't ' ' characters be converted to %20 instead ?

> I'm importing a full rfc1783 decode and encode impelementation from
> squid, but a patch to 3.4 may wish to just remove those lines.

Yep, am preparing a patch to do just that.

> (Anyway, I'm tired, but I wanted to mention this before I merged in the
> replacement code)

I'm going to add a bug and fix this in all branches.

Jeremy.

[Andrew Bartlett]

On Fri, 2009-10-30 at 13:47 -0700, Jeremy Allison wrote:

> On Sat, Oct 31, 2009 at 12:13:01AM +1100, Andrew Bartlett wrote:
> > I noticed doing some work in master that the current rfc1783_decode()
> > has:
> >
> > while ((p=strchr(p,'+')))
> > *p = ' ';
> >
> > As far as I can see, this was merged incorrectly from Samba4 when the
> > string util code was brought back in common in Nov last year.  The
> > Samba3 code is deliberately missing this loop, with this loop move to
> > SWAT specificity.
> >
> > Anyway, the long and the short of it is that ntlm_auth uses
> > rfc1783_decode(), and when Squid calls the plaintext ntlm_auth helpers,
> > it does not replace space characters with +, so this merge (I suspect)
> > breaks squid.
>
> I can't find the spec that requires ' ' characters to be
> replaced with '+' on encode. Which spec is this ? Why was this added ?
> Shouldn't ' ' characters be converted to %20 instead ?

There is a convention in browsers that " " is converted to + in URLs.  

This attempts to cope with that convention in SWAT' server: web/cgi.c:

/**
 URL encoded strings can have a '+', which should be replaced with a
space
 (This was in rfc1738_unescape(), but that broke the squid helper)
**/

static void plus_to_space_unescape(char *buf)
{
        char *p=buf;
        while ((p=strchr_m(p,'+')))
                *p = ' ';
}

With it then being used like in web/cgi.c:
                        plus_to_space_unescape(variables[num_variables].value);
rfc1738_unescape(variables[num_variables].value);

> > I'm importing a full rfc1783 decode and encode impelementation from
> > squid, but a patch to 3.4 may wish to just remove those lines.
>
> Yep, am preparing a patch to do just that.

Thanks!

> > (Anyway, I'm tired, but I wanted to mention this before I merged in the
> > replacement code)
>
> I'm going to add a bug and fix this in all branches.

Do check, but I believe 3.3 is unaffected (being before this particular
merge).

For master, I'll be pushing the change to use the squid rfc1783.c
shortly, on the basis that if they can't get URI encoding/decoding
right, nobody can, and that I didn't feel like contributing to the
collection of new, independent buggy encoders myself :-)

Andrew Bartlett
--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.