Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (asf.osuosl.org: local policy)
Errors-To: <wrowe@rowe-clan.net>
Message-Id: <6.2.1.2.2.20050707223300.06f18650@pop3.rowe-clan.net>
Date: Thu, 07 Jul 2005 23:27:18 -0500
To: dev@httpd.apache.org
From: "William A. Rowe, Jr." <wrowe@rowe-clan.net>
Subject: [vote(s)] [Patch 1.3] Strict proxy C-L / T-E conformance
Mime-Version: 1.0
Content-Type: multipart/mixed;
	boundary="=====================_996877937==_"

--=====================_996877937==_
Content-Type: text/plain; charset="us-ascii"

At 08:35 AM 7/7/2005, Roy T. Fielding wrote:
>
> 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.

Actually for 1.3, this patch is doing a couple things for the
proxy response from the origin server to the client;

*) It enforces 'chunked' as the only legitimate T-E

*) It unsets the C-L header if the T-E header is present

*) To avoid 'suspect' responses, it won't cache any response
   with both a T-E and C-L header  (If we used keep alive
   backend connections, it would also want to close that.)

I corrected the ap_strtol result tests, based on the fact that
it's *our* strtol, and we know we will hiccup errno if we see
out of range, or no digits at all, and end_ptr is guarenteed
to be set.

Remember in 1.3 mod_proxy won't pass up a chunked request body
so that whole scenario is ignored for now.  If the client (or
previous proxy hop) passes T-E, the request is rejected.

I believe this patch handles all the scenarios in 1.3, combined
with Jeff's request TE+CL patch.  Further comment or specific
votes to the behaviors above are welcome.  Would like to commit
tomorrow to roll a release candidate.

I am still waiting for the answer to the question; does mod_gzip
or any other 3p module we know of actually introduce an alternate
Transfer-Encoding to 1.3?

Bill
--=====================_996877937==_
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: attachment; filename="httpd-1.3-proxy-CL-strict-r2.patch"

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 */

--=====================_996877937==_
Content-Type: text/plain; charset="us-ascii"


--=====================_996877937==_--