[SECURITY] H1, H2 and H3 are not filtered in global/header.html.tmpl

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Bugzilla-General
--
major
RESOLVED FIXED
11 years ago
11 years ago

People

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

Tracking

2.15
Bugzilla 2.18
Dependency tree / graph
Bug Flags:
approval +
approval2.22 +
blocking2.22.1 +
approval2.20 +
blocking2.20.3 +
approval2.18 +
blocking2.18.6 +

Details

(Whiteboard: [ready for 2.18.6][ready for 2.20.3][ready for 2.22.1][ready for 2.23.3])

Attachments

(4 attachments)

(Assignee)

Description

11 years ago
[% IF !h1.defined %][% h1 = title %][% END %]

    <title>[% title FILTER html %]</title>

  [% IF h1 %]
    <h1>[% h1 %]</h1>
  [% END %]


If h1 is not defined, which is the case on most pages, h1 has the same content as title, but only title is filtered, h1 is not (see above).

I was reviewing bug 330290 and I wondered why only part of the user identity was displayed in h1 while it was correctly displayed in the title. The reason was because we had:

<title>Edit user Guest 4 &lt;guest4&#64;bugzilla.org&gt;</title>

<div id="header">
    <h1>Edit user Guest 4 <guest4@bugzilla.org></h1>
</div>

"<" and ">" have not been filtered in h1 and Firefox ignored this "tag". If I wrote "script ..." instead of guest4, the JS code would have been executed.

Comment 1

11 years ago
Also discussed in bug#317126

Need to firm up the interface to headers.html.tmpl. Either callers filter all params, or headers template filters all, or we don't default <h1>, and require it from callers

Changes will be required in a few callers.

Comment 2

11 years ago
That makes this a sec bug.
Group: webtools-security
(Assignee)

Comment 3

11 years ago
For security reasons, it's safer if global/headers.html.tmpl filters titles and H1-H3 itself.
Hmm, can title tag contain HTML formatting? I thought it was pure text.
(Assignee)

Comment 5

11 years ago
(In reply to comment #4)
> Hmm, can title tag contain HTML formatting? I thought it was pure text.
> 

It is, that's why <title> must be filtered in all cases. Consequently, we have no other choices than filtering H1-H3 too. And to be sure this is done, we should do it in a centralized way.
(In reply to comment #5)
> (In reply to comment #4)
> > Hmm, can title tag contain HTML formatting? I thought it was pure text.
> It is, that's why <title> must be filtered in all cases. Consequently, we have

Then why some templates are trying to output HTML to title anyway? Like your patch in attachment 216459 [details] [diff] [review] on bug 44595. Hmm, maybe they are trying to provide value for h1 (that I think can have some markup, but I'm not sure). That's just stupid. They should use h1 specifically and provide two different values (one for title and one for h1).

This begins to sound like I want title and h1 decoupled on the header template. :)
Blocks: 321556
(Assignee)

Updated

11 years ago
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
Version: 2.20 → 2.18.5
(Assignee)

Comment 7

11 years ago
I just did a quick audit of some of our templates on tip, and I realized that most titles and H1-H3 strings were filtered in templates themselves, before being passed to header.html.tmpl. In comment 3 and even today on IRC, I suggested to change this behavior and do the filtering in header.html.tmpl. But now I change my mind for the following reasons:

1. Looks like > 95% of all templates pass filtered strings. So fixing the remaining 5% is much less invasive and prone to regressions.

2. If we decided to pass unfiltered strings to header.html.tmpl, filterexceptions.pl (and 008filter.t) would complain that these fields are... unfiltered, forcing us to add boring "FILTER none" almost everywhere, or even worse: forcing us to add these fields in filterexceptions.pl, which I consider as a potentially security problem, as 008filter.t wouldn't complain anymore if we forget to filter a directive elsewhere in the template. So this last suggestion is not acceptable.

And we can as well write "FILTER html" instead of "FILTER none" directly in templates. The number of characters is exactly the same! ;)

I want this bug fixed asap, in addition to bug 206037. This problem is even more visible on 2.18 and 2.20, where title filtering is almost inexistant.
Assignee: general → LpSolit
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Blocks: 346524
(Assignee)

Updated

11 years ago
Blocks: 346525
(Assignee)

Updated

11 years ago
No longer blocks: 346524
(Assignee)

Updated

11 years ago
Flags: blocking2.22.1?
Flags: blocking2.20.3?
Flags: blocking2.18.6?

Comment 8

11 years ago
The filter test wouldn't complain that a variable is unfiltered inside of a PROCESS statement, or inside of a SET statement, right?

I think the only reason to require callers to filter h1/title/h2 is if we *want* to have HTML code in them. Otherwise, if they should never have HTML, seems like they should be filtered inside header.html.tmpl.

I see at least in our 2.20 example that this is exploitable on an admin page, so that makes this a valid sec. blocker.
Flags: blocking2.22.1?
Flags: blocking2.22.1+
Flags: blocking2.20.3?
Flags: blocking2.20.3+
Flags: blocking2.18.6?
Flags: blocking2.18.6+
(Assignee)

Comment 9

11 years ago
Created attachment 231308 [details] [diff] [review]
patch for tip, v1
Attachment #231308 - Flags: review?(justdave)
(Assignee)

Comment 10

11 years ago
Created attachment 231318 [details] [diff] [review]
patch for 2.22, v1
Attachment #231318 - Flags: review?(justdave)
(Assignee)

Comment 11

11 years ago
Created attachment 231335 [details] [diff] [review]
patch for 2.20, v1
Attachment #231335 - Flags: review?(justdave)
(Assignee)

Comment 12

11 years ago
Created attachment 231345 [details] [diff] [review]
patch for 2.18, v1

For 2.18, I forced PutHeader() in CGI.pl to filter title, h1 and h2 itself for not yet templatized pages. This is way much less invasive than fixing all these edit*.cgi scripts. And in opposition to 2.20 and newer, URLs are never passed to headers so it's safe to do it here and doesn't break current behavior.
Attachment #231345 - Flags: review?(justdave)
(Assignee)

Updated

11 years ago
Blocks: 317126
This bug is scheduled for the same release as bug 321556, but they depend on each other. As this is a sec bug, and as such needs to go in more or less at time of release, leaving no time for bug 321556, it'd make sense to reverse the dependency--but this would hit this bug's tip patch with bad rottage. Shall we reverse the dependency anyway?
(Assignee)

Comment 14

11 years ago
Marc, please no. We would like to start QA tests next week, assuming all security patches will be reviewed meanwhile. Anyway, I won't prevent you from working on your skin patch. If yours is checked in before this one is reviewed, I will unbitrot it (I have no other choices anyway). So let's keep the dependencies unchanged, but feel free to work on your patch.
(Assignee)

Updated

11 years ago
Whiteboard: patches awaiting review from justdave
Comment on attachment 231308 [details] [diff] [review]
patch for tip, v1

This looks good as is.  Although there isn't a dependency set, I seem to recall someone saying this depended on the patch on bug 206037, and I had both that patch and this one applied in tandem for review purposes.
Attachment #231308 - Flags: review?(justdave) → review+
Attachment #231318 - Flags: review?(justdave) → review+
Attachment #231335 - Flags: review?(justdave) → review+
Attachment #231345 - Flags: review?(justdave) → review+
(Assignee)

Updated

11 years ago
Flags: approval?
Flags: approval2.22?
Flags: approval2.20?
Flags: approval2.18?
Whiteboard: patches awaiting review from justdave → [ready for 2.18.6][ready for 2.20.3][ready for 2.22.1][ready for 2.23.3]

Comment 16

11 years ago
As nearly as I can tell from looking at bonsai, every version of Bugzilla that's ever had templates was affected by this bug, meaning it goes back to about 2.15. So I'm setting the version field appropriately. (This stuff is needed for the security advisory.)
Version: 2.18.5 → 2.15
Flags: approval?
Flags: approval2.22?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
(Assignee)

Comment 17

11 years ago
tip:

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.80; previous revision: 1.79
done
Checking in template/en/default/admin/flag-type/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.21; previous revision: 1.20
done
Checking in template/en/default/admin/groups/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/admin/groups/remove.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/remove.html.tmpl,v  <--  remove.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/users/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/admin/users/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/bug/show.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.html.tmpl,v  <--  show.html.tmpl
new revision: 1.15; previous revision: 1.14
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.68; previous revision: 1.67
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.47; previous revision: 1.46
done
Checking in template/en/default/reports/duplicates.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/duplicates.html.tmpl,v  <--  duplicates.html.tmpl
new revision: 1.17; previous revision: 1.16
done


2.22:

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.61.2.2; previous revision: 1.61.2.1
done
Checking in template/en/default/admin/flag-type/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.14.2.2; previous revision: 1.14.2.1
done
Checking in template/en/default/admin/groups/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.5.6.2; previous revision: 1.5.6.1
done
Checking in template/en/default/admin/groups/remove.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/remove.html.tmpl,v  <--  remove.html.tmpl
new revision: 1.1.10.1; previous revision: 1.1
done
Checking in template/en/default/admin/users/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.7.2.3; previous revision: 1.7.2.2
done
Checking in template/en/default/admin/users/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.2.2.2; previous revision: 1.2.2.1
done
Checking in template/en/default/bug/show.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.html.tmpl,v  <--  show.html.tmpl
new revision: 1.11.2.1; previous revision: 1.11
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.40.2.1; previous revision: 1.40
done
Checking in template/en/default/reports/components.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/components.html.tmpl,v  <--  components.html.tmpl
new revision: 1.9.4.2; previous revision: 1.9.4.1
done
Checking in template/en/default/reports/duplicates.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/duplicates.html.tmpl,v  <--  duplicates.html.tmpl
new revision: 1.15.2.1; previous revision: 1.15
done


2.20.2:

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.85.2.10; previous revision: 1.85.2.9
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.43.2.6; previous revision: 1.43.2.5
done
Checking in template/en/default/admin/flag-type/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.11.4.2; previous revision: 1.11.4.1
done
Checking in template/en/default/admin/groups/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.5.4.2; previous revision: 1.5.4.1
done
Checking in template/en/default/admin/groups/remove.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/remove.html.tmpl,v  <--  remove.html.tmpl
new revision: 1.1.8.1; previous revision: 1.1
done
Checking in template/en/default/admin/products/groupcontrol/confirm-edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/groupcontrol/confirm-edit.html.tmpl,v  <--  confirm-edit.html.tmpl
new revision: 1.5.10.2; previous revision: 1.5.10.1
done
Checking in template/en/default/admin/users/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.4.2.6; previous revision: 1.4.2.5
done
Checking in template/en/default/admin/users/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.1.4.2; previous revision: 1.1.4.1
done
Checking in template/en/default/bug/show.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.html.tmpl,v  <--  show.html.tmpl
new revision: 1.10.6.1; previous revision: 1.10
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.39.4.2; previous revision: 1.39.4.1
done
Checking in template/en/default/list/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.37.2.3; previous revision: 1.37.2.2
done
Checking in template/en/default/reports/components.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/components.html.tmpl,v  <--  components.html.tmpl
new revision: 1.9.2.2; previous revision: 1.9.2.1
done
Checking in template/en/default/reports/duplicates.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/duplicates.html.tmpl,v  <--  duplicates.html.tmpl
new revision: 1.14.10.1; previous revision: 1.14
done


2.18.5:

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/Attic/CGI.pl,v  <--  CGI.pl
new revision: 1.211.2.11; previous revision: 1.211.2.10
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.38.2.4; previous revision: 1.38.2.3
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.16.2.4; previous revision: 1.16.2.3
done
Checking in template/en/default/admin/flag-type/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.6.2.5; previous revision: 1.6.2.4
done
Checking in template/en/default/admin/products/groupcontrol/confirm-edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/groupcontrol/confirm-edit.html.tmpl,v  <--  confirm-edit.html.tmpl
new revision: 1.5.2.1; previous revision: 1.5
done
Checking in template/en/default/bug/show.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.html.tmpl,v  <--  show.html.tmpl
new revision: 1.6.2.5; previous revision: 1.6.2.4
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.24.2.6; previous revision: 1.24.2.5
done
Checking in template/en/default/list/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.25.2.3; previous revision: 1.25.2.2
done
Checking in template/en/default/reports/components.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/components.html.tmpl,v  <--  components.html.tmpl
new revision: 1.8.2.2; previous revision: 1.8.2.1
done
Checking in template/en/default/reports/duplicates.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/duplicates.html.tmpl,v  <--  duplicates.html.tmpl
new revision: 1.14.2.1; previous revision: 1.14
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Summary: H1, H2 and H3 are not filtered in global/header.html.tmpl → [SECURITY] H1, H2 and H3 are not filtered in global/header.html.tmpl

Comment 18

11 years ago
Security Advisory has been sent, so this bug is no longer private.
Group: webtools-security
You need to log in before you can comment on or make changes to this bug.