[Patch 1.3] Strict proxy C-L / T-E conformance

[William A. Rowe Jr.]

The attached patch restricts T-E responses to 'chunked', and
discards C-L in that case, setting the response to nocache
as a just-in-case.

It also adds a note in case anyone wants to use connection
keep-alive, warning of dire circumstances.

Comments/votes?

Bill

[William A. Rowe Jr.]

Attached is the mystery patch [omitted from the last note - whoops].

IMHO we should apply the same to ap_http_filter() in 2.1's
http_filters.c

Bill

At 10:35 PM 7/5/2005, William A. Rowe, Jr. wrote:

>The attached patch restricts T-E responses to 'chunked', and
>discards C-L in that case, setting the response to nocache
>as a just-in-case.
>
>It also adds a note in case anyone wants to use connection
>keep-alive, warning of dire circumstances.
>
>Comments/votes?
>
>Bill

Index: src/modules/proxy/proxy_http.c
===================================================================
--- src/modules/proxy/proxy_http.c (revision 209384)
+++ src/modules/proxy/proxy_http.c (working copy)
@@ -338,7 +338,12 @@
         ap_table_mergen(req_hdrs, "X-Forwarded-Server", r->server->server_hostname);
     }
 
-    /* we don't yet support keepalives - but we will soon, I promise! */
+    /* we don't yet support keepalives - but we will soon, I promise!
+     * XXX: This introduces various HTTP Request vulnerabilies if not
+     * properly implemented.  Before changing this .. be certain to
+     * add a hard-close of the connection if the T-E and C-L headers
+     * are both present, or the C-L header is malformed.
+     */
     ap_table_set(req_hdrs, "Connection", "close");
 
     reqhdrs_arr = ap_table_elts(req_hdrs);
@@ -475,25 +480,38 @@
         }
 
         /* is this content chunked? */
-        chunked = ap_find_last_token(r->pool,
-                                     ap_table_get(resp_hdrs, "Transfer-Encoding"),
-                                     "chunked");
+        chunked = ap_table_get(resp_hdrs, "Transfer-Encoding");
+        if (chunked && (strcasecmp(chunked, "chunked") != 0)) {
+            ap_kill_timeout(r);
+            return ap_proxyerror(r, HTTP_BAD_GATEWAY, ap_pstrcat(r->pool,
+                                 "Unsupported Transfer-Encoding ", chunked,
+                                 " from remote server", NULL));
+        }
 
         /* strip hop-by-hop headers defined by Connection and RFC2616 */
         ap_proxy_clear_connection(p, resp_hdrs);
 
         content_length = ap_table_get(resp_hdrs, "Content-Length");
         if (content_length != NULL) {
-            c->len = ap_strtol(content_length, NULL, 10);
+            if (chunked) {
+                /* XXX: We would unset keep-alive here, for safety's sake,
+                 * but we aren't keep-alive (forced Connection: close)
+                 */
+                nocache = 1;        /* do not cache this suspect file */
+                ap_table_unset(resp_hdrs, "Content-Length");
+            }
+            else {
+                char *len_end;
+                c->len = ap_strtol(content_length, *len_end, 10);
 
-    if (c->len < 0) {
- ap_kill_timeout(r);
- return ap_proxyerror(r, HTTP_BAD_GATEWAY, ap_pstrcat(r->pool,
-     "Invalid Content-Length from remote server",
-                                      NULL));
+                if ((c->len < 0) || *len_end) {
+                    ap_kill_timeout(r);
+                    return ap_proxyerror(r, HTTP_BAD_GATEWAY, ap_pstrcat(r->pool,
+                                         "Invalid Content-Length from remote server",
+                                         NULL));
+                }
     }
         }
-
     }
     else {
         /* an http/0.9 response */

[Jim Jagielski]

+                char *len_end;
+                c->len = ap_strtol(content_length, *len_end, 10);
-        if (c->len < 0) {
-        ap_kill_timeout(r);
-        return ap_proxyerror(r, HTTP_BAD_GATEWAY, ap_pstrcat(r->pool,
-                     "Invalid Content-Length from remote server",
-                                      NULL));
+                if ((c->len < 0) || *len_end) {


Oops... Should be:

      c->len = ap_strtol(content_length, &len_end, 10);

and

     if ((c->len < 0) || (len_end && *len_end))

I think.. :)

[Jim Jagielski]

On Jul 6, 2005, at 9:06 AM, Jim Jagielski wrote:

> +                char *len_end;
> +                c->len = ap_strtol(content_length, *len_end, 10);
> -        if (c->len < 0) {
> -        ap_kill_timeout(r);
> -        return ap_proxyerror(r, HTTP_BAD_GATEWAY, ap_pstrcat(r->pool,
> -                     "Invalid Content-Length from remote server",
> -                                      NULL));
> +                if ((c->len < 0) || *len_end) {
>
>
> Oops... Should be:
>
>      c->len = ap_strtol(content_length, &len_end, 10);
>
> and
>
>     if ((c->len < 0) || (len_end && *len_end))
>
> I think.. :)
>

You know, to be anal about it we should check for range errors; it  
should be:

     char *len_end;
     int errno = 0;
       ...
     if ((c->len < 0) || errno || (len_end && *len_end))

[William A. Rowe Jr.]

At 08:12 AM 7/6/2005, Jim Jagielski wrote:

>On Jul 6, 2005, at 9:06 AM, Jim Jagielski wrote:
>
>>+                char *len_end;
>>+                c->len = ap_strtol(content_length, *len_end, 10);
>>+                if ((c->len < 0) || *len_end) {
>>
>>Oops... Should be:
>>
>>     c->len = ap_strtol(content_length, &len_end, 10);
>
>You know, to be anal about it we should check for range errors; it  
>should be:
>
>    char *len_end;
>    int errno = 0;
>      ...
>    if ((c->len < 0) || errno || (len_end && *len_end))

Agreed... revised patch attached.

The patch does leave me some questions, as I began looking
at Apache 2.1.  Doing this in http_filter might be too late,
it seems that the http_header filter could be a better place.

Also, I'm not clear if mod_gzip can inject itself into the backend
proxy code, and whether or not mod_gzip supports T-E encoding or
only C-E, or if any other modules play with different T-E fields.
I don't have the sources handy.

But in 2.1, we allow filters to inject themselves in all sorts of
places.  I'm unsure how early we want to reject T-E other than
the 'chunked' token.

Bill  
Index: src/modules/proxy/proxy_http.c
===================================================================
--- src/modules/proxy/proxy_http.c (revision 209415)
+++ src/modules/proxy/proxy_http.c (working copy)
@@ -121,7 +121,7 @@
     char portstr[32];
     pool *p = r->pool;
     int destport = 0;
-    int chunked = 0;
+    const char *chunked = NULL;
     char *destportstr = NULL;
     const char *urlptr = NULL;
     const char *datestr, *urlstr;
@@ -338,7 +338,12 @@
         ap_table_mergen(req_hdrs, "X-Forwarded-Server", r->server->server_hostname);
     }
 
-    /* we don't yet support keepalives - but we will soon, I promise! */
+    /* we don't yet support keepalives - but we will soon, I promise!
+     * XXX: This introduces various HTTP Request vulnerabilies if not
+     * properly implemented.  Before changing this .. be certain to
+     * add a hard-close of the connection if the T-E and C-L headers
+     * are both present, or the C-L header is malformed.
+     */
     ap_table_set(req_hdrs, "Connection", "close");
 
     reqhdrs_arr = ap_table_elts(req_hdrs);
@@ -475,25 +480,40 @@
         }
 
         /* is this content chunked? */
-        chunked = ap_find_last_token(r->pool,
-                                     ap_table_get(resp_hdrs, "Transfer-Encoding"),
-                                     "chunked");
+        chunked = ap_table_get(resp_hdrs, "Transfer-Encoding");
+        if (chunked && (strcasecmp(chunked, "chunked") != 0)) {
+            ap_kill_timeout(r);
+            return ap_proxyerror(r, HTTP_BAD_GATEWAY, ap_pstrcat(r->pool,
+                                 "Unsupported Transfer-Encoding ", chunked,
+                                 " from remote server", NULL));
+        }
 
         /* strip hop-by-hop headers defined by Connection and RFC2616 */
         ap_proxy_clear_connection(p, resp_hdrs);
 
         content_length = ap_table_get(resp_hdrs, "Content-Length");
         if (content_length != NULL) {
-            c->len = ap_strtol(content_length, NULL, 10);
+            if (chunked) {
+                /* XXX: We would unset keep-alive here, to the proxy
+                 * origin server, for safety's sake but we aren't using
+                 * keep-alives (we force Connection: close  above)
+                 */
+                nocache = 1;        /* do not cache this suspect file */
+                ap_table_unset(resp_hdrs, "Content-Length");
+            }
+            else {
+                char *len_end;
+                errno = 0;
+                c->len = ap_strtol(content_length, &len_end, 10);
 
-    if (c->len < 0) {
- ap_kill_timeout(r);
- return ap_proxyerror(r, HTTP_BAD_GATEWAY, ap_pstrcat(r->pool,
-     "Invalid Content-Length from remote server",
-                                      NULL));
+                if (errno || (c->len < 0) || (len_end && *len_end)) {
+                    ap_kill_timeout(r);
+                    return ap_proxyerror(r, HTTP_BAD_GATEWAY,
+                                         "Invalid Content-Length from remote"
+                                         " server");
+                }
     }
         }
-
     }
     else {
         /* an http/0.9 response */
@@ -612,7 +632,7 @@
  * content length is not known. We need to make 100% sure c->len is always
  * set correctly before we get here to correctly do keepalive.
  */
-        ap_proxy_send_fb(f, r, c, c->len, 0, chunked, conf->io_buffer_size);
+        ap_proxy_send_fb(f, r, c, c->len, 0, (int)chunked, conf->io_buffer_size);
     }
 
     /* ap_proxy_send_fb() closes the socket f for us */

[Joe Orton]

On Wed, Jul 06, 2005 at 11:45:21AM -0500, William Rowe wrote:
...
> +            else {
> +                char *len_end;
> +                errno = 0;
> +                c->len = ap_strtol(content_length, &len_end, 10);
...
> +                if (errno || (c->len < 0) || (len_end && *len_end)) {

You should copy the logic used on the trunk to get the correct tests for
a strtol parse error:

  errno || len_end == content_length || *len_end || c->len < 0

joe

[Jim Jagielski]

On Jul 6, 2005, at 2:22 PM, Joe Orton wrote:

> On Wed, Jul 06, 2005 at 11:45:21AM -0500, William Rowe wrote:
> ...
>
>> +            else {
>> +                char *len_end;
>> +                errno = 0;
>> +                c->len = ap_strtol(content_length, &len_end, 10);
>>
> ...
>
>> +                if (errno || (c->len < 0) || (len_end &&  
>> *len_end)) {
>>
>
> You should copy the logic used on the trunk to get the correct  
> tests for
> a strtol parse error:
>
>   errno || len_end == content_length || *len_end || c->len < 0
>

The len_end == content_length just means that there was no digits
seen (possibly a blank)... not sure if we should consider that
an error. That's why in all other places we've used
the (len_end && *len_end) check, which ensures that
it's valid *and* that it's seen an invalid char. ap_strtol(" "...)
returns 0 but isn't an "error" (though in this context,
I see your point).

[William A. Rowe Jr.]

At 01:22 PM 7/6/2005, Joe Orton wrote:

>You should copy the logic used on the trunk to get the correct tests for
>a strtol parse error:
>
>  errno || len_end == content_length || *len_end || c->len < 0

What is len_end == content_length?  wouldn't *content_length
be clearer?  And this test doesn't test for a null len_end, is
that safe?

Bill

[Roy T. Fielding]

On Jul 5, 2005, at 8:56 PM, William A. Rowe, Jr. wrote:

> Attached is the mystery patch [omitted from the last note - whoops].
>
> IMHO we should apply the same to ap_http_filter() in 2.1's
> http_filters.c

It looks like a band-aid to me -- how does this module know that
the server doesn't support other transfer encodings?  Wouldn't
it be better to register a set of transfer encoding filters and
then error if none matches?  Oh, never mind, this is for 1.3. +0.

....Roy

[Joe Orton]

On Wed, Jul 06, 2005 at 02:53:52PM -0400, Jim Jagielski wrote:

>
> On Jul 6, 2005, at 2:22 PM, Joe Orton wrote:
>
> >On Wed, Jul 06, 2005 at 11:45:21AM -0500, William Rowe wrote:
> >...
> >
> >>+            else {
> >>+                char *len_end;
> >>+                errno = 0;
> >>+                c->len = ap_strtol(content_length, &len_end, 10);
> >>
> >...
> >
> >>+                if (errno || (c->len < 0) || (len_end &&  
> >>*len_end)) {
> >>
> >
> >You should copy the logic used on the trunk to get the correct  
> >tests for
> >a strtol parse error:
> >
> >  errno || len_end == content_length || *len_end || c->len < 0
> >
>
> The len_end == content_length just means that there was no digits
> seen (possibly a blank)... not sure if we should consider that
> an error.

An empty string, right: I think it's certainly correct to treat that as
invalid C-L header; indeed some strtol's themselves set errno for that
case.  (the perl-framework C-L tests picked up this inconsistency a
while back)

> That's why in all other places we've used the (len_end && *len_end)
> check, which ensures that

There is no case where strtol will set len_end = NULL so the first half
of the conditional is redundant.  (also len_end was not NULL-initialized
so if it was an attempt to catch cases where strtol does *not* set
len_end, it was not correct ;)

> it's valid *and* that it's seen an invalid char. ap_strtol(" "...)
> returns 0 but isn't an "error" (though in this context, I see your
> point).

[William A. Rowe Jr.]

At 08:35 AM 7/7/2005, Roy T. Fielding wrote:

>On Jul 5, 2005, at 8:56 PM, William A. Rowe, Jr. wrote:
>
>>Attached is the mystery patch [omitted from the last note - whoops].
>>
>>IMHO we should apply the same to ap_http_filter() in 2.1's
>>http_filters.c
>
>It looks like a band-aid to me -- how does this module know that
>the server doesn't support other transfer encodings?  Wouldn't
>it be better to register a set of transfer encoding filters and
>then error if none matches?  Oh, never mind, this is for 1.3. +0.

To that idea, for Apache 2.2, a huge ++1!  I'll ensure we are
a bit more flexible for 2.1-dev.

As I think about it, we are wasting cycles injecting a filter
which keeps looking left and right to decide if it is handling
a C-L body or a T-E body.  IMHO these need to become two different
filters, and the body filter would -only- be injected in the absence
of all other T-E options.

A registered T-E filter would stack (with 'chunked' at the lowest
layer of the stack).  The order of stacking remains the only puzzle
to be solved.

Bill

[Jim Jagielski]

Joe Orton wrote:
>
>
> An empty string, right: I think it's certainly correct to treat that as
> invalid C-L header;

Bill just asked Roy about this very question.

> indeed some strtol's themselves set errno for that
> case.  (the perl-framework C-L tests picked up this inconsistency a
> while back)

This is also noted in the comments when we fixed the invalid
c-l logic (overflow/underflow) some time ago.

>
> There is no case where strtol will set len_end = NULL so the first half
> of the conditional is redundant.  (also len_end was not NULL-initialized
> so if it was an attempt to catch cases where strtol does *not* set
> len_end, it was not correct ;)
>

This was, iirc, to handle cases where a strtol could possibly set it
to NULL; someone, can't recall who, seemed to remember one implementation
which did that, so we just figured to-hell-with-it and add a safety
check, just in case :)


--
===========================================================================
   Jim Jagielski   [|]   jim@...   [|]   http://www.jaguNET.com/
                     "Sith happens"  -  Yoda

[Jim Jagielski]

Joe Orton wrote:
>
> An empty string, right: I think it's certainly correct to treat that as
> invalid C-L header;
>

While we wait on Roy's response, my own PoV is that we should
accept it and assume it means '0', so be as lenient and forgiving
as possible in input ("be generous in input, rigorous in output").

But if Roy says it's invalid, then we need to do what's right :)

--
===========================================================================
   Jim Jagielski   [|]   jim@...   [|]   http://www.jaguNET.com/
                     "Sith happens"  -  Yoda

[William A. Rowe Jr.]

At 12:09 PM 7/7/2005, Jim Jagielski wrote:

>This was, iirc, to handle cases where a strtol could possibly set it
>to NULL; someone, can't recall who, seemed to remember one implementation
>which did that, so we just figured to-hell-with-it and add a safety
>check, just in case :)

In httpd-1.3, src/ap/ap_strtol.c, don't we provide our own
strtol which we can trust?

Bill

[Jim Jagielski]

William A. Rowe, Jr. wrote:

>
> At 12:09 PM 7/7/2005, Jim Jagielski wrote:
>
> >This was, iirc, to handle cases where a strtol could possibly set it
> >to NULL; someone, can't recall who, seemed to remember one implementation
> >which did that, so we just figured to-hell-with-it and add a safety
> >check, just in case :)
>
> In httpd-1.3, src/ap/ap_strtol.c, don't we provide our own
> strtol which we can trust?
>

Yep, we do.

--
===========================================================================
   Jim Jagielski   [|]   jim@...   [|]   http://www.jaguNET.com/
                     "Sith happens"  -  Yoda