On Mon, Mar 16, 2020 at 12:55:42PM +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 | 26 +++++++++++++++++++++++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/resp.c b/resp.c
> index 3075c28..e461a5d 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,29 @@ suffix(int t)
> return "";
> }
>
> +void
> +htmlescape(char *src, char *dst)
> +{
> + for (; *src; src++) {
> + switch (*src) {
> + case '<': strcpy(dst, "<"); dst += sizeof("<")-1; break;
> + case '>': strcpy(dst, ">"); dst += sizeof(">")-1; break;
> + case '&': strcpy(dst, "&"); dst += sizeof("&")-1; break;
> + case '"': strcpy(dst, """); dst += sizeof(""")-1;
> break;
> + case '\'': strcpy(dst, "'"); dst += sizeof("'")-1;
> break;
> + default: *dst++ = *src; *dst = '\0'; break;
> + }
> + }
> +}
> +
I think the previous version was fine. There is no need to NUL terminate after
every character.
> enum status
> resp_dir(int fd, char *name, struct request *r)
> {
> struct dirent **e;
> size_t i;
> int dirlen, s;
> + /* 6 - strlen("""), largest escape */
> + char escapebuf[PATH_MAX*6];
> static char t[TIMESTAMP_LEN];
>
> /* read directory */
> @@ -64,12 +82,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;
> }
> @@ -81,11 +100,12 @@ resp_dir(int fd, char *name, struct request *r)
> 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
>
>
Thanks for the patch. Laslo now maintains the new quark and decides if the
patch can be applied in this form.
--
Kind regards,
Hiltjo
