From 1bcc672503351f6b7c3c1f4a6632f0589f230929 Mon Sep 17 00:00:00 2001
From: Thomas Kolb <thomas@tkolb.de>
Date: Fri, 16 Dec 2016 23:40:44 +0100
Subject: [PATCH] Reimplemented memory management in dirlisting generation

This should eliminate all bufferoverflows introduced by the previous
usage of strcat() and various assumptions.
---
 include/util.h   |  1 +
 src/dirlisting.c | 74 ++++++++++++++----------------------------------
 src/util.c       | 34 ++++++++++++++++++++++
 3 files changed, 57 insertions(+), 52 deletions(-)

diff --git a/include/util.h b/include/util.h
index 13838bd..bc6ee3a 100644
--- a/include/util.h
+++ b/include/util.h
@@ -17,5 +17,6 @@
 
 void remove_trailing_slash(char *str);
 void urlencode(const char *str, char *result);
+char* safe_append(char *target, size_t *targetsize, const char *src);
 
 #endif // UTIL_H
diff --git a/src/dirlisting.c b/src/dirlisting.c
index 62df779..82ed552 100644
--- a/src/dirlisting.c
+++ b/src/dirlisting.c
@@ -79,21 +79,20 @@ char* gen_html(const char *url, struct Entry *entries, size_t numentries, int up
   result = malloc(allocated * sizeof(char));
   result[0] = '\0';
 
-  // this is still safe
-  strcat(result, HEADER1);
+  result = safe_append(result, &allocated, HEADER1);
 
   snprintf(buf, BUFSIZE, "<title>Directory listing for %s</title>", url);
-  strcat(result, buf);
+  result = safe_append(result, &allocated, buf);
 
-  strcat(result, HEADER2);
+  result = safe_append(result, &allocated, HEADER2);
 
   snprintf(buf, BUFSIZE, "<h1>Directory listing for %s</h1>", url);
-  strcat(result, buf);
+  result = safe_append(result, &allocated, buf);
 
   int isrootdir = strcmp(url, "/") == 0;
 
   // Begin Navigation
-  strcat(result, "<p>Navigation: ");
+  result = safe_append(result, &allocated, "<p>Navigation: ");
 
   // tokenize the URL: init
   char *token, *urlclone;
@@ -103,30 +102,31 @@ char* gen_html(const char *url, struct Entry *entries, size_t numentries, int up
   strcpy(fullpath, "/");
 
   if(isrootdir) {
-    strcat(result, "/");
+    result = safe_append(result, &allocated, "/");
   } else {
-    strcat(result, "<a href=\"/\">/</a>");
+    result = safe_append(result, &allocated, "<a href=\"/\">/</a>");
 
     // create a link for each token in URL
     do {
       LOG(LVL_DUMP, "Found token of %s: %s", url, token);
 
+      // TODO: check if this is safe
       strcat(fullpath, token);
       strcat(fullpath, "/");
 
       snprintf(buf, BUFSIZE, " <a href=\"%s\">%s</a> /", fullpath, token);
-      strcat(result, buf);
+      result = safe_append(result, &allocated, buf);
     }
     while((token = strtok(NULL, "/")) != NULL);
   }
 
   free(urlclone);
 
-  strcat(result, "</p>");
+  result = safe_append(result, &allocated, "</p>");
 
   // begin listing subdirs
-  strcat(result, "<h2>Sub-directories</h2>");
-  strcat(result, "<ul>");
+  result = safe_append(result, &allocated, "<h2>Sub-directories</h2>");
+  result = safe_append(result, &allocated, "<ul>");
 
   for(i = 0; i < numentries; i++) {
     // skip all non-directories
@@ -135,21 +135,6 @@ char* gen_html(const char *url, struct Entry *entries, size_t numentries, int up
 
     urlencode(entries[i].name, encName);
 
-    size_t entrylen = 64 + strlen(url) + strlen(entries[i].name) + strlen(encName);
-    size_t resultlen = strlen(result);
-
-    if(allocated < resultlen + entrylen) {
-      allocated = 2*allocated + entrylen; // be on the safe side
-      LOG(LVL_DEBUG, "reallocating result string (subdirs) - new size: %lu bytes", allocated * sizeof(char));
-      char *tmpresult = realloc(result, allocated * sizeof(char));
-      if(!tmpresult) {
-        LOG(LVL_ERR, "realloc failed for result string: %s", strerror(errno));
-        break;
-      }
-
-      result = tmpresult;
-    }
-
     if(isrootdir) {
       snprintf(buf, BUFSIZE, "<li><a href=\"/%s/\">%s/</a></li>\n",
           encName, entries[i].name);
@@ -157,14 +142,14 @@ char* gen_html(const char *url, struct Entry *entries, size_t numentries, int up
       snprintf(buf, BUFSIZE, "<li><a href=\"%s/%s/\">%s/</a></li>\n",
           url, encName, entries[i].name);
     }
-    strcat(result, buf);
+    result = safe_append(result, &allocated, buf);
   }
 
-  strcat(result, "</ul>");
+  result = safe_append(result, &allocated, "</ul>");
 
   // begin listing files
-  strcat(result, "<h2>Files</h2>");
-  strcat(result, "<ul>");
+  result = safe_append(result, &allocated, "<h2>Files</h2>");
+  result = safe_append(result, &allocated, "<ul>");
 
   // here the listing entries are generated
   for(i = 0; i < numentries; i++) {
@@ -174,21 +159,6 @@ char* gen_html(const char *url, struct Entry *entries, size_t numentries, int up
 
     urlencode(entries[i].name, encName);
 
-    size_t entrylen = 64 + strlen(url) + strlen(entries[i].name) + strlen(encName);
-    size_t resultlen = strlen(result);
-
-    if(allocated < resultlen + entrylen) {
-      allocated = 2*allocated + entrylen; // be on the safe side
-      LOG(LVL_DEBUG, "reallocating result string (files) - new size: %lu bytes", allocated * sizeof(char));
-      char *tmpresult = realloc(result, allocated * sizeof(char));
-      if(!tmpresult) {
-        LOG(LVL_ERR, "realloc failed for result string: %s", strerror(errno));
-        break;
-      }
-
-      result = tmpresult;
-    }
-
     if(isrootdir) {
       snprintf(buf, BUFSIZE, "<li><a href=\"/%s\">%s</a> [%s]</li>\n",
           encName, entries[i].name, format_size(entries[i].size));
@@ -196,15 +166,15 @@ char* gen_html(const char *url, struct Entry *entries, size_t numentries, int up
       snprintf(buf, BUFSIZE, "<li><a href=\"%s/%s\">%s</a> [%s]</li>\n",
           url, encName, entries[i].name, format_size(entries[i].size));
     }
-    strcat(result, buf);
+    result = safe_append(result, &allocated, buf);
   }
 
-  strcat(result, "</ul>");
+  result = safe_append(result, &allocated, "</ul>");
 
   if(uploadEnabled) {
-    strcat(result, "<h2>Upload</h2>");
-    strcat(result, "<p>Upload a file to this directory:</p>");
-    strcat(result,
+    result = safe_append(result, &allocated, "<h2>Upload</h2>");
+    result = safe_append(result, &allocated, "<p>Upload a file to this directory:</p>");
+    result = safe_append(result, &allocated,
         "<p>"
         "  <form method=\"POST\" action=\".\" enctype=\"multipart/form-data\">"
         "    <input name=\"data\" type=\"file\" size=\"30\">"
@@ -214,7 +184,7 @@ char* gen_html(const char *url, struct Entry *entries, size_t numentries, int up
         );
   }
 
-  strcat(result, FOOTER);
+  result = safe_append(result, &allocated, FOOTER);
 
   LOG(LVL_DEBUG, "result string (files) usage: %lu/%lu bytes", strlen(result), allocated);
 
diff --git a/src/util.c b/src/util.c
index b42eb29..665eca5 100644
--- a/src/util.c
+++ b/src/util.c
@@ -11,8 +11,11 @@
 #include <stdio.h>
 #include <stdint.h>
 #include <string.h>
+#include <malloc.h>
+#include <errno.h>
 
 #include "util.h"
+#include "logger.h"
 
 void remove_trailing_slash(char *str) {
   size_t offset = strlen(str)-1;
@@ -49,3 +52,34 @@ void urlencode(const char *str, char *result) {
     str++;
   }
 }
+
+/*!
+ * Append src to target, reallocating target on the way if necessary.
+ *
+ * \param target     The string to append to.
+ * \param targetsize A pointer to the size of target. Will be updated by this
+ *                   function when target is reallocated.
+ * \param src        The string to append to target.
+ * \returns          A pointer to new string (target becomes invalid in the
+ *                   event of reallocation) or NULL on a realloc error.
+ */
+char* safe_append(char *target, size_t *targetsize, const char *src) {
+  size_t targetlen = strlen(target);
+  size_t srclen = strlen(src);
+  size_t newsize = targetlen + 2*srclen + 1;
+
+  // check if reallocation is necessary
+  if((targetlen + srclen) > *targetsize) {
+    LOG(LVL_DEBUG, "safe_append: reallocating target string: %lu -> %lu characters", *targetsize, newsize);
+
+    target = realloc(target, newsize * sizeof(char));
+    if(target == NULL) {
+      LOG(LVL_ERR, "safe_append: realloc failed: %s", strerror(errno));
+      return NULL;
+    }
+
+    *targetsize = newsize;
+  }
+
+  return strcat(target, src);
+}