On 28/04/2014 3:51 a.m., Kinkie wrote: > On Sun, Apr 27, 2014 at 4:52 PM, Amos Jeffries <[email protected]> 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
