924932 - (CVE-2013-1743) [SECURITY] Field values are (still) not escaped correctly in tabular reports

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Mateusz Goik]

1. Create bug with summary: test'">'"><img src=x onerror=alert(1); '">

2.

https://landfill.bugzilla.org/bugzilla-tip/report.cgi?x_axis_field=short_desc&y_axis_field=short_desc&z_axis_field=short_desc&no_redirect=1&query_format=report-table&short_desc_type=allwordssubstr&short_desc=&resolution=---&longdesc_type=allwordssubstr&longdesc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&deadlinefrom=&deadlineto=&bug_id=22386%2C22387&bug_id_type=anyexact&emailassigned_to1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailcc2=1&emailtype2=substring&email2=&emaillongdesc3=1&emailtype3=substring&email3=&chfieldvalue=&chfieldfrom=&chfieldto=Now&j_top=AND&f1=noop&o1=noop&v1=&format=table&action=wrap

Comment 1

[Frédéric Buclin]

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 2

[Mateusz Goik]

Not only JS:

https://landfill.bugzilla.org/bugzilla-tip/userprefs.cgi?tab=account
Your real name (optional, but encouraged): TEST'"></onmouseover=alert(1) = " "


https://landfill.bugzilla.org/bugzilla-tip/report.cgi?x_axis_field=reporter_realname&y_axis_field=reporter_realname&z_axis_field=reporter_realname&no_redirect=0&query_format=report-table&short_desc_type=allwordssubstr&short_desc=&resolution=---&longdesc_type=allwordssubstr&longdesc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&deadlinefrom=&deadlineto=&bug_id=22388%2C5914&bug_id_type=anyexact&emailassigned_to1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailcc2=1&emailtype2=substring&email2=&emaillongdesc3=1&emailtype3=substring&email3=&chfieldvalue=&chfieldfrom=&chfieldto=Now&j_top=AND&f1=noop&o1=noop&v1=&format=table&action=wrap

.....
<div \""="" \"="" =="" onmouseover="alert(1)" \x3e\x3c\="" id="tabular_report_container_TEST\'\">
<table border="1" id="tabular_report">
    <thead>
    <tr>
      <th class="t1">
      </th>
        
        <th class="t2">&nbsp;
        </th>
        
        <th class="t1">TEST'"&gt;&lt;/onmouseover=alert(1) = " "
        </th>
      <th class="ttotal">
        Total
      </th>
    </tr>
    </thead>
  <tbody>
....

Comment 3

[Frédéric Buclin]

(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]

(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]

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]

I think I found the problem: oColumn.field needs to be escaped.

Comment 7

[Daniel Veditz [:dveditz]]

This bug shall be known as CVE-2013-1743

Comment 8

[Frédéric Buclin]

Attachment: 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]

Attachment: 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]

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 = '<a href="buglist.cgi?action=wrap&classification=aaaaaaa&classification=Unclassified&resolution=---&reporter_realname='.a*eval(alert(1))*-'&reporter_realname='
                          + bz_encode(oColumn.field)
                          + '">' + oData + '</a>';
    else
      elLiner.innerHTML = '<a href="buglist.cgi?action=wrap&classification=aaaaaaa&classification=Unclassified&resolution=---&reporter_realname='.a*eval(alert(1))*-'&reporter_realname='
                          + bz_encode(oRecord.getData("row_title"), 1)
                          + '&reporter_realname='
                          + bz_encode(oColumn.field) + '">' + oData + '</a>';
....

Comment 11

[Frédéric Buclin]

@Mateusz: is your testcase with my patch applied or not?

Comment 12

[Mateusz Goik]

(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]

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]

Attachment: patch, v2

Thanks for your testing, Mateusz, that's very helpful. :)

Comment 15

[Reed Loden [:reed]]

Comment on attachment 816391 [details] [diff] [review]
patch, v2

Can we use YAHOO.lang.escapeHTML() here?

Comment 16

[Reed Loden [:reed]]

See also bug 657194

Comment 17

[Frédéric Buclin]

(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]

Mateusz: did you manage to trigger XSS with my new patch?

Comment 19

[Mateusz Goik]

(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]]

Comment on attachment 816391 [details] [diff] [review]
patch, v2

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

r=dkl

Comment 21

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/reports/report-table.html.tmpl
Committed revision 8780.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified template/en/default/reports/report-table.html.tmpl
Committed revision 8626.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified template/en/default/reports/report-table.html.tmpl
Committed revision 8234.

Comment 22

[Frédéric Buclin]

Security advisory sent.