CVE-2011-4312: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: Multiple cross-site scripting (XSS) vulnerabilities in the commenting system in Review Board before 1.5.7 and 1.6.x before 1.6.3 allow remote attackers to inject arbitrary web script or HTML via vectors involving the (1) diff viewer or (2) screenshot component.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
Published: 2011-11-24T04:01Z
Repository: https://github.com/reviewboard/reviewboard
Language: JavaScript
Files changed: reviewboard/htdocs/media/rb/js/diffviewer.js, reviewboard/htdocs/media/rb/js/screenshots.js, reviewboard/reviews/templatetags/reviewtags.py
Commit date: 2011-11-15 02:46:40 -0800
Commit message: Fix a comment vulnerability allowing scripts to be loaded.

Due to the way that comments were loaded in, it was possible to
terminate a script and inject a new one while loading the diff viewer.
This isn't believed to have been a problem in the wild, but is certainly
an important one to fix.

We now ensure that the text is escaped at the point where it's being fed
into the JavaScript. It's no longer possible to inject scripts.

Thanks to Damian Johnson for the heads up and for the fix that this
change is based on.

This will be going into 1.5.7 and 1.6.3 releases.

Diff: diff --git a/reviewboard/htdocs/media/rb/js/diffviewer.js b/reviewboard/htdocs/media/rb/js/diffviewer.js
index ce96de60..abc94b5e 100644
--- a/reviewboard/htdocs/media/rb/js/diffviewer.js
+++ b/reviewboard/htdocs/media/rb/js/diffviewer.js
@@ -158,10 +158,13 @@ function DiffCommentBlock(beginRow, endRow, beginLineNum, endLineNum,
      */
     if (comments && comments.length > 0) {
         for (var i in comments) {
             var comment = comments[i];
 
+            // We load in encoded text, so decode it.
+            comment.text = $("<div/>").html(comment.text).text();
+
             if (comment.localdraft) {
                 this._createDraftComment(comment.text);
             } else {
                 this.comments.push(comment);
             }
diff --git a/reviewboard/htdocs/media/rb/js/screenshots.js b/reviewboard/htdocs/media/rb/js/screenshots.js
index 896e2ac1..7a40e3d3 100644
--- a/reviewboard/htdocs/media/rb/js/screenshots.js
+++ b/reviewboard/htdocs/media/rb/js/screenshots.js
@@ -34,10 +34,13 @@ function CommentBlock(x, y, width, height, container, comments) {
      */
     if (comments && comments.length > 0) {
         for (var i in comments) {
             var comment = comments[i];
 
+            // We load in encoded text, so decode it.
+            comment.text = $("<div/>").html(comment.text).text();
+
             if (comment.localdraft) {
                 this._createDraftComment(comment.text);
             } else {
                 this.comments.push(comment);
             }
diff --git a/reviewboard/reviews/templatetags/reviewtags.py b/reviewboard/reviews/templatetags/reviewtags.py
index 81071dc1..2eb988e9 100644
--- a/reviewboard/reviews/templatetags/reviewtags.py
+++ b/reviewboard/reviews/templatetags/reviewtags.py
@@ -2,10 +2,11 @@ from django import template
 from django.conf import settings
 from django.db.models import Q
 from django.template import NodeList, TemplateSyntaxError
 from django.template.loader import render_to_string
 from django.utils import simplejson
+from django.utils.html import escape
 from django.utils.translation import ugettext_lazy as _
 from djblets.util.decorators import basictag, blocktag
 from djblets.util.misc import get_object_or_none
 from djblets.util.templatetags.djblets_utils import humanize_list
 
@@ -124,11 +125,11 @@ def commentcounts(context, filediff, interfilediff=None):
         if review and (review.public or review.user == user):
             key = (comment.first_line, comment.num_lines)
 
             comment_dict.setdefault(key, []).append({
                 'comment_id': comment.id,
-                'text': comment.text,
+                'text': escape(comment.text),
                 'line': comment.first_line,
                 'num_lines': comment.num_lines,
                 'user': {
                     'username': review.user.username,
                     'name': review.user.get_full_name() or review.user.username,
@@ -183,11 +184,11 @@ def screenshotcommentcounts(context, screenshot):
             position = '%dx%d+%d+%d' % (comment.w, comment.h, \
                                         comment.x, comment.y)
 
             comments.setdefault(position, []).append({
                 'id': comment.id,
-                'text': comment.text,
+                'text': escape(comment.text),
                 'user': {
                     'username': review.user.username,
                     'name': review.user.get_full_name() or review.user.username,
                 },
                 'url': comment.get_review_url(),

Vulnerable code:                 'text': comment.text,
                'text': comment.text,
Fixed code:             // We load in encoded text, so decode it.
            comment.text = $("<div/>").html(comment.text).text();

            // We load in encoded text, so decode it.
            comment.text = $("<div/>").html(comment.text).text();

from django.utils.html import escape
                'text': escape(comment.text),
                'text': escape(comment.text),