Re: [PATCH] BUG/MINOR: reject malformed HTTP/0.9 requests

[Apollon Oikonomopoulos]

Hi Willy,

On 07:57 Sun 06 Apr , Willy Tarreau wrote:
> 
> In fact I intentionally made this choice because Apache does the same and
> wanted to ensure the least possible breakage by inserting haproxy in front
> of it (you can't imagine how users are picky when something does not work
> anymore after they install your product). But I agree that the URI is
> mandatory according to the RFC, and even according to Tim BL's original
> WWWDaemon (which used to only process GET requests containing a URI).
> 
> These days we don't need to prove that we don't break anything, so I think
> it's fine to change this behaviour. So I have merged your patch into 1.5.
> I don't intend to backport it into 1.4-stable however.

Thanks! I already suspected that you had your reasons for this, but 
having the req_proto_http ACL match requests like "HEAD HTTP/1.0" as 
valid seemed a bit strange! Just for the record, Apache 2.2 still 
accepts URI-less requests, nginx exhibits our "new" behavior and varnish 
requires a request URI but does not mandate GET for HTTP/0.9 requests.

Regards,
Apollon

Re: [PATCH] BUG/MINOR: reject malformed HTTP/0.9 requests

[Willy Tarreau]

Hi Apollon,

On Sun, Apr 06, 2014 at 02:46:00AM +0300, Apollon Oikonomopoulos wrote:
> RFC 1945 (§4.1) defines an HTTP/0.9 request ("Simple-Request") as:
> 
>   Simple-Request  = "GET" SP Request-URI CRLF
> 
> HAProxy tries to automatically upgrade HTTP/0.9 requests to
> to HTTP/1.0, by appending "HTTP/1.0" to the request and setting the
> Request-URI to "/" if it was not present. The latter however is
> RFC-incompatible, as HTTP/0.9 requests must already have a Request-URI
> according to the definition above. Additionally,
> http_upgrade_v09_to_v10() does not check whether the request method is
> indeed GET (the mandatory method for HTTP/0.9).
> 
> As a result, any single- or double-word request line is regarded as a
> valid HTTP request. We fix this by failing in http_upgrade_v09_to_v10()
> if the request method is not GET or the request URI is not present.

In fact I intentionally made this choice because Apache does the same and
wanted to ensure the least possible breakage by inserting haproxy in front
of it (you can't imagine how users are picky when something does not work
anymore after they install your product). But I agree that the URI is
mandatory according to the RFC, and even according to Tim BL's original
WWWDaemon (which used to only process GET requests containing a URI).

These days we don't need to prove that we don't break anything, so I think
it's fine to change this behaviour. So I have merged your patch into 1.5.
I don't intend to backport it into 1.4-stable however.

Thanks!
Willy

[PATCH] BUG/MINOR: reject malformed HTTP/0.9 requests

[Apollon Oikonomopoulos]

RFC 1945 (§4.1) defines an HTTP/0.9 request ("Simple-Request") as:

  Simple-Request  = "GET" SP Request-URI CRLF

HAProxy tries to automatically upgrade HTTP/0.9 requests to
to HTTP/1.0, by appending "HTTP/1.0" to the request and setting the
Request-URI to "/" if it was not present. The latter however is
RFC-incompatible, as HTTP/0.9 requests must already have a Request-URI
according to the definition above. Additionally,
http_upgrade_v09_to_v10() does not check whether the request method is
indeed GET (the mandatory method for HTTP/0.9).

As a result, any single- or double-word request line is regarded as a
valid HTTP request. We fix this by failing in http_upgrade_v09_to_v10()
if the request method is not GET or the request URI is not present.
---
 src/proto_http.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/proto_http.c b/src/proto_http.c
index df33991..c23fa54 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -1777,14 +1777,16 @@ static int http_upgrade_v09_to_v10(struct http_txn *txn)
if (msg->sl.rq.v_l != 0)
return 1;
 
+   /* RFC 1945 allows only GET for HTTP/0.9 requests */
+   if (txn->meth != HTTP_METH_GET)
+   return 0;
+
cur_end = msg->chn->buf->p + msg->sl.rq.l;
delta = 0;
 
if (msg->sl.rq.u_l == 0) {
-   /* if no URI was set, add "/" */
-   delta = buffer_replace2(msg->chn->buf, cur_end, cur_end, " /", 
2);
-   cur_end += delta;
-   http_msg_move_end(msg, delta);
+   /* HTTP/0.9 requests *must* have a request URI, per RFC 1945 */
+   return 0;
}
/* add HTTP version */
delta = buffer_replace2(msg->chn->buf, cur_end, cur_end, " 
HTTP/1.0\r\n", 11);
-- 
1.9.1