XSS in cgit

XSS in cgit

[Jason]

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

XSS in cgit

[normalperson]

"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.

XSS in cgit

[john]

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.

XSS in cgit

[Jason]

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?

XSS in cgit

[john]

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.

XSS in cgit

[mailings]

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

XSS in cgit

[Jason]

I like this idea. The hard part is -- when HTML-serving mode is not
enabled, what mime types do we restrict? Krzysztof - is there a safe
and future-proof list of mimetypes that we can blacklist?

XSS in cgit

[Jason]

I've begun work on this. I'll post a summary of things in a bit.

Krzysztof - still waiting to hear from you.

XSS in cgit

[Jason]

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>

XSS in cgit

[Jason]

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?

XSS in cgit

[Jason]

Hi Michael,

Thanks for your response. So the use case was in fact quite specific,
and it seems like our recent treatment of the /plain endpoint handles
that quite well and in a safe manner too.

Okay, I feel solid about the change now. Thanks a bunch.

Jason

XSS in cgit

[john]

On Sat, Jan 16, 2016 at 01:23:39AM +0100, Jason A. Donenfeld wrote:
> Thanks for your response. So the use case was in fact quite specific,
> and it seems like our recent treatment of the /plain endpoint handles
> that quite well and in a safe manner too.
> 
> Okay, I feel solid about the change now. Thanks a bunch.

It doesn't look like Michael's email made it to the list.  Would you
mind summarising the use case?

Fwd: XSS in cgit

[Jason]

---------- Forwarded message ----------
From: Michael Krelin <hacker at klever.net>
Date: Fri, Jan 15, 2016 at 7:17 PM
Subject: Re: XSS in cgit
To: "Jason A. Donenfeld" <Jason at zx2c4.com>
Cc: "cgit at lists.zx2c4.com" <cgit at lists.zx2c4.com>



Hey,

I can?t remember all the details (2008!), but the main idea was to
feed the URL directly to something that would process it according to
the content type header. In particular, I believe I linked xml files
using xinclude from another xml processed by xsltproc and generating
some html. And maybe linked some pictures too. It?s been a while since
I?ve done that though I think I still use that setup (haven?t updated
cgit there for a while tho).

That is not to say you?ve done me wrong by removing the feature, I?m
not in the position to judge without diving deeper into background of
the change ;-)

Love,
H