On 02/20/2013 11:19 PM, Amos Jeffries wrote:
> On 21/02/2013 6:41 p.m., Alex Rousskov wrote:
>> On 02/20/2013 07:26 PM, Amos Jeffries wrote:
>>> On 21/02/2013 2:47 p.m., Alex Rousskov wrote:
>>>> It looks like NTLM and possibly Negotiate authentication is broken
>>>> in trunk because Squid eats the bare backslash that AF responses use to
>>>> separate authentication domain and user names. With the backslash
>>>> removed, the merged domainuser name never matches, of course.
>>> strwordtok() is a function of our own creation so it is probably best
>>> long term to adjust its API to include a flag for skipping the
>>> slashDecode behaviour instead of creating duplicate code.
>> Please note that the function I added does not duplicate strwordtok()
>> behavior. If anything, it duplicates strtok(), but for good reasons.
> Noted.
>> When two functions cover very different use cases and have rather
>> different semantics (not to mention implementation), I think they both
>> deserve to live.
> I do disagree that two functions are needed and am happy with a flag
> disabling all the special handling in strwordtok().
>>> I like the extra documentation you added about statics and use cases.
>>> Can you write something similar about strwordtok() ?
>> Sure, will do (regardless of whether we add a second tokenizing function
>> or not).
Hi Amos,
The attached patch dumbs strwordtok() further down when the new
optional decode parameter is false. I also adjusted the comment as promised.
Looking at the final result, I am even more certain that two functions
are better than one function with a parameter in this case. It would be
sad to see other, more performance sensitive code to start using
strwordtok(decode=false) for parsing instead of a simple and efficient
implementation needed for that.
If nobody else wants to weigh in, I will let you decide how to proceed.
Please commit the code that you think should go in now.
Thank you,
Alex.
Preserve bare backslashes in AF and TT NTLM/Negotiate helper responses.
Bare backslashes separate authentication domain and user name in AF.
When authentication helper protocol was enhanced to support annotations, we
started to use strwordtok() to parse AF and TT responses, but strwordtok()
ate bare backslashes (and did other decoding that might not be safe
in AF and AT context?).
This patch adds an optional decode parameter to strwordtok() to allow the
caller to prevent strwordtok() from decoding backslashes and quoted strings.
The new parameter is used to decode AF and TT responses.
This is an alternative patch. The original patch added a new tokenizer that
just split words like strtok() but without bad side effects.
=== modified file 'src/HelperReply.cc'
--- src/HelperReply.cc 2013-02-14 05:42:57 +0000
+++ src/HelperReply.cc 2013-03-15 04:28:02 +0000
@@ -40,61 +40,61 @@
debugs(84, 3, "Buff length is larger than 2");
// some helper formats (digest auth, URL-rewriter) just send a data string
// we must also check for the ' ' character after the response token (if anything)
if (!strncmp(p,"OK",2) && (len == 2 || p[2] == ' ')) {
debugs(84, 3, "helper Result = OK");
result = HelperReply::Okay;
p+=2;
} else if (!strncmp(p,"ERR",3) && (len == 3 || p[3] == ' ')) {
debugs(84, 3, "helper Result = ERR");
result = HelperReply::Error;
p+=3;
} else if (!strncmp(p,"BH",2) && (len == 2 || p[2] == ' ')) {
debugs(84, 3, "helper Result = BH");
result = HelperReply::BrokenHelper;
p+=2;
} else if (!strncmp(p,"TT ",3)) {
// NTLM challenge token
result = HelperReply::TT;
p+=3;
// followed by an auth token
- char *w1 = strwordtok(NULL, &p);
+ char *w1 = strwordtok(NULL, &p, false);
if (w1 != NULL) {
MemBuf authToken;
authToken.init();
authToken.append(w1, strlen(w1));
notes.add("token",authToken.content());
} else {
// token field is mandatory on this response code
result = HelperReply::BrokenHelper;
notes.add("message","Missing 'token' data");
}
} else if (!strncmp(p,"AF ",3)) {
// NTLM/Negotate OK response
result = HelperReply::Okay;
p+=3;
// followed by:
// an optional auth token and user field
// or, an optional username field
- char *w1 = strwordtok(NULL, &p);
- char *w2 = strwordtok(NULL, &p);
+ char *w1 = strwordtok(NULL, &p, false);
+ char *w2 = strwordtok(NULL, &p, false);
if (w2 != NULL) {
// Negotiate "token user"
MemBuf authToken;
authToken.init();
authToken.append(w1, strlen(w1));
notes.add("token",authToken.content());
MemBuf user;
user.init();
user.append(w2,strlen(w2));
notes.add("user",user.content());
} else if (w1 != NULL) {
// NTLM "user"
MemBuf user;
user.init();
user.append(w1,strlen(w1));
notes.add("user",user.content());
}
} else if (!strncmp(p,"NA ",3)) {
=== modified file 'src/SquidString.h'
--- src/SquidString.h 2012-09-22 10:56:48 +0000
+++ src/SquidString.h 2013-03-15 04:32:56 +0000
@@ -167,23 +167,31 @@
size_type len_; /* current length */
char *buf_;
_SQUID_INLINE_ void set(char const *loc, char const ch);
_SQUID_INLINE_ void cutPointer(char const *loc);
};
_SQUID_INLINE_ std::ostream & operator<<(std::ostream& os, String const &aString);
_SQUID_INLINE_ bool operator<(const String &a, const String &b);
#if _USE_INLINE_
#include "String.cci"
#endif
const char *checkNullString(const char *p);
int stringHasWhitespace(const char *);
int stringHasCntl(const char *);
-char *strwordtok(char *buf, char **t);
+
+/** Like strtok(start, " ") but does not use a static area that makes strtok()
+ * unsafe when multiple callers might call strtok() in one context. Sets next
+ * argument to the area after the found word, so that the strtok(NULL, "")
+ * hack is not required at the end when we do not want to extract all words.
+ * If decode is true, does rudimentary decoding of quoted strings and
+ * backslashes (recognizing \r and \n sequences).
+ */
+char *strwordtok(char *buf, char **next, bool decode = true);
#endif /* SQUID_STRING_H */
=== modified file 'src/String.cc'
--- src/String.cc 2012-10-04 09:14:06 +0000
+++ src/String.cc 2013-03-15 01:21:34 +0000
@@ -333,62 +333,63 @@
stringHasCntl(const char *s)
{
unsigned char c;
while ((c = (unsigned char) *s++) != '\0') {
if (c <= 0x1f)
return 1;
if (c >= 0x7f && c <= 0x9f)
return 1;
}
return 0;
}
/*
* Similar to strtok, but has some rudimentary knowledge
* of quoting
*/
char *
-strwordtok(char *buf, char **t)
+strwordtok(char *buf, char **t, bool decode)
{
unsigned char *word = NULL;
unsigned char *p = (unsigned char *) buf;
unsigned char *d;
unsigned char ch;
int quoted = 0;
if (!p)
p = (unsigned char *) *t;
if (!p)
goto error;
while (*p && xisspace(*p))
++p;
if (!*p)
goto error;
word = d = p;
+ if (decode) {
while ((ch = *p)) {
switch (ch) {
case '\\':
++p;
switch (*p) {
case 'n':
ch = '\n';
break;
case 'r':
ch = '\r';
break;
default:
ch = *p;
@@ -407,40 +408,52 @@
case '"':
quoted = !quoted;
++p;
break;
default:
if (!quoted && xisspace(*p)) {
++p;
goto done;
}
*d = *p;
++d;
++p;
break;
}
}
+ } else {
+ while ((ch = *p)) {
+ if (xisspace(ch)) {
+ ++p;
+ goto done;
+ }
+
+ *d = ch;
+ ++d;
+ ++p;
+ }
+ }
done:
*d = '\0';
error:
*t = (char *) p;
return (char *) word;
}
const char *
checkNullString(const char *p)
{
return p ? p : "(NULL)";
}
const char *
String::pos(char const *aString) const
{
if (undefined())
return NULL;
