Re: [PATCH] Bug 1961 pt2: store URI userinfo (aka login) in class URL

On 28/04/2014 3:51 a.m., Kinkie wrote:
> On Sun, Apr 27, 2014 at 4:52 PM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
>> This continues the bug 1961 redesign of URL handling.
>
> Hi,
>
> + SBuf tmp = checklist->request->url.userInfo();
> + // NOTE: unescape works in-place and may decrease the c-string
> length, never increase.
> + rfc1738_unescape(const_cast<char*>(tmp.c_str()));
>
> is troublesome: you are most likely trampling the userinfo data as
> rfc1738_unescape is acting on a shared MemBlob (shared by tmp and
> userInfo at least) and c_str() doesn't guarantee a COW. The best way
> to do this is by defining a SBuf rfc1738_unescape (const SBuf &) which
> copies the param if needed.
>
>
> + if (colonPos != 0) {
>
> shouldn't this be if (colonPos != SBuf::npos) ?

No. The edge cases of user-info is that the colon may be the first
character to signal missing username, and if ':' is not found at all the
entire string is username.
 So if colonPos==0 then we dont extract one, but all other values
regardless including npos we do.

The edges case for password is that if colon is missing entirely we have
none, all other cases have a password. So the password extraction only
happens on colonPos!=npos.

Special case is that 0-length username is invalid (unlike absent
password). The colonPos==0 case also helps omit replacing any existing
username with an empty string - whereas the empty password case *does*
replace the pasword with empty string.

>
> + memcpy(user, userName.rawContent(), userName.length());
> + user[userName.length()] = '\0';
>
> you could use
> SBuf::size_type upTo = min(userName.length(), sizeof(user));
> userName.copy(user, upto);
> user[upto]='\0';
>
> Same for
> + memcpy(loginbuf,
> request->url.userInfo().rawContent(), len);
>

Thank you for tha pointer. Adding a slight variant, copy() does min()
internally and returs the size used so we can drop another line.

>
>
> + static char result[MAX_URL*2]; // should be big enough
> for a single URI segment
> That conditional is worrisome. Is there any risk of the result not
> being big enough?
>

Oops, small bug there should have been " < sizeof(result)-1" instead of
"> 0".

I have added the conditional for two reasons. One is to avoid compiler
complaints about unused return value from the new base64 decoder. Second
is to explicitly stop the mapping entirely if the base64 encoding hits a
problem edge case (result buffer not large enough) so that it fails any
actual authentication with an obvious Squid issue.

Previous behaviour was to silently truncate the Basic auth token at 8KB
- very hard to see in manual testing, and easily triggered with certain
URLs containing long (4KB +9 bytes) strign of UTF-8 characters, and not
easy to identify if it were bugs in an auth system somewhere remote.

The intention is to now drop the header entirely if the Basic auth token
exceeds 16KB - very easy to see, the limit seems to be absurdly long for
a header field and easily altered if/when ever needed (HTTP/2.0 requires
absolute 4000 byte header limit IIRC).

Amos
Received on Mon Apr 28 2014 - 02:39:55 MDT