From 87cf51406cea6d6861c8c2a04ffa8fc6be998ce8 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Fri, 19 Aug 2011 22:57:24 +0200 Subject: [PATCH] [MEDIUM] http: make x-forwarded-for addition conditional If "option forwardfor" has the "if-none" argument, then the header is only added when the request did not already have one. This option has security implications, and should not be set blindly. --- doc/configuration.txt | 35 +++++++++++++++++++++++------------ include/types/proxy.h | 4 ++-- src/cfgparse.c | 13 ++++++++----- src/proto_http.c | 10 +++++++++- 4 files changed, 42 insertions(+), 20 deletions(-) diff --git a/doc/configuration.txt b/doc/configuration.txt index 291cb013f..284a667f0 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -3069,7 +3069,7 @@ no option forceclose See also : "option httpclose" and "option http-pretend-keepalive" -option forwardfor [ except ] [ header ] +option forwardfor [ except ] [ header ] [ if-none ] Enable insertion of the X-Forwarded-For header to requests sent to servers May be used in sections : defaults | frontend | listen | backend yes | yes | yes | yes @@ -3105,15 +3105,23 @@ option forwardfor [ except ] [ header ] network will not cause an addition of this header. Most common uses are with private networks or 127.0.0.1. + Alternatively, the keyword "if-none" states that the header will only be + added if it is not present. This should only be used in perfectly trusted + environment, as this might cause a security issue if headers reaching haproxy + are under the control of the end-user. + This option may be specified either in the frontend or in the backend. If at least one of them uses it, the header will be added. Note that the backend's setting of the header subargument takes precedence over the frontend's if - both are defined. + both are defined. In the case of the "if-none" argument, if at least one of + the frontend or the backend does not specify it, it wants the addition to be + mandatory, so it wins. - It is important to note that as long as HAProxy does not support keep-alive - connections, only the first request of a connection will receive the header. - For this reason, it is important to ensure that "option httpclose" is set - when using this option. + It is important to note that by default, HAProxy works in tunnel mode and + only inspects the first request of a connection, meaning that only the first + request will have the header appended, which is certainly not what you want. + In order to fix this, ensure that any of the "httpclose", "forceclose" or + "http-server-close" options is set when using this option. Examples : # Public HTTP address also used by stunnel on the same machine @@ -3126,7 +3134,8 @@ option forwardfor [ except ] [ header ] mode http option forwardfor header X-Client - See also : "option httpclose" + See also : "option httpclose", "option http-server-close", + "option forceclose" option http-no-delay @@ -3685,10 +3694,11 @@ option originalto [ except ] [ header ] setting of the header subargument takes precedence over the frontend's if both are defined. - It is important to note that as long as HAProxy does not support keep-alive - connections, only the first request of a connection will receive the header. - For this reason, it is important to ensure that "option httpclose" is set - when using this option. + It is important to note that by default, HAProxy works in tunnel mode and + only inspects the first request of a connection, meaning that only the first + request will have the header appended, which is certainly not what you want. + In order to fix this, ensure that any of the "httpclose", "forceclose" or + "http-server-close" options is set when using this option. Examples : # Original Destination address @@ -3701,7 +3711,8 @@ option originalto [ except ] [ header ] mode http option originalto header X-Client-Dst - See also : "option httpclose" + See also : "option httpclose", "option http-server-close", + "option forceclose" option persist diff --git a/include/types/proxy.h b/include/types/proxy.h index da64bb1d7..d51eed1f0 100644 --- a/include/types/proxy.h +++ b/include/types/proxy.h @@ -80,12 +80,12 @@ enum { #define PR_O_COOK_ANY (PR_O_COOK_RW | PR_O_COOK_IND | PR_O_COOK_INS | PR_O_COOK_PFX) #define PR_O_DISPATCH 0x00000040 /* use dispatch mode */ #define PR_O_KEEPALIVE 0x00000080 /* follow keep-alive sessions */ -#define PR_O_FWDFOR 0x00000100 /* insert x-forwarded-for with client address */ +#define PR_O_FWDFOR 0x00000100 /* conditionally insert x-forwarded-for with client address */ #define PR_O_BIND_SRC 0x00000200 /* bind to a specific source address when connect()ing */ #define PR_O_NULLNOLOG 0x00000400 /* a connect without request will not be logged */ #define PR_O_COOK_NOC 0x00000800 /* add a 'Cache-control' header with the cookie */ #define PR_O_COOK_POST 0x00001000 /* don't insert cookies for requests other than a POST */ -/* unused: 0x00002000 */ +#define PR_O_FF_ALWAYS 0x00002000 /* always set x-forwarded-for */ #define PR_O_PERSIST 0x00004000 /* server persistence stays effective even when server is down */ #define PR_O_LOGASAP 0x00008000 /* log as soon as possible, without waiting for the session to complete */ #define PR_O_HTTP_CLOSE 0x00010000 /* force 'connection: close' in both directions */ diff --git a/src/cfgparse.c b/src/cfgparse.c index f6a0ef2fe..1e3a92cf1 100644 --- a/src/cfgparse.c +++ b/src/cfgparse.c @@ -3484,7 +3484,7 @@ stats_error_parsing: * set default options (ie: bitfield, header name, etc) */ - curproxy->options |= PR_O_FWDFOR; + curproxy->options |= PR_O_FWDFOR | PR_O_FF_ALWAYS; free(curproxy->fwdfor_hdr_name); curproxy->fwdfor_hdr_name = strdup(DEF_XFORWARDFOR_HDR); @@ -3516,9 +3516,12 @@ stats_error_parsing: curproxy->fwdfor_hdr_name = strdup(args[cur_arg+1]); curproxy->fwdfor_hdr_len = strlen(curproxy->fwdfor_hdr_name); cur_arg += 2; + } else if (!strcmp(args[cur_arg], "if-none")) { + curproxy->options &= ~PR_O_FF_ALWAYS; + cur_arg += 1; } else { /* unknown suboption - catchall */ - Alert("parsing [%s:%d] : '%s %s' only supports optional values: 'except' and 'header'.\n", + Alert("parsing [%s:%d] : '%s %s' only supports optional values: 'except', 'header' and 'if-none'.\n", file, linenum, args[0], args[1]); err_code |= ERR_ALERT | ERR_FATAL; goto out; @@ -3538,7 +3541,7 @@ stats_error_parsing: curproxy->orgto_hdr_name = strdup(DEF_XORIGINALTO_HDR); curproxy->orgto_hdr_len = strlen(DEF_XORIGINALTO_HDR); - /* loop to go through arguments - start at 2, since 0+1 = "option" "forwardfor" */ + /* loop to go through arguments - start at 2, since 0+1 = "option" "originalto" */ cur_arg = 2; while (*(args[cur_arg])) { if (!strcmp(args[cur_arg], "except")) { @@ -6089,11 +6092,11 @@ out_uri_auth_compat: curproxy->uri_auth = NULL; } - if (curproxy->options & PR_O_FWDFOR) { + if (curproxy->options & (PR_O_FWDFOR | PR_O_FF_ALWAYS)) { Warning("config : 'option %s' ignored for %s '%s' as it requires HTTP mode.\n", "forwardfor", proxy_type_str(curproxy), curproxy->id); err_code |= ERR_WARN; - curproxy->options &= ~PR_O_FWDFOR; + curproxy->options &= ~(PR_O_FWDFOR | PR_O_FF_ALWAYS); } if (curproxy->options & PR_O_ORGTO) { diff --git a/src/proto_http.c b/src/proto_http.c index 578ebf7a6..acf2ea3ad 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -3541,7 +3541,15 @@ int http_process_request(struct session *s, struct buffer *req, int an_bit) * asks for it. */ if ((s->fe->options | s->be->options) & PR_O_FWDFOR) { - if (s->req->prod->addr.c.from.ss_family == AF_INET) { + struct hdr_ctx ctx = { .idx = 0 }; + + if (!((s->fe->options | s->be->options) & PR_O_FF_ALWAYS) && + http_find_header2("X-Forwarded-For", 15, txn->req.sol, &txn->hdr_idx, &ctx)) { + /* The header is set to be added only if none is present + * and we found it, so don't do anything. + */ + } + else if (s->req->prod->addr.c.from.ss_family == AF_INET) { /* Add an X-Forwarded-For header unless the source IP is * in the 'except' network range. */