Author: John Keeping <john@keeping.me.uk>
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>
cgit.c | 11 +++-------- cgit.h | 3 ++- shared.c | 15 +++++++++++++++ ui-shared.c | 14 +++++---------
diff --git a/cgit.c b/cgit.c index ca3034cbb1b80ad39c4339ef93eb4da821f1044b..6f75db14a5c64e0d1684edaf876dd61667938c0b 100644 --- a/cgit.c +++ b/cgit.c @@ -155,9 +155,7 @@ ctx.cfg.module_link = xstrdup(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 @@ /* If virtual-root isn't specified in cgitrc, lets pretend * 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 --- a/cgit.h +++ b/cgit.h @@ -190,7 +190,7 @@ char *root_readme; 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 --- a/shared.c +++ b/shared.c @@ -115,6 +115,21 @@ return NULL; 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 --- a/ui-shared.c +++ b/ui-shared.c @@ -57,7 +57,7 @@ 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 @@ 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 *tmp; 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 ofs) { 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 @@ } 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("/");