XSS in cgit

[Jason A. Donenfeld]

Hi folks,

Krzysztof Katowicz-Kowalewsk has contacted me about two XSS security
vulnerabilities he's found in cgit. Upon investigation the problem
looks a bit deeper than he initially reported, and I found a few more
related vulnerabilities, and I think it generally deserves a bit of
discussion for the best way of patching it.

First (1), the big bad one. In ui-blob.c, we have:

ctx.page.mimetype = ctx.qry.mimetype;
cgit_print_http_headers();

This invokes, from ui-shared.c:
  htmlf("Content-Type: %s\n", ctx.page.mimetype);
or
  htmlf("Content-Type: %s; charset=%s\n", ctx.page.mimetype, ctx.page.charset);


A malicious user can pass a mime type such as text/html followed by a
few new lines and then some malicious javascript in a script tag to
launch an XSS attack. The obvious solution here is to ensure
ctx.page.mimetype doesn't contain new lines, null characters, and
other naughty fields according to the HTTP spec.

Second (2), there's another place where something similar happens:
  htmlf("Content-Disposition: inline; filename=\"%s\"\n", ctx.page.filename);

In this case, ctx.page.filename is related to what the user has put in
the git archive. Filenames could contain new lines, resulting in a
similar vulnerability.

Thus, for (1) and (2), I will gladly accept patches that run
parameters to header functions through a sanitation function.

Third (3), a user may upload an HTML page to a git repository. Upon
fetching it via /blob, it will use the inferred mime-type of
text/html. Though I have in the past enjoyed this as a means of
serving HTML from my personal cgit, this is probably not a desired
state of affairs for folks hosting cgit in shared environments, or
using cgit to mirror third party repositories. I therefore believe we
should use either a whitelist or a blacklist for allowed mime-types.
Furthermore, before dumping these blobs, we should add these two
headers:

        html("X-Content-Type-Options: nosniff\n");
        html("Content-Security-Policy: default-src 'none'\n");

I will thus accept patches for (3) that somehow safely limit the
mime-types served for /blob and /plain. If a whitelist,
ctx.qry.mimetype can then be removed entirely. If a blacklist, we can
keep ctx.qry.mimetype, but we'll need to be ensure that there's a
future proof way of preventing XSS via a mimetype blacklist; I'm not
yet convinced such a list can be compiled securely.

Given all this, could somebody remind me why we have both /plain and
/blob handlers? And if it's still necessary to maintain a distinction?
If not, I will gladly accept patches to unify these.

Thanks,
Jason