On Thu, Nov 11, 2021 at 04:29:58PM +0100, Otto Moerbeek wrote:
> On Thu, Nov 11, 2021 at 04:13:37PM +0100, Florian Obser wrote:
>
> >
> > No idea how to reproduce this, I'm just running an httpd with debug
> > symbols and kern.nosuidcoredump=3
> > Pretty sure this is the crash various people mumbled about.
> >
> > Smells like a use-after-fruit to me.
>
> In server_http.c:351 desc->http_query is set to point in the middle of
> a string. In the cases of goto fail belows that it will not be
> strdupped. A free of desc->http_query then later bombs.
>
I debugged something like this with tobhe@ the other day. I remember I
mentioned to benno@ to look out for this dumb pattern of first assigning a
variable and then doing variable = strdup(variable). He fixed one case but
forgot the other.
I think all this http_query handling can be moved down and while at it use
a local variable as well.
--
:wq Claudio
Index: server_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.148
diff -u -p -r1.148 server_http.c
--- server_http.c 5 Nov 2021 19:01:02 -0000 1.148
+++ server_http.c 11 Nov 2021 15:36:41 -0000
@@ -228,7 +228,7 @@ server_read_http(struct bufferevent *bev
struct evbuffer *src = EVBUFFER_INPUT(bev);
char *line = NULL, *key, *value;
const char *errstr;
- char *http_version;
+ char *http_version, *query;
size_t size, linelen;
int version;
struct kv *hdr = NULL;
@@ -348,9 +348,6 @@ server_read_http(struct bufferevent *bev
}
*http_version++ = '\0';
- desc->http_query = strchr(desc->http_path, '?');
- if (desc->http_query != NULL)
- *desc->http_query++ = '\0';
/*
* We have to allocate the strings because they could
@@ -378,10 +375,13 @@ server_read_http(struct bufferevent *bev
goto fail;
}
- if (desc->http_query != NULL &&
- (desc->http_query =
- strdup(desc->http_query)) == NULL)
- goto fail;
+ query = strchr(desc->http_path, '?');
+ if (query != NULL) {
+ *query++ = '\0';
+
+ if ((desc->http_query = strdup(query)) == NULL)
+ goto fail;
+ }
} else if (desc->http_method != HTTP_METHOD_NONE &&
strcasecmp("Content-Length", key) == 0) {
