Summary

Escaping review and image comments

Review Request #2708 — Created Nov. 14, 2011 and submitted Nov. 14, 2011, 6:45 p.m.

Information

Owner

atagar

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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

Testing Done

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.

Change Summary:: Forgot to include gitweb link.
Description:: 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

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.

Status:: Completed

		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