629395 - Unescaped interpolations in "title" calls in global/messages.html.tmpl

Status: NEW

(Bugzilla :: User Interface, defect)

Description

[Gervase Markham [:gerv]]

(Marking secure just in case).

We don't filter the "title" attribute in header.html.tmpl. This may have been a questionable design decision; it means that every call site has to filter the passed value instead (and there are a large number). Fortunately, often we do it as:

[% title = BLOCK %]
...
[% END %]

[% PROCESS global/header.html.tmpl title = title %]

which means that 008filter.t will catch any unfiltered directives. But if you do directly:

[% PROCESS global/header.html.tmpl title = "Something About $variable" %]

then it won't get picked up by that test.

I did a quick audit and only found a couple of places where a value is used in this interpolated way without escaping:

      [% title = "User $loginold updated" %]
      [% title = "User $otheruser.login not changed" %]
      [% title = "User $otheruser.login deleted" %]
(all from global/messages.html.tmpl).

Now I think that's a validated user object and so it can only contain email characters and so we are safe... but a) I want confirmation, b) my check wasn't very rigorous and c) perhaps others can think of ways we need to audit the code based on the above analysis.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Also, we should probably document somewhere that when people are making page.cgi pages, passing unescaped values to header.html.tmpl in 'title' is a no-no. (That's how I found this.)

Gerv

Comment 2

[Frédéric Buclin]

'title' being unfiltered is per design, see bug 330555. So this is intentional. The three titles you list in comment 0 all come from bug 119485, which landed in Bugzilla 2.20. But the login name cannot contain dangerous stuff. validate_email_syntax() excludes all these characters:

  $addr !~ /[\\\(\)<>&,;:"\[\] \t\r\n]/

So this is not a security bug. What we should do, though, is to forbid $variables in quotes, to force us to correctly escape them (or explicitly use FILTER none). The only allowed $variables should be $terms.foo, as those are controlled by the maintainer only.

So we should morphe this bug into "Add a test which catches $variables in quotes".

Comment 3

[Gervase Markham [:gerv]]

LpSolit: I seem to remember us deciding to do it, but couldn't remember why; your memory is better than mine :-) I still think the use case for HTML in <title> is very weak, and we would have been better off filtering it in header.html.tmpl. Still, you are right - the right thing to do now is to have a test to catch $ usage in titles. But given that someone could do:

[% title = BLOCK %]
  "Title is " _ [% something FILTER html %] _ 'More
   string' _ "A \'string\'"
   ... <arbitrary complexity>
   "\'$Foo'" _ " Bar"
[% END %]

is it actually possible to do that using a regexp?

Gerv

Comment 4

[Frédéric Buclin]

If someone is crazy enough to write such an unreadable string, reviewers won't let this go through. Note that what I said in comment 2 is not limited to titles, but to all variables used in quotes, being for titles or not.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #2)
> So this is not a security bug. What we should do, though, is to forbid
> $variables in quotes, 

  Ah, no, that's an important functionality.

> So we should morphe this bug into "Add a test which catches $variables in
> quotes".

  Well, or some better code in the filter.t test that makes sure that those variables are actually being filtered properly (or being added to filterexceptions).