Re: httpd(8) - Internal Server error (500) on invalid request
Matthias Pressfreund(m...@fn.de) on 2021.10.23 17:16:18 +0200: > On 2021-10-21 16:38, Sebastian Benoit wrote: > > > > 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. > > > I don't understand why an invalid HTTP version sent by the client > should result in a server error while the problem actually is on > the other side, isn't it? Wouldn't a "400 Bad Request" response > instead of a "505 HTTP Version Not Supported" be more appropriate? because 505 HTTP Version Not Supported means The server does not support the HTTP protocol version used in the request. I think its appropiate, nginx responds the same. > Second, the latest version correctly detects "HTTP/4.0" (for > example) as an unsupported version while "HTTP/123" wrongly is > accepted as "HTTP/1.1". i might add that indeed, thx. > > The suggestion below changes both of the above. > > Index: usr.sbin/httpd/server_http.c > === > RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v > retrieving revision 1.145 > diff -u -p -r1.145 server_http.c > --- usr.sbin/httpd/server_http.c 22 Oct 2021 08:51:50 - 1.145 > +++ usr.sbin/httpd/server_http.c 23 Oct 2021 14:39:50 - > @@ -206,8 +206,12 @@ http_version_num(char *version) > return (9); > if (strcmp(version, "HTTP/1.0") == 0) > return (10); > + /* version strings other than 8 chars long are invalid */ > + if (strlen(version) != 8) > + return (0); > /* any other version 1.x gets downgraded to 1.1 */ > - if (strncmp(version, "HTTP/1", 6) == 0) > + if (strncmp(version, "HTTP/1.", 7) == 0 && > + version[7] >= '1' && version[7] <= '9') > return (11); > > return (0); > @@ -350,13 +354,13 @@ server_read_http(struct bufferevent *bev >* 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. > + * Anything else is a client error (malformed version). >*/ > > version = http_version_num(http_version); > > if (version == 0) { > - server_abort_http(clt, 505, "bad http version"); > + server_abort_http(clt, 400, "malformed"); > goto abort; > } else if (version == 11) { > if ((desc->http_version = > > > > > > > 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 @@ intserver_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_errorhttp_errors[] = HTTP_ERRORS; > > @@ -198,6 +199,19 @@ done: > > return (ret); > > } > > > > +int http_version_num(char *version) > > +{ > > + 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_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
Re: httpd(8) - Internal Server error (500) on invalid request
On 2021-10-21 16:38, Sebastian Benoit wrote: > > 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. I don't understand why an invalid HTTP version sent by the client should result in a server error while the problem actually is on the other side, isn't it? Wouldn't a "400 Bad Request" response instead of a "505 HTTP Version Not Supported" be more appropriate? Second, the latest version correctly detects "HTTP/4.0" (for example) as an unsupported version while "HTTP/123" wrongly is accepted as "HTTP/1.1". The suggestion below changes both of the above. Index: usr.sbin/httpd/server_http.c === RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v retrieving revision 1.145 diff -u -p -r1.145 server_http.c --- usr.sbin/httpd/server_http.c22 Oct 2021 08:51:50 - 1.145 +++ usr.sbin/httpd/server_http.c23 Oct 2021 14:39:50 - @@ -206,8 +206,12 @@ http_version_num(char *version) return (9); if (strcmp(version, "HTTP/1.0") == 0) return (10); + /* version strings other than 8 chars long are invalid */ + if (strlen(version) != 8) + return (0); /* any other version 1.x gets downgraded to 1.1 */ - if (strncmp(version, "HTTP/1", 6) == 0) + if (strncmp(version, "HTTP/1.", 7) == 0 && + version[7] >= '1' && version[7] <= '9') return (11); return (0); @@ -350,13 +354,13 @@ server_read_http(struct bufferevent *bev * 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. +* Anything else is a client error (malformed version). */ version = http_version_num(http_version); if (version == 0) { - server_abort_http(clt, 505, "bad http version"); + server_abort_http(clt, 400, "malformed"); goto abort; } else if (version == 11) { if ((desc->http_version = > > 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) > +{ > + 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_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); > + if (version == 11) { > + if ((desc->http_version = > + strdup("HTTP/1.1")) == NULL) > +
Re: httpd(8) - Internal Server error (500) on invalid request
Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.10.21 17:19:02 +0200: > > + 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. Yes, this was actually the reason for the convoluted if (version.. ) bits, because server_abort_http() calls into server_close() to free desc->http_version and that is a crash when not pointing to the strduped string. Fixed. > > + 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 { > } Fixed. ok? diff --git usr.sbin/httpd/server_http.c usr.sbin/httpd/server_http.c index 6a74f3e45c5..e65a667c556 100644 --- usr.sbin/httpd/server_http.c +++ usr.sbin/httpd/server_http.c @@ -51,6 +51,7 @@ intserver_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_errorhttp_errors[] = HTTP_ERRORS; @@ -111,7 +112,7 @@ server_httpdesc_free(struct http_descriptor *desc) desc->http_query = NULL; free(desc->http_query_alias); desc->http_query_alias = NULL; - free(desc->http_version); + free(desc->http_version); //XX desc->http_version = NULL; free(desc->http_host); desc->http_host = NULL; @@ -198,6 +199,20 @@ done: return (ret); } +int +http_version_num(char *version) +{ + 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) { @@ -206,7 +221,9 @@ server_read_http(struct bufferevent *bev, void *arg) struct evbuffer *src = EVBUFFER_INPUT(bev); char*line = NULL, *key, *value; const char *errstr; + char*http_version; size_t size, linelen; + int version; struct kv *hdr = NULL; getmonotime(>clt_tv_last); @@ -317,24 +334,39 @@ server_read_http(struct bufferevent *bev, void *arg) if (desc->http_path == NULL) goto fail; - desc->http_version = strchr(desc->http_path, ' '); - if (desc->http_version == NULL) { + http_version = strchr(desc->http_path, ' '); + if (http_version == NULL) { server_abort_http(clt, 400, "malformed"); goto abort; } - *desc->http_version++ = '\0'; + *http_version++ = '\0'; desc->http_query = strchr(desc->http_path, '?'); if (desc->http_query != NULL) *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(http_version); + + if (version == 0) { +
Re: httpd(8) - Internal Server error (500) on invalid request
Hi, On 21.10.21 13:31, Claudio Jeker wrote: >> >> Hi, >> >> yes. The server should probably answer with a "Bad Request" instead. >> >> Fix below. ok? > > OK claudio@ > Thanks for the quick fix! 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. BTW: A request from me (for this list): I'm still having troubles with the OpenBSD mailing list. My DKIM key will always be removed. Don't know why. My DMARC setting is restrict. No issues with Google as example. Don't want to break or spam the list. Can somebody tell me how to fix this, please? I'm really desperate. :(
Re: httpd(8) - Internal Server error (500) on invalid request
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_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
Re: httpd(8) - Internal Server error (500) on invalid request
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 @@ intserver_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_errorhttp_errors[] = HTTP_ERRORS; @@ -198,6 +199,19 @@ done: return (ret); } +int http_version_num(char *version) +{ + 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_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); + 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; + } + if (desc->http_query != NULL && (desc->http_query =
Re: httpd(8) - Internal Server error (500) on invalid request
On 2021-10-21, J. K. wrote: > A request from me (for this list): > I'm still having troubles with the OpenBSD mailing list. My DKIM key > will always be removed. Don't know why. My DMARC setting is restrict. > No issues with Google as example. > > Don't want to break or spam the list. > > Can somebody tell me how to fix this, please? > I'm really desperate. :( DKIM signatures are stripped because they'll very often fail for emails sent via the list (e.g. in many cases the list modifies mails by converting to text/plain which breaks the set of headers that many people are signing). Except in the case of restrictive DMARC signatures, this is a slight improvement. I think the only ways around this from your side would be to either remove p=quarantine/sp=reject or use a different sender domain that doesn't have restrictive settings. The other way would be if the list config was changed to rewrite >From headers. Some other mailing lists resorted to doing this, but it's not very convenient for the normal list user.
Re: httpd(8) - Internal Server error (500) on invalid request
On Thu, Oct 21, 2021 at 01:21:33PM +0200, Sebastian Benoit wrote: > J. K.(openbsd.l...@krottmayer.com) on 2021.10.21 11:55:47 +0200: > > Hi, > > > > I don't know if this is a real issue from OpenBSD's httpd(8). > > Tried some requests to httpd(8) for the purpose of education. > > > > Simple tried the following request: > > > > $ telnet 10.42.42.183 80 > > Trying 10.42.42.183... > > Connected to 10.42.42.183. > > Escape character is '^]'. > > GET / HTTP/1.1 > > fasfsdfsfd > > > > Here without the colon httpd(8) return an internal server > > error. > > > > Can somebody verify this behavior? > > > > Noticed with OpenBSD 7.0. Is this a correct behavior (RFC > > conform)? > > > > Thanks in advance! > > > > Kind regrads, > > > > J. K. > > Hi, > > yes. The server should probably answer with a "Bad Request" instead. > > Fix below. ok? OK claudio@ > diff --git usr.sbin/httpd/server_http.c usr.sbin/httpd/server_http.c > index 732add41283..fce3c21af72 100644 > --- usr.sbin/httpd/server_http.c > +++ usr.sbin/httpd/server_http.c > @@ -268,8 +268,14 @@ server_read_http(struct bufferevent *bev, void *arg) > else if (*key == ' ' || *key == '\t') > /* Multiline headers wrap with a space or tab */ > value = NULL; > - else > + else { > + /* Not a multiline header, should have a : */ > value = strchr(key, ':'); > + if (value == NULL) { > + server_abort_http(clt, 400, "malformed"); > + goto abort; > + } > + } > if (value == NULL) { > if (clt->clt_line == 1) { > server_abort_http(clt, 400, "malformed"); > -- :wq Claudio
Re: httpd(8) - Internal Server error (500) on invalid request
J. K.(openbsd.l...@krottmayer.com) on 2021.10.21 11:55:47 +0200: > Hi, > > I don't know if this is a real issue from OpenBSD's httpd(8). > Tried some requests to httpd(8) for the purpose of education. > > Simple tried the following request: > > $ telnet 10.42.42.183 80 > Trying 10.42.42.183... > Connected to 10.42.42.183. > Escape character is '^]'. > GET / HTTP/1.1 > fasfsdfsfd > > Here without the colon httpd(8) return an internal server > error. > > Can somebody verify this behavior? > > Noticed with OpenBSD 7.0. Is this a correct behavior (RFC > conform)? > > Thanks in advance! > > Kind regrads, > > J. K. Hi, yes. The server should probably answer with a "Bad Request" instead. Fix below. ok? diff --git usr.sbin/httpd/server_http.c usr.sbin/httpd/server_http.c index 732add41283..fce3c21af72 100644 --- usr.sbin/httpd/server_http.c +++ usr.sbin/httpd/server_http.c @@ -268,8 +268,14 @@ server_read_http(struct bufferevent *bev, void *arg) else if (*key == ' ' || *key == '\t') /* Multiline headers wrap with a space or tab */ value = NULL; - else + else { + /* Not a multiline header, should have a : */ value = strchr(key, ':'); + if (value == NULL) { + server_abort_http(clt, 400, "malformed"); + goto abort; + } + } if (value == NULL) { if (clt->clt_line == 1) { server_abort_http(clt, 400, "malformed");