Opened 11 years ago

Closed 10 years ago

#4896 defect closed fixed (fixed)

twisted.web.util.formatFailure vulnerable to XSS in rare cases

Reported by: ivank Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: web Keywords: security
Cc: jknight, ivank Branch: branches/template-failure-formatting-4896-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

twisted.web.util.formatFailure assumes that locals and instance variables do not contain HTML, but that isn't necessarily the case; for example, put this in a render_GET:

locals()['<script>alert("hi")</script>'] = 1
1/0

(if you have site.displayTracebacks enabled, you'll see an alert pop up on the error page.)

Attachments (1)

4896-001-escape-everything.patch (1.3 KB) - added by ivank 11 years ago.
no unit tests here

Download all attachments as: .zip

Change History (15)

comment:1 Changed 11 years ago by DefaultCC Plugin

Cc: jknight added

Changed 11 years ago by ivank

no unit tests here

comment:2 Changed 11 years ago by ivank

While the locals() example is very contrived, it's possible that someone is putting arbitrary user input into object attributes via setattr.

comment:3 Changed 11 years ago by ivank

Cc: ivank added

comment:4 Changed 10 years ago by washort

Now that we have twisted.web.template we should use it in formatFailure and Request.processingFailed.

comment:5 Changed 10 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/template-failure-formatting-4896

(In [33212]) Branching to 'template-failure-formatting-4896'

comment:6 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added

I re-implemented formatFailure in terms of twisted.web.template. I included a template that roughly matches the old markup.

I dropped the rendering of locals and globals, because as of #5011, twisted.web no longer passes any failures to formatFailure that have any locals or globals in them. Perhaps we will want to re-introduce this functionality somehow.

I deprecated all of the support functions in twisted.web.util that are no longer used. I also added a bunch of new support classes; possibly these would be better off private, or in another module.

Build results

comment:7 Changed 10 years ago by Jean-Paul Calderone

Oops. There appears to be some overlap between this ticket and #4948.

comment:8 Changed 10 years ago by Jonathan Jacobs

Keywords: review removed
Owner: set to Jean-Paul Calderone

Firstly, great job on this! Secondly, interpolating strings into HTML is nasty, so hooray for this change!

Some things:

  1. I notice you're using the @ decorator syntax, at one point this was forbidden (although the coding standard doesn't currently say anything about decorators.) Was this because they weren't supported in Python 2.4 and support for that was recently dropped?
  1. Why not yield SourceLineElement(...) in SourceFragmentElement.sourceLines instead of building the list and returning it?
  1. formatFailure calls flattenString which is apparently an asynchronous API, it returns a Deferred anyway, but formatFailure is written in a synchronous fashion and makes some assumptions about the implementation of flattenString. What's going on here? I realise that the API for formatFailure was previously a synchronous one but the current code strikes me as pretty weird.
  1. I think that the new support classes should exist in a new private module.
  1. The CSS in failure.xhtml uses monotype, which is not a thing, when I think it should actually use monospace.

It's unfortunate that without patterns 20 lines of markup explodes into around 200 lines of supporting code and potentially diminished readability, although I think the tests do benefit.

comment:9 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Thanks for the review!

>  1. I notice you're using the @ decorator syntax, at one point this was forbidden (although the coding standard doesn't currently say anything about decorators.) Was this because they weren't supported in Python 2.4 and support for that was recently dropped? 

Yea. With Python 2.4 support out the window, there is no longer any compatibility reason to avoid using the syntax. I don't think we need to pour over the codebase adding uses of it to every possible site, but using it in new code like this seems like the right idea.

>  2. Why not yield SourceLineElement(...) in SourceFragmentElement.sourceLines instead of building the list and returning it? 

Hmm. I don't remember. I changed it to a generator.

>  3. formatFailure calls flattenString which is apparently an asynchronous API, it returns a Deferred anyway, but formatFailure is written in a synchronous fashion and makes some assumptions about the implementation of flattenString. What's going on here? I realise that the API for formatFailure was previously a synchronous one but the current code strikes me as pretty weird. 

It's true, this is a bit tricky. Since I know there are no Deferreds in the document being flattened, I know that flattenString is going to have finished by the time it returns. I wouldn't want to write a lot of code that makes this assumption, but it seems to be more or less the only way to preserve the existing synchronous interface.

>  4. I think that the new support classes should exist in a new private module. 

I dunno if they need a whole module. I can make them private in twisted.python.util though. I'd like to leave FailureElement public, though, and encourage people to use it instead of formatFailure.

>  5. The CSS in failure.xhtml uses monotype, which is not a thing, when I think it should actually use monospace. 

Okay, changed.

comment:10 in reply to:  9 Changed 10 years ago by Jonathan Jacobs

Keywords: review removed
Owner: set to Jean-Paul Calderone

Replying to exarkun:

Hmm. I don't remember. I changed it to a generator.

I thought it might have something to do with the tests, but those all still pass.

It's true, this is a bit tricky. Since I know there are no Deferreds in the document being flattened, I know that flattenString is going to have finished by the time it returns. I wouldn't want to write a lot of code that makes this assumption, but it seems to be more or less the only way to preserve the existing synchronous interface.

Perhaps then there should be a test that fails when flattenString doesn't operate synchronously to ensure that formatFailure does not accidentally get broken.

I dunno if they need a whole module. I can make them private in twisted.python.util though. I'd like to leave FailureElement public, though, and encourage people to use it instead of formatFailure.

I just thought it would be easier than adding _ to everything everywhere, but it looks like you already did that. :)

I think this looks good to merge, please go ahead.

comment:11 Changed 10 years ago by Jean-Paul Calderone

Branch: branches/template-failure-formatting-4896branches/template-failure-formatting-4896-2

(In [33498]) Branching to 'template-failure-formatting-4896-2'

comment:12 Changed 10 years ago by Jean-Paul Calderone

(In [33500]) Bump the version number on the new deprecation, since we missed 12.0. Also jam a @since onto the new public element.

refs #4896

comment:13 Changed 10 years ago by Jean-Paul Calderone

(In [33502]) Return the test Deferreds, or else.

refs #4896

comment:14 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [33503]) Merge template-failure-formatting-4896-2

Author: exarkun Reviewer: jonathanj Fixes: #4896

Change twisted.web.util.formatFailure, an API for rendering a Failure to an HTML string, to use twisted.web.template. This simplifies the code and fixes a number of potential security issues caused by insufficient quoting in the previous implementation.

Note: See TracTickets for help on using tickets.