Closed
Bug 192677
Opened 22 years ago
Closed 22 years ago
Audit all templates for FILTER usage
Categories
(Bugzilla :: Bugzilla-General, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: gerv, Assigned: gerv)
References
Details
(Whiteboard: [fixed in 2.16.3] [fixed in 2.17.4])
Attachments
(6 files, 18 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•22 years ago
|
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.18
Assignee | ||
Comment 1•22 years ago
|
||
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•22 years ago
|
||
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)
Assignee | ||
Comment 3•22 years ago
|
||
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•22 years ago
|
||
That sounds like a cool idea. Zach?
Comment 5•22 years ago
|
||
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.
Assignee | ||
Comment 6•22 years ago
|
||
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•22 years ago
|
||
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?
Assignee | ||
Comment 8•22 years ago
|
||
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•22 years ago
|
||
Securing, since we now have known exploits. Adding the maintainers of the localized templates that we know of, since this affects them.
Group: webtools-security
Assignee | ||
Comment 10•22 years ago
|
||
Kicking off people I don't know; the code I'm about to attach reveals XSS vulnerabilities. If anyone can vouch for bugzilla@chimpychompy.org, please add a comment. Gerv
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•22 years ago
|
||
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
Assignee | ||
Updated•22 years ago
|
Attachment #114602 -
Flags: review?(justdave)
Assignee | ||
Updated•22 years ago
|
Attachment #114602 -
Flags: review?(zach)
Comment 12•22 years ago
|
||
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....
Attachment #114602 -
Flags: review?(justdave) → review-
Assignee | ||
Comment 13•22 years ago
|
||
> 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•22 years ago
|
||
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.
Assignee | ||
Comment 15•22 years ago
|
||
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•22 years ago
|
||
Comment 17•22 years ago
|
||
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.
Attachment #114602 -
Attachment is obsolete: true
Comment 18•22 years ago
|
||
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•22 years ago
|
||
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.
Whiteboard: [wanted for 2.16.3][wanted for trunk]
Comment 20•22 years ago
|
||
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
Attachment #114621 -
Attachment is obsolete: true
Comment 21•22 years ago
|
||
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.
Attachment #114620 -
Attachment is obsolete: true
Assignee | ||
Comment 22•22 years ago
|
||
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
Assignee | ||
Comment 23•22 years ago
|
||
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
Attachment #114638 -
Attachment is obsolete: true
Comment 24•22 years ago
|
||
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?
Assignee | ||
Comment 25•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 27•22 years ago
|
||
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
Assignee | ||
Comment 28•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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
Attachment #114640 -
Attachment is obsolete: true
Assignee | ||
Comment 31•22 years ago
|
||
> * 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•22 years ago
|
||
> 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•22 years ago
|
||
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.
Assignee | ||
Comment 34•22 years ago
|
||
> 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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 37•22 years ago
|
||
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•22 years ago
|
||
Shouldn't be doing != shouldn't test here What if FILTER is part of a string somewhere? :)
Comment 39•22 years ago
|
||
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.
Assignee | ||
Comment 40•22 years ago
|
||
> 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•22 years ago
|
||
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•22 years ago
|
||
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 :).
Assignee | ||
Comment 43•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 45•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 47•22 years ago
|
||
> 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•22 years ago
|
||
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?
Assignee | ||
Comment 49•22 years ago
|
||
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•22 years ago
|
||
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?
Assignee | ||
Comment 51•22 years ago
|
||
Just a thought: in view of bug 38862, why are we even bothering? :-) Gerv
Assignee | ||
Comment 52•22 years ago
|
||
Your filterexceptions.pl needs a 1; at the end. Gerv
Comment 53•22 years ago
|
||
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?
Assignee | ||
Comment 54•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 57•22 years ago
|
||
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•22 years ago
|
||
>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•22 years ago
|
||
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)
Assignee | ||
Comment 60•22 years ago
|
||
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
Attachment #114625 -
Attachment is obsolete: true
Attachment #115154 -
Attachment is obsolete: true
Attachment #115704 -
Attachment is obsolete: true
Attachment #116505 -
Attachment is obsolete: true
Assignee | ||
Comment 61•22 years ago
|
||
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•22 years ago
|
||
Whats a 'buffer-type problem'?
Assignee | ||
Comment 63•22 years ago
|
||
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
Assignee | ||
Comment 64•22 years ago
|
||
Comment on attachment 116624 [details] [diff] [review] Patch v.4 Dave: any chance of a review? :-) Gerv
Attachment #116624 -
Flags: review?(justdave)
Assignee | ||
Updated•22 years ago
|
Attachment #114602 -
Flags: review?(zach)
Comment 65•22 years ago
|
||
*** Bug 193561 has been marked as a duplicate of this bug. ***
Comment 66•22 years ago
|
||
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?
Assignee | ||
Comment 67•22 years ago
|
||
> 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•22 years ago
|
||
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.
Assignee | ||
Comment 69•22 years ago
|
||
> 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
Assignee | ||
Comment 70•22 years ago
|
||
> 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
Assignee | ||
Comment 71•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 73•22 years ago
|
||
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•22 years ago
|
||
> <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.
Assignee | ||
Comment 75•22 years ago
|
||
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
Attachment #116624 -
Attachment is obsolete: true
Comment 76•22 years ago
|
||
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...
Assignee | ||
Comment 77•22 years ago
|
||
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
Attachment #118495 -
Flags: review?(bbaetz)
Comment 78•22 years ago
|
||
Well, it can be the query url - it depends what argument you pass to it :)
Updated•22 years ago
|
Attachment #116624 -
Flags: review?(justdave)
Assignee | ||
Comment 79•22 years ago
|
||
Here's an unrotted version of the trunk patch. Gerv
Attachment #118495 -
Attachment is obsolete: true
Comment 80•22 years ago
|
||
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 <bbaetz@student.usyd.edu.au> 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 <bbaetz@student.usyd.edu.au> 25 Oct 2002 18:57 Bug 147833 - start using CGI.pm r=gerv, justdave - [%+ key %]: [%+ variables.$key %] + [%+ key FILTER html %]: [%+ variables.$key FILTER html %]
Updated•22 years ago
|
Attachment #118880 -
Flags: review?(gerv)
Updated•22 years ago
|
Attachment #118880 -
Flags: review?(bbaetz)
Comment 81•22 years ago
|
||
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.
Attachment #118871 -
Flags: review+
Comment 82•22 years ago
|
||
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•22 years ago
|
||
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.
Updated•22 years ago
|
Attachment #118871 -
Flags: review?
Comment 84•22 years ago
|
||
>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•22 years ago
|
||
reference comment 83, he means the code currently running on bmo (not the trunk) with this patch applied.
Comment 86•22 years ago
|
||
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
Attachment #118871 -
Flags: review?
Assignee | ||
Comment 87•22 years ago
|
||
Comment on attachment 118880 [details] [diff] [review] Patch v6 backported to 2.16 branch Looks OK to me. Gerv
Attachment #118880 -
Flags: review?(gerv) → review+
Comment 88•22 years ago
|
||
These all look fine - tehres one more thing I want to test, and I'll do that in 3-4 hours' time.
Comment 89•22 years ago
|
||
.. or rather, I would have, but I got sidetracked. Sorry - hopefully tomorrow afternoon my time.
Comment 90•22 years ago
|
||
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.
Assignee | ||
Comment 91•22 years ago
|
||
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•22 years ago
|
||
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?
Attachment #118880 -
Flags: review?(bbaetz) → review-
Comment 93•22 years ago
|
||
>>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•22 years ago
|
||
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.
Assignee | ||
Comment 95•22 years ago
|
||
> 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•22 years ago
|
||
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.
Assignee | ||
Comment 97•22 years ago
|
||
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•22 years ago
|
||
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 :)
Assignee | ||
Comment 99•22 years ago
|
||
Here's v.7, with markups from bbaetz for the 2.16 patch applied. Gerv
Attachment #118871 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #119645 -
Flags: review+
Assignee | ||
Updated•22 years ago
|
Attachment #119645 -
Flags: review+
Comment 100•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Attachment #119645 -
Flags: review?(justdave)
Assignee | ||
Updated•22 years ago
|
Attachment #119645 -
Flags: review?(bbaetz)
Comment 104•22 years ago
|
||
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...
Attachment #119645 -
Flags: review?(bbaetz) → review+
Comment 105•22 years ago
|
||
Attachment #118880 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #120327 -
Flags: review?(bbaetz)
Updated•22 years ago
|
Attachment #120327 -
Flags: review?(gerv)
Updated•22 years ago
|
Attachment #119645 -
Flags: review?(justdave)
Updated•22 years ago
|
Attachment #120327 -
Flags: review?(gerv)
Attachment #120327 -
Flags: review?(bbaetz)
Comment 106•22 years ago
|
||
Goofed up the diff command line and didn't realize it till after it was posted. Here's the correct patch.
Attachment #120327 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #120331 -
Flags: review?(bbaetz)
Updated•22 years ago
|
Attachment #120331 -
Flags: review?(gerv)
Comment 107•22 years ago
|
||
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'
Updated•22 years ago
|
Attachment #120331 -
Flags: review?(bbaetz) → review+
Assignee | ||
Comment 108•22 years ago
|
||
Comment on attachment 120331 [details] [diff] [review] Patch v7 for 2.16 branch (corrected) r=gerv. Looks OK to me. Gerv
Attachment #120331 -
Flags: review?(gerv)
Comment 109•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Updated•22 years ago
|
Attachment #120856 -
Flags: review?(gerv)
Updated•22 years ago
|
Attachment #120856 -
Flags: review?(bbaetz)
Comment 116•22 years ago
|
||
For no apparent reason, cwd on second pass after 'chdir $topdir' remains "<whatever>/bugzilla/template/en/default"
Comment 117•22 years ago
|
||
Got it. $topdir has a trailing newline.
Comment 118•22 years ago
|
||
OK it works after my $topdir = substr(cwd,0,-1); Back to template audit.
Comment 119•22 years ago
|
||
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 :)
Assignee | ||
Comment 120•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Attachment #120856 -
Flags: review?(bbaetz) → review+
Updated•22 years ago
|
Attachment #118495 -
Flags: review?(bbaetz)
Comment 123•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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?
Assignee | ||
Comment 129•22 years ago
|
||
I'd say we need a "localisers" mailing list for issues like that. Gerv
Comment 130•22 years ago
|
||
Done. localizers@bugzilla.org now exists, and all of the localizers have been subcribed to it at their last-known email address.
Comment 131•22 years ago
|
||
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•22 years ago
|
||
adding es_MX maintainer
Comment 133•22 years ago
|
||
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].
Attachment #119645 -
Attachment is obsolete: true
Attachment #120856 -
Attachment is obsolete: true
Attachment #120869 -
Attachment is obsolete: true
Comment 134•22 years ago
|
||
same as Patch v7 for the 2.16 branch, with the revised 008filter.t test.
Updated•22 years ago
|
Attachment #120331 -
Attachment is obsolete: true
Updated•22 years ago
|
Flags: approval+
Comment 135•22 years ago
|
||
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).
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [wanted for 2.16.3][wanted for trunk] → [fixed in 2.16.3][fixed in 2.17.4]
Updated•22 years ago
|
Whiteboard: [fixed in 2.16.3][fixed in 2.17.4] → [fixed in 2.16.3] [fixed in 2.17.4]
Assignee | ||
Updated•22 years ago
|
Attachment #120856 -
Flags: review?(gerv)
Comment 136•22 years ago
|
||
Security Advisory has been posted, removing security group
Group: webtools-security
Comment 137•22 years ago
|
||
If you're having problems with the 008filter.t test, please see bug 203318
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•