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

Tue, 10 Mar 2020 16:42:24 -0700 

On Tue, Mar 10, 2020 at 07:52:17PM +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 | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/resp.c b/resp.c
> index 3075c28..70170bc 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,31 @@ 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;
> +             default: *dst++ = *src; break;
> +             }
> +     }
> +     *dst = '\0';
> +}
> +

I think its good practise to close HTML entities with semi-colon. The numeric
entity should be closed.
So "&amp" to "&amp;".

>  enum status
>  resp_dir(int fd, char *name, struct request *r)
>  {
>       struct dirent **e;
>       size_t i;
>       int dirlen, s;
> +     /* 5 - strlen("&quot"), largest escape */
> +     char escaped_filename[NAME_MAX*5];
> +     char escaped_dirname[PATH_MAX*5];

This buffer can be reused saving stack space.

>       static char t[TIMESTAMP_LEN];
>  
>       /* read directory */
> @@ -64,12 +84,13 @@ resp_dir(int fd, char *name, struct request *r)
>       }
>  
>       if (r->method == M_GET) {
> +             htmlescape(name, escaped_dirname);
>               /* 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) {
> +                         escaped_dirname) < 0) {
>                       s = S_REQUEST_TIMEOUT;
>                       goto cleanup;
>               }
> @@ -80,12 +101,12 @@ resp_dir(int fd, char *name, struct request *r)
>                       if (e[i]->d_name[0] == '.') {
>                               continue;
>                       }
> -
> +                     htmlescape(e[i]->d_name, escaped_filename);
>                       /* entry line */
>                       if (dprintf(fd, "<br />\n\t\t<a href=\"%s%s\">%s%s</a>",
> -                                 e[i]->d_name,
> +                                 escaped_filename,
>                                   (e[i]->d_type == DT_DIR) ? "/" : "",
> -                                 e[i]->d_name,
> +                                 escaped_filename,
>                                   suffix(e[i]->d_type)) < 0) {
>                               s = S_REQUEST_TIMEOUT;
>                               goto cleanup;
> -- 
> 2.25.1
> 
> 

-- 
Kind regards,
Hiltjo

Reply via email to