The port_only converter looks for the last ':' (or ']') in its input and
hands everything that follows to strl2ui(), which is documented as doing
"no check at all". Every byte is accumulated as if it were a digit using
"i = i * 10 - '0' + c" on an unsigned int, so a malformed port is not
rejected but silently turned into an arbitrary number:
str(example.com:abc),port_only returns 5451
str(example.com:80abc),port_only returns 85451
str(example.com:-80),port_only returns 4294967076
str(example.com:99999999999999999999),port_only returns 1661992959
str(example.com:4294967739),port_only returns 443
The input of this converter is normally the Host header, which is under
the client's control and which the HTTP parsers accept with such a port,
over HTTP/1 as well as HTTP/2. As a result the client picks the value the
converter returns: "Host: example.com:ZG" makes it return 443, and so
does "Host: example.com:4294967739" by wrapping around on 32 bits, which
does not even require a non-digit character. A configuration matching on
the port (for example "req.hdr(host),port_only -m int eq 443") can be
fooled this way, and a port forwarded to the server (for example in an
X-Forwarded-Port header) may carry a value the client never sent. Values
above 65535 could be returned as well.
Fix this by using http_get_host_port(), which already performs the lookup
correctly by only accepting a ':' immediately followed by digits up to
the end of the input, and by converting the result with strl2irc(), which
rejects non-digits and overflows on its own, the way other ports are
already parsed in src/http_fetch.c. As before, an input without a usable
port makes the converter return 0, so an invalid port is not
distinguishable from an absent one. The documentation is updated to say
so.
The host_only converter is left untouched on purpose: it keeps stripping
everything after the last colon, so for an input such as
"example.com:http" it still returns "example.com" while port_only now
returns 0. Making the pair agree would change the result for valid inputs
and is not needed to fix this bug.
The converter was introduced in 2.7 by commit dd754cba1 ("MINOR: sample:
add the host_only and port_only converters"), so this patch should be
backported to all stable branches having it (2.8 and above).
This depends on commit 3e666065e ("BUG/MINOR: http: fix an out-of-bounds
read in http_get_host_port() on empty host"), without which an empty Host
header makes the helper read one byte past the value. That commit is
already present in 2.8.28, 3.0.27 and 3.2.23, but not in 3.1.17, where it
must be backported first.
---
Notes for reviewers, not part of the commit message:
Testing:
- built and checked on master and on the 3.2.21 release tarball, which
this patch applies to with line offsets only;
- the reg-test added by the next patch passes on both patched builds and
fails on an unpatched 3.2.21, where "example.com:http" gives 63544;
- reg-tests/converter passes as a whole (19 passed, 6 skipped for the
SSL/Lua features my build lacks, 0 failed);
- valid ports are unchanged over HTTP/1 and HTTP/2, including leading
zeroes and 65535.
Two more things I deliberately did not change, in case you would rather
see them handled differently (the host_only case is in the commit message
above):
- an invalid port and an absent port both give 0 and stay
indistinguishable. Making the converter fail instead would also change
the existing "no port" behaviour and break working configurations.
- a bracket-less IPv6 literal such as "::1" still gives 1. It is not a
valid Host header value, and the lookup involved is shared with h1.c
and http_htx.c.
doc/configuration.txt | 21 +++++++++++++++++++--
src/sample.c | 25 +++++++++++++------------
2 files changed, 32 insertions(+), 14 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 8e3bc2d..46a70d4 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -22846,9 +22846,26 @@ port_only
(rfc9110#section-7.2). It will support that kind of input: hostname,
hostname:80, 127.0.0.1, 127.0.0.1:80, [::1], [::1]:80.
- If no port were provided in the input, it will return 0.
+ If no port were provided in the input, it will return 0. This is also the
+ case when what follows the last colon is not a valid port, either because
+ there is nothing after it ("example.com:"), because it is not only made
of
+ digits ("example.com:http"), or because it does not fit in the 0..65535
+ range ("example.com:70000"). Leading zeroes are accepted. An explicit
port
+ 0 returns 0 as well, so the returned value alone does not tell a missing
+ port from an invalid one.
- See also: "host_only" converter which will return the host.
+ Example :
+ str(example.com:8080),port_only # 8080
+ str(example.com),port_only # 0
+ str([::1]:80),port_only # 80
+ str(example.com:080),port_only # 80
+ str(example.com:),port_only # 0
+ str(example.com:http),port_only # 0
+ str(example.com:70000),port_only # 0
+
+ See also: "host_only" converter which will return the host. It does not
+ perform the same validation: for "example.com:http" it returns
+ "example.com" while "port_only" returns 0.
protobuf(<field_number>[,<field_type>])
This extracts the protocol buffers message field in raw mode of an input
binary
diff --git a/src/sample.c b/src/sample.c
index 59c14b7..239e980 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -4059,21 +4059,22 @@ static int sample_conv_host_only(const struct arg
*arg_p, struct sample *smp, vo
static int sample_conv_port_only(const struct arg *arg_p, struct sample
*smp, void *private)
{
/* Working cases: hostname00, hostname00:80, 127.0.0.1, 127.0.0.1:80,
[::1], [::1]:80 */
- char *beg = smp->data.u.str.area;
- char *end = smp->data.u.str.area + smp->data.u.str.data - 1;
- char *p;
+ struct ist port;
+ int port_val;
- for (p = end; p >= beg; p--) {
- if (*p == ':' || *p == ']')
- break;
- }
+ port = http_get_host_port(ist2(smp->data.u.str.area,
smp->data.u.str.data));
smp->data.type = SMP_T_SINT;
- if (p >= beg && *p == ':' && ++p <= end) {
- smp->data.u.sint = strl2ui(p, smp->data.u.str.data + smp->data.u.str.area
- p);
- } else {
- smp->data.u.sint = 0;
- }
+ smp->data.u.sint = 0;
+
+ /* Anything that is not a valid port leaves the result at 0, just like an
+ * input that has no port at all. strl2irc() rejects non-digits and
+ * overflows by itself, so the result does not depend on what
+ * http_get_host_port() lets through.
+ */
+ if (strl2irc(istptr(port), istlen(port), &port_val) == 0 &&
+ port_val >= 0 && port_val <= 65535)
+ smp->data.u.sint = port_val;
return 1;
}
--
2.47.3
