httpd server configuration evaluation bug

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

httpd server configuration evaluation bug

Base Pr1me
Hi,

I discovered that the wrong server configuration is evaluated in the
server_read_http function. Only the first server in httpd.conf is checked. For
example, I have five servers setup in httpd.conf and the third server is the
only one with connection { max request body ####} set, because I desire it to
accept larger uploads than the other servers. When the upload is initiated,
server one dictates the max request body size, globally.

The attached diff moves the queue loop out of the server_response function in to
its own function, as to not duplicate code.

I don't know if this is the only place the wrong information is evaluated. Also,
I'm not sure this is the best method to fix the problem, but it should point the
powers that be in the right direction.

Thanks,

Tracey


httpd.diff (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: httpd server configuration evaluation bug

Base Pr1me
Sorry, this time with the correct diff.

On 7/25/18 4:15 PM, Base Pr1me wrote:

> Hi,
>
> I discovered that the wrong server configuration is evaluated in the
> server_read_http function. Only the first server in httpd.conf is checked. For
> example, I have five servers setup in httpd.conf and the third server is the
> only one with connection { max request body ####} set, because I desire it to
> accept larger uploads than the other servers. When the upload is initiated,
> server one dictates the max request body size, globally.
>
> The attached diff moves the queue loop out of the server_response function in to
> its own function, as to not duplicate code.
>
> I don't know if this is the only place the wrong information is evaluated. Also,
> I'm not sure this is the best method to fix the problem, but it should point the
> powers that be in the right direction.
>
> Thanks,
>
> Tracey
>


httpd.cvs.diff (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: httpd server configuration evaluation bug

Tracey Emery
On Mon, Jul 30, 2018 at 10:24:03AM -0600, Base Pr1me wrote:

> Sorry, this time with the correct diff.
>
> On 7/25/18 4:15 PM, Base Pr1me wrote:
> > Hi,
> >
> > I discovered that the wrong server configuration is evaluated in the
> > server_read_http function. Only the first server in httpd.conf is checked. For
> > example, I have five servers setup in httpd.conf and the third server is the
> > only one with connection { max request body ####} set, because I desire it to
> > accept larger uploads than the other servers. When the upload is initiated,
> > server one dictates the max request body size, globally.
> >
> > The attached diff moves the queue loop out of the server_response function in to
> > its own function, as to not duplicate code.
> >
> > I don't know if this is the only place the wrong information is evaluated. Also,
> > I'm not sure this is the best method to fix the problem, but it should point the
> > powers that be in the right direction.
> >
> > Thanks,
> >
> > Tracey
> >
>
Hello,

I reworked the last sent diff. I missed fixing up the hostname. This was causing
an incorrect 301 on urls not containing an ending slash. I also moved the
srv_conf assignment into the new function.

Again, this is to use the correct server config information from the queue for
server_read_http and remove code duplication.

If anyone is willing to look at this and make suggestions, that'd be great. If
not, that'd be great too! LOL. Have a great weekend.

Thanks,
Tracey

Index: src/usr.sbin/httpd/httpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
retrieving revision 1.142
diff -u -p -u -r1.142 httpd.h
--- src/usr.sbin/httpd/httpd.h 11 Oct 2018 09:52:22 -0000 1.142
+++ src/usr.sbin/httpd/httpd.h 26 Oct 2018 20:52:26 -0000
@@ -691,6 +691,8 @@ const char *
 char *server_http_parsehost(char *, char *, size_t, int *);
 ssize_t server_http_time(time_t, char *, size_t);
 int server_log_http(struct client *, unsigned int, size_t);
+int server_check_client_config(struct server_config *, struct client *,
+    struct kv *, char *);
 
 /* server_file.c */
 int server_file(struct httpd *, struct client *);
Index: src/usr.sbin/httpd/server_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.126
diff -u -p -u -r1.126 server_http.c
--- src/usr.sbin/httpd/server_http.c 15 Oct 2018 08:16:17 -0000 1.126
+++ src/usr.sbin/httpd/server_http.c 26 Oct 2018 20:52:26 -0000
@@ -204,7 +204,7 @@ server_read_http(struct bufferevent *bev
  char *line = NULL, *key, *value;
  const char *errstr;
  size_t size, linelen;
- struct kv *hdr = NULL;
+ struct kv *hdr = NULL, kv_key, *host;
 
  getmonotime(&clt->clt_tv_last);
 
@@ -344,6 +344,15 @@ server_read_http(struct bufferevent *bev
  goto abort;
  }
 
+ kv_key.kv_key = "Host";
+ if ((host = kv_find(&desc->http_headers, &kv_key)) !=
+    NULL && host->kv_value == NULL)
+ host = NULL;
+
+ if (server_check_client_config(srv_conf, clt, host,
+    NULL))
+ goto fail;
+
  /*
  * Need to read data from the client after the
  * HTTP header.
@@ -1183,10 +1192,7 @@ server_response(struct httpd *httpd, str
  struct server *srv = clt->clt_srv;
  struct server_config *srv_conf = &srv->srv_conf;
  struct kv *kv, key, *host;
- struct str_find sm;
- int portval = -1, ret;
- char *hostval, *query;
- const char *errstr = NULL;
+ char *query;
 
  /* Decode the URL */
  if (desc->http_path == NULL ||
@@ -1234,58 +1240,8 @@ server_response(struct httpd *httpd, str
  if (clt->clt_pipelining && clt->clt_toread > 0)
  clt->clt_persist = 0;
 
- /*
- * Do we have a Host header and matching configuration?
- * XXX the Host can also appear in the URL path.
- */
- if (host != NULL) {
- if ((hostval = server_http_parsehost(host->kv_value,
-    hostname, sizeof(hostname), &portval)) == NULL)
- goto fail;
-
- TAILQ_FOREACH(srv_conf, &srv->srv_hosts, entry) {
-#ifdef DEBUG
- if ((srv_conf->flags & SRVFLAG_LOCATION) == 0) {
- DPRINTF("%s: virtual host \"%s:%u\""
-    " host \"%s\" (\"%s\")",
-    __func__, srv_conf->name,
-    ntohs(srv_conf->port), host->kv_value,
-    hostname);
- }
-#endif
- if (srv_conf->flags & SRVFLAG_LOCATION)
- continue;
- else if (srv_conf->flags & SRVFLAG_SERVER_MATCH) {
- str_find(hostname, srv_conf->name,
-    &sm, 1, &errstr);
- ret = errstr == NULL ? 0 : -1;
- } else {
- ret = fnmatch(srv_conf->name,
-    hostname, FNM_CASEFOLD);
- }
- if (ret == 0 &&
-    (portval == -1 ||
-    (portval != -1 && portval == srv_conf->port))) {
- /* Replace host configuration */
- clt->clt_srv_conf = srv_conf;
- srv_conf = NULL;
- break;
- }
- }
- }
-
- if (srv_conf != NULL) {
- /* Use the actual server IP address */
- if (server_http_host(&clt->clt_srv_ss, hostname,
-    sizeof(hostname)) == NULL)
- goto fail;
- } else {
- /* Host header was valid and found */
- if (strlcpy(hostname, host->kv_value, sizeof(hostname)) >=
-    sizeof(hostname))
- goto fail;
- srv_conf = clt->clt_srv_conf;
- }
+ if (server_check_client_config(srv_conf, clt, host, hostname))
+ goto fail;
 
  if ((desc->http_host = strdup(hostname)) == NULL)
  goto fail;
@@ -1764,4 +1720,69 @@ done:
  free(agent_v);
 
  return (ret);
+}
+
+int
+server_check_client_config(struct server_config *srv_conf, struct client *clt,
+    struct kv *host, char *hostname)
+{
+ struct server           *srv = clt->clt_srv;
+ struct str_find          sm;
+ size_t len = HOST_NAME_MAX+1;
+ int portval = -1, ret;
+ char                    *hostval;
+ const char              *errstr = NULL;
+
+ /*
+ * Do we have a Host header and matching configuration?
+ * XXX the Host can also appear in the URL path.
+ */
+ if (host != NULL) {
+ if ((hostval = server_http_parsehost(host->kv_value,
+    hostname, len, &portval)) == NULL)
+ return(-1);
+
+ TAILQ_FOREACH(srv_conf, &srv->srv_hosts, entry) {
+#ifdef DEBUG
+ if ((srv_conf->flags & SRVFLAG_LOCATION) == 0) {
+ DPRINTF("%s: virtual host \"%s:%u\""
+    " host \"%s\" (\"%s\")",
+    __func__, srv_conf->name,
+    ntohs(srv_conf->port), host->kv_value,
+    hostname);
+ }
+#endif
+ if (srv_conf->flags & SRVFLAG_LOCATION)
+ continue;
+ else if (srv_conf->flags & SRVFLAG_SERVER_MATCH) {
+ str_find(hostname, srv_conf->name,
+    &sm, 1, &errstr);
+ ret = errstr == NULL ? 0 : -1;
+ } else {
+ ret = fnmatch(srv_conf->name,
+    hostname, FNM_CASEFOLD);
+ }
+ if (ret == 0 &&
+    (portval == -1 ||
+    (portval != -1 && portval == srv_conf->port))) {
+ /* Replace host configuration */
+ clt->clt_srv_conf = srv_conf;
+ srv_conf = NULL;
+ break;
+ }
+ }
+ }
+
+ if (srv_conf != NULL) {
+ /* Use the actual server IP address */
+ if (server_http_host(&clt->clt_srv_ss, hostname,
+    len) == NULL)
+ return(-1);
+ } else {
+ /* Host header was valid and found */
+ if (strlcpy(hostname, host->kv_value, len) >= len)
+ return(-1);
+ srv_conf = clt->clt_srv_conf;
+ }
+ return(0);
 }

httpd.diff (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: httpd server configuration evaluation bug

Florian Obser-2
On Fri, Oct 26, 2018 at 03:08:11PM -0600, Tracey Emery wrote:

> On Mon, Jul 30, 2018 at 10:24:03AM -0600, Base Pr1me wrote:
> > Sorry, this time with the correct diff.
> >
> > On 7/25/18 4:15 PM, Base Pr1me wrote:
> > > Hi,
> > >
> > > I discovered that the wrong server configuration is evaluated in the
> > > server_read_http function. Only the first server in httpd.conf is checked. For
> > > example, I have five servers setup in httpd.conf and the third server is the
> > > only one with connection { max request body ####} set, because I desire it to
> > > accept larger uploads than the other servers. When the upload is initiated,
> > > server one dictates the max request body size, globally.
> > >
> > > The attached diff moves the queue loop out of the server_response function in to
> > > its own function, as to not duplicate code.
> > >
> > > I don't know if this is the only place the wrong information is evaluated. Also,
> > > I'm not sure this is the best method to fix the problem, but it should point the
> > > powers that be in the right direction.
> > >
> > > Thanks,
> > >
> > > Tracey
> > >
> >
>
> Hello,
>
> I reworked the last sent diff. I missed fixing up the hostname. This was causing
> an incorrect 301 on urls not containing an ending slash. I also moved the
> srv_conf assignment into the new function.
>
> Again, this is to use the correct server config information from the queue for
> server_read_http and remove code duplication.
>
> If anyone is willing to look at this and make suggestions, that'd be great. If
> not, that'd be great too! LOL. Have a great weekend.
>
> Thanks,
> Tracey

(moving back from bugs@)

Sorry for slacking of on this for so long and thanks for prodding
patiently :)

The issue is that in server_read_http we haven't found the correct
virtual server & location yet.
clt->clt_srv_conf contains the server_config struct for the listening IP.

But there is no need to check maxrequestbody just yet, we can do it in
server_response() where we do know the virtual host.

Tracey, can you please test this, it should solve your problem.

OK?

diff --git server_http.c server_http.c
index e05cec56dfc..52698a66b2e 100644
--- server_http.c
+++ server_http.c
@@ -198,7 +198,6 @@ void
 server_read_http(struct bufferevent *bev, void *arg)
 {
  struct client *clt = arg;
- struct server_config *srv_conf = clt->clt_srv_conf;
  struct http_descriptor *desc = clt->clt_descreq;
  struct evbuffer *src = EVBUFFER_INPUT(bev);
  char *line = NULL, *key, *value;
@@ -357,11 +356,6 @@ server_read_http(struct bufferevent *bev, void *arg)
  server_abort_http(clt, 500, errstr);
  goto abort;
  }
- if ((size_t)clt->clt_toread >
-    srv_conf->maxrequestbody) {
- server_abort_http(clt, 413, NULL);
- goto abort;
- }
  }
 
  if (strcasecmp("Transfer-Encoding", key) == 0 &&
@@ -1334,6 +1328,12 @@ server_response(struct httpd *httpd, struct client *clt)
  srv_conf = server_getlocation(clt, desc->http_path);
  }
 
+ if (clt->clt_toread > 0 && (size_t)clt->clt_toread >
+    srv_conf->maxrequestbody) {
+ server_abort_http(clt, 413, NULL);
+ return (-1);
+ }
+
  if (srv_conf->flags & SRVFLAG_BLOCK) {
  server_abort_http(clt, srv_conf->return_code,
     srv_conf->return_uri);



--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: httpd server configuration evaluation bug

Tracey Emery
On Fri, Nov 30, 2018 at 10:16:45AM +0100, Florian Obser wrote:

> On Fri, Oct 26, 2018 at 03:08:11PM -0600, Tracey Emery wrote:
> > On Mon, Jul 30, 2018 at 10:24:03AM -0600, Base Pr1me wrote:
> > > Sorry, this time with the correct diff.
> > >
> > > On 7/25/18 4:15 PM, Base Pr1me wrote:
> > > > Hi,
> > > >
> > > > I discovered that the wrong server configuration is evaluated in the
> > > > server_read_http function. Only the first server in httpd.conf is checked. For
> > > > example, I have five servers setup in httpd.conf and the third server is the
> > > > only one with connection { max request body ####} set, because I desire it to
> > > > accept larger uploads than the other servers. When the upload is initiated,
> > > > server one dictates the max request body size, globally.
> > > >
> > > > The attached diff moves the queue loop out of the server_response function in to
> > > > its own function, as to not duplicate code.
> > > >
> > > > I don't know if this is the only place the wrong information is evaluated. Also,
> > > > I'm not sure this is the best method to fix the problem, but it should point the
> > > > powers that be in the right direction.
> > > >
> > > > Thanks,
> > > >
> > > > Tracey
> > > >
> > >
> >
> > Hello,
> >
> > I reworked the last sent diff. I missed fixing up the hostname. This was causing
> > an incorrect 301 on urls not containing an ending slash. I also moved the
> > srv_conf assignment into the new function.
> >
> > Again, this is to use the correct server config information from the queue for
> > server_read_http and remove code duplication.
> >
> > If anyone is willing to look at this and make suggestions, that'd be great. If
> > not, that'd be great too! LOL. Have a great weekend.
> >
> > Thanks,
> > Tracey
>
> (moving back from bugs@)
>
> Sorry for slacking of on this for so long and thanks for prodding
> patiently :)
>
> The issue is that in server_read_http we haven't found the correct
> virtual server & location yet.
> clt->clt_srv_conf contains the server_config struct for the listening IP.
>
> But there is no need to check maxrequestbody just yet, we can do it in
> server_response() where we do know the virtual host.
>
> Tracey, can you please test this, it should solve your problem.
>
> OK?
>
> diff --git server_http.c server_http.c
> index e05cec56dfc..52698a66b2e 100644
> --- server_http.c
> +++ server_http.c
> @@ -198,7 +198,6 @@ void
>  server_read_http(struct bufferevent *bev, void *arg)
>  {
>   struct client *clt = arg;
> - struct server_config *srv_conf = clt->clt_srv_conf;
>   struct http_descriptor *desc = clt->clt_descreq;
>   struct evbuffer *src = EVBUFFER_INPUT(bev);
>   char *line = NULL, *key, *value;
> @@ -357,11 +356,6 @@ server_read_http(struct bufferevent *bev, void *arg)
>   server_abort_http(clt, 500, errstr);
>   goto abort;
>   }
> - if ((size_t)clt->clt_toread >
> -    srv_conf->maxrequestbody) {
> - server_abort_http(clt, 413, NULL);
> - goto abort;
> - }
>   }
>  
>   if (strcasecmp("Transfer-Encoding", key) == 0 &&
> @@ -1334,6 +1328,12 @@ server_response(struct httpd *httpd, struct client *clt)
>   srv_conf = server_getlocation(clt, desc->http_path);
>   }
>  
> + if (clt->clt_toread > 0 && (size_t)clt->clt_toread >
> +    srv_conf->maxrequestbody) {
> + server_abort_http(clt, 413, NULL);
> + return (-1);
> + }
> +
>   if (srv_conf->flags & SRVFLAG_BLOCK) {
>   server_abort_http(clt, srv_conf->return_code,
>      srv_conf->return_uri);
>
>
>
> --
> I'm not entirely sure you are real.

Thank you, sir. That does, indeed, fix the maxrequestbody issue.

As an aside, when doing verbose debugging, the log still shows the first virtual
server when doing the upload. It's not as an important issue as the request
body, but certainly something to be aware of when you have the time to look.
Naturally, I've changed domain names below! ;)

www.upload.vs 192.168.1.2 - - [30/Nov/2018:08:01:40 -0700] "GET /support/file_upload.php?iss_id=917 HTTP/1.1" 200 0
www.upload.vs 192.168.1.2 - - [30/Nov/2018:08:01:47 -0700] "POST /support/ajax/upload.php?file=dropfile HTTP/1.1" 200 0
# wrong virt serv #
server first_vs_in_httpd.conf, client 1 (1 active), 192.168.1.2:15244 -> 10.0.0.10:443, closed
www.upload.vs 192.168.1.2 - - [30/Nov/2018:08:01:51 -0700] "POST /support/file_upload.php HTTP/1.1" 200 0
www.upload.v 192.168.1.2 - - [30/Nov/2018:08:01:53 -0700] "GET /support/view.php?id=917 HTTP/1.1" 200 0

Thanks,
Tracey

Reply | Threaded
Open this post in threaded view
|

Re: httpd server configuration evaluation bug

Florian Obser-2
In reply to this post by Florian Obser-2
anyone?

On Fri, Nov 30, 2018 at 10:16:45AM +0100, Florian Obser wrote:

> On Fri, Oct 26, 2018 at 03:08:11PM -0600, Tracey Emery wrote:
> > On Mon, Jul 30, 2018 at 10:24:03AM -0600, Base Pr1me wrote:
> > > Sorry, this time with the correct diff.
> > >
> > > On 7/25/18 4:15 PM, Base Pr1me wrote:
> > > > Hi,
> > > >
> > > > I discovered that the wrong server configuration is evaluated in the
> > > > server_read_http function. Only the first server in httpd.conf is checked. For
> > > > example, I have five servers setup in httpd.conf and the third server is the
> > > > only one with connection { max request body ####} set, because I desire it to
> > > > accept larger uploads than the other servers. When the upload is initiated,
> > > > server one dictates the max request body size, globally.
> > > >
> > > > The attached diff moves the queue loop out of the server_response function in to
> > > > its own function, as to not duplicate code.
> > > >
> > > > I don't know if this is the only place the wrong information is evaluated. Also,
> > > > I'm not sure this is the best method to fix the problem, but it should point the
> > > > powers that be in the right direction.
> > > >
> > > > Thanks,
> > > >
> > > > Tracey
> > > >
> > >
> >
> > Hello,
> >
> > I reworked the last sent diff. I missed fixing up the hostname. This was causing
> > an incorrect 301 on urls not containing an ending slash. I also moved the
> > srv_conf assignment into the new function.
> >
> > Again, this is to use the correct server config information from the queue for
> > server_read_http and remove code duplication.
> >
> > If anyone is willing to look at this and make suggestions, that'd be great. If
> > not, that'd be great too! LOL. Have a great weekend.
> >
> > Thanks,
> > Tracey
>
> (moving back from bugs@)
>
> Sorry for slacking of on this for so long and thanks for prodding
> patiently :)
>
> The issue is that in server_read_http we haven't found the correct
> virtual server & location yet.
> clt->clt_srv_conf contains the server_config struct for the listening IP.
>
> But there is no need to check maxrequestbody just yet, we can do it in
> server_response() where we do know the virtual host.
>
> Tracey, can you please test this, it should solve your problem.
>
> OK?
>
> diff --git server_http.c server_http.c
> index e05cec56dfc..52698a66b2e 100644
> --- server_http.c
> +++ server_http.c
> @@ -198,7 +198,6 @@ void
>  server_read_http(struct bufferevent *bev, void *arg)
>  {
>   struct client *clt = arg;
> - struct server_config *srv_conf = clt->clt_srv_conf;
>   struct http_descriptor *desc = clt->clt_descreq;
>   struct evbuffer *src = EVBUFFER_INPUT(bev);
>   char *line = NULL, *key, *value;
> @@ -357,11 +356,6 @@ server_read_http(struct bufferevent *bev, void *arg)
>   server_abort_http(clt, 500, errstr);
>   goto abort;
>   }
> - if ((size_t)clt->clt_toread >
> -    srv_conf->maxrequestbody) {
> - server_abort_http(clt, 413, NULL);
> - goto abort;
> - }
>   }
>  
>   if (strcasecmp("Transfer-Encoding", key) == 0 &&
> @@ -1334,6 +1328,12 @@ server_response(struct httpd *httpd, struct client *clt)
>   srv_conf = server_getlocation(clt, desc->http_path);
>   }
>  
> + if (clt->clt_toread > 0 && (size_t)clt->clt_toread >
> +    srv_conf->maxrequestbody) {
> + server_abort_http(clt, 413, NULL);
> + return (-1);
> + }
> +
>   if (srv_conf->flags & SRVFLAG_BLOCK) {
>   server_abort_http(clt, srv_conf->return_code,
>      srv_conf->return_uri);
>
>
>
> --
> I'm not entirely sure you are real.
>

--
I'm not entirely sure you are real.