Escaping review and image comments

Review Request #2708 - Created Nov. 14, 2011 and submitted

Damian Johnson
Review Board
reviewboard
Due to the html parser, javascript literals with closing script tags (for
instance, 'var foo = "</script>";') cause javascript blocks to be prematurely
terminated. For more information see...
http://www.wwco.com/~wls/blog/2007/04/25/using-script-in-a-javascript-literal/

This is a XSS vector, easily reproduced by making a comment of
"</script><script>alert(document);</script>" (quotes are escaped so examples
like 'alert("hello world");' won't work). Demo...
http://demo.reviewboard.org/r/6347/diff/

This is an issue with multiple comment types...

- Diff Comments
This reproduces in ReviewBoard 1.5 and 1.6.

- Screenshot Comments
Reproduced and fixed this in 1.5. Screenshot comments seem to have changed
quite a bit since then so I'm not sure if it's still an issue, but might as
well be safe.

- Attachment Comments
Iirc I accidently stumbled across this for 1.6 though I might be remembering
wrong. This patch doesn't include a fix for attachment comments, but it should
be a similar change around the 'file_attachment_comments' function in...
  reviewboard/reviews/templatetags/reviewtags.py
  reviewboard/templates/reviews/review_request_box.html

Change can be fetched from...
https://github.com/atagar/ReviewBoard/commit/7539dc1f42155fc499430633e462059412eb1b59
I made and tested this fix in RB 1.5 and 1.6, but I don't have a 1.6.2 instance
around so it would be a good idea to give this a sanity test before merging.
Damian Johnson
Christian Hammond
  1. Thanks again, Damian. I've made a change based on this one, but moving the point at which decoding the string happens into the main comment loading code, and making use of jQuery to turn the decode into a one-liner. The effect is the same, and I've verified that script injection no longer works. I'm doing releases tonight for this.
  2. 
      
Damian Johnson
Review request changed

Status: Closed (submitted)

Loading...