On Thu, Oct 21, 2021 at 04:38:43PM +0200, Sebastian Benoit wrote:
> J. K.(openbsd.l...@krottmayer.com) on 2021.10.21 14:10:16 +0200:
> > Another question, to httpd(8). Tried the following query.
> > Used an invalid HTTP Version number (typo).
> >
> > $ telnet 10.42.42.183 80
> > [Shortened]
> > GET / HTTP/1.2
> > [content]
> >
> > httpd provide here the site. Without checking the not existent version
> > (1.2) number and the Host. Okay, that's maybe stupid from me to
> > start a request with an invalid version number. But should not also
> > the server answer with 400 (bad request)?
> >
> > According to the source only HTTP/1.1 is checked. All other request
> > will be accepted. Okay, I'm not a RFC specialist. Still a newbie.
>
> This diff makes httpd return "505 HTTP Version Not Supported"
> for < 0.9 and > 1.9 http versions. Anything from 1.1 to 1.9 is
> interpreted as 1.1. This is what nginx does too.
>
> ok?
>
> diff --git usr.sbin/httpd/server_http.c usr.sbin/httpd/server_http.c
> index 6a74f3e45c5..52aaf3711c2 100644
> --- usr.sbin/httpd/server_http.c
> +++ usr.sbin/httpd/server_http.c
> @@ -51,6 +51,7 @@ int server_http_authenticate(struct server_config
> *,
> struct client *);
> char *server_expand_http(struct client *, const char *,
> char *, size_t);
> +int http_version_num(char *);
>
> static struct http_method http_methods[] = HTTP_METHODS;
> static struct http_error http_errors[] = HTTP_ERRORS;
> @@ -198,6 +199,19 @@ done:
> return (ret);
> }
>
> +int http_version_num(char *version)
KNF please.
> +{
> + if (strcmp(version, "HTTP/0.9") == 0)
> + return (9);
> + if (strcmp(version, "HTTP/1.0") == 0)
> + return (10);
> + /* any other version 1.x gets downgraded to 1.1 */
> + if (strncmp(version, "HTTP/1", 6) == 0)
> + return (11);
> +
> + return (0);
> +}
> +
> void
> server_read_http(struct bufferevent *bev, void *arg)
> {
> @@ -207,6 +221,7 @@ server_read_http(struct bufferevent *bev, void *arg)
> char *line = NULL, *key, *value;
> const char *errstr;
> size_t size, linelen;
> + int version;
> struct kv *hdr = NULL;
>
> getmonotime(&clt->clt_tv_last);
> @@ -329,12 +344,29 @@ server_read_http(struct bufferevent *bev, void *arg)
> *desc->http_query++ = '\0';
>
> /*
> - * Have to allocate the strings because they could
> + * We have to allocate the strings because they could
> * be changed independently by the filters later.
> + * Allow HTTP version 0.9 to 1.1.
> + * Downgrade http version > 1.1 <= 1.9 to version 1.1.
> + * Return HTTP Version Not Supported for anything else.
> */
> - if ((desc->http_version =
> - strdup(desc->http_version)) == NULL)
> - goto fail;
> +
> + version = http_version_num(desc->http_version);
I woud prefer if this code would store the version not in
desc->http_version until after the strdup(). The way these strdup work is
just wonky. Especil in the failure cases this may result in calling free
on the wrong thing.
> + if (version == 11) {
> + if ((desc->http_version =
> + strdup("HTTP/1.1")) == NULL)
> + goto fail;
> + } else {
> + if ((desc->http_version =
> + strdup(desc->http_version)) == NULL)
> + goto fail;
> + }
> +
> + if (version == 0) {
> + server_abort_http(clt, 505, "bad http version");
> + goto abort;
> + }
I would prefer to have this as:
if (version == 0) {
} else if if (version == 11) {
} else {
}
--
:wq Claudio
