From: Aurelien DARRAGON Date: Mon, 30 Jan 2023 08:28:57 +0000 (+0100) Subject: BUG/MINOR: http_ext/7239: ipv6 dumping relies on out of scope variables X-Git-Tag: v2.8-dev3~56 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8436c910f55249895d933ab37f05f99789aa8979;p=thirdparty%2Fhaproxy.git 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 d3b57645b2..3875d3b748 100644 --- a/src/http_ext.c +++ b/src/http_ext.c @@ -485,12 +485,25 @@ int http_validate_7239_header(struct ist hdr, int required_steps, struct forward 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 @@ static inline void http_build_7239_header_nodename(struct buffer *out, } 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 @@ static inline void http_build_7239_header_nodename(struct buffer *out, 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 @@ static inline void http_build_7239_header_nodename(struct buffer *out, 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)) {