aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorGravatar John Keeping <john@keeping.me.uk>2013-04-02 03:03:34 (JST)
committerGravatar Jason A. Donenfeld <Jason@zx2c4.com>2013-04-08 22:59:46 (JST)
commitb1f17f168b91d709c0c0e62608de301a36f06da9 (patch)
tree9aff1fe903087cb5016ab2afdb32a7ca1cc139b2
parent4b4a62d507adc61e20e75e2748301ef307a6c95f (diff)
downloadcgit-b1f17f168b91d709c0c0e62608de301a36f06da9.zip
cgit-b1f17f168b91d709c0c0e62608de301a36f06da9.tar.gz
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>
-rw-r--r--cgit.c11
-rw-r--r--cgit.h3
-rw-r--r--shared.c15
-rw-r--r--ui-shared.c14
4 files changed, 25 insertions, 18 deletions
diff --git a/cgit.c b/cgit.c
index ca3034c..6f75db1 100644
--- a/cgit.c
+++ b/cgit.c
@@ -155,9 +155,7 @@ static void config_cb(const char *name, const char *value)
155 else if (!strcmp(name, "strict-export")) 155 else if (!strcmp(name, "strict-export"))
156 ctx.cfg.strict_export = xstrdup(value); 156 ctx.cfg.strict_export = xstrdup(value);
157 else if (!strcmp(name, "virtual-root")) { 157 else if (!strcmp(name, "virtual-root")) {
158 ctx.cfg.virtual_root = trim_end(value, '/'); 158 ctx.cfg.virtual_root = ensure_end(value, '/');
159 if (!ctx.cfg.virtual_root && (!strcmp(value, "/")))
160 ctx.cfg.virtual_root = "";
161 } else if (!strcmp(name, "nocache")) 159 } else if (!strcmp(name, "nocache"))
162 ctx.cfg.nocache = atoi(value); 160 ctx.cfg.nocache = atoi(value);
163 else if (!strcmp(name, "noplainemail")) 161 else if (!strcmp(name, "noplainemail"))
@@ -833,11 +831,8 @@ int main(int argc, const char **argv)
833 * that virtual-root equals SCRIPT_NAME, minus any possibly 831 * that virtual-root equals SCRIPT_NAME, minus any possibly
834 * trailing slashes. 832 * trailing slashes.
835 */ 833 */
836 if (!ctx.cfg.virtual_root && ctx.cfg.script_name) { 834 if (!ctx.cfg.virtual_root && ctx.cfg.script_name)
837 ctx.cfg.virtual_root = trim_end(ctx.cfg.script_name, '/'); 835 ctx.cfg.virtual_root = ensure_end(ctx.cfg.script_name, '/');
838 if (!ctx.cfg.virtual_root)
839 ctx.cfg.virtual_root = "";
840 }
841 836
842 /* If no url parameter is specified on the querystring, lets 837 /* If no url parameter is specified on the querystring, lets
843 * use PATH_INFO as url. This allows cgit to work with virtual 838 * use PATH_INFO as url. This allows cgit to work with virtual
diff --git a/cgit.h b/cgit.h
index 081f669..fc3fc6f 100644
--- a/cgit.h
+++ b/cgit.h
@@ -190,7 +190,7 @@ struct cgit_config {
190 char *script_name; 190 char *script_name;
191 char *section; 191 char *section;
192 char *repository_sort; 192 char *repository_sort;
193 char *virtual_root; 193 char *virtual_root; /* Always ends with '/'. */
194 char *strict_export; 194 char *strict_export;
195 int cache_size; 195 int cache_size;
196 int cache_dynamic_ttl; 196 int cache_dynamic_ttl;
@@ -300,6 +300,7 @@ extern int chk_positive(int result, char *msg);
300extern int chk_non_negative(int result, char *msg); 300extern int chk_non_negative(int result, char *msg);
301 301
302extern char *trim_end(const char *str, char c); 302extern char *trim_end(const char *str, char c);
303extern char *ensure_end(const char *str, char c);
303extern char *strlpart(char *txt, int maxlen); 304extern char *strlpart(char *txt, int maxlen);
304extern char *strrpart(char *txt, int maxlen); 305extern char *strrpart(char *txt, int maxlen);
305 306
diff --git a/shared.c b/shared.c
index cc06930..1fa9c99 100644
--- a/shared.c
+++ b/shared.c
@@ -115,6 +115,21 @@ char *trim_end(const char *str, char c)
115 return xstrndup(str, len); 115 return xstrndup(str, len);
116} 116}
117 117
118char *ensure_end(const char *str, char c)
119{
120 size_t len = strlen(str);
121 char *result;
122
123 if (len && str[len - 1] == c)
124 return xstrndup(str, len);
125
126 result = xmalloc(len + 2);
127 memcpy(result, str, len);
128 result[len] = '/';
129 result[len + 1] = '\0';
130 return result;
131}
132
118char *strlpart(char *txt, int maxlen) 133char *strlpart(char *txt, int maxlen)
119{ 134{
120 char *result; 135 char *result;
diff --git a/ui-shared.c b/ui-shared.c
index 945d560..c1f3c20 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -57,7 +57,7 @@ const char *cgit_hosturl()
57const char *cgit_rooturl() 57const char *cgit_rooturl()
58{ 58{
59 if (ctx.cfg.virtual_root) 59 if (ctx.cfg.virtual_root)
60 return fmt("%s/", ctx.cfg.virtual_root); 60 return ctx.cfg.virtual_root;
61 else 61 else
62 return ctx.cfg.script_name; 62 return ctx.cfg.script_name;
63} 63}
@@ -65,7 +65,7 @@ const char *cgit_rooturl()
65char *cgit_repourl(const char *reponame) 65char *cgit_repourl(const char *reponame)
66{ 66{
67 if (ctx.cfg.virtual_root) { 67 if (ctx.cfg.virtual_root) {
68 return fmt("%s/%s/", ctx.cfg.virtual_root, reponame); 68 return fmt("%s%s/", ctx.cfg.virtual_root, reponame);
69 } else { 69 } else {
70 return fmt("?r=%s", reponame); 70 return fmt("?r=%s", reponame);
71 } 71 }
@@ -78,7 +78,7 @@ char *cgit_fileurl(const char *reponame, const char *pagename,
78 char *delim; 78 char *delim;
79 79
80 if (ctx.cfg.virtual_root) { 80 if (ctx.cfg.virtual_root) {
81 tmp = fmt("%s/%s/%s/%s", ctx.cfg.virtual_root, reponame, 81 tmp = fmt("%s%s/%s/%s", ctx.cfg.virtual_root, reponame,
82 pagename, (filename ? filename:"")); 82 pagename, (filename ? filename:""));
83 delim = "?"; 83 delim = "?";
84 } else { 84 } else {
@@ -126,11 +126,9 @@ static void site_url(const char *page, const char *search, const char *sort, int
126{ 126{
127 char *delim = "?"; 127 char *delim = "?";
128 128
129 if (ctx.cfg.virtual_root) { 129 if (ctx.cfg.virtual_root)
130 html_attr(ctx.cfg.virtual_root); 130 html_attr(ctx.cfg.virtual_root);
131 if (ctx.cfg.virtual_root[strlen(ctx.cfg.virtual_root) - 1] != '/') 131 else
132 html("/");
133 } else
134 html(ctx.cfg.script_name); 132 html(ctx.cfg.script_name);
135 133
136 if (page) { 134 if (page) {
@@ -201,8 +199,6 @@ static char *repolink(const char *title, const char *class, const char *page,
201 html(" href='"); 199 html(" href='");
202 if (ctx.cfg.virtual_root) { 200 if (ctx.cfg.virtual_root) {
203 html_url_path(ctx.cfg.virtual_root); 201 html_url_path(ctx.cfg.virtual_root);
204 if (ctx.cfg.virtual_root[strlen(ctx.cfg.virtual_root) - 1] != '/')
205 html("/");
206 html_url_path(ctx.repo->url); 202 html_url_path(ctx.repo->url);
207 if (ctx.repo->url[strlen(ctx.repo->url) - 1] != '/') 203 if (ctx.repo->url[strlen(ctx.repo->url) - 1] != '/')
208 html("/"); 204 html("/");