Fix out-of-bounds memory accesses with virtual_root=""

The CGit configuration variable virtual_root is normalized so that it
does not have a trailing '/' character, but it is allowed to be empty
(the empty string and NULL have different meanings here) and there is
code that is insufficiently cautious when checking if it ends in a '/':

	if (virtual_root[strlen(virtual_root) - 1] != '/')

Clearly this check is redundant, but rather than simply removing it we
get a slight efficiency improvement by switching the normalization so
that the virtual_root variable always ends in '/'.  Do this with a new
"ensure_end" helper.

Signed-off-by: John Keeping <john@keeping.me.uk>

diff --git a/cgit.c b/cgit.c
index ca3034cbb1b80ad39c4339ef93eb4da821f1044b..6f75db14a5c64e0d1684edaf876dd61667938c0b 100644 (file)
--- a/cgit.c
+++ b/cgit.c
@@ -155,9 +155,7 @@ static void config_cb(const char *name, const char *value)
        else if (!strcmp(name, "strict-export"))
                ctx.cfg.strict_export = xstrdup(value);
        else if (!strcmp(name, "virtual-root")) {
-               ctx.cfg.virtual_root = trim_end(value, '/');
-               if (!ctx.cfg.virtual_root && (!strcmp(value, "/")))
-                       ctx.cfg.virtual_root = "";
+               ctx.cfg.virtual_root = ensure_end(value, '/');
        } else if (!strcmp(name, "nocache"))
                ctx.cfg.nocache = atoi(value);
        else if (!strcmp(name, "noplainemail"))
@@ -833,11 +831,8 @@ int main(int argc, const char **argv)
         * that virtual-root equals SCRIPT_NAME, minus any possibly
         * trailing slashes.
         */
-       if (!ctx.cfg.virtual_root && ctx.cfg.script_name) {
-               ctx.cfg.virtual_root = trim_end(ctx.cfg.script_name, '/');
-               if (!ctx.cfg.virtual_root)
-                       ctx.cfg.virtual_root = "";
-       }
+       if (!ctx.cfg.virtual_root && ctx.cfg.script_name)
+               ctx.cfg.virtual_root = ensure_end(ctx.cfg.script_name, '/');
 
        /* If no url parameter is specified on the querystring, lets
         * use PATH_INFO as url. This allows cgit to work with virtual

diff --git a/cgit.h b/cgit.h
index 081f669a8a5c438265cfd39b1443e0ebf876f3db..fc3fc6fb29b36cd65db6fa2231ea3caa8f248229 100644 (file)
--- a/cgit.h
+++ b/cgit.h
@@ -190,7 +190,7 @@ struct cgit_config {
        char *script_name;
        char *section;
        char *repository_sort;
-       char *virtual_root;
+       char *virtual_root;     /* Always ends with '/'. */
        char *strict_export;
        int cache_size;
        int cache_dynamic_ttl;
@@ -300,6 +300,7 @@ extern int chk_positive(int result, char *msg);
 extern int chk_non_negative(int result, char *msg);
 
 extern char *trim_end(const char *str, char c);
+extern char *ensure_end(const char *str, char c);
 extern char *strlpart(char *txt, int maxlen);
 extern char *strrpart(char *txt, int maxlen);
 

diff --git a/shared.c b/shared.c
index cc06930f68acad64c430ae1fea7c8bdd9c13ae60..1fa9c99d5d8cc699e537d39bb1d38428383b941b 100644 (file)
--- a/shared.c
+++ b/shared.c
@@ -115,6 +115,21 @@ char *trim_end(const char *str, char c)
        return xstrndup(str, len);
 }
 
+char *ensure_end(const char *str, char c)
+{
+       size_t len = strlen(str);
+       char *result;
+
+       if (len && str[len - 1] == c)
+               return xstrndup(str, len);
+
+       result = xmalloc(len + 2);
+       memcpy(result, str, len);
+       result[len] = '/';
+       result[len + 1] = '\0';
+       return result;
+}
+
 char *strlpart(char *txt, int maxlen)
 {
        char *result;

diff --git a/ui-shared.c b/ui-shared.c
index 945d560a300780083e9dabc3f024d2a0dc6eaf5d..c1f3c202c90592d49874c46a8b6a2a3da9c749be 100644 (file)
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -57,7 +57,7 @@ const char *cgit_hosturl()
 const char *cgit_rooturl()
 {
        if (ctx.cfg.virtual_root)
-               return fmt("%s/", ctx.cfg.virtual_root);
+               return ctx.cfg.virtual_root;
        else
                return ctx.cfg.script_name;
 }
@@ -65,7 +65,7 @@ const char *cgit_rooturl()
 char *cgit_repourl(const char *reponame)
 {
        if (ctx.cfg.virtual_root) {
-               return fmt("%s/%s/", ctx.cfg.virtual_root, reponame);
+               return fmt("%s%s/", ctx.cfg.virtual_root, reponame);
        } else {
                return fmt("?r=%s", reponame);
        }
@@ -78,7 +78,7 @@ char *cgit_fileurl(const char *reponame, const char *pagename,
        char *delim;
 
        if (ctx.cfg.virtual_root) {
-               tmp = fmt("%s/%s/%s/%s", ctx.cfg.virtual_root, reponame,
+               tmp = fmt("%s%s/%s/%s", ctx.cfg.virtual_root, reponame,
                          pagename, (filename ? filename:""));
                delim = "?";
        } else {
@@ -126,11 +126,9 @@ static void site_url(const char *page, const char *search, const char *sort, int
 {
        char *delim = "?";
 
-       if (ctx.cfg.virtual_root) {
+       if (ctx.cfg.virtual_root)
                html_attr(ctx.cfg.virtual_root);
-               if (ctx.cfg.virtual_root[strlen(ctx.cfg.virtual_root) - 1] != '/')
-                       html("/");
-       } else
+       else
                html(ctx.cfg.script_name);
 
        if (page) {
@@ -201,8 +199,6 @@ static char *repolink(const char *title, const char *class, const char *page,
        html(" href='");
        if (ctx.cfg.virtual_root) {
                html_url_path(ctx.cfg.virtual_root);
-               if (ctx.cfg.virtual_root[strlen(ctx.cfg.virtual_root) - 1] != '/')
-                       html("/");
                html_url_path(ctx.repo->url);
                if (ctx.repo->url[strlen(ctx.repo->url) - 1] != '/')
                        html("/");