[22 hours - ping]
Votes please? 2/3 of our users use 1.3, do you?
Jim reminded me we don't ap_strtol everywhere, so the NULL check
I guess remains a good idea, as would a (len_end == content_length)
test which I will add before committing.
Bill
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
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 || *len_end || (c->len < 0)) {
+ 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 */
