Re: [REVIEW] Carefully verify digest responses

2010-03-06 Thread Henrik Nordstrom
tis 2010-03-02 klockan 18:06 +0100 skrev Henrik Nordstrom:

> Comments are very welcome while I validate the parser changes.

Validation completed and committed to trunk.

Forgot to mantion the relevant bug reports in commit messages.

Parser: 2845

Stale: 2367

both changes needs to get merged back all way to 3.0.

Regards
Henrik



Re: [REVIEW] Carefully verify digest responses

2010-03-05 Thread Henrik Nordstrom
tor 2010-03-04 klockan 20:57 -0700 skrev Alex Rousskov:

> Please consider adding a cppunit test to check a few common and corner
> parsing cases.

Unfortunately the existing auth tests we have do not come close to
touching actual request parsing/handling, and trying to get things in
shape to the level that this can be exercised with cppunit is a little
heaver than I have time for today.

Regards
Henrik



Re: [REVIEW] Carefully verify digest responses

2010-03-04 Thread Alex Rousskov
On 03/02/2010 10:06 AM, Henrik Nordstrom wrote:
> First drop of improved digest auth parser.
> 
> Focused on the parser & input validation, but as you can see there is
> further room for improvement by moving more processing over to String.
> 
> The state of the old parser was rather scary.. not even comparing field
> names correctly and several other ugly things...
> 
> Comments are very welcome while I validate the parser changes. Expect to
> submit this for merge in a day or two.

Please consider adding a cppunit test to check a few common and corner
parsing cases.

Cheers,

Alex.




Re: [REVIEW] Carefully verify digest responses

2010-03-03 Thread Henrik Nordstrom
ons 2010-03-03 klockan 22:19 +1300 skrev Amos Jeffries:

> 
> This debugs seems to have incorrect output for the test being done:
> + /* check cnonce */
> + if (!digest_request->cnonce || digest_request->cnonce[0] == '\0') {
> + debugs(29, 2, "authenticateDigestDecode: Missing URI field");

Thanks. Fixed.

Regards
Henrik



Re: [REVIEW] Carefully verify digest responses

2010-03-03 Thread Amos Jeffries

Henrik Nordstrom wrote:

First drop of improved digest auth parser.

Focused on the parser & input validation, but as you can see there is
further room for improvement by moving more processing over to String.

The state of the old parser was rather scary.. not even comparing field
names correctly and several other ugly things...

Comments are very welcome while I validate the parser changes. Expect to
submit this for merge in a day or two.

A note of warning: This changes the quoted-string parser to actually
parse quoted-string.. which impacts the Surrogate-Control parser. If
that uses the parsed value to construct a new header then we also need
to make sure to properly produce quoted-string as the value is now
normalized as a token (quotes & escapes removed) where it before just
har the quotes removed.

Regards
Henrik



Logic looks okay at face value.
Hope it pans out in the testing. :)


This debugs seems to have incorrect output for the test being done:
+   /* check cnonce */
+   if (!digest_request->cnonce || digest_request->cnonce[0] == '\0') {
+   debugs(29, 2, "authenticateDigestDecode: Missing URI field");


Amos
--
Please be using
  Current Stable Squid 2.7.STABLE7 or 3.0.STABLE24
  Current Beta Squid 3.1.0.17


[REVIEW] Carefully verify digest responses

2010-03-02 Thread Henrik Nordstrom
First drop of improved digest auth parser.

Focused on the parser & input validation, but as you can see there is
further room for improvement by moving more processing over to String.

The state of the old parser was rather scary.. not even comparing field
names correctly and several other ugly things...

Comments are very welcome while I validate the parser changes. Expect to
submit this for merge in a day or two.

A note of warning: This changes the quoted-string parser to actually
parse quoted-string.. which impacts the Surrogate-Control parser. If
that uses the parsed value to construct a new header then we also need
to make sure to properly produce quoted-string as the value is now
normalized as a token (quotes & escapes removed) where it before just
har the quotes removed.

Regards
Henrik
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: hen...@henriknordstrom.net-20100302165813-\
#   xhabeaopfdvbi372
# target_branch: ../trunk/
# testament_sha1: 9ef7f250de84e6076ea7f172491f9695736e71f4
# timestamp: 2010-03-02 17:59:33 +0100
# base_revision_id: squid...@squid-cache.org-20100301011317-\
#   5miwqblkua9c5gzq
# 
# Begin patch
=== modified file 'src/HttpHeaderTools.cc'
--- src/HttpHeaderTools.cc	2009-09-10 03:08:35 +
+++ src/HttpHeaderTools.cc	2010-03-02 14:08:22 +
@@ -335,25 +335,29 @@
 {
 const char *end, *pos;
 val->clean();
-assert (*start == '"');
+if (*start != '"') {
+	debugs(66, 2, "failed to parse a quoted-string header field near '" << start << "'");
+	return 0;
+}
 pos = start + 1;
 
-while (1) {
-if (!(end = index (pos,'"'))) {
-debugs(66, 2, "failed to parse a quoted-string header field near '" << start << "'");
-return 0;
-}
-
-/* check for quoted-chars */
-if (*(end - 1) != '\\') {
-/* done */
-val->append(start + 1, end-start-1);
-return 1;
-}
-
-/* try for the end again */
-pos = end + 1;
+while (*pos != '"') {
+	if (*pos == '\\') {
+	pos++;
+	}
+	if (!*pos) {
+	debugs(66, 2, "failed to parse a quoted-string header field near '" << start << "'");
+	val->clean();
+	return 0;
+	}
+	end = pos + strcspn(pos, "\"\\");
+	val->append(pos, end-pos);
+	pos = end;
 }
+/* Make sure it's defined even if empty "" */
+if (!val->defined())
+	val->limitInit("", 0);
+return 1;
 }
 
 /**

=== modified file 'src/auth/digest/auth_digest.cc'
--- src/auth/digest/auth_digest.cc	2010-02-14 00:09:05 +
+++ src/auth/digest/auth_digest.cc	2010-03-02 16:58:13 +
@@ -67,6 +67,33 @@
 
 CBDATA_TYPE(DigestAuthenticateStateData);
 
+enum http_digest_attr_type {
+DIGEST_USERNAME=1,
+DIGEST_REALM,
+DIGEST_QOP,
+DIGEST_ALGORITHM,
+DIGEST_URI,
+DIGEST_NONCE,
+DIGEST_NC,
+DIGEST_CNONCE,
+DIGEST_RESPONSE,
+DIGEST_ENUM_END
+};
+
+static const HttpHeaderFieldAttrs DigestAttrs[DIGEST_ENUM_END] = {
+{"username",  (http_hdr_type)DIGEST_USERNAME},
+{"realm", (http_hdr_type)DIGEST_REALM},
+{"qop", (http_hdr_type)DIGEST_QOP},
+{"algorithm", (http_hdr_type)DIGEST_ALGORITHM},
+{"uri", (http_hdr_type)DIGEST_URI},
+{"nonce", (http_hdr_type)DIGEST_NONCE},
+{"nc", (http_hdr_type)DIGEST_NC},
+{"cnonce", (http_hdr_type)DIGEST_CNONCE},
+{"response", (http_hdr_type)DIGEST_RESPONSE},
+};
+
+static HttpHeaderFieldInfo *DigestFieldsInfo = NULL;
+
 /*
  *
  * Nonce Functions
@@ -505,6 +532,9 @@
 if (digestauthenticators)
 helperShutdown(digestauthenticators);
 
+httpHeaderDestroyFieldsInfo(DigestFieldsInfo, DIGEST_ENUM_END);
+DigestFieldsInfo = NULL;
+
 authdigest_initialised = 0;
 
 if (!shutting_down) {
@@ -867,6 +897,7 @@
 AuthDigestConfig::init(AuthConfig * scheme)
 {
 if (authenticate) {
+	DigestFieldsInfo = httpHeaderBuildFieldsInfo(DigestAttrs, DIGEST_ENUM_END);
 authenticateDigestNonceSetup();
 authdigest_initialised = 1;
 
@@ -1090,124 +1121,84 @@
 String temp(proxy_auth);
 
 while (strListGetItem(&temp, ',', &item, &ilen, &pos)) {
-if ((p = strchr(item, '=')) && (p - item < ilen))
-ilen = p++ - item;
-
-if (!strncmp(item, "username", ilen)) {
-/* white space */
-
-while (xisspace(*p))
-p++;
-
-/* quote mark */
-p++;
-
+	String value;
+	size_t nlen;
+	/* isolate directive name */
+	if ((p = (const char *)memchr(item, '=', ilen)) && (p - item < ilen)) {
+nlen = p++ - item;
+	if (!httpHeaderParseQuotedString(p, &value))
+		value.limitInit(p, ilen - (p - item));
+	} else
+	nlen = ilen;
+
+	if (!value.defined()) {
+	debugs(29, 9, "authDigestDecodeAuth: Failed to parse attribute '" << temp << "' in '" << proxy_auth << "'");
+	continue;
+	}
+
+	/* find type */
+	http_digest_attr_type type = (http_digest_attr_type)httpHeaderIdByName(item, nlen, DigestFieldsInfo, DIGEST_ENUM_END);
+
+	switch (typ