[dev] [quark] one byte stack overflow

[Aaron Burrow]

quark has a 1 byte NULL stack overflow triggered by an HTTP request for a
folder path not ending with '/' and having length PATH_MAX-1. Relevant
code from http.c:http_send_response()

if (S_ISDIR(st.st_mode)) {
/* add / to target if not present */
len = strlen(realtarget);
if (len == PATH_MAX - 2) {
return http_send_status(fd, S_REQUEST_TOO_LARGE);
}
if (len && realtarget[len - 1] != '/') {
realtarget[len] = '/';
realtarget[len + 1] = '\0';
}
}

The issue is mitigated by changing the second conditional to

if (len >= PATH_MAX - 2) {

There are two ways to trigger the bug. On my system, PATH_MAX = 4096 and the
default quark config has HEADER_MAX = 4096. The bug is triggered by folder
paths of length 4095. The `-m` option lets me increase the path length
despite the HEADER_MAX constraint. Something like,

# mkdir -p $(ruby -e 'puts "#{("/"+"X"*99)*40 + "/" + "X"*94}"')
# ./quark -h 127.0.0.1 -p 10000 -d / -m "foo /X $(ruby -e 'puts "/" + "X"*17')"

# curl -vvvv -X GET -H 'Host:' -H 'User-Agent:' -H 'Accept:' $(ruby -e 'puts "http://127.0.0.1:10000#{"/" + "X"*83 + ("/"+"X"*99)*39 + "/" + "X"*94}"')

Seventeen bytes are used for 'GET', 'HTTP/1.1' and whitespace. The longest
queryable path in a valid HTTP header is 4096-17=4079 bytes. The quark map
increases the path length by 16 bytes. The forward slash appending code
writes a NULL byte to realtarget[4079+16+1=4096], which is out of bounds.

Alternatively, the bug can be triggered without `-m` if the system has a
'small' PATH_MAX or the configuration has a 'large' HEADER_MAX. The sufficient
condition is,

HEADER_MAX >= (PATH_MAX - 1) + 17

I set HEADER_MAX to 8192 (allows me to leave the request headers) and triggered
the bug like this,

# mkdir -p $(ruby -e 'puts "#{("/"+"X"*99)*40 + "/" + "X"*94}"')
# ./quark -h 127.0.0.1 -p 10000 -d /

# curl -vvvv -X GET $(ruby -e 'puts "http://127.0.0.1:10000#{("/"+"X"*99)*40 + "/" + "X"*94}"')


quark version,

$ git log -n 1
commit 9ff3f780e1d1912b89727140df7c1529ab8c5146
Author: Laslo Hunhold
Date: Mon Jul 2 18:43:06 2018 +0200

Send a relative redirection header wherever possible

This makes quark much more flexible when it is run behind a network
filter or other kind of tunnel. Only send an absolute redirection when
we are handling vhosts.


PATH_MAX,

$ grep PATH_MAX /usr/include/linux/limits.h
#define PATH_MAX 4096 /* # chars in a path name including nul */


patch,


Fix one byte NULL stack overflow.

Don't append a forward slash if the length of a folder is PATH_MAX-1. This can
happen if HEADER_MAX is larger than PATH_MAX or if the `-m` option is used to
increase the path length.
---
http.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http.c b/http.c
index f0b84b1..7a801a5 100644
--- a/http.c
+++ b/http.c
@@ -430,7 +430,7 @@ http_send_response(int fd, struct request *r)
if (S_ISDIR(st.st_mode)) {
/* add / to target if not present */
len = strlen(realtarget);
- if (len == PATH_MAX - 2) {
+ if (len >= PATH_MAX - 2) {
return http_send_status(fd, S_REQUEST_TOO_LARGE);
}
if (len && realtarget[len - 1] != '/') {

[Laslo Hunhold]

On Mon, 16 Jul 2018 13:35:14 -0700
Aaron Burrow <***@charstarstar.com> wrote:

Hey Aaron,

Post by Aaron Burrow
quark has a 1 byte NULL stack overflow triggered by an HTTP request
for a folder path not ending with '/' and having length PATH_MAX-1.
Relevant code from http.c:http_send_response()
if (S_ISDIR(st.st_mode)) {
/* add / to target if not present */
len = strlen(realtarget);
if (len == PATH_MAX - 2) {
return http_send_status(fd, S_REQUEST_TOO_LARGE);
}
if (len && realtarget[len - 1] != '/') {
realtarget[len] = '/';
realtarget[len + 1] = '\0';
}
}
The issue is mitigated by changing the second conditional to
if (len >= PATH_MAX - 2) {
There are two ways to trigger the bug. On my system, PATH_MAX = 4096
and the default quark config has HEADER_MAX = 4096. The bug is
triggered by folder paths of length 4095. The `-m` option lets me
increase the path length despite the HEADER_MAX constraint.
[...]
Alternatively, the bug can be triggered without `-m` if the system
has a 'small' PATH_MAX or the configuration has a 'large' HEADER_MAX.
[...]
patch
[...]

I am genuinely impressed by the detail you gave in this mail! Very
nice write-up and report with an included fix. Very well done!

Yes, quark still needs an audit before its first release and there are
probably more bugs like this floating around, however, if more bugs
are present they'll mostly be within these "few" URL-modification and
handling sections. The HTTP parser itself is probably safe and I'd
trust it with my life to be honest.

Thank you very much for the report and sending in a fix. It's really
appreciated. I have applied[0] your patch and attributed you accordingly
in the LICENSE.
If you find any more problems, please let me know! Thanks for using and
testing quark.

With best regards

Laslo Hunhold

[0]:https://git.suckless.org/quark/commit/?id=d2013a6337972c62a71f01324e87af0e55579245