[PATCH 1/3] ui-refs: escape HTML chars in author and tagger names

[PATCH 1/3] ui-refs: escape HTML chars in author and tagger names

[john]

Everywhere else we use html_txt to escape any special characters in
these variables.  Do so here as well.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
I spotted this while looking at Jason's jd/gravatar series.  The
following two patches cover other similar issues I spotted while
auditing all uses of "html()".  I think everything else is OK.

 ui-refs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui-refs.c b/ui-refs.c
index 20c91e3..c97b0c6 100644
--- a/ui-refs.c
+++ b/ui-refs.c
@@ -155,9 +155,9 @@ static int print_tag(struct refinfo *ref)
 	html("</td><td>");
 	if (info) {
 		if (info->tagger)
-			html(info->tagger);
+			html_txt(info->tagger);
 	} else if (ref->object->type == OBJ_COMMIT) {
-		html(ref->commit->author);
+		html_txt(ref->commit->author);
 	}
 	html("</td><td colspan='2'>");
 	if (info) {
-- 
1.8.5.226.g0d60d77

[PATCH 2/3] ui-shared: URL-escape script_name

[john]

As far as I know, there is no requirement that $SCRIPT_NAME contain only
URL-safe characters, so we need to make sure that any special characters
are escaped.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-shared.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index 2c12de7..abe15cd 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -139,7 +139,7 @@ static void site_url(const char *page, const char *search, const char *sort, int
 	if (ctx.cfg.virtual_root)
 		html_attr(ctx.cfg.virtual_root);
 	else
-		html(ctx.cfg.script_name);
+		html_url_path(ctx.cfg.script_name);
 
 	if (page) {
 		htmlf("?p=%s", page);
@@ -219,7 +219,7 @@ static char *repolink(const char *title, const char *class, const char *page,
 				html_url_path(path);
 		}
 	} else {
-		html(ctx.cfg.script_name);
+		html_url_path(ctx.cfg.script_name);
 		html("?url=");
 		html_url_arg(ctx.repo->url);
 		if (ctx.repo->url[strlen(ctx.repo->url) - 1] != '/')
-- 
1.8.5.226.g0d60d77

[PATCH 3/3] ui-repolist: HTML-escape cgit_rooturl() response

[john]

This is for consistency with other callers.  The value returned from
cgit_rooturl is not guaranteed to be HTML-safe.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-repolist.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ui-repolist.c b/ui-repolist.c
index d4ee279..e8d5eb6 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -106,7 +106,9 @@ static int is_in_url(struct cgit_repo *repo)
 
 static void print_sort_header(const char *title, const char *sort)
 {
-	htmlf("<th class='left'><a href='%s?s=%s", cgit_rooturl(), sort);
+	html("<th class='left'><a href='");
+	html_attr(cgit_rooturl());
+	htmlf("?s=%s", sort);
 	if (ctx.qry.search) {
 		html("&amp;q=");
 		html_url_arg(ctx.qry.search);
-- 
1.8.5.226.g0d60d77

[PATCH 2/3] ui-shared: URL-escape script_name

[Jason]

Are there any circumstances in which this could have prior lead to an XSS?

[PATCH 2/3] ui-shared: URL-escape script_name

[john]

On Sun, Jan 12, 2014 at 10:18:30PM +0100, Jason A. Donenfeld wrote:
> Are there any circumstances in which this could have prior lead to an XSS?

I'm pretty sure this is entirely under the control of the system
administrator, so it should be fine.

[PATCH 1/3] ui-refs: escape HTML chars in author and tagger names

[Jason]

Same question here -- XSS potential?

[PATCH 3/3] ui-repolist: HTML-escape cgit_rooturl() response

[Jason]

Merged this series.

[PATCH 1/3] ui-refs: escape HTML chars in author and tagger names

[john]

On Sun, Jan 12, 2014 at 11:02:01PM +0100, Jason A. Donenfeld wrote:
> Same question here -- XSS potential?

This is the one that worries me.  But actually, Git strips "<", ">" and
"\n" from GIT_*_NAME, so the question becomes whether we can manually
construct a Git object to exploit this.

I think the parsing.c::parse_user() function then saves us by stopping
the name as soon as it hits "<".  So there cannot be any way to insert
HTML elements here.