192677 - Audit all templates for FILTER usage

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Gervase Markham [:gerv]]

Before we release 2.18, we should assign every template to someone, preferably
not its author, who should audit it for correct FILTER usage. I'm bored of bugs
like bug 192661.

Gerv

Comment 1

[Gervase Markham [:gerv]]

One idea (and I think this sucks a bit, but I suggest it merely to provoke
discussion) is to have _all_ printed things filtered in some way, but have a
no-op filter for when it's not actually needed. This forces template authors to
think about filtering.

Gerv

Comment 2

[Dave Miller [:justdave]]

I might point out that the situation in bug 192661 does not involve a template,
the data in question is being generated by perl code.

The templates already got audited in bug 133423.  (though I'll admit there's
been a lot of templates touched since then)

Comment 3

[Gervase Markham [:gerv]]

IMO we either need to do it before every release, or invent some automated way
of doing it. Blue sky idea: a Perl script goes through each templates, looking
for all directives. Any without FILTER it flags, unless they are in an
"exclusion list" for that template. If you change the template to add new things
which definitely don't need to be filtered, you change the exclusion list.

How feasible would that be?

Gerv

Comment 4

[Dave Miller [:justdave]]

That sounds like a cool idea.  Zach?

Comment 5

[Bradley Baetz (:bbaetz)]

I think that thats sort of difficult. I guess you could use Template::Parser to
do the parsing, but....

Even ignoring the NPC-ness of the problem in general (which may not arise for
TT's limited grammar if we don't call any perl subs), note that you'd have to track:

[% foo = bar | html %]

[% foo %]

as being OK. Don't forget about:

[% IF bar %] [% foo = foo | html %] [% END %]
...
[% foo %] -- error!
...
[% foo IF bar %] -- OK!

I'll note that gcc doesn't get that last case right yet, wrt uninited var warnings.

Unless you want to spend a _lot_ of effort on it, I suspect that you're going to
get too my false positives to make it useful.

I'm willing to be proved wrong, though - it may be that we don't do anything
complicated enough in TT to worry about.

The other option would be to taint the vars, and have the print at the end check
for taintedness. I really don't think that we want to go there, though.

Comment 6

[Gervase Markham [:gerv]]

It wouldn't be anything like that smart. If your template, file.html.tmpl was

[% fred FILTER html %]
[% bar = foo FILTER html %]
[% bar %]
[% foo %]
[% wilma %]
[% bug.${wibble}.fish %]

And your exclusion file was:

file.html.tmpl:
foo
bug.${wibble}.fish

Then it would flag an issue with lines 3 and 5 because they don't contain the
word FILTER (which we are standardising on instead of | for easier user
modification and understanding) and their text isn't in the exclusion file.

You would need to add "bar" to the list of "OK, I know this one's fine", and
notice that the use of wilma is a bug.

So it would be merely textual comparison of the contents of the tag. I think
this would catch most of the problems, although you might need to be smart about
multi-line tags.

Gerv

Comment 7

[Zach Lipton [:zach]]

Gerv: thanks for saving my soul, I really didn't want to deal with a NP-Complete
problem. This doesn't seem like it would be that hard, but there's a few issues
that I can see:

1. Line wrapping: I would be inclined to parse through each template file
line-by-line, but what if there is something like this?

[% foo 
bar %]

Would a split() call of some sort be a better way to do this?

2. Time: Depending on how nasty the regexps get for this, it could take a pretty
long time to check.

3. Exception list: What would you think about including the exception list right
in the template file (as a comment) so that it's easy to maintain?

Comment 8

[Gervase Markham [:gerv]]

Line wrapping: we could probably do it by ignoring all lines other than the
first. More specifically:

Read foo.html.tmpl. Get all occurrences of:

/\[%\s*(.*)\s*(%\]|\n)/

i.e. terminate with either %] or \n. Remove all of those which match FILTER, or
begin with a capital letter (IF, WHILE etc). Sort the remainder, sort the
exclusion list, and remove duplicates. Print errors for all remaining tags, and
warnings for redundant exclusion entries.

Maintaining the exclusion list is easy - you just copy and paste the first line
(or the only line) of your tag.

Regexps: see above.

Exception list: this might work. Perhaps we could have it at the end - I was
hoping to keep general ugliness out of sight.

Another issue: localisations. Do we ask localisers to maintain the list? After
all, some entries might be:
[% foo = "Some Translatable Text" %]
and if they translated it, it would no longer match. Hmm. Well, if it's a test,
then they won't be running them anyway. So that's OK.

I'm still trying to think of a nice way to mark each one inline as "no filter",
but I think we'd need to hack TT to do that - all of the solutions I can think
of are either ugly or syntax errors. Some other toolkit auto-HTML-filter
everything, unless you specifically ask it not to. But they probably won't do
that for us.

Gerv

Comment 9

[Dave Miller [:justdave]]

Securing, since we now have known exploits.

Adding the maintainers of the localized templates that we know of, since this
affects them.

Comment 10

[Gervase Markham [:gerv]]

Kicking off people I don't know; the code I'm about to attach reveals XSS
vulnerabilities. If anyone can vouch for [e-mail], please add a
comment.

Gerv

Comment 11

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This adds a test to extract all TT directives and, after eliminating certain
classes of safe ones, check them against a safe list.

It particularly needs checking that:

- I've used all the correct code idioms and styles for tests
- My use of include "foo.pl" couldn't be improved

There's a hack in it which makes the test pass if that template currently
doesn't have an exclusion list in the file. This is to make sure tests continue
to pass during the transition period. Once all templates have lists, this hack
will be removed.

Gerv

Comment 12

[Dave Miller [:justdave]]

Comment on attachment 114602 [details] [diff] [review]
Patch v.1

>Index: t/008filter.t
>+    # XXX Currently, we come to a crashing halt if this file isn't found,
>+    # rather than execute the error code.                        
>+    if (!require "filterexceptions.pl") {

If filterexceptions.pl is located at template/en/default, then shouldn't we do
something to either move into that directory, or explicitly include the path to
it with the filename?  In fact, this crashes when I run it, with the
filterexceptions.pl in the correct place.

>+            # Params
>+            next if $directive =~ /^Param\(/;

I'm not sure all of the params are safe.  Although on the other hand, we can
probably assume that the admin wouldn't be stupid enough or mean enough to put
exploits in the params....

Comment 13

[Gervase Markham [:gerv]]

> If filterexceptions.pl is located at template/en/default, then shouldn't we do
> something to either move into that directory, or explicitly include the path to
> it with the filename?  In fact, this crashes when I run it, with the
> filterexceptions.pl in the correct place.

I probably need some help with this; the construct I used is the only way I
could get it to work. A "print `pwd`;" inside and outside the loop indicated
that the loop construct changed the working directory to the $path, so I just
referenced the file without any path stuff, and it worked for me. Suggestions as
to what is going on are welcome.

> I'm not sure all of the params are safe.  Although on the other hand, we can
> probably assume that the admin wouldn't be stupid enough or mean enough to put
> exploits in the params....

Given that XSS is all about auth stealing, and the admin has full auth, yeah :-) 

Gerv

Comment 14

[Dave Miller [:justdave]]

ok, if I put $path/ in front of both exceptionlist.pl and $file (when it gets
opened), then it works.

Failed Test   Stat Wstat Total Fail  Failed
t/008filter.t   80 20480   110   80  72.73%

oof.

Comment 15

[Gervase Markham [:gerv]]

Attachment: Output of test script - possibly dodgy directives

This file is the output if you let the test script loose on all the templates
without any exclusions. To find the problems, you need to go through everything
on this list and check whether it's safe or not (which takes about 7 hours, top
to bottom.) Creating filterexceptions.pl as you go is the best way; I just
added everything to filterexceptions.pl and then added comments if a particular
directive was an XSS hole, or an email address, or revealed a bug, or whatever.

Comment 16

[Dave Miller [:justdave]]

Attachment: same output from the 2.16 branch

Comment 17

[Dave Miller [:justdave]]

Attachment: Patch v2 (for trunk)

Here's the test again with the modifications I had to make to get it to
actually run.  Other than explicitly stating paths where needed, there's no
difference from what Gerv uploaded.

Comment 18

[Dave Miller [:justdave]]

Attachment: Patch v2 for 2.16 branch

This patch contains both the new test and the additional changes to the testing
structure to allow the new test to run on the 2.16 branch.

Comment 19

[Dave Miller [:justdave]]

Since localization of the templates in most cases probably didn't fix any of the
security holes in the process, I would suggest that the localization maintainers
help coordinate with this.  At this point it'd probably be safest to either help
with the trunk's or the 2.16 branch's English templates or just sit back and
wait until we get the process down, and once we know what we're doing, then go
back and run the same procedure over all of the localized templates.  If we get
everyone ready, we can run a joint security advisory which covers all of them
either later this week or this coming weekend.

If you haven't updated your 2.16.x templates for 2.16.2, this would probably be
a good time to dive in and make them compatible with 2.16.3. :-)  This is some
pretty serious security vulnerabilities here.

The German templates are the only ones I'm aware of that have been somewhat kept
up-to-date on the trunk...

A good way to find out what's changed in the templates since the version you
made them against is to run a bonsai query like the following, which shows all
changes to date within the template/en/default directory on the 2.16 branch from
2.16.0 to the present:

http://bonsai.mozilla.org/cvsquery.cgi?branch=BUGZILLA-2_16-BRANCH&branchtype=match&dir=mozilla%2Fwebtools%2Fbugzilla%2Ftemplate%2Fen%2Fdefault&sortby=Date&date=explicit&mindate=2002-07-28

hint: there's been 0 changes to templates on the 2.16 branch since 2.16.2 except
for the ones we're about to make as a result of this patch.

The tip however has been a moving target.

Comment 20

[Dave Miller [:justdave]]

Comment on attachment 114621 [details] [diff] [review]
Patch v2 for 2.16 branch

The testing structure on the 2.16 branch has been updated and checked in to
accomodate this.  (And yes, I remembered to put back processmail and
syncshadowdb on the "additional_files" list)

The same patch will now apply cleanly on both branches for adding the new test.
 I've made more modifications to the test, which I'll be uploading shortly

Comment 21

[Dave Miller [:justdave]]

Attachment: Patch v3 (applies to both branches)

Revised test.  changes:

1. this copy actually tests everything (got rid of the commented-out stuff)
2. removed the linefeed stripping.  If you use /s on the end of the regexp, it
ignores them (and \s within the regexp will match the newline), and if we were
going to do it, we should have been converting to spaces instead of just
getting rid of them anyway, so things wouldn't accidently get concatenated as a
single word).
3. Now reports the line number on which the dodgey filter was found, in
addition to the name of the variable or string which wasn't filtered.

The runtests.pl script in cvs (on both branches) now allows you to pass it a
test number to run only that test, so if you don't want to sit through the
other 7, you can do "./runtests.pl --verbose 008" to run only the filter test.

Comment 22

[Gervase Markham [:gerv]]

Attachment: New 008filter.t

Since I uploaded my version of the patch, I made several tweaks to the regexps
to cover more safe cases. This copy of 008filter.t is a merge of Dave's changes
and mine.

Changes:

- We don't scan png or txt files. Neither supports embedded scripting.
- Allow $ in the names of variables on the left side of an assignment
- Deal with comments which also have a whitespace control modifier
- Fix the "trailing whitespace" regexp so it removes modifiers correctly.

I've had to remove the random "+1" on the end of the count of files to scan,
and I've also had to remove the $path variable to get it to work for me. If you
need it, but it back - but we really need bbaetz or zach to explain what the
heck is going on here :-|

Gerv

Comment 23

[Gervase Markham [:gerv]]

Attachment: 008filter.t v.2

And another update. Not all FILTERs are created equal; we now have a whitelist
of escaping filters, which means that, for example, "FILTER strike" does not
bypass the test. Not that I'm convinced that this filter is worth having for
the one time we use it, but anyway...

Gerv

Comment 24

[Vitaly Fedrushkov]

Had the latter ran against Bugzilla-Russian.  Result is self-explanatory, as B-
R is 2.16, too.

Is it OK to post a patch to SourceForge, or do we need some time before things 
could be disclosed safely?  Is there any deadline for 2.16.3?

Comment 25

[Gervase Markham [:gerv]]

Please do not post a patch to Sourceforge yet, or mention this anywhere public.
(How would you know what to patch, by the way?) 

We need some time to make sure we've found them all, then evaluate each
vulnerability, find which codebases it is in and when it was introduced, and
produce patches for 2.16, 2.17 and any localised template sets. Ideally, these
patches will all be released together.

We are dealing with this problem with the greatest possible urgency, but the
scope means that we will not be in a position to release patches for all
affected versions of Bugzilla for a while yet.

Gerv

Comment 26

[Tobias Burnus]

Since the filterexceptions.pl mostly contains data base names, how about using a
central exception file, e.g. located at templates/ instead of iterating over all
the include paths?

Or alternatively check whether templates/*/{custom,default}/filterexceptions.pl
exists and if not load templates/filterexceptions.pl instead.

Comment 27

[Gervase Markham [:gerv]]

We want to make it easy to distribute exception lists with e.g. language packs.
We hope langpack authors and custom template authors will use this mechanism to
keep their templates safe too.

The conceptually simplest and easiest method is one file per template set IMO.

Gerv

Comment 28

[Gervase Markham [:gerv]]

Is anyone apart from me and Dave able to give some time to help with sorting
this out? All this auditing is quite a bit of work, and we could do with some help.

Gerv

Comment 29

[Jacob Steenhagen]

I was just about to try putting some time toward this. Can you ping my on IRC
and give some specific direction (which files, etc)?

Comment 30

[Jacob Steenhagen]

Attachment: 008filter.t v3

Made the following changes so far:
* Added the $path stuff ... don't know why it's being different
* Added 'csv' the the list of templates that don't require filters
* Added support for the alternate comment type[1]
* Added an exception for PerformSubsts	(which pulls info from Param()s)
* Added support for the .push template method
* Ignore the [% tmpl.variable ? 2 : 1 %] format as long as '2' & '1' and ints

1. http://www.template-toolkit.org/docs/plain/Manual/Syntax.html#Comments

Comment 31

[Gervase Markham [:gerv]]

> * Added 'csv' the the list of templates that don't require filters

I specifically _didn't_ do that. Although it's not a source of security
problems, CSV directives need FILTERing too, and this tool is a handy way to
make sure that happens.

> * Added support for the alternate comment type[1]

No - the correct answer is to standardise the comments. :-)

> * Added an exception for PerformSubsts	(which pulls info from Param()s)

Are the results of this guaranteed safe? How many times do we call PerformSubsts?

> * Added support for the .push template method
> * Ignore the [% tmpl.variable ? 2 : 1 %] format as long as '2' & '1' and ints

This forms are only used in a few places, and the regexps needed to catch them
are somewhat complicated (particularly for alternation). I am not confident that
it might not accidentally let something through that it shouldn't.

IMO, automatic exceptions should only be granted to large classes of directive
which are guaranteed safe. I have thought long and hard about the exceptions
permitted, and experimented extensively to make sure the current ones don't
leak. Each exception imposes a burden if you want to prove that the script
catches all possible holes, and so we should try and minimise the exceptions.

Gerv

Comment 32

[Jacob Steenhagen]

> CSV directives need FILTERing too

So did you just ignore all the CSV errors or add them to exceptions?

> No - the correct answer is to standardise the comments. :-)

I agree (and that's what I originally did), but that's a bit beyond what we
should be changing for a security release, isn't it?

> Are the results of this guaranteed safe? How many times do we call 
> PerformSubsts?

It's called once. I don't remember why but I had a hard to adding it to the
exception file (I couldn't get it to work).  It's as safe as the Param() calls are.

Or maybe it was the .push call that I was having a hard time with... I don't
remember for sure, now.  The ? : construct is used 4 times in the templates with
numbers. While that's not really a lot, it's also not in any way possible to be
harmful. It's also used at least another 14 times according to grep without
numbers. I don't recall running into any of those yet, however.

Comment 33

[Bradley Baetz (:bbaetz)]

            next if $directive =~ /^(IF|END|UNLESS|FOREACH|PROCESS|INCLUDE|
                                     BLOCK|USE|ELSE|NEXT|LAST|DEFAULT|FLUSH|
                                     ELSIF|SET)/x;

missing a $ at the end

PerformSubsts may or may not be safe, depending on what its substing.

            next if $directive =~ /FILTER\ (html|csv|js|url_quote|quoteUrls|
                                            time|uri|xml)/x;

any amount of whatiespace is ok - use \s+. You also should catch '|', too. Tis
filter is also not necessarily ok - for example, csv filterinsg isn't safe for
an html page. That said, this isn't meant to be perfect, so its probably ok.
There should be a warning added to the script along those lines, though.

Comment 34

[Gervase Markham [:gerv]]

> So did you just ignore all the CSV errors or add them to exceptions?

Added them to exceptions (and to the list of things we need to filter
post-security-release.)

> > No - the correct answer is to standardise the comments. :-)
> 
> I agree (and that's what I originally did), but that's a bit beyond what we
> should be changing for a security release, isn't it?

So we add them to the list of exceptions (there's only about three of them) and
mark them for filtering post-security-release. See a pattern developing here :-)

> > Are the results of this guaranteed safe? How many times do we call 
> > PerformSubsts?
> 
> It's called once. I don't remember why but I had a hard to adding it to the
> exception file (I couldn't get it to work).  It's as safe as the Param() calls
> are.

Try harder to get it to work :-) Copy and paste from the error message is your
friend; watch out for trailing spaces on the end of lines if it's multi-line,
and remember to escape single quotes.

>             next if $directive =~ /^(IF|END|UNLESS|FOREACH|PROCESS|INCLUDE|
>                                      BLOCK|USE|ELSE|NEXT|LAST|DEFAULT|FLUSH|
>                                      ELSIF|SET)/x;
> 
> missing a $ at the end

No, it's not - unless we have many templates which have [% IF %] in them ;-)

>             next if $directive =~ /FILTER\ (html|csv|js|url_quote|quoteUrls|
>                                             time|uri|xml)/x;
> 
> any amount of whatiespace is ok - use \s+. 

IMO, it's nicer if the tool enforces the "standard" (to the extent that anythin
we do is standard) spacing :-)

> You also should catch '|', too. 

This is only used twice; my plan was to (add them to the exceptions list in the
mean time and) standardise on FILTER. Regexping for | would be tricky, and it's
also less obvious what's going on when admins come to edit the templates.

> Tis filter is also not necessarily ok - for example, csv filterinsg isn't 
> safe for an html page. 

Yes, but a mistake of that magnitude really should be spotted in review/code
read :-)

Gerv

Comment 35

[Dave Miller [:justdave]]

the content type is in the filename...  we could always change the automatic
exceptions based on the content type.  For example if the ctype is csv, then a
cvs filter is legal, if it's not then it isn't.

Comment 36

[Bradley Baetz (:bbaetz)]

Oh, I thought $directive was just the word. In that case, its missing a \b :)

You need a \s because the FILTER can be on a new line when a directive is broken
u to make it more readable. Besides, the check should try to find as much as
possible, not just stuff which is styalistically correct?

Whats the issue with '|'? Its not harder to read once you get told (once) what
its for, and its also more consise.

Comment 37

[Gervase Markham [:gerv]]

We could do, if we felt the added complexity was worth it. Hmm. It's not too
complicated for that one case. But (roughly; please feel free not to pick nits
in this):

html only in HTML or NONE
csv only in CSV
js only in HTML or JS or XML
url_quote only in HTML or JS
quoteUrls only in HTML
time anywhere
uri only in HTML or JS
xml only in XML

It all gets a bit more complicated if you start doing this.

> Oh, I thought $directive was just the word. In that case, its missing a \b :)

Fair cop :-)

> You need a \s because the FILTER can be on a new line when a directive is 
> broken up to make it more readable. 

IMO, you shouldn't be breaking between FILTER and the filter name, for
readability reasons. No-one does currently.

> Besides, the check should try to find as much as
> possible, not just stuff which is styalistically correct?

If it's not stylistically correct, an error will be flagged. It can then either
be fixed, or added as a special exception if there's a one-off need to use a
strange FILTER construct.

> Whats the issue with '|'? Its not harder to read once you get told (once) what
> its for, and its also more consise.

Once you get told - exactly. As I remember it, one of the good things about TT,
and one of the reasons that you separate presentation from logic, is to make it
easy for someone who doesn't know Perl or TT to tweak templates. This is why, in
general, we use the long names for things (like AND and OR.) To many people, a |
is an OR symbol, whereas you can guess what "FILTER html" does in a file which
produces HTML. 

Also, doing regexps right on it is hard. For example, think about [% " $bar |
foo" %] and similar arbitrarily-complicated things.

Gerv

Comment 38

[Bradley Baetz (:bbaetz)]

Shouldn't be doing != shouldn't test here

What if FILTER is part of a string somewhere? :)

Comment 39

[Dave Miller [:justdave]]

Dave should have a look at this too.  If nothing else, he can use the test on
his own templates, since his look a lot different, and our security patch might
be a tight fit for him.

Comment 40

[Gervase Markham [:gerv]]

> Shouldn't be doing != shouldn't test here

Yer wot?

> What if FILTER is part of a string somewhere? :)

It would have to be "FILTER <filtertype>". And what do you think the chances of
this are, given that we don't have templates that generate templates? Orders of
magnitude less likely than someone using |, anyway.

> and our security patch might be a tight fit for him.

Is everyone being cryptic today? What does this mean? :-)

Gerv

Comment 41

[Dave Miller [:justdave]]

That means he's modified his templates sufficiently enough that the stuff we
find may or may not even apply to his templates, and the tests may or may not
find additional spots in his templates that weren't problems in ours.  Didn't
realize that was that hard to figure out from the context....

Comment 42

[Jacob Steenhagen]

Attachment: Jake's partial FILTER work (don't peak if you're doing the same templates :)

I'm going on vacation for the next week so I won't be able to finish my audit. 
I'm attaching to this bug what I currently have done in case somebody wants to
do the other half. I have combed through about 1/2 (50 out of 110) of the
templates and found a few exploitable areas.  Everything that I couldn't
exploit and I could find a place in the code that explained why I couldn't
exploit it was placed in the filterexceptions list (for the purpose of the
security release).  I also occationally added comments about why it was safe
and areas that I still had some concern about even though I couldn't get an
exploit to work.

My recommended course of action for this attachment is to ignore it except
possibly to see what templates I've done if you wish to work on the other half.
I was in the middle of site-navigation.tmpl.html when I stopped. The other
exception to this recommendation is if you've already made your own list and
wish to compare your findings to mine (the only person I know of that currently
fits this category is Gerv :).

Comment 43

[Gervase Markham [:gerv]]

So, here's Jake's scorecard so far :-)

XSS holes correctly identified: 6
- Well done :-)

XSS holes incorrectly identified: 8 
- This is not too serious, but bloats any patch
- All the .first and .last bug_id ones are not exploitable. They come from your 
  own BUGLIST cookie, which is not under an attacker's control.

XSS holes missed: 2
- "reverse" in reports/duplicates.html.tmpl; he adds a comment to say it's not 
   exploitable, but it is.
- "type" in global/code-error.html.tmpl; he adds a comment to say there are no 
   users of field_type_mismatch; there's one in Search.pm.

Probable XSS holes missed: 3
- I'm fairly sure that, with a malicious client, you could exploit "buffer" in
list/change-columns.html.tmpl and "urlquerypart" in list/list.html.tmpl and
list/table.html.tmpl, although I've not managed it myself.

XSS holes found that I missed: 0
- Which is a relief for me, at any rate :-)

So, is anyone going to do the second half of them?

Gerv

Comment 44

[Oscar Manuel Gómez Senovilla]

I'd like to help, but I don't know how.

If somebody wants to give a example of the work to do, I could try to help,
since I'm feeling that with the appropiate knowledge and not too much time. I
have a little bit of hope that I could do something useful with this bug, and
that's why I haven't removed myself from the cc, so please "feed me" (there
could be others like me!!), and I'll be able to answer the request done several
times "who can do the rest?".

If you think that the needed knowledge is too high to do something and think
that I won't be able to do anything, please say it so and/or remove me from the cc.

Don't think that I'm annoyed, but just trying not to be unuseful.

Comment 45

[Gervase Markham [:gerv]]

Well, what you have to do approximately is as follows:

Download and install Bugzilla from CVS. Insert 008filter.t v.2 (not 3 ;-) into
your "t" directory. Run the tests. You'll get a whole load of failures from test
8. Copy this information to file.

Every failure represents a template directive (inside [% %]) which isn't
FILTERed. Most of these are safe; some are not. You have to look at each one in
context, work out where the contents come from, and whether a malicious user can
construct a URL parts of which get re-echoed to the client unescaped, or whether
they can insert stuff into the database which gets re-echoed unescaped (worse.) 

As you go, create a file called filterexceptions.pl in template/en/default/ -
see "Jake's partial filter work" above for an example. You probably want to
start with this one so you don't duplicate his work. For each template, add any
directives you know to be safe. So, at the end, when you run the tests again,
the only failures will be the holes you found. Document those holes, and post
the results here.

Got it? ;-)

Gerv

Comment 46

[Oscar Manuel Gómez Senovilla]

I downloaded from the CVS and the 008filter.tv.2, but it didn't install
successfully, because now it seems to require additional perl modules. I'll
"diguise" and drop a post to the newsgroup. Since the patch doesn't work (I
think) for versions not in the CVS, please tell me if I still can do something.
I have a 2.16.2 and a 2.17.3 well installed, for testing translations.

Comment 47

[Gervase Markham [:gerv]]

> I downloaded from the CVS and the 008filter.tv.2, but it didn't install
> successfully, because now it seems to require additional perl modules.

That's not a very good bug report :-) Which Perl modules does it claim it wants?
It shouldn't need any.

Please keep discussion of this issue in this bug.

Gerv

Comment 48

[Oscar Manuel Gómez Senovilla]

Ah, you're right. It's not the module itselft, but the version. I went to
/root/.cpan/<module_path> and "make install" worked ("make test" failed). In the
case of the GD module, it doesn't compile. Checksetup gave some errors about the
bugs db, so I deleted the database and forced checksetup to re-create, and this
has been done successfully. Now I have ths CVS version working. It shows 2.17.3
(I had a 2.17.3 working before). The results of the patch:

# ./runtests.sh
t/001compile.......ok
t/002goodperl......ok
t/003safesys.......ok
t/004template......ok
t/005no_tabs.......ok
t/006spellcheck....ok
t/007util..........ok
t/008filter........NOK 1#     Failed test (t/008filter.t at line 52)
# Looks like you planned 110 tests but only ran 1.
t/008filter........dubious
        Test returned status 110 (wstat 28160, 0x6e00)
DIED. FAILED tests 1-110
        Failed 110/110 tests, 0.00% okay
Failed Test   Stat Wstat Total Fail  Failed  List of Failed
-------------------------------------------------------------------------------
t/008filter.t  110 28160   110  110 100.00%  1-110
Uncaught exception from user code:
        Failed 1/8 test scripts, 87.50% okay. 110/1230 subtests failed, 91.06% okay.
        Test::Harness::_show_results('HASH(0x81343fc)', 'HASH(0x81343e4)')
called at /usr/share/perl/5.6.1/Test/Harness.pm line 341
        Test::Harness::runtests('t/001compile.t', 't/002goodperl.t',
't/003safesys.t', 't/004template.t', 't/005no_tabs.t', 't/006spellcheck.t',
't/007util.t', 't/008filter.t') called at runtests.pl line 41

Now what?

Comment 49

[Gervase Markham [:gerv]]

Oh, now I see :-) I thought you meant that 008filter.t requires new modules, but
you meant that Bugzilla requires new modules.

Anyway, well done on getting it working. But you need a filterexception.pl as
well, and you need to run runtests.sh --verbose to get the full error output.
Don't post it here - but it should look something like attachment 114617 [details].

Gerv

Comment 50

[Oscar Manuel Gómez Senovilla]

Just the end of the output (with an empty filterexceptions.pl):

t/008filter........1..110
filterexceptions.pl did not return a true value at t/008filter.t line 56.
# Looks like your test died before it could output anything.
dubious
        Test returned status 255 (wstat 65280, 0xff00)
DIED. FAILED tests 1-110
        Failed 110/110 tests, 0.00% okay
Failed Test   Stat Wstat Total Fail  Failed  List of Failed
-------------------------------------------------------------------------------
t/008filter.t  255 65280   110  110 100.00%  1-110
Uncaught exception from user code:
        Failed 1/8 test scripts, 87.50% okay. 110/1230 subtests failed, 91.06% okay.
        Test::Harness::_show_results('HASH(0x81343fc)', 'HASH(0x81343e4)')
called at /usr/share/perl/5.6.1/Test/Harness.pm line 341
        Test::Harness::runtests('t/001compile.t', 't/002goodperl.t',
't/003safesys.t', 't/004template.t', 't/005no_tabs.t', 't/006spellcheck.t',
't/007util.t', 't/008filter.t') called at runtests.pl line 41

Next step?

Comment 51

[Gervase Markham [:gerv]]

Just a thought: in view of bug 38862, why are we even bothering? :-)

Gerv

Comment 52

[Gervase Markham [:gerv]]

Your filterexceptions.pl needs a

1;

at the end.

Gerv

Comment 53

[Oscar Manuel Gómez Senovilla]

Well, the last question is the implantantion and correction of not ok lines.
Example: test says

t/008filter........1..110
ok 1 - index.html.tmpl is filter-safe
not ok 2 - template/en/default/sidebar.xul.tmpl has unfiltered directives:
#   25:template_version

The line 25 is

<!-- [% template_version %] -->


How do I carry out the filtering?

Comment 54

[Gervase Markham [:gerv]]

You don't need to worry about the implementation and correction of non-OK lines.
That's my bag :-) But if you've got that far, that's great - please upload your
results, and I'll compare them with mine. Then, because 2 people have been over
every stretch, I can produce a fix patch, and we can get 2.16.3 and 2.17.4 out
of the door.

Wahey :-)

Gerv

Comment 55

[Myk Melez [:myk] [@mykmelez]]

Attachment: Myk's partial filter work (110-88 backwards)

I used 008filter.t v2 with $path added in the appropriate places.  My results:

not ok 102 - template/en/default/account/prefs/email.html.tmpl has unfiltered
directives:
#   58:watchedusers

-> unvalidated list of email addresses; these should just be validated, as
valid email addresses can't contain XSS vulnerabilities


"template/en/default/account/login.html.tmpl" => [
  "target",
]

-> not a vulnerability yet, but confirm_login takes an unused parameter that
could create a vulnerability if used wrong; we should remove it

template/en/default/admin/flag-type/confirm-delete.html.tmpl

-> $name didn't get caught on line 26, but this is a problem with the test, not
an XSS vul.

not ok 92 - template/en/default/admin/products/groupcontrol/edit.html.tmpl has
unfiltered directives:
#   30:filt_product

-> in theory this should be fine, but a bug makes filt_product not be filtered:

line 23: [% filt_product = product FILTER html %]
should be: [% filt_product = BLOCK %][% product FILTER html %][% END %]

Comment 56

[Bradley Baetz (:bbaetz)]

That confirm_login thing is gone in my auth patch. I've been really busy this
week; I hope ot have time to work on this tomorrow.

Comment 57

[Gervase Markham [:gerv]]

Thanks, Myk :-) Some good catches. Particularly:

> -> in theory this should be fine, but a bug makes filt_product not be filtered:
> 
> line 23: [% filt_product = product FILTER html %]
> should be: [% filt_product = BLOCK %][% product FILTER html %][% END %]

I missed this one - but I can't see how the first line of code is wrong. Surely
it does exactly what it looks like? Or is it a bracketing issue?

Gerv

Comment 58

[Myk Melez [:myk] [@mykmelez]]

>I missed this one - but I can't see how the first line of code is wrong. Surely
>it does exactly what it looks like? Or is it a bracketing issue?

Err, it looks like it works now.  In an earlier version of TT it did [% (foo =
bar) FILTER html %] which left foo unfiltered.

Comment 59

[Bradley Baetz (:bbaetz)]

Well, this helped me find bug 196420, at least :)

Looking over this list, we seem to have lots of stuff which is 'obviously'
correct (like the sidebar template_version, for example), and very few values
which are truly passed to the script as html. This leads to lots of values,
which may explain why people missed some.

I wonder how difficult a TT patch which considered the incoming vars to be
tainted (and aborted if the final output was tainted) would be... You'd have to
deal with references, though, which probably makes that impossible to do without
affecting the original state (which would be a no-no)

Comment 60

[Gervase Markham [:gerv]]

Attachment: Patch v.4

OK, here's the plan.

This patch contains the test, the filterexceptions.pl file, and the minimum
changes necessary to close all the holes in 2.17. I haven't had a chance to
test it at all, and I have to go away for a few days. A useful thing to do
would be to apply the patch, then read it, see where the holes were, check that
they are no longer exploitable, and that I haven't broken anything by doing the
filtering. There's only 20, so this shouldn't be too taxing :-)

It's the minimum patch necessary to cut down on the testing effort. After we
release the security releases, there are comments in filterexceptions.pl which
will allow us to come back and filter a whole load more stuff (and fix a few
bugs) that isn't an XSS problem, but which should be filtered anyway.

I'll be back on Wednesday; it would be great to have this patch well tested by
then. :-)

Gerv

Comment 61

[Gervase Markham [:gerv]]

Attachment: Research into each vulnerability

This file is my research. It details each vulnerability and possible
vulnerability (email addresses; we aren't fixing those because they are safe by
default), what template it's in, what line it's on in CVS, when it was checked
in, and what is the earliest released version which has the problem.

This should help someone to prepare the patch for 2.16.

Gerv

Comment 62

[Bradley Baetz (:bbaetz)]

Whats a 'buffer-type problem'?

Comment 63

[Gervase Markham [:gerv]]

It's where we reprint a URL; my original thought was that, because of encoding
stuff in the browser, you need a malicious submitting client to exploit these.
But I could have been totally wrong. Probably best to ignore those comments :-)

Gerv

Comment 64

[Gervase Markham [:gerv]]

Comment on attachment 116624 [details] [diff] [review]
Patch v.4

Dave: any chance of a review? :-)

Gerv

Comment 65

[Dave Miller [:justdave]]

*** Bug 193561 has been marked as a duplicate of this bug. ***

Comment 66

[Bradley Baetz (:bbaetz)]

After having gone through user-errors.html.tmpl this afternoon for other
reasons, you're missing filtering the product in product_edit_denied.

template/en/default/list/table.html.tmpl filters urlquerypart - hasn't that
alrady been filtered based on wherever we extract it from? (so that you just
need to html filter it)?

url_quote is also wrong there - that will filter everything, including : and /
for http://. You want the uri filter for a full uri, + the html filter
afterwards. url_quote is only for bits of a url, like a query string or something.

In template/en/default/reports/duplicates.html.tmpl, nomalise sortby to 0 or 1
in the cgi. Ditto for reverse. Changesince should be a number anyway, or the
daysago function will act strangly. Verify that in the cgi instead of the template.

template/en/default/global/choose-product.html.tmpl needs a - for the [% IF so
that the tags are combined without bogus spacings.

template/en/default/bug/show-multiple.html.tmpl - as I mentioned, url_quote is
wrong. Thats often used for 'misformed' uris, to test mozilla, and even when not
used on mozilla.org, we do want to use exactly what the user typed in. Maybe
just html FILTER it?

Comment 67

[Gervase Markham [:gerv]]

> After having gone through user-errors.html.tmpl this afternoon for other
> reasons, you're missing filtering the product in product_edit_denied.

Nope; that directive is created like this:

    $oldhash{'product'} = get_product_name($oldhash{'product_id'});
    if (!CanEditProductId($oldhash{'product_id'})) {
        $vars->{'product'} = $oldhash{'product'};
        ThrowUserError("product_edit_denied");
    }

So it will only ever be a valid product name. As I've said, this patch is the
minimum necessary to close security holes. There are a number of places we
should be filtering to stop things breaking if e.g. a product name contains HTML
chars; those will be done in a separate patch.

> template/en/default/list/table.html.tmpl filters urlquerypart - hasn't that
> alrady been filtered based on wherever we extract it from? (so that you just
> need to html filter it)?

Things like column.name in the same file are processed as "FILTER url_quote
FILTER html". Basically, this var contains a raw query string.

> url_quote is also wrong there - that will filter everything, including : and /
> for http://. You want the uri filter for a full uri, + the html filter
> afterwards. url_quote is only for bits of a url, like a query string or 
> something.

urlquerypart _is_ a query string.

> template/en/default/bug/show-multiple.html.tmpl - as I mentioned, url_quote is
> wrong. 

Now here you are right :-) This needs fixing (and the other things you mention.)

Gerv

Comment 68

[Bradley Baetz (:bbaetz)]

Product could contain html... Just fix it; its not like it costs anything. And
if you add it to the exclusion list, then people will just have to go trhough it
again laer.

Comment 69

[Gervase Markham [:gerv]]

> Product could contain html... Just fix it; its not like it costs anything. And
> if you add it to the exclusion list, then people will just have to go trhough 
> it again laer.

It does cost something; if I fix this one, I have to fix the thirty or so other
places where we don't filter things like products, target milestones etc. - and
that makes the patch that much harder to review, audit and verify on our stable
branch.

I have a list of all the directives that should be filtered - they are marked
with a # in the list. So, after we release the security update, it is fairly
easy to go through and do a patch filtering those too. That includes this one.

Gerv

Comment 70

[Gervase Markham [:gerv]]

> In template/en/default/reports/duplicates.html.tmpl, nomalise sortby to 0 or 1
> in the cgi. 

Er... sortby is a string.

> template/en/default/global/choose-product.html.tmpl needs a - for the [% IF so
> that the tags are combined without bogus spacings.

That's not true (I've tested it) because we have PRE_CHOMP on.

Gerv

Comment 71

[Gervase Markham [:gerv]]

bbaetz: can you give concrete examples of when to use url_quote, when to use
uri, and when to use HTML, in the context of an HTML template? The more I think
about this, the more confused I get.

Gerv

Comment 72

[Bradley Baetz (:bbaetz)]

see http://www.template-toolkit.org/docs/plain/Manual/Filters.html#uri for uri
vs html

The problem with that is when an attribtue contains (for example) &. Its valid
in a uri, and you don't always want to escape it, but if your product name has &
in it, then you do (so that its still considered one cgi param). Thats what
url_quote is for. url_quote should basically only be used when you have
something like:

&product=[% product | url_quote %]

If you have an entire query string, and passed it through url_quote,m you'd end
up withh one really large parmater to the destination cgi.

Comment 73

[Gervase Markham [:gerv]]

bbaetz: I know what they _do_ - that's not the issue. The issue is exactly when
to use them, and which to use, which you have partly, but not totally explained.
In plain HTML, I know you just FILTER html, but what you do in URLs is less clear.

For example, what do I do with:

<a href="foo.cgi?[% urlquerypart %]">
where urlquerypart is "foo=bar&baz=quux".

<a href="foo.cgi?product=[% product %]">

<a href="[% bug.bug_file_loc %]">

?

Gerv

Comment 74

[Bradley Baetz (:bbaetz)]

> <a href="foo.cgi?[% urlquerypart %]">
> where urlquerypart is "foo=bar&baz=quux".

Assuming that |bar| and |quux| have been escaped already, then uri and html. If
urlquerypart comes from the query string, then you may not need the uri part,
depending on whether hatever you use to get that has undone the space<=>%20
mapping for you

Note that if |bar| and |quux| hve not been escaped, then you're stuffed, since
you cannot differentiate the case of two cgi params, foo (With value bar) and
baz (with value quux) from that of one param, foo, with value "bar&baz=quux"

> <a href="foo.cgi?product=[% product %]">

here product can contain anything, so url_quote it.

> <a href="[% bug.bug_file_loc %]">

This one is special. Just html-quote it. If someone enters a url with a space in
it, then thats what the href attribute should have.

Comment 75

[Gervase Markham [:gerv]]

Attachment: Patch v.5

Hopefully, everything is now filtered using the correct filter.

Incidentally, this patch demonstrates why we need this checking; in addition to
previous changes, it fixes a recent XSS vulnerability introduced by bbaetz in
the Auth rewrite - the "target" param in account/auth/login.html.tmpl . 

Gerv

Comment 76

[Bradley Baetz (:bbaetz)]

Comment on attachment 118495 [details] [diff] [review]
Patch v.5

Err, I just moved that from the other file.

Besides, that isn't an XSS hole; its a filename which the user can't exactly
affect...

Comment 77

[Gervase Markham [:gerv]]

Comment on attachment 118495 [details] [diff] [review]
Patch v.5

Oh, right. $cgi->url is not the query URL, it's the URL of the CGI. That makes
more sense :-) I should have checked more carefully - apologies.

Gerv

Comment 78

[Bradley Baetz (:bbaetz)]

Well, it can be the query url - it depends what argument you pass to it :)

Comment 79

[Gervase Markham [:gerv]]

Attachment: Patch v.6

Here's an unrotted version of the trunk patch.

Gerv

Comment 80

[Dave Miller [:justdave]]

Attachment: Patch v6 backported to 2.16 branch

I dumped filterexceptions.pl and started from scratch on that, copying pieces
from the 2.17 file when appropriate, but auditing as I went.

In addition to the other stuff, I found two items in 2.16 that had already been
fixed on the trunk prior to this audit, as parts of other checkins.

template/en/default/reports/duplicates.html.tmpl
1.7 <[e-mail]> 09 Nov 2002 18:50
Bug 176599, Improve performance of duplicates.cgi
original patch iteration by gerv, change to use Bugzilla:Search by me
r=myk, a=justdave

-	     <option name="[% p %]"
-	     [% " selected" IF product == p %]>[% p %]</option>
+	     <option name="[% p FILTER html %]"
+	     [% " selected" IF product == p %]>[% p FILTER html %]</option>

template/en/default/global/code-error.html.tmpl
1.20 <[e-mail]> 25 Oct 2002 18:57
Bug 147833 - start using CGI.pm
r=gerv, justdave

-    [%+ key %]: [%+ variables.$key %]
+    [%+ key FILTER html %]: [%+ variables.$key FILTER html %]

Comment 81

[Dave Miller [:justdave]]

Comment on attachment 118871 [details] [diff] [review]
Patch v.6

I was comparing to the trunk as I went with the 2.16 audit, and I'll supply an
r= for this.  I'd like a second one from someone else, too.

Comment 82

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 118871 [details] [diff] [review]
Patch v.6

>Index: template/en/default/list/change-columns.html.tmpl

>-  <input type="hidden" name="rememberedquery" value="[% buffer %]">
>+  <input type="hidden" name="rememberedquery" value="[% buffer FILTER html %]">

buffer isn't exploitable in colchange.cgi since Bugzilla escapes URL
meta-characters if the user-agent didn't:

[myk@localhost myk]$ telnet mothra.mozilla.org 80
Trying 207.200.81.216...
Connected to mothra.mozilla.org.
Escape character is '^]'.
GET /webtools/bugzilla/colchange.cgi?short_desc=< HTTP/1.0
...
  <input type="hidden" name="rememberedquery" value="short_desc=%3C">
...
  <input type="hidden" name="rememberedquery" value="short_desc=%3C">

Since all HTML meta-characters are also URL meta-characters, this also means it
doesn't need to be filtered in general.

Comment 83

[Myk Melez [:myk] [@mykmelez]]

Here's the Bugzilla trunk with the patch v.6 applied:

http://mothra.mozilla.org/webtools/bz192677/

Everything went fine (a few fuzzes, a few offsets) except for login.html.tmpl,
which doesn't exist on b.m.o.

Comment 84

[Myk Melez [:myk] [@mykmelez]]

>Since all HTML meta-characters are also URL meta-characters, this also means it
>doesn't need to be filtered in general.

Perhaps I spoke too soon.  Bugzilla escapes <, ", and > but leaves & intact,
which isn't an XSS hole but should be filtered for [X]HTML [n.nn] compatibility,
so buffer (and urlquerypart in list/list.html.tmpl and list/table.html.tmpl)
should still be filtered, even if they aren't an XSS hole.

Comment 85

[Dave Miller [:justdave]]

reference comment 83, he means the code currently running on bmo (not the trunk)
with this patch applied.

Comment 86

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 118871 [details] [diff] [review]
Patch v.6

Ok, I've tested most of the fixes on b.m.o, including many of the obvious ones,
and they all work as advertised; before the fix I can exploit the hole, and
after the fix I can't.	r=myk

Comment 87

[Gervase Markham [:gerv]]

Comment on attachment 118880 [details] [diff] [review]
Patch v6 backported to 2.16 branch

Looks OK to me.

Gerv

Comment 88

[Bradley Baetz (:bbaetz)]

These all look fine - tehres one more thing I want to test, and I'll do that in
3-4 hours' time.

Comment 89

[Bradley Baetz (:bbaetz)]

.. or rather, I would have, but I got sidetracked. Sorry - hopefully tomorrow
afternoon my time.

Comment 90

[Tobias Burnus]

I found something else which should be included (since it would be a potential
security hole if working):
In global/user-error.html.tmpl there is this line:
  [% ELSIF error == "no_dupe_stats_error_whenever" %]
    [% title = "Error Reading Previous Dupes File" %]
    An error occurred opening $changedsince days ago ($whenever)'s
where at least "changedsince" should be HTML filtered (see below why). Since
$changedsince doesn't work (it should be [% changedsince %]) this is not a
security problem ;-)

Relevant lines from duplicates.cgi:
  my $changedsince = formvalue("changedsince", 7);
  my $whenever = days_ago($changedsince);
  # If error then
        $vars->{'changedsince'} = $changedsince;
        $vars->{'whenever'} = $whenever;
        ThrowUserError("no_dupe_stats_error_whenever");
  # later (new since attachment 118871 [details] [diff] [review]):
  detaint_natural($changedsince)
     || ThrowUserError("invalid_changedsince", { changedsince => $changedsince });

If I understand this correctly $changedsince it not filtered at all while 
$whenever = sprintf "%04d-%02d-%02d" (localtime(time - ($_[0]*24*60*60)))[3, 4, 5];
should take care of changedsince.

Suggested fix:
 detaint_natural($changedsince)
   || ThrowUserError("invalid_changedsince", { changedsince => $changedsince });
should be moved up.

Comment 91

[Gervase Markham [:gerv]]

Tobias: that's a bug, not a security problem (as you point out.) We've
discovered several bugs during this process, and will fix them in another round
of patches.

Gerv

Comment 92

[Bradley Baetz (:bbaetz)]

Comment on attachment 118880 [details] [diff] [review]
Patch v6 backported to 2.16 branch

>Index: duplicates.cgi

detaint_natural destroys the passed in value - you need to save off $origmaxros
and $changedsince first.

>+detaint_natural($maxrows)
>+  || ThrowUserError("The maximum number of rows, '" . html_quote($maxrows) .
>+                    "', must be a positive integer.",
>+                    "Invalid Max Rows");
>+
>+detaint_natural($changedsince)
>+  || ThrowUserError("The 'changed since' value, '" . html_quote($changedsince) .
>+                    "', must be an integer >= 0.",
>+                    "Invalid Changed Since");

>Index: template/en/default/filterexceptions.pl

>+%::safe = (

>+'search/form.html.tmpl' => [

>+  'button_name',

#

>+],
>+
>+'search/knob.html.tmpl' => [
>+  'button_name',

#

text strings passed from the front end should always end up being html_escaped.

>+'reports/duplicates-table.html.tmpl' => [

>+  'vis_bug_ids.push(bug.id)',

This should probably be a CALL in the template; push returns a number in perl,
although it doens't appear to be doing so in TT

>+'list/edit-multiple.html.tmpl' => [

>+  'selected IF resolution == "FIXED"', #

Why is this #?

>+
>+'global/hidden-fields.html.tmpl' => [
>+  'mvalue | html | html_linebreak', # Need to eliminate | usage

Why?

>+  'field.value | html | html_linebreak',
>+],
>+
>+'global/message.html.tmpl' => [
>+  'url', # Need to clear up who escapes 

... and you should do so before this patch, cause its exploitable.

Try:

http://localhost/bugzilla-2.16/buglist.cgi?">abc</a>&cmd-add

in netscape 4 (not moz, which over-escapes stuff in the query string)

Also try it w/o the cmd-add stuff

You need 2.16; I can't do this on bmo.

Guess that the assumption that $::buffer/url stuff/etc is safe isn't true.....

>+  'link', #

ditto, I guess

>+
>+'bug/create/make-template.html.tmpl' => [
>+  'url',
>+],

This needs filtering too, for the same reasons

>Index: template/en/default/attachment/edit.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v
>retrieving revision 1.4.2.2
>diff -u -r1.4.2.2 edit.html.tmpl
>--- template/en/default/attachment/edit.html.tmpl	11 Jun 2002 09:27:21 -0000	1.4.2.2
>+++ template/en/default/attachment/edit.html.tmpl	30 Mar 2003 01:32:09 -0000
>@@ -50,7 +50,7 @@
>       // If this is a plaintext document, remove cruft that Mozilla adds
>       // because it treats it as an HTML document with a big PRE section.
>       // http://bugzilla.mozilla.org/show_bug.cgi?id=86012
>-      var contentType = '[% contenttype %]';
>+      var contentType = '[% contenttype FILTER js %]';
>       if ( contentType == 'text/plain' )

Remind me why this |if| isn't done in the template?

Comment 93

[Dave Miller [:justdave]]

>>Index: duplicates.cgi
>
>detaint_natural destroys the passed in value - you need to save off $origmaxros
>and $changedsince first.

I have absolutely no clue what was being done to duplicates.cgi.  On that one I
just backported what Gerv did on the trunk patch.  So if it's broken it's broken
on the trunk patch, too.

>>+'list/edit-multiple.html.tmpl' => [
>
>>+  'selected IF resolution == "FIXED"', #
>
>Why is this #?

Because it should be |"selected"| instead of |selected|.  There's no variable
named selected defined anywhere.

The rest I'll defer to Gerv because it's all copied out of his patch, and I
didn't see anything wrong with it when I looked at it :)

Comment 94

[Bradley Baetz (:bbaetz)]

Comment on attachment 118880 [details] [diff] [review]
Patch v6 backported to 2.16 branch

Oh, and that url thing - you can't have spaces, which may limit the
exploitability. although %<whatever-&-is>%<#>032; may work. It doesn't in ns4,
but....

The without cmd-add bit is fixed with the patch, but the message thing still
occurs.

Comment 95

[Gervase Markham [:gerv]]

> detaint_natural destroys the passed in value - you need to save off $origmaxros
> and $changedsince first.

That makes it really clunky to use. Is there a way we can improve this interface? 

>>+'search/form.html.tmpl' => [
>>+  'button_name',
> 
> #

Yes, yes. :-) We'll go through and work out the hashes later. Getting them right
is not a prerequisite for this patch being ready. We are fixing security holes
only here, to make the patch reviewable and testable.

>>+'reports/duplicates-table.html.tmpl' => [
> 
>>+  'vis_bug_ids.push(bug.id)',
> 
> This should probably be a CALL in the template; push returns a number in perl,
> although it doens't appear to be doing so in TT

We've discovered several "bugs" as we are going along; again, we'll fix them later.

>>+'global/hidden-fields.html.tmpl' => [
>>+  'mvalue | html | html_linebreak', # Need to eliminate | usage
> 
> Why?

Because the test checks for the word FILTER, because it's much easier than
checking for |, where it would be very hard to avoid false positives. Also, for
admins tweaking templates, FILTER is much easier to understand than |, which
looks like logical OR, or is just confusing.

>>+'global/message.html.tmpl' => [
>>+  'url', # Need to clear up who escapes 
> 
> ... and you should do so before this patch, cause its exploitable.

So you are saying it needs html filtering in this patch, for 2.16 only?

> Guess that the assumption that $::buffer/url stuff/etc is safe isn't true.....

I never assumed that - I kept asking people to help me work out whether it was
true, but got no response. My patch doesn't assume it; that's why it has lots of
escaped buffers.

>>+  'link', #
> 
> ditto, I guess

I don't think so - this is defined by the calling CGI, rather than user input,
isn't it?

Gerv

Comment 96

[Bradley Baetz (:bbaetz)]

Someone wanted me to do that when I did the taint filtering way back when. I
can't remember who or why, though

> So you are saying it needs html filtering in this patch, for 2.16 only?

No, for 2.17 too - try my test url. I don't know if its exploitable (because of
the lack of space issue), but it would take a lot longer to try to prove it one
way or the other then to just filter the thing. Its definatley buggy, and its
possible to put unescaped data onto the page, as my test shows.

Comment 97

[Gervase Markham [:gerv]]

bbaetz: about escaping of [% url %], you said:
> You need 2.16; I can't do this on bmo.
and then you said:
> No, for 2.17 too - try my test url.

Which is right? :-)

Gerv

Comment 98

[Dave Miller [:justdave]]

Do note that Myk already applied an earlier version of this patch to b.m.o, so
testing it there isn't a good place to try to exploit.  Landfill is back, use it :)

Comment 99

[Gervase Markham [:gerv]]

Attachment: Patch v.7

Here's v.7, with markups from bbaetz for the 2.16 patch applied.

Gerv

Comment 100

[Bradley Baetz (:bbaetz)]

gerv: There are two issues - the message text, and then the headers on
buglist.cgi. 2.17 isn't affected by the second, but both are affected by the
message text stuff.

Comment 101

[Dave Miller [:justdave]]

Just FYI for everyone, here's the current plan for this bug:

There's still debate going on here so I don't consider these final yet, but we
can't afford to wait much longer, so unless someone comes up with something
exploitable that these patches don't cover, I'm going to go with what we have
within the next couple days.

Once the trunk and 2.16 patches are finalized, then we need to get the
localizers to run the tests on the localized templates and fix the same issues
and any other similar ones that they might have created as part of the
localization process.  New localization tarballs should *not* be posted yet, but
once you have it created, post here to let us know you're ready.  Once
everyone's ready, we'll set a date a couple days out, everyone knows to watch
for the security advisory on that day.  We'll release the security advisory
stating that updates to the localized versions will be available within 24
hours, and everyone posts their updated versions as soon as they see the advisory.

Backporting the patch from the trunk to 2.16 took about 7 hours.  I would
suspect that it would take much shorter time to port it from English 2.16 to a
localized 2.16 template set, since the actual code parts of the templates won't
have changed much.

Questions, comments, suggestions, advice?  :)

Comment 102

[Oscar Manuel Gómez Senovilla]

i would suggest, in order to save time for us localizers, to build a simple
script using diff (or whatever you find proper) stating the files and/or
differences in the templates, so it isn't necessary to review all the code, just
to know where to look at. There might be many files not affected or some with
minor changes, and having something "change this in file x, line(s) a, b, c"
instead of diving the whole tree is easier.

Comment 103

[Dave Miller [:justdave]]

That's pretty much what the existing patches are, if you look at them. :)  Which
is the reason I think it won't take as long if your localization set is based on
either 2.16.2 or the current trunk.

Comment 104

[Bradley Baetz (:bbaetz)]

Comment on attachment 119645 [details] [diff] [review]
Patch v.7

OK, this looks fine (via interdiff from the last patch) No guarentee that we
haven't missed something, but we've certainly fixed bugs...

Comment 105

[Dave Miller [:justdave]]

Attachment: Patch v7 for 2.16 branch

Comment 106

[Dave Miller [:justdave]]

Attachment: Patch v7 for 2.16 branch (corrected)

Goofed up the diff command line and didn't realize it till after it was posted.
 Here's the correct patch.

Comment 107

[Tobias Burnus]

Ok, using Patch v.7 (for HEAD) I'm done for bugzilla-de.sf.net.
The 008filter runs through without any problem, but this one

not ok 62 - template/de/default/list/list.rdf.tmpl has unfiltered directives:
  39:bug.bug_id
  41:bug.bug_id

This is due to the recent fix which changed bug.id to bug.bug_id, but in
filterexceptions.pl is still:
'list/list.rdf.tmpl' => [
  'template_version', 
  'bug.id', 
The last line should be have a 'bug.bug_id'

Comment 108

[Gervase Markham [:gerv]]

Comment on attachment 120331 [details] [diff] [review]
Patch v7 for 2.16 branch (corrected)

r=gerv. Looks OK to me.

Gerv

Comment 109

[Dave Miller [:justdave]]

OK, 2.16.3 patch is approved, now it's time for the localizers that have 2.16.x
based templates to do their part.

Here's your instructions:

1) Download the 2.16 branch patch from this bug.
2) Apply it to your Bugzilla working tree so that it applies the patches to the
English (en) templates and not your localized ones.
3) Copy template/en/default/filterexceptions.pl into template/<lang>/default
where <lang> is the language code for your localization
4) Run the following command:
   /usr/bonsaitools/bin/perl runtests.pl --verbose 008
5) Fix any errors reported.  Feel free to look at a copy of the patches that
applied to the English templates to see how they were fixed in the English ones.
 If you get anything reported by the test that wasn't fixed in the English
templates by the attached patches, feel free to ask here what's the best way to
fix it.
6) Create a new tarball of your localized templates.  Make sure the
filterexceptions.pl file is included.

DO NOT POST the new tarball for download anywhere yet, but report here when it's
ready.  I'll give everyone until late evening US time on Saturday to get to this
point, after that we'll run without you because we've been sitting on this far
too long already.  We might go sooner than that if everyone reports back sooner
that they're ready.  Once we're ready to go I'll give 24 hours notice by posting
on this bug the specific date and time when we're going to do the release
announcement.  The release announcement and security advisory will state that
updated localized templates will be posted on their respective sites within 24
hours.  If we go ahead without someone, we'll provide a list of which ones we
expect to be posted.  As soon as the release announcement is out, feel free to
post your tarball.

If anyone has any questions or concerns with this plan, let me know.

Comment 110

[Vitaly Fedrushkov]

Well, it won't be easy without having a full weekend available.

Hope this won't take more than 8 hours, will try at my best to meet the 
deadline.

Comment 111

[Dave Miller [:justdave]]

If you think you need the weekend go for it.  I'll wait if it's just for a day
or two :)

It took me 7 hours to backport the tip patch to 2.16.x.  It will probably take
you much less time since the filterexceptions file is already built.  In most
cases, unless you've added variables in the templates, it should just be a
matter of copying the english patches to your localized templates (but it'll
probably have to be done by hand, since the english context will prevent patch
from matching much).

Comment 112

[Vitaly Fedrushkov]

Followed instructions from comment 109 for 2.16.2:

Statement of facts

1. runtests.pl is a feature of 2.17.  Borrowed from here.

2. 008filter.t requires Test::More, not used elsewhere.  Got it with `perl
-MCPAN -e 'install "Test::More"'`

3. Then ran into a problem:

--- 8< ---
$ perl runtests.pl --verbose 008
t/008filter....Use of uninitialized value in string eq at
	/usr/lib/perl5/site_perl/5.6.1/Test/More.pm line 182 (#1)
    (W uninitialized) An undefined value was used as if it were already
    defined.  It was interpreted as a "" or a 0, but maybe it was a mistake.
    To suppress this warning assign a defined value to your variables.
    
    To help you figure out what was undefined, perl tells you what operation
    you used the undefined value in.  Note, however, that perl optimizes your
    program and the operation displayed in the warning may not necessarily
    appear literally in your program.  For example, "that $foo" is
    usually optimized into "that " . $foo, and the warning will refer to
    the concatenation (.) operator, even though there is no . in your
    program.
    
Got an undefined number of tests.  Looks like you tried to say how many tests
you plan to run but made a mistake.
BEGIN failed--compilation aborted at t/008filter.t line 39.
dubious
	Test returned status 255 (wstat 65280, 0xff00)
Uncaught exception from user code:
	FAILED--1 test script could be run, alas--no output ever seen
	Test::Harness::_show_results('HASH(0x8299e74)', 'HASH(0x8299e5c)') called at
/usr/lib/perl5/5.6.1/Test/Harness.pm line 341
	Test::Harness::runtests('t/008filter.t') called at runtests.pl line 41
--- 8< ---

Alas I'm not an expert in Perl 5.  What's happening?

Comment 113

[Dave Miller [:justdave]]

Willy:  cvs -q update -rBUGZILLA-2_16-BRANCH -dP

Then run checksetup.pl and try again.  You need the current 2.16 branch from cvs
(there's other minor changes besides the stuff on this bug, the testing stuff is
among those changes).

Sorry I forgot to mention that part...

If anyone has trouble pulling the current 2.16 branch from cvs let me know and I
can get you a tarball.

Comment 114

[Vitaly Fedrushkov]

Thanks, looks like I've misread patch attachment name.

Let's roll:

t/008filter.t does a relative chdir $path first, and never descends back.  
Quick hack:  chdir "../../.." at end of loop.  Then:

...
ok 77 - account/email/confirm.html.tmpl is filter-safe
not ok 78 - template/ru/default has templates but no filterexceptions.pl file. -
-ERROR
#     Failed test (t/008filter.t at line 55)
...

I'm absolutely sure template/ru/default/filterexceptions.pl is alive and 
readable.

What else can be wrong?

Comment 115

[Dave Miller [:justdave]]

Attachment: Revised 008filter test

This patch contains only the t/008filter.t and
template/en/default/filterexceptions.pl files.

The filter test has been revised so it properly handles multiple simultaneous
language directories.

Changes:
- It now does a "use Cwd;" at the top and grabs the current directory before
doing any chdirs, and chdirs back to that directory before using the relative
chdir into any template directory
- It now uses "do" instead of "require" on the filterexceptions.pl file. 
Apparently Perl doesn't realize that the directory has changed, and assumes it
has already loaded filterexceptions.pl, so the second time around the require
is a noop.

This patch assumes neither of those files exist, so if you've already applied
the previous patch, delete those two files before applying this one.  We will
need to produce new "final" patches that include both these tests and the
template changes prior to checkin.

Comment 116

[Vitaly Fedrushkov]

For no apparent reason, cwd on second pass after 'chdir $topdir' remains
"<whatever>/bugzilla/template/en/default"

Comment 117

[Vitaly Fedrushkov]

Got it.  $topdir has a trailing newline.

Comment 118

[Vitaly Fedrushkov]

Attachment: my 008filter.t

OK it works after

my $topdir = substr(cwd,0,-1);

Back to template audit.

Comment 119

[Dave Miller [:justdave]]

Vitaly: are you on Windows?

The docs for the cwd command specifically say that it *won't* have a trailing
newline on the end of it, and that's the selling reason why to use that instead of
my $topdir = `pwd`;  even on Linux  :)

Comment 120

[Gervase Markham [:gerv]]

I'm at home for Easter and not in a particularly good position to be doing
reviews. But I'm sure it's fine :-)

Gerv

Comment 121

[Dave Miller [:justdave]]

bah, I completely forgot it was Easter this weekend (between Gerv's comment, and
my looking at my calendar and realizing I was planning on going to church this
evening...  :)    If anyone needs a few days past this weekend I guess we'll
wait.  Don't want to screw up anyone's family happenings over this.

Comment 122

[Bradley Baetz (:bbaetz)]

Comment on attachment 120856 [details] [diff] [review]
Revised 008filter test

Right; you need |do| because |require| matches "./filterexceptions", because
'.' is the dir in @INC.

Because of how @::safe is  assigned, you don't need to clear it before the
|do|, but it may be an idea to do so anyway, just to be safe/for
documentation/etc.

Comment 123

[Vitaly Fedrushkov]

Dave,

I am not on windows.  On my machine 'cwd' in `perl -e 'use Cwd; print cwd;'`
works as expected, but not from within Test::Harness.  It took several debug
printouts to believe my own eyes on 1:55 AM...

BTW, 'chomp' doesn't work there, either.  Fix at comment 118 reflects this.

If I manage to reproduce on more machines, will file a separate bug about
008filter.t.

Comment 124

[Vitaly Fedrushkov]

Attachment: Bugzilla-ru cleanup report

Russian templates are done.  Three minor issues are:

1. bug/dependency-tree.html.tmpl: filterexceptions.pl appears to be
language-dependent here.
   There's no problem to fix it, as we have separate filterexceptions.pl for
each template set. 
   To make filters more generic, one could implement pattern matching for any
text within double 
   quotes.  Is it practical?

2. account/prefs/prefs.html.tmpl: mine is lacking "current_tab.description
FILTER lower" sample.
   IMHO it is safe to ignore, and filterexceptions.pl can be fixed.

3. runtests.pl dies after summary line with:

Uncaught exception from user code:
	Failed 1/1 test scripts, 0.00% okay. 1/149 subtests failed, 99.33%
okay.	     Test::Harness::_show_results('HASH(0x8299d14)', 'HASH(0x8299cfc)')
called at /usr/lib/perl5/5.6.1/Test/Harness.pm line 341
	Test::Harness::runtests('t/008filter.t') called at runtests.pl line 41

I guess it is safe.

Hope I've met the deadline :-)

Comment 125

[Dave Miller [:justdave]]

Gerv, bbaetz: any ideas on Vitaly's comment 124?

Oscar: have you had a chance to look at this yet since the patches got
finalized?  Do you know if they guy doing the Mexican templates knows about
this?  He doesn't appear to have a bugzilla account (I couldn't find it anyway)

I emailed the Chinese localizer privately a few times because he also doesn't
have a bugzilla account.  Have not heard back from him.

At this point looks like we're planning to release late Apr 22 or early Apr 23
(California time) unless someone steps up and says they're working on it and
need more time...  If I don't know you're working on it, we'll go without you. 
(We've been sitting on this far too long)

Comment 126

[Oscar Manuel Gómez Senovilla]

I've tried to help, but I've been unable the understand the solutions (I didn't
dedicated too much time, anyway), so go ahead without me. I don't want you to be
delayed because of me. I'll wait for the new release and act as if I didn't know
anything about this, unless there's something involved that makes mandatory
every localization to be reviewed.

In this later case, I suppose that instructions (if any) for new localizations
will be provided in the site.

Comment 127

[Bradley Baetz (:bbaetz)]

1/2 are correct, which is why these files are per template dir.

3 is some weird testing interaction we see when any test fails. I think theres a
bug filed on Zach for it,

Comment 128

[Vitaly Fedrushkov]

Then there may be another can of worms for localizers: what if we at main trunk
have reconsidered some line at filterexceptions.pl as unsafe, and deleted it?

Shouldn't we split filterexceptions.pl into 'generic' and 'specific' parts and
store them in root Bugzilla directory, and template/*/default, respectively?

Comment 129

[Gervase Markham [:gerv]]

I'd say we need a "localisers" mailing list for issues like that.

Gerv

Comment 130

[Dave Miller [:justdave]]

Done.  [e-mail] now exists, and all of the localizers have been
subcribed to it at their last-known email address.

Comment 131

[Vitaly Fedrushkov]

Comment 128 applies to template/en/custom as well, which is instance specific 
and cannot be addressed as easily as it can be done to a few localization 
projects.

None the less, bug 192677 has in fact landed, and current discussion is no more 
than seismic aftershock of such landing :-)

Comment 132

[Dave Miller [:justdave]]

adding es_MX maintainer

Comment 133

[Dave Miller [:justdave]]

Attachment: Patch v8 for trunk

all-in-one patch for the cvs tip.  This is the same as Patch v7 with the RDF
bug_id thing fixed and the revised 008filter.t test from attachment 120856 [details] [diff] [review].

Comment 134

[Dave Miller [:justdave]]

Attachment: Patch v8 for 2.16 branch

same as Patch v7 for the 2.16 branch, with the revised 008filter.t test.

Comment 135

[Dave Miller [:justdave]]

Checked in on both HEAD and BUGZILLA-2_16-BRANCH (I'll spare the paste of the
checkin results due to the massive number of files touched).

Comment 136

[Dave Miller [:justdave]]

Security Advisory has been posted, removing security group

Comment 137

[Dave Miller [:justdave]]

If you're having problems with the 008filter.t test, please see bug 203318