BUG/MINOR: http_ext/7239: ipv6 dumping relies on out of scope variables
In http_build_7239_header_nodename(), ip6 address dumping is performed
at a single place to prevent code duplication:
A goto statement combined with a local pointer variable (ip6_addr) were used
to perform ipv6 dump from different calling places inside the function.
However, when the goto was performed (ie: sample expression handling),
ip6_addr pointer was assigned to limited scope variable's address that is not
valid within the dumping code.
Because of this, we have an undefined behavior that could result in a bug or
a crash depending on the platform that is running haproxy.
This was found by Coverity (GH #2018)
To fix this, we add a simple ip6 printing helper that takes the ip6_addr
pointer as an argument.
This prevents any scope related bug as the function is executed under the
proper context.
if/else guards inside the function were reviewed to make sure that the goto
removal won't affect existing behavior.
----------
No backport needed, except if the commit ("MINOR: proxy/http_ext: introduce
proxy forwarded option") is backported.
Given that this commit needs to be backported with
"MINOR: proxy/http_ext: introduce proxy forwarded option", We're using it as a
reminder for another bug that was introduced with
"MINOR: proxy/http_ext: introduce proxy forwarded option" but has been silently
fixed since with
"MEDIUM: proxy/http_ext: implement dynamic http_ext".
If "MINOR: proxy/http_ext: introduce proxy forwarded option" needs to be
backported without
"MEDIUM: proxy/http_ext: implement dynamic http_ext", you should manually apply
the following patch on top of it:
| diff --git a/src/http_ext.c b/src/http_ext.c
| index fcb5a07bc..3921357a3 100644
| --- a/src/http_ext.c
| +++ b/src/http_ext.c
| @@ -609,7 +609,7 @@ static inline void http_build_7239_header_node(struct buffer *out,
| if (forby->np_mode)
| chunk_appendf(out, "\"");
| offset_save = out->data;
| - http_build_7239_header_node(out, s, curproxy, addr, &curproxy->http.fwd.p_by);
| + http_build_7239_header_nodename(out, s, curproxy, addr, forby);
| if (offset_save == out->data) {
| /* could not build nodename, either because some
| * data is not available or user is providing bad input
| @@ -619,7 +619,7 @@ static inline void http_build_7239_header_node(struct buffer *out,
| if (forby->np_mode) {
| chunk_appendf(out, ":");
| offset_save = out->data;
| - http_build_7239_header_nodeport(out, s, curproxy, addr, &curproxy->http.fwd.p_by);
| + http_build_7239_header_nodeport(out, s, curproxy, addr, forby);
| if (offset_save == out->data) {
| /* could not build nodeport, either because some data is
| * not available or user is providing bad input
(If you don't, forwarded option won't work properly and will crash
haproxy (stack overflow) when building 'for' or 'by' parameter)
diff --git a/src/http_ext.c b/src/http_ext.c
index d3b5764..3875d3b 100644
--- a/src/http_ext.c
+++ b/src/http_ext.c
@@ -485,12 +485,25 @@
return 0;
}
+static inline void _7239_print_ip6(struct buffer *out, struct in6_addr *ip6_addr, int quoted)
+{
+ char pn[INET6_ADDRSTRLEN];
+
+ inet_ntop(AF_INET6,
+ ip6_addr,
+ pn, sizeof(pn));
+ if (!quoted)
+ chunk_appendf(out, "\""); /* explicit quoting required for ipv6 */
+ chunk_appendf(out, "[%s]", pn);
+}
+
static inline void http_build_7239_header_nodename(struct buffer *out,
struct stream *s, struct proxy *curproxy,
const struct sockaddr_storage *addr,
struct http_ext_7239_forby *forby)
{
struct in6_addr *ip6_addr;
+ int quoted = !!forby->np_mode;
if (forby->nn_mode == HTTP_7239_FORBY_ORIG) {
if (addr && addr->ss_family == AF_INET) {
@@ -500,17 +513,7 @@
}
else if (addr && addr->ss_family == AF_INET6) {
ip6_addr = &((struct sockaddr_in6 *)addr)->sin6_addr;
- print_ip6:
- {
- char pn[INET6_ADDRSTRLEN];
-
- inet_ntop(AF_INET6,
- ip6_addr,
- pn, sizeof(pn));
- if (!forby->np_mode)
- chunk_appendf(out, "\""); /* explicit quoting required for ipv6 */
- chunk_appendf(out, "[%s]", pn);
- }
+ _7239_print_ip6(out, ip6_addr, quoted);
}
/* else: not supported */
}
@@ -524,10 +527,10 @@
if (smp->data.type == SMP_T_IPV6) {
/* smp is valid IP6, print with RFC compliant output */
ip6_addr = &smp->data.u.ipv6;
- goto print_ip6;
+ _7239_print_ip6(out, ip6_addr, quoted);
}
- if (sample_casts[smp->data.type][SMP_T_STR] &&
- sample_casts[smp->data.type][SMP_T_STR](smp)) {
+ else if (sample_casts[smp->data.type][SMP_T_STR] &&
+ sample_casts[smp->data.type][SMP_T_STR](smp)) {
struct ist validate_n = ist2(smp->data.u.str.area, smp->data.u.str.data);
struct ist validate_o = ist2(smp->data.u.str.area, smp->data.u.str.data);
struct forwarded_header_nodename nodename;
@@ -539,12 +542,13 @@
nodename.ip.ss_family == AF_INET6) {
/* special care needed for valid ip6 nodename (quoting) */
ip6_addr = &((struct sockaddr_in6 *)&nodename.ip)->sin6_addr;
- goto print_ip6;
+ _7239_print_ip6(out, ip6_addr, quoted);
+ } else {
+ /* no special care needed, input is already rfc compliant,
+ * just print as regular non quoted string
+ */
+ chunk_cat(out, &smp->data.u.str);
}
- /* no special care needed, input is already rfc compliant,
- * just print as regular non quoted string
- */
- chunk_cat(out, &smp->data.u.str);
}
else if (http_7239_extract_obfs(&validate_o, NULL) &&
!istlen(validate_o)) {