Bug 924932 (CVE-2013-1743)

[SECURITY] Field values are (still) not escaped correctly in tabular reports

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Reporting/Charting
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Mateusz Goik, Assigned: Frédéric Buclin)

Tracking

({sec-critical, wsec-xss})

4.1.1
Bugzilla 4.2
sec-critical, wsec-xss
Dependency tree / graph
Bug Flags:
approval +
approval4.4 +
blocking4.4.1 +
approval4.2 +
blocking4.2.7 +
sec-bounty +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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
Flags: sec-bounty?
(Assignee)

Comment 1

4 years ago
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).
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → Bugzilla 4.2
Version: unspecified → 4.1.1
(Assignee)

Updated

4 years ago
Summary: XSS in report.cgi → [SECURITY] Field values are (still) not escaped correctly in tabular reports
(Reporter)

Comment 2

4 years ago
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>
....
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
I can reproduce comment 0 only if the horizontal axis has evil data. If only the vertical axis has it, it's fine.
(Assignee)

Comment 6

4 years ago
I think I found the problem: oColumn.field needs to be escaped.
(Assignee)

Updated

4 years ago
Flags: blocking4.4.1+
Flags: blocking4.2.7+
This bug shall be known as CVE-2013-1743
Alias: CVE-2013-1743
(Assignee)

Updated

4 years ago
Blocks: 926085
(Assignee)

Comment 8

4 years ago
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.
Assignee: charting → LpSolit
Status: NEW → ASSIGNED
Attachment #816273 - Flags: review?(dkl)
(Assignee)

Updated

4 years ago
Blocks: 859188
(Assignee)

Updated

4 years ago
Blocks: 912643
(Assignee)

Comment 9

4 years ago
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 "> <").
Attachment #816273 - Attachment is obsolete: true
Attachment #816273 - Flags: review?(dkl)
Attachment #816319 - Flags: review?(dkl)
(Reporter)

Comment 10

4 years ago
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;';
....
(Assignee)

Comment 11

4 years ago
@Mateusz: is your testcase with my patch applied or not?
(Reporter)

Comment 12

4 years ago
(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])
(Assignee)

Comment 13

4 years ago
The problem only happens when the z-axis is set to the field containing evil data. I will investigate.
(Assignee)

Comment 14

4 years ago
Created attachment 816391 [details] [diff] [review]
patch, v2

Thanks for your testing, Mateusz, that's very helpful. :)
Attachment #816319 - Attachment is obsolete: true
Attachment #816319 - Flags: review?(dkl)
Attachment #816391 - Flags: review?(dkl)
Comment on attachment 816391 [details] [diff] [review]
patch, v2

Can we use YAHOO.lang.escapeHTML() here?
See also bug 657194
(Assignee)

Comment 17

4 years ago
(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().
(Assignee)

Comment 18

4 years ago
Mateusz: did you manage to trigger XSS with my new patch?
(Reporter)

Comment 19

4 years ago
(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 on attachment 816391 [details] [diff] [review]
patch, v2

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

r=dkl
Attachment #816391 - Flags: review?(dkl) → review+

Updated

4 years ago
Flags: approval?
Flags: approval4.4?
Flags: approval4.2?
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval4.2?
Flags: approval4.2+
Flags: approval+
(Assignee)

Comment 21

4 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Group: bugzilla-security
(Assignee)

Comment 22

4 years ago
Security advisory sent.
Flags: sec-bounty? → sec-bounty+
Keywords: sec-critical, wsec-xss
(Assignee)

Updated

4 years ago
Duplicate of this bug: 961656
You need to log in before you can comment on or make changes to this bug.