Merge pull request #2284 from cesanta/enhance-mg_remove_double_dots

enhanced path sanitizing

examples/file-upload-multiple-posts/main.c

@@ -12,11 +12,13 @@ static void cb(struct mg_connection *c, int ev, void *ev_data, void *fn_data) {
     if (mg_http_match_uri(hm, "/upload")) {
       char path[80], name[64];
       mg_http_get_var(&hm->query, "name", name, sizeof(name));
+      mg_snprintf(path, sizeof(path), "/tmp/%s", name);
       if (name[0] == '\0') {
         mg_http_reply(c, 400, "", "%s", "name required");
+      } else if (!mg_path_is_sane(path)) {
+        mg_http_reply(c, 400, "", "%s", "invalid path");
       } else {
-        mg_snprintf(path, sizeof(path), "/tmp/%s", name);
-        mg_http_upload(c, hm, &mg_fs_posix, mg_remove_double_dots(path), 99999);
+        mg_http_upload(c, hm, &mg_fs_posix, path, 99999);
       }
     } else {
       struct mg_http_serve_opts opts = {.root_dir = "web_root"};

mongoose.c

@@ -1845,6 +1845,7 @@ struct printdirentrydata {
   const char *dir;
 };
 
+#if MG_ENABLE_DIRLIST
 static void printdirentry(const char *name, void *userdata) {
   struct printdirentrydata *d = (struct printdirentrydata *) userdata;
   struct mg_fs *fs = d->opts->fs == NULL ? &mg_fs_posix : d->opts->fs;
@@ -1951,6 +1952,7 @@ static void listdir(struct mg_connection *c, struct mg_http_message *hm,
   memcpy(c->send.buf + off - 12, tmp, n);  // Set content length
   c->is_resp = 0;                          // Mark response end
 }
+#endif
 
 // Resolve requested file into `path` and return its fs->st() result
 static int uri_to_path2(struct mg_connection *c, struct mg_http_message *hm,
@@ -1959,13 +1961,20 @@ static int uri_to_path2(struct mg_connection *c, struct mg_http_message *hm,
   int flags, tmp;
   // Append URI to the root_dir, and sanitize it
   size_t n = mg_snprintf(path, path_size, "%.*s", (int) dir.len, dir.ptr);
-  if (n > path_size) n = path_size;
+  if (n > path_size) {
+    mg_http_reply(c, 400, "", "Exceeded path size");
+    return -1;
+  }
   path[path_size - 1] = '\0';
-  if (n + 2 < path_size) path[n++] = '/', path[n] = '\0';
+  // Terminate root dir with /
+  if (n + 2 < path_size && path[n-1] != '/') path[n++] = '/', path[n] = '\0';
   mg_url_decode(hm->uri.ptr + url.len, hm->uri.len - url.len, path + n,
                 path_size - n, 0);
   path[path_size - 1] = '\0';  // Double-check
-  mg_remove_double_dots(path);
+  if (!mg_path_is_sane(path)) {
+    mg_http_reply(c, 400, "", "Invalid path");
+    return -1;
+  }
   n = strlen(path);
   while (n > 1 && path[n - 1] == '/') path[--n] = 0;  // Trim trailing slashes
   flags = mg_vcmp(&hm->uri, "/") == 0 ? MG_FS_DIR : fs->st(path, NULL, NULL);
@@ -2025,7 +2034,11 @@ void mg_http_serve_dir(struct mg_connection *c, struct mg_http_message *hm,
   if (flags < 0) {
     // Do nothing: the response has already been sent by uri_to_path()
   } else if (flags & MG_FS_DIR) {
+#if MG_ENABLE_DIRLIST
     listdir(c, hm, opts, path);
+#else
+    mg_http_reply(c, 403, "", "Forbidden\n");
+#endif
   } else if (flags && sp != NULL &&
              mg_globmatch(sp, strlen(sp), path, strlen(path))) {
     mg_http_serve_ssi(c, opts->root_dir, path);
@@ -5405,25 +5418,14 @@ void mg_unhex(const char *buf, size_t len, unsigned char *to) {
   }
 }
 
-char *mg_remove_double_dots(char *s) {
-  char *saved = s, *p = s;
-  while (*s != '\0') {
-    *p++ = *s++;
-    if (s[-1] == '/' || s[-1] == '\\') {
-      while (s[0] != '\0') {
-        if (s[0] == '/' || s[0] == '\\') {
-          s++;
-        } else if (s[0] == '.' && s[1] == '.' &&
-                   (s[2] == '/' || s[2] == '\\')) {
-          s += 2;
-        } else {
-          break;
-        }
-      }
+bool mg_path_is_sane(const char *path) {
+  const char *s = path;
+  for (; s[0] != '\0'; s++) {
+    if (s == path || s[0] == '/' || s[0] == '\\') {  // Subdir?
+      if (s[1] == '.' && s[2] == '.') return false;  // Starts with ..
     }
   }
-  *p = '\0';
-  return saved;
+  return true;
 }
 
 #ifdef MG_ENABLE_LINES

mongoose.h

@@ -256,30 +256,30 @@ static inline int mg_mkdir(const char *path, mode_t mode) {
 #include <pico/stdlib.h>
 int mkdir(const char *, mode_t);
 #endif
-
-
-#if MG_ARCH == MG_ARCH_RTTHREAD
-
-#include <rtthread.h>
-#include <ctype.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <sys/socket.h>
-#include <sys/select.h>
-#include <stdarg.h>
-#include <stdbool.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/types.h>
-#include <time.h>
-
-#ifndef MG_IO_SIZE
-#define MG_IO_SIZE 1460
-#endif
-
-#endif // MG_ARCH == MG_ARCH_RTTHREAD
+
+
+#if MG_ARCH == MG_ARCH_RTTHREAD
+
+#include <rtthread.h>
+#include <ctype.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/socket.h>
+#include <sys/select.h>
+#include <stdarg.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+#include <time.h>
+
+#ifndef MG_IO_SIZE
+#define MG_IO_SIZE 1460
+#endif
+
+#endif // MG_ARCH == MG_ARCH_RTTHREAD
 
 
 #if MG_ARCH == MG_ARCH_ARMCC || MG_ARCH == MG_ARCH_CMSIS_RTOS1 || \
@@ -859,7 +859,7 @@ char *mg_hex(const void *buf, size_t len, char *dst);
 void mg_unhex(const char *buf, size_t len, unsigned char *to);
 unsigned long mg_unhexn(const char *s, size_t len);
 int mg_check_ip_acl(struct mg_str acl, uint32_t remote_ip);
-char *mg_remove_double_dots(char *s);
+bool mg_path_is_sane(const char *path);
 
 
 

src/http.c

@@ -613,6 +613,7 @@ struct printdirentrydata {
   const char *dir;
 };
 
+#if MG_ENABLE_DIRLIST
 static void printdirentry(const char *name, void *userdata) {
   struct printdirentrydata *d = (struct printdirentrydata *) userdata;
   struct mg_fs *fs = d->opts->fs == NULL ? &mg_fs_posix : d->opts->fs;
@@ -719,6 +720,7 @@ static void listdir(struct mg_connection *c, struct mg_http_message *hm,
   memcpy(c->send.buf + off - 12, tmp, n);  // Set content length
   c->is_resp = 0;                          // Mark response end
 }
+#endif
 
 // Resolve requested file into `path` and return its fs->st() result
 static int uri_to_path2(struct mg_connection *c, struct mg_http_message *hm,
@@ -727,13 +729,20 @@ static int uri_to_path2(struct mg_connection *c, struct mg_http_message *hm,
   int flags, tmp;
   // Append URI to the root_dir, and sanitize it
   size_t n = mg_snprintf(path, path_size, "%.*s", (int) dir.len, dir.ptr);
-  if (n > path_size) n = path_size;
+  if (n > path_size) {
+    mg_http_reply(c, 400, "", "Exceeded path size");
+    return -1;
+  }
   path[path_size - 1] = '\0';
-  if (n + 2 < path_size) path[n++] = '/', path[n] = '\0';
+  // Terminate root dir with /
+  if (n + 2 < path_size && path[n-1] != '/') path[n++] = '/', path[n] = '\0';
   mg_url_decode(hm->uri.ptr + url.len, hm->uri.len - url.len, path + n,
                 path_size - n, 0);
   path[path_size - 1] = '\0';  // Double-check
-  mg_remove_double_dots(path);
+  if (!mg_path_is_sane(path)) {
+    mg_http_reply(c, 400, "", "Invalid path");
+    return -1;
+  }
   n = strlen(path);
   while (n > 1 && path[n - 1] == '/') path[--n] = 0;  // Trim trailing slashes
   flags = mg_vcmp(&hm->uri, "/") == 0 ? MG_FS_DIR : fs->st(path, NULL, NULL);
@@ -793,7 +802,11 @@ void mg_http_serve_dir(struct mg_connection *c, struct mg_http_message *hm,
   if (flags < 0) {
     // Do nothing: the response has already been sent by uri_to_path()
   } else if (flags & MG_FS_DIR) {
+#if MG_ENABLE_DIRLIST
     listdir(c, hm, opts, path);
+#else
+    mg_http_reply(c, 403, "", "Forbidden\n");
+#endif
   } else if (flags && sp != NULL &&
              mg_globmatch(sp, strlen(sp), path, strlen(path))) {
     mg_http_serve_ssi(c, opts->root_dir, path);

src/str.c

@@ -187,23 +187,12 @@ void mg_unhex(const char *buf, size_t len, unsigned char *to) {
   }
 }
 
-char *mg_remove_double_dots(char *s) {
-  char *saved = s, *p = s;
-  while (*s != '\0') {
-    *p++ = *s++;
-    if (s[-1] == '/' || s[-1] == '\\') {
-      while (s[0] != '\0') {
-        if (s[0] == '/' || s[0] == '\\') {
-          s++;
-        } else if (s[0] == '.' && s[1] == '.' &&
-                   (s[2] == '/' || s[2] == '\\')) {
-          s += 2;
-        } else {
-          break;
-        }
-      }
+bool mg_path_is_sane(const char *path) {
+  const char *s = path;
+  for (; s[0] != '\0'; s++) {
+    if (s == path || s[0] == '/' || s[0] == '\\') {  // Subdir?
+      if (s[1] == '.' && s[2] == '.') return false;  // Starts with ..
     }
   }
-  *p = '\0';
-  return saved;
+  return true;
 }

src/str.h

@@ -35,4 +35,4 @@ char *mg_hex(const void *buf, size_t len, char *dst);
 void mg_unhex(const char *buf, size_t len, unsigned char *to);
 unsigned long mg_unhexn(const char *s, size_t len);
 int mg_check_ip_acl(struct mg_str acl, uint32_t remote_ip);
-char *mg_remove_double_dots(char *s);
+bool mg_path_is_sane(const char *path);

test/unit_test.c

@@ -643,11 +643,13 @@ static void eh1(struct mg_connection *c, int ev, void *ev_data, void *fn_data) {
     } else if (mg_http_match_uri(hm, "/upload")) {
       char path[80], name[64];
       mg_http_get_var(&hm->query, "name", name, sizeof(name));
+      mg_snprintf(path, sizeof(path), "./%s", name);
       if (name[0] == '\0') {
         mg_http_reply(c, 400, "", "%s", "name required");
+      } else if (!mg_path_is_sane(path)) {
+        mg_http_reply(c, 400, "", "%s", "invalid path");
       } else {
-        mg_snprintf(path, sizeof(path), "./%s", name);
-        mg_http_upload(c, hm, &mg_fs_posix, mg_remove_double_dots(path), 99999);
+        mg_http_upload(c, hm, &mg_fs_posix, path, 99999);
         c->is_hexdumping = 1;
       }
     } else if (mg_http_match_uri(hm, "/test/")) {
@@ -854,11 +856,11 @@ static void test_http_server(void) {
   ASSERT(fetch(&mgr, buf, url, "GET /київ.txt HTTP/1.0\n\n") == 200);
   ASSERT(cmpbody(buf, "є\n") == 0);
 
-  ASSERT(fetch(&mgr, buf, url, "GET /../fuzz.c HTTP/1.0\n\n") == 404);
-  ASSERT(fetch(&mgr, buf, url, "GET /.%%2e/fuzz.c HTTP/1.0\n\n") == 404);
-  ASSERT(fetch(&mgr, buf, url, "GET /.%%2e%%2ffuzz.c HTTP/1.0\n\n") == 404);
-  ASSERT(fetch(&mgr, buf, url, "GET /..%%2f%%20fuzz.c HTTP/1.0\n\n") == 404);
-  ASSERT(fetch(&mgr, buf, url, "GET /..%%2ffuzz.c%%20 HTTP/1.0\n\n") == 404);
+  ASSERT(fetch(&mgr, buf, url, "GET /../fuzz.c HTTP/1.0\n\n") == 400);
+  ASSERT(fetch(&mgr, buf, url, "GET /.%%2e/fuzz.c HTTP/1.0\n\n") == 400);
+  ASSERT(fetch(&mgr, buf, url, "GET /.%%2e%%2ffuzz.c HTTP/1.0\n\n") == 400);
+  ASSERT(fetch(&mgr, buf, url, "GET /..%%2f%%20fuzz.c HTTP/1.0\n\n") == 400);
+  ASSERT(fetch(&mgr, buf, url, "GET /..%%2ffuzz.c%%20 HTTP/1.0\n\n") == 400);
 
   ASSERT(fetch(&mgr, buf, url, "GET /dredir HTTP/1.0\n\n") == 301);
   ASSERT(cmpheader(buf, "Location", "/dredir/"));
@@ -884,9 +886,8 @@ static void test_http_server(void) {
   ASSERT(cmpheader(buf, "Content-Encoding", "gzip") == true);
   ASSERT(gethm(buf).body.len == 23);
 
-  ASSERT(fetch(&mgr, buf, url, "GET /..ddot HTTP/1.0\n\n") == 301);
-  ASSERT(fetch(&mgr, buf, url, "GET /..ddot/ HTTP/1.0\n\n") == 200);
-  ASSERT(cmpbody(buf, "hi\n") == 0);
+  ASSERT(fetch(&mgr, buf, url, "GET /..ddot HTTP/1.0\n\n") == 400);
+  ASSERT(fetch(&mgr, buf, url, "GET /..ddot/ HTTP/1.0\n\n") == 400);
   ASSERT(fetch(&mgr, buf, url,
                "GET /a.txt HTTP/1.0\n"
                "Content-Length: -123\n\n") == 0);
@@ -1052,7 +1053,7 @@ static void test_http_server(void) {
     remove("uploaded.txt");
     ASSERT((p = mg_file_read(&mg_fs_posix, "uploaded.txt", NULL)) == NULL);
     ASSERT(fetch(&mgr, buf, url,
-                 "POST /upload?name=../uploaded.txt HTTP/1.0\r\n"
+                 "POST /upload?name=uploaded.txt HTTP/1.0\r\n"
                  "Content-Length: 5\r\n"
                  "\r\nhello") == 200);
     ASSERT((p = mg_file_read(&mg_fs_posix, "uploaded.txt", NULL)) != NULL);