From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Mon, 2 Aug 2010 16:43:03 +0000 (-0600)
Subject: Compliance: Improved HTTP Range header field validation.
X-Git-Tag: take1~423
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=8beb32247d4ba7895c1f342da4716f00b71dc51c;p=thirdparty%2Fsquid.git

Compliance: Improved HTTP Range header field validation.

1) Improved HttpHdrRangeSpec::parseInit() to parse syntactically valid
range specs:

  * Suffix ranges with 0 length (i.e. -0) are syntactically valid.

  * Check that last-byte-pos is greater than or equal to first-byte-pos.

After the change, HttpHdrRangeSpec::parseInit() successfully parses suffix
ranges with 0 length. They were rejected before. RFC 2616 section 14.35.1 says
such range specs are syntactically valid but unsatisfiable. Thus, we should
ignore the range spec itself, but not the whole range header. These range
specs will be rejected later, during canonization.


2) In HttpHdrRangeSpec::parseInit(), ignore the whole range header if one of
range specs is syntactically invalid (i.e. range spec parsing fails).


Co-Advisor test case: test_clause/rfc2616/invalidRange
---

diff --git a/src/HttpHdrRange.cc b/src/HttpHdrRange.cc
index 8139d32e6a..401b46c136 100644
--- a/src/HttpHdrRange.cc
+++ b/src/HttpHdrRange.cc
@@ -98,7 +98,7 @@ HttpHdrRangeSpec::parseInit(const char *field, int flen)
     } else
         /* must have a '-' somewhere in _this_ field */
         if (!((p = strchr(field, '-')) || (p - field >= flen))) {
-            debugs(64, 2, "ignoring invalid (missing '-') range-spec near: '" << field << "'");
+            debugs(64, 2, "invalid (missing '-') range-spec near: '" << field << "'");
             return false;
         } else {
             if (!httpHeaderParseOffset(field, &offset))
@@ -113,18 +113,18 @@ HttpHdrRangeSpec::parseInit(const char *field, int flen)
                 if (!httpHeaderParseOffset(p, &last_pos))
                     return false;
 
+                // RFC 2616 s14.35.1 MUST: last-byte-pos >= first-byte-pos
+                if (last_pos < offset) {
+                    debugs(64, 2, "invalid (last-byte-pos < first-byte-pos) range-spec near: " << field);
+                    return false;
+                }
+
                 HttpHdrRangeSpec::HttpRange aSpec (offset, last_pos + 1);
 
                 length = aSpec.size();
             }
         }
 
-    /* we managed to parse, check if the result makes sence */
-    if (length == 0) {
-        debugs(64, 2, "ignoring invalid (zero length) range-spec near: '" << field << "'");
-        return false;
-    }
-
     return true;
 }
 
@@ -248,7 +248,6 @@ HttpHdrRange::parseInit(const String * range_spec)
     const char *item;
     const char *pos = NULL;
     int ilen;
-    int count = 0;
     assert(this && range_spec);
     ++ParsedCount;
     debugs(64, 8, "parsing range field: '" << range_spec << "'");
@@ -264,19 +263,21 @@ HttpHdrRange::parseInit(const String * range_spec)
     while (strListGetItem(range_spec, ',', &item, &ilen, &pos)) {
         HttpHdrRangeSpec *spec = HttpHdrRangeSpec::Create(item, ilen);
         /*
-         * HTTP/1.1 draft says we must ignore the whole header field if one spec
-         * is invalid. However, RFC 2068 just says that we must ignore that spec.
+         * RFC 2616 section 14.35.1: MUST ignore Range with
+         * at least one syntactically invalid byte-range-specs.
          */
+        if (!spec) {
+            while (!specs.empty())
+                delete specs.pop_back();
+            debugs(64, 2, "ignoring invalid range field: '" << range_spec << "'");
+            break;
+        }
 
-        if (spec)
-            specs.push_back(spec);
-
-        ++count;
+        specs.push_back(spec);
     }
 
-    debugs(64, 8, "parsed range range count: " << count << ", kept " <<
-           specs.size());
-    return specs.count != 0;
+    debugs(64, 8, "got range specs: " << specs.size());
+    return !specs.empty();
 }
 
 HttpHdrRange::~HttpHdrRange()