Fwd: XSS in cgit
-- Forwarded message -- From: Michael Krelin <hac...@klever.net> Date: Fri, Jan 15, 2016 at 7:17 PM Subject: Re: XSS in cgit To: "Jason A. Donenfeld" <ja...@zx2c4.com> Cc: "cgit@lists.zx2c4.com" <cgit@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 ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit
Re: XSS in cgit
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? ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit
Re: XSS in cgit
On Jan 13, 2016 9:11 PM, "Eric Wong"wrote: > > "Jason A. Donenfeld" 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? ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit
Re: XSS in cgit
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 ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit
Re: XSS in cgit
On Thu, Jan 14, 2016 at 11:57 AM, John Keepingwrote: > 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? ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit
Re: XSS in cgit
On Thu, Jan 14, 2016 at 12:01:57PM +0100, Jason A. Donenfeld wrote: > On Thu, Jan 14, 2016 at 11:57 AM, John Keepingwrote: > > 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. ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit
Re: XSS in cgit
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. ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit
Re: XSS in cgit
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 Keepingwrote: 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 ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit
XSS in cgit
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 ___ CGit mailing list CGit@lists.zx2c4.com http://lists.zx2c4.com/mailman/listinfo/cgit