Re: httpd(8) - Internal Server error (500) on invalid request

2021-10-23 Thread Sebastian Benoit
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

2021-10-23 Thread Matthias Pressfreund
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

2021-10-21 Thread Sebastian Benoit
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

2021-10-21 Thread J. K.
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

2021-10-21 Thread Claudio Jeker
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

2021-10-21 Thread Sebastian Benoit
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

2021-10-21 Thread Stuart Henderson
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

2021-10-21 Thread Claudio Jeker
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

2021-10-21 Thread Sebastian Benoit
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");