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
> @@ -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>
> @@ -38,12 +39,30 @@ suffix(int t)
> return "";
> }
>
> +void
> +htmlescape(char *src, char *dst)
> +{
> + for (; *src; src++) {
> + switch (*src) {
> + case '<': strcat(dst, "<"); dst += sizeof("<")-1; break;
> + case '>': strcat(dst, ">"); dst += sizeof(">")-1; break;
> + case '&': strcat(dst, "&"); dst += sizeof("&")-1; break;
> + case '"': strcat(dst, """); dst += sizeof(""")-1;
> break;
> + case '\'': strcat(dst, "'"); dst += sizeof("'")-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("""), largest escape */
> + char escapebuf[PATH_MAX*6];
escapebuf should be initialized (empty string), probably in htmlescape().
> static char t[TIMESTAMP_LEN];
>
> /* read directory */
> @@ -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;
> }
> @@ -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
