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
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? :)
(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++;
- *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);
