* XSS in cgit
@ 2016-01-13 16:07 Jason
2016-01-13 19:11 ` normalperson
2016-01-14 10:57 ` john
0 siblings, 2 replies; 13+ messages in thread
From: Jason @ 2016-01-13 16:07 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* XSS in cgit
2016-01-13 16:07 XSS in cgit Jason
@ 2016-01-13 19:11 ` normalperson
2016-01-15 11:34 ` Jason
2016-01-14 10:57 ` john
1 sibling, 1 reply; 13+ messages in thread
From: normalperson @ 2016-01-13 19:11 UTC (permalink / raw)
"Jason A. Donenfeld" <Jason at zx2c4.com> wrote:
> 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.
IIRC, the main difference was blob allows serving tree objects
as-is in binary form while plain generates an HTML directory listing.
^ permalink raw reply [flat|nested] 13+ messages in thread
* XSS in cgit
2016-01-13 19:11 ` normalperson
@ 2016-01-15 11:34 ` Jason
2016-01-15 15:18 ` Jason
0 siblings, 1 reply; 13+ messages in thread
From: Jason @ 2016-01-15 11:34 UTC (permalink / raw)
On Jan 13, 2016 9:11 PM, "Eric Wong" <normalperson at yhbt.net> wrote:
>
> "Jason A. Donenfeld" <Jason at zx2c4.com> wrote:
> > 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.
>
> IIRC, the main difference was blob allows serving tree objects
> as-is in binary form while plain generates an HTML directory listing.
Thanks, this is what I thought, which leads me to the more relevant
question:
Given that the /blob handler returns binary directory data, it must conform
to a particular API in order to be useful. Was the recently removed
/blob/?mimetype= query string parameter part of such an API? Have we maimed
something useful?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20160115/51f88745/attachment.html>
^ permalink raw reply [flat|nested] 13+ messages in thread
* XSS in cgit
2016-01-15 11:34 ` Jason
@ 2016-01-15 15:18 ` Jason
[not found] ` <7B8B10EF-8DCA-4115-9D33-4DD56F670BAB@klever.net>
0 siblings, 1 reply; 13+ messages in thread
From: Jason @ 2016-01-15 15:18 UTC (permalink / raw)
Hi Michael,
Care to enlighten us what the use case behind 42effc9 [1] was?
Thanks,
Jason
[1] http://git.zx2c4.com/cgit/commit/?id=42effc939090b2fbf1b2b76cd1d9c30fabcd230e
On Fri, Jan 15, 2016 at 12:34 PM, Jason A. Donenfeld <Jason at zx2c4.com> wrote:
>
> On Jan 13, 2016 9:11 PM, "Eric Wong" <normalperson at yhbt.net> wrote:
>>
>> "Jason A. Donenfeld" <Jason at zx2c4.com> wrote:
>> > 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.
>>
>> IIRC, the main difference was blob allows serving tree objects
>> as-is in binary form while plain generates an HTML directory listing.
>
> Thanks, this is what I thought, which leads me to the more relevant
> question:
>
> Given that the /blob handler returns binary directory data, it must conform
> to a particular API in order to be useful. Was the recently removed
> /blob/?mimetype= query string parameter part of such an API? Have we maimed
> something useful?
^ permalink raw reply [flat|nested] 13+ messages in thread
* XSS in cgit
2016-01-13 16:07 XSS in cgit Jason
2016-01-13 19:11 ` normalperson
@ 2016-01-14 10:57 ` john
2016-01-14 11:01 ` Jason
1 sibling, 1 reply; 13+ messages in thread
From: john @ 2016-01-14 10:57 UTC (permalink / raw)
On Wed, Jan 13, 2016 at 05:07:12PM +0100, Jason A. Donenfeld wrote:
> 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.
I wonder if we should just drop support for the "mimetype" query
parameter and see if anyone complains. In general, I would expect it to
be the server's responsibility to decide on the type of its output and
allowing the client to override it seems like a problem in general.
^ permalink raw reply [flat|nested] 13+ messages in thread
* XSS in cgit
2016-01-14 10:57 ` john
@ 2016-01-14 11:01 ` Jason
2016-01-14 11:07 ` john
0 siblings, 1 reply; 13+ messages in thread
From: Jason @ 2016-01-14 11:01 UTC (permalink / raw)
On Thu, Jan 14, 2016 at 11:57 AM, John Keeping <john at keeping.me.uk> wrote:
> I wonder if we should just drop support for the "mimetype" query
> parameter and see if anyone complains. In general, I would expect it to
> be the server's responsibility to decide on the type of its output and
> allowing the client to override it seems like a problem in general.
Agreed here.
We still have the other issue of git repos containing valid html with
malicious scripts and whatnot, though. Can we simply kill the feature
of allowing HTML to be served from cgit? This would indeed fix the
security issue in the best way. But would folks complain?
^ permalink raw reply [flat|nested] 13+ messages in thread
* XSS in cgit
2016-01-14 11:01 ` Jason
@ 2016-01-14 11:07 ` john
2016-01-14 11:53 ` mailings
0 siblings, 1 reply; 13+ messages in thread
From: john @ 2016-01-14 11:07 UTC (permalink / raw)
On Thu, Jan 14, 2016 at 12:01:57PM +0100, Jason A. Donenfeld wrote:
> On Thu, Jan 14, 2016 at 11:57 AM, John Keeping <john at keeping.me.uk> wrote:
> > I wonder if we should just drop support for the "mimetype" query
> > parameter and see if anyone complains. In general, I would expect it to
> > be the server's responsibility to decide on the type of its output and
> > allowing the client to override it seems like a problem in general.
>
> Agreed here.
>
> We still have the other issue of git repos containing valid html with
> malicious scripts and whatnot, though. Can we simply kill the feature
> of allowing HTML to be served from cgit? This would indeed fix the
> security issue in the best way. But would folks complain?
Unlike the "mimetype" query parameter, I can see valid usecases for
serving HTML from repositories with CGit (I've even used it myself in
the past), so I expect there will be complaints for that one.
Could we add a config knob for serving HTML and turn if off by default?
That will allow people who trust their repository contents to use this
feature while protecting everyone else.
^ permalink raw reply [flat|nested] 13+ messages in thread
* XSS in cgit
2016-01-14 11:07 ` john
@ 2016-01-14 11:53 ` mailings
2016-01-14 12:59 ` Jason
0 siblings, 1 reply; 13+ messages in thread
From: mailings @ 2016-01-14 11:53 UTC (permalink / raw)
On 14/01/16 12:07, John Keeping wrote:
> On Thu, Jan 14, 2016 at 12:01:57PM +0100, Jason A. Donenfeld wrote:
>> On Thu, Jan 14, 2016 at 11:57 AM, John Keeping <john at keeping.me.uk> wrote:
>>> I wonder if we should just drop support for the "mimetype" query
>>> parameter and see if anyone complains. In general, I would expect it to
>>> be the server's responsibility to decide on the type of its output and
>>> allowing the client to override it seems like a problem in general.
>>
>> Agreed here.
>>
Me too.
>> We still have the other issue of git repos containing valid html with
>> malicious scripts and whatnot, though. Can we simply kill the feature
>> of allowing HTML to be served from cgit? This would indeed fix the
>> security issue in the best way. But would folks complain?
>
> Unlike the "mimetype" query parameter, I can see valid usecases for
> serving HTML from repositories with CGit (I've even used it myself in
> the past), so I expect there will be complaints for that one.
>
> Could we add a config knob for serving HTML and turn if off by default?
> That will allow people who trust their repository contents to use this
> feature while protecting everyone else.
Good idea.
With a big fat warning that enabling it will possibly open you up to XSS
attacks, especially when the repo is not under your control
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-01-17 16:23 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-13 16:07 XSS in cgit Jason
2016-01-13 19:11 ` normalperson
2016-01-15 11:34 ` Jason
2016-01-15 15:18 ` Jason
[not found] ` <7B8B10EF-8DCA-4115-9D33-4DD56F670BAB@klever.net>
2016-01-16 0:23 ` Jason
2016-01-16 9:38 ` john
2016-01-17 16:23 ` Fwd: " Jason
2016-01-14 10:57 ` john
2016-01-14 11:01 ` Jason
2016-01-14 11:07 ` john
2016-01-14 11:53 ` mailings
2016-01-14 12:59 ` Jason
2016-01-14 13:35 ` Jason
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).