Last Comment Bug 330555 - [SECURITY] 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
Status: RESOLVED FIXED
[ready for 2.18.6][ready for 2.20.3][...
:
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 2.15
: All All
-- major (vote)
: Bugzilla 2.18
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
Depends on:
Blocks: 317126 321556 330290 346525
  Show dependency treegraph
 
Reported: 2006-03-15 03:45 PST by Frédéric Buclin
Modified: 2006-10-15 03:00 PDT (History)
6 users (show)
justdave: approval+
justdave: approval2.22+
mkanat: blocking2.22.1+
justdave: approval2.20+
mkanat: blocking2.20.3+
justdave: approval2.18+
mkanat: blocking2.18.6+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for tip, v1 (7.90 KB, patch)
2006-07-30 09:18 PDT, Frédéric Buclin
justdave: review+
Details | Diff | Splinter Review
patch for 2.22, v1 (7.91 KB, patch)
2006-07-30 11:30 PDT, Frédéric Buclin
justdave: review+
Details | Diff | Splinter Review
patch for 2.20, v1 (10.13 KB, patch)
2006-07-30 14:12 PDT, Frédéric Buclin
justdave: review+
Details | Diff | Splinter Review
patch for 2.18, v1 (7.56 KB, patch)
2006-07-30 16:49 PDT, Frédéric Buclin
justdave: review+
Details | Diff | Splinter Review

Description User image Frédéric Buclin 2006-03-15 03:45:57 PST
[% 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 User image GavinS 2006-03-15 04:05:16 PST
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 User image Max Kanat-Alexander 2006-03-15 10:37:33 PST
That makes this a sec bug.
Comment 3 User image Frédéric Buclin 2006-04-17 09:47:03 PDT
For security reasons, it's safer if global/headers.html.tmpl filters titles and H1-H3 itself.
Comment 4 User image Teemu Mannermaa (:wicked) 2006-04-17 09:53:17 PDT
Hmm, can title tag contain HTML formatting? I thought it was pure text.
Comment 5 User image Frédéric Buclin 2006-04-17 09:56:29 PDT
(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.
Comment 6 User image Teemu Mannermaa (:wicked) 2006-04-17 13:25:59 PDT
(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. :)
Comment 7 User image Frédéric Buclin 2006-07-29 18:19:46 PDT
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.
Comment 8 User image Max Kanat-Alexander 2006-07-30 08:06:33 PDT
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.
Comment 9 User image Frédéric Buclin 2006-07-30 09:18:08 PDT
Created attachment 231308 [details] [diff] [review]
patch for tip, v1
Comment 10 User image Frédéric Buclin 2006-07-30 11:30:52 PDT
Created attachment 231318 [details] [diff] [review]
patch for 2.22, v1
Comment 11 User image Frédéric Buclin 2006-07-30 14:12:16 PDT
Created attachment 231335 [details] [diff] [review]
patch for 2.20, v1
Comment 12 User image Frédéric Buclin 2006-07-30 16:49:10 PDT
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.
Comment 13 User image Marc Schumann [:Wurblzap] 2006-09-08 23:02:03 PDT
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?
Comment 14 User image Frédéric Buclin 2006-09-09 02:23:23 PDT
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.
Comment 15 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2006-09-10 13:22:13 PDT
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.
Comment 16 User image Max Kanat-Alexander 2006-09-23 22:07:59 PDT
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.)
Comment 17 User image Frédéric Buclin 2006-10-14 13:55:29 PDT
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
Comment 18 User image Max Kanat-Alexander 2006-10-15 03:00:54 PDT
Security Advisory has been sent, so this bug is no longer private.

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