Re: [hackers] [quark][PATCH] HTML-escape dirlist dynamic content

From: Hiltjo Posthuma <hiltjo_AT_codemadness.org>
Date: Wed, 11 Mar 2020 22:26:57 +0100

On Wed, Mar 11, 2020 at 10:33:06PM +0200, guysv wrote:
> Because ",',<,>,& are all valid unix filename characters,
> filenames containing those characters can glitch-out a dirlist
> response.
>
> A funny example would be:
> "><img src="blabla" onerror="alert(1)"
>
> This commit escapes dynamic input, and fixes the bug.
> ---
> resp.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/resp.c b/resp.c
> index 3075c28..7b95557 100644
> --- a/resp.c
> +++ b/resp.c
> _AT_@ -1,5 +1,6 @@
> /* See LICENSE file for copyright and license details. */
> #include <dirent.h>
> +#include <limits.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> _AT_@ -38,12 +39,30 @@ suffix(int t)
> return "";
> }
>
> +void
> +htmlescape(char *src, char *dst)
> +{
> + for (; *src; src++) {
> + switch (*src) {
> + case '<': strcat(dst, "&lt"); dst += sizeof("&lt;")-1; break;
> + case '>': strcat(dst, "&gt"); dst += sizeof("&gt;")-1; break;
> + case '&': strcat(dst, "&amp"); dst += sizeof("&amp;")-1; break;
> + case '"': strcat(dst, "&quot"); dst += sizeof("&quot;")-1; break;
> + case '\'': strcat(dst, "&#39"); dst += sizeof("&#39;")-1; break;

The entities in strcat should be changed with a semi-colon too.

> + default: *dst++ = *src; break;
> + }
> + }
> + *dst = '\0';
> +}
> +
> enum status
> -resp_dir(int fd, char *name, struct request *r)
> +resp_dir(int fd, char name[PATH_MAX], struct request *r)

I'm not a fan of this style char name[PATH_MAX] and prefer to keep char *name.
It also does not make sense because resp_dir is called like this:

        return resp_dir(fd, RELPATH(realtarget), r);

RELPATH is defined as:

        #define RELPATH(x) ((!*(x) || !strcmp(x, "/")) ? "." : ((x) + 1))

> {
> struct dirent **e;
> size_t i;
> int dirlen, s;
> + /* 6 - strlen("&quot;"), largest escape */
> + char escapebuf[PATH_MAX*6];

escapebuf should be initialized (empty string), probably in htmlescape().

> static char t[TIMESTAMP_LEN];
>
> /* read directory */
> _AT_@ -64,12 +83,13 @@ resp_dir(int fd, char *name, struct request *r)
> }
>
> if (r->method == M_GET) {
> + htmlescape(name, escapebuf);
> /* listing header */
> if (dprintf(fd,
> "<!DOCTYPE html>\n<html>\n\t<head>"
> "<title>Index of %s</title></head>\n"
> "\t<body>\n\t\t<a href=\"..\">..</a>",
> - name) < 0) {
> + escapebuf) < 0) {
> s = S_REQUEST_TIMEOUT;
> goto cleanup;
> }
> _AT_@ -80,12 +100,12 @@ resp_dir(int fd, char *name, struct request *r)
> if (e[i]->d_name[0] == '.') {
> continue;
> }
> -
> + htmlescape(e[i]->d_name, escapebuf);
> /* entry line */
> if (dprintf(fd, "<br />\n\t\t<a href=\"%s%s\">%s%s</a>",
> - e[i]->d_name,
> + escapebuf,
> (e[i]->d_type == DT_DIR) ? "/" : "",
> - e[i]->d_name,
> + escapebuf,
> suffix(e[i]->d_type)) < 0) {
> s = S_REQUEST_TIMEOUT;
> goto cleanup;
> --
> 2.25.1
>
>

I think the general idea of this patch is good though.

Thanks,

-- 
Kind regards,
Hiltjo
Received on Wed Mar 11 2020 - 22:26:57 CET

This archive was generated by hypermail 2.3.0 : Wed Mar 11 2020 - 22:36:35 CET