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

NEW
Unassigned

Status

()

Bugzilla
User Interface
7 years ago
4 years ago

People

(Reporter: gerv, Unassigned)

Tracking

Details

(Reporter)

Description

7 years ago
(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
(Reporter)

Comment 1

7 years ago
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

7 years ago
'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".
Group: bugzilla-security
(Reporter)

Comment 3

7 years ago
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

7 years ago
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

7 years ago
(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).
You need to log in before you can comment on or make changes to this bug.