On 2023-06-30 10:46 +02, Omar Polo <o...@omarpolo.com> wrote:
> On 2023/06/29 23:43:25 +0200, Omar Polo <o...@omarpolo.com> wrote:
>> On 2023/06/29 19:55:52 +0200, Florian Obser <flor...@openbsd.org> wrote:
>> > I'm worried that we pass un-sanitized input through to fcgi.
>> > Of course we are passing *a lot* of un-sanitized input through to fcgi,
>> > so does this matter in the grand scheme of things?
>> > But I'd like if server_http_parsehost() enforces syntactically valid
>> > hostnames first.
>> >
>> > There is valid_domain() in usr.bin/ssh/misc.c or res_hnok in
>> > resolv.h. Which is probably fine to use since we don't care too much
>> > about portable.
>>
>> right, httpd is happily accepting any nonsense as Host. Here's an
>> adaptation of millert' valid_domain() from usr.bin/ssh/misc.c.
>> Hope noone is relying on using an empty/malformed Host :>
>
> actually this is wrong. it breaks when an ipv6 address is used as
> Host. this seems to be an issue in ssh too, although I haven't
IIRC ssh only uses valid_domain() when it already knows that it's not an
IPv6 address.
> checked if the ssh URI specification inherits the authority from
> rfc3986 as-is.
>
> here's a slightly revised version, but just for the sake of the
> discussion since its ipv6 parsing is too permissive.
>
> should I make it strictier (not done just because it's a bit long and
> definitely not fun to validate ipv6 addresses), or there's something
> else in base that I can steal fom? :)
getaddrinfo(3) with AI_NUMERICHOST.
>
> (while here I've tweaked slightly the nonsense in
> server_http_parsehost wrt ipv6 addresses and port numbers.)
>
>
> Thanks!
>
>
> diff /usr/src
> commit - 9832f5035d799ae12e9f848cff5650481b673260
> path + /usr/src
> blob - 091cc86fec946a4a9d422c04a48f5cbfae015ce0
> file + usr.sbin/httpd/server_http.c
> --- usr.sbin/httpd/server_http.c
> +++ usr.sbin/httpd/server_http.c
> @@ -831,6 +831,60 @@ char *
> return (buf);
> }
>
> +static int
> +valid_domain(char *name, const char **errstr)
> +{
> + size_t i;
> + unsigned char c, last = '\0';
> +
> + *errstr = NULL;
> +
> + if (*name == '\0') {
> + *errstr = "empty domain name";
> + return 0;
> + }
> +
> + if (name[0] == '[') {
> + for (i = 1; name[i] != '\0'; ++i) {
> + if (name[i] == ']')
> + break;
> + if (!isxdigit((unsigned char)name[i]) &&
> + name[i] != ':') {
> + *errstr = "invalid IPv6 address";
> + return 0;
> + }
> + }
> + if (name[i] != ']' || name[i + 1] != '\0') {
> + *errstr = "invalid IPv6 address";
> + return 0;
> + }
> + return 1;
> + }
> +
> + if (!isalpha((unsigned char)name[0]) &&
> + !isdigit((unsigned char)name[0])) {
> + *errstr = "domain name starts with an invalid character";
> + return 0;
> + }
> + for (i = 0; name[i] != '\0'; ++i) {
> + c = tolower((unsigned char)name[i]);
> + name[i] = c;
> + if (last == '.' && c == '.') {
> + *errstr = "domain name contains consecutive separators";
> + return 0;
> + }
> + if (c != '.' && c != '-' && !isalnum(c) &&
> + c != '_') /* technically invalid, but common */ {
> + *errstr = "domain name contains invalid characters";
> + return 0;
> + }
> + last = c;
> + }
> + if (name[i - 1] == '.')
> + name[i - 1] = '\0';
> + return 1;
> +}
> +
> char *
> server_http_parsehost(char *host, char *buf, size_t len, int *portval)
> {
> @@ -847,13 +901,11 @@ server_http_parsehost(char *host, char *buf, size_t le
>
> if (*start == '[' && (end = strchr(start, ']')) != NULL) {
> /* Address enclosed in [] with port, eg. [2001:db8::1]:80 */
> - start++;
are you forgetting to skip over the opening '['?
> - *end++ = '\0';
> - if ((port = strchr(end, ':')) == NULL || *port == '\0')
> - port = NULL;
> - else
> - port++;
> - memmove(buf, start, strlen(start) + 1);
> + end++;
> + if (*end == ':') {
> + *end++ = '\0';
> + port = end;
> + }
> } else if ((end = strchr(start, ':')) != NULL) {
> /* Name or address with port, eg. www.example.com:80 */
> *end++ = '\0';
> @@ -863,6 +915,11 @@ server_http_parsehost(char *host, char *buf, size_t le
> port = NULL;
> }
>
> + if (!valid_domain(start, &errstr)) {
> + log_debug("%s: %s: %s", __func__, errstr, host);
> + return (NULL);
> + }
> +
> if (port != NULL) {
> /* Save the requested port */
> *portval = strtonum(port, 0, 0xffff, &errstr);
>
>
--
In my defence, I have been left unsupervised.
