Hello,
I'm realizing that we never responded to this patch, very likely because
there are two patches titled "1/2" with the same subject and we probably
waited for 2/2 to arrive...
On Thu, Sep 10, 2026 at 08:56:46PM +0900, Haerang Kim wrote:
> 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.
I gnerally agree. I'm pretty sure that that some users employ it to
isolate a number that follows a colon and that reducing the value to
16 bits may cause problems. But the converter's name is pretty explicit,
it's called "port_only" and if one relies on it to extract something not
a port number, they're taking their own risks.
> 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,
I'm not fond of that layer inversion. If we'd change the HTTP parser layer
we'd incidentally break the port_only parser. Maybe better move that
parser to tools.c and call it get_host_port() so that it's generic, then
you can use it from both places.
> 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.
Great.
> 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.
Indeed, I didn't remember we also used zero to mark the port's absence
instead of not returning anything. That probably remains OK, though it
means we can't differentiate "example.org:0" from "example.org" despite
clearly being different :-/
> 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.
Yeah, let's not touch that one.
> 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.
Exactly. Maybe we'll add an option ("strict") to the converter so that it
doesn't return anything on an invalid value.
> - 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.
Sure but we can't decide what users use it with. Also I'm not fond of
backporting the <65536 limitation, that's a breaking change. I think
that what could be done is:
- a first patch fixing all the parsing issues (overflows and invalid
chars), marked for backport
- a second patch that also enforces the 0..65535 range, that will not
be backported and that will be part of the release notes as a
breaking change.
We could imagine a third patch adding a "strict" argument that would
make all invalid values not return anything instead of zero, including
those outside the range 0..65535, and we could even imagine that this
one would not return 1 for "::1". Maybe support another value "relaxed"
thta maintains the current behavior, then we can have another patch
changing the default from "relaxed" to "strict" for 3.5 and keeping
relaxed for earlier versions.
> 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));
Your mailer mangled spaces/tabs, better send the patch attached next
time.
Thanks!
Willy
