Last Comment Bug 924932 - (CVE-2013-1743) [SECURITY] Field values are (still) not escaped correctly in tabular reports
(CVE-2013-1743)
: [SECURITY] Field values are (still) not escaped correctly in tabular reports
Status: RESOLVED FIXED
: sec-critical, wsec-xss
Product: Bugzilla
Classification: Server Software
Component: Reporting/Charting (show other bugs)
: 4.1.1
: All All
: -- normal (vote)
: Bugzilla 4.2
Assigned To: Frédéric Buclin
: default-qa
Mentors:
: 961656 (view as bug list)
Depends on:
Blocks: 859188 912643 926085
  Show dependency treegraph
 
Reported: 2013-10-09 07:51 PDT by Mateusz Goik
Modified: 2014-07-24 16:54 PDT (History)
6 users (show)
glob: approval+
glob: approval4.4+
LpSolit: blocking4.4.1+
glob: approval4.2+
LpSolit: blocking4.2.7+
dveditz: sec‑bounty+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (3.55 KB, patch)
2013-10-12 16:04 PDT, Frédéric Buclin
no flags Details | Diff | Review
patch, v1.1 (3.60 KB, patch)
2013-10-13 04:00 PDT, Frédéric Buclin
no flags Details | Diff | Review
patch, v2 (3.64 KB, patch)
2013-10-13 14:31 PDT, Frédéric Buclin
dkl: review+
Details | Diff | Review

Comment 1 Frédéric Buclin 2013-10-09 10:12:16 PDT
Hum, this was supposed to be fixed by bug 790296. There must be another place in the JS code which also needs HTML-filtering. (It's a bit irritating that YUI is unable to filter stuff itself).
Comment 3 Frédéric Buclin 2013-10-09 10:56:43 PDT
(In reply to Mateusz Goik from comment #2)
> Not only JS:

Right. I found what the problem was for this specific case. A div had |FILTER js| instead of |FILTER html|. The root cause for comment 0 is still unknown, though.
Comment 4 Frédéric Buclin 2013-10-09 10:58:37 PDT
(In reply to Frédéric Buclin from comment #3)
> Right. I found what the problem was for this specific case. A div had
> |FILTER js| instead of |FILTER html|.

... which was a regression due to bug 640987.
Comment 5 Frédéric Buclin 2013-10-09 11:04:08 PDT
I can reproduce comment 0 only if the horizontal axis has evil data. If only the vertical axis has it, it's fine.
Comment 6 Frédéric Buclin 2013-10-09 11:27:07 PDT
I think I found the problem: oColumn.field needs to be escaped.
Comment 7 Daniel Veditz [:dveditz] 2013-10-11 21:27:08 PDT
This bug shall be known as CVE-2013-1743
Comment 8 Frédéric Buclin 2013-10-12 16:04:45 PDT
Created attachment 816273 [details] [diff] [review]
patch, v1

This patch applies to all branches (4.2 and newer). As a side-effect, it also fixes bug 859188.
Comment 9 Frédéric Buclin 2013-10-13 04:00:01 PDT
Created attachment 816319 [details] [diff] [review]
patch, v1.1

Better patch: only decode HTML entities when necessary, i.e. for row values only (column values are not HTML-escaped). This correctly handles improbable strings such as "&amp;gt; &lt;" (which must not be converted to "&gt; <" or "> <").
Comment 10 Mateusz Goik 2013-10-13 05:26:25 PDT
http://localhost/bugzilla_copy/userprefs.cgi
Your real name (optional, but encouraged): '.a*eval(alert(1))*-'


http://localhost/bugzilla_copy/report.cgi?x_axis_field=reporter_realname&y_axis_field=reporter_realname&z_axis_field=reporter_realname&no_redirect=1&query_format=report-table&short_desc_type=allwordssubstr&short_desc=&classification=aaaaaaa&classification=Unclassified&resolution=---&longdesc_type=allwordssubstr&longdesc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&keywords_type=allwords&keywords=&deadlinefrom=&deadlineto=&bug_id=&bug_id_type=anyexact&votes=&votes_type=greaterthaneq&emailtype1=substring&email1=&emailtype2=substring&email2=&emailtype3=substring&email3=&chfieldvalue=&chfieldfrom=&chfieldto=Now&j_top=AND&f1=noop&o1=noop&v1=&format=table&action=wrap

...
  elLiner.innerHTML = '&lt;a href="buglist.cgi?action=wrap&amp;amp;classification=aaaaaaa&amp;amp;classification=Unclassified&amp;amp;resolution=---&amp;amp;reporter_realname='.a*eval(alert(1))*-'&amp;amp;reporter_realname='
                          + bz_encode(oColumn.field)
                          + '"&gt;' + oData + '&lt;/a&gt;';
    else
      elLiner.innerHTML = '&lt;a href="buglist.cgi?action=wrap&amp;amp;classification=aaaaaaa&amp;amp;classification=Unclassified&amp;amp;resolution=---&amp;amp;reporter_realname='.a*eval(alert(1))*-'&amp;amp;reporter_realname='
                          + bz_encode(oRecord.getData("row_title"), 1)
                          + '&amp;amp;reporter_realname='
                          + bz_encode(oColumn.field) + '"&gt;' + oData + '&lt;/a&gt;';
....
Comment 11 Frédéric Buclin 2013-10-13 06:45:36 PDT
@Mateusz: is your testcase with my patch applied or not?
Comment 12 Mateusz Goik 2013-10-13 12:09:54 PDT
(In reply to Frédéric Buclin from comment #11)
> @Mateusz: is your testcase with my patch applied or not?

with your patch (Attachment #816319 [details] [diff])
Comment 13 Frédéric Buclin 2013-10-13 13:51:32 PDT
The problem only happens when the z-axis is set to the field containing evil data. I will investigate.
Comment 14 Frédéric Buclin 2013-10-13 14:31:36 PDT
Created attachment 816391 [details] [diff] [review]
patch, v2

Thanks for your testing, Mateusz, that's very helpful. :)
Comment 15 Reed Loden [:reed] (use needinfo?) 2013-10-13 14:47:44 PDT
Comment on attachment 816391 [details] [diff] [review]
patch, v2

Can we use YAHOO.lang.escapeHTML() here?
Comment 16 Reed Loden [:reed] (use needinfo?) 2013-10-13 14:48:19 PDT
See also bug 657194
Comment 17 Frédéric Buclin 2013-10-13 15:30:32 PDT
(In reply to Reed Loden [:reed] from comment #15)
> Can we use YAHOO.lang.escapeHTML() here?

escapeHTML() is similar to |FILTER html|. encodeURIComponent() is similar to |FILTER uri|. What we want is |FILTER uri|, i.e. encodeURIComponent().
Comment 18 Frédéric Buclin 2013-10-15 09:18:45 PDT
Mateusz: did you manage to trigger XSS with my new patch?
Comment 19 Mateusz Goik 2013-10-15 09:38:07 PDT
(In reply to Frédéric Buclin from comment #18)
> Mateusz: did you manage to trigger XSS with my new patch?

it's ok now
Comment 20 David Lawrence [:dkl] 2013-10-16 08:49:42 PDT
Comment on attachment 816391 [details] [diff] [review]
patch, v2

Review of attachment 816391 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl
Comment 21 Frédéric Buclin 2013-10-16 10:27:00 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/reports/report-table.html.tmpl
Committed revision 8780.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified template/en/default/reports/report-table.html.tmpl
Committed revision 8626.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified template/en/default/reports/report-table.html.tmpl
Committed revision 8234.
Comment 22 Frédéric Buclin 2013-10-17 07:58:52 PDT
Security advisory sent.
Comment 24 Frédéric Buclin 2014-01-20 08:36:00 PST
*** Bug 961656 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.