From 18c9d798951a96e1edf64ea22436262fcbc36c03 Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Sun, 15 Oct 2006 03:34:28 +0000 Subject: [PATCH] =?utf8?q?Bug=20206037:=20[SECURITY]=20Fix=20escaping/quot?= =?utf8?q?ing=20in=20edit*.cgi=20scripts=20-=20Patch=20by=20Fr=C3=A9d?= =?utf8?q?=C3=A9ric=20Buclin=20=20r=3Djustdave=20a=3Dju?= =?utf8?q?stdave?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit --- Bugzilla/Constants.pm | 7 ++ Bugzilla/Template.pm | 4 +- Bugzilla/Util.pm | 95 ++++++++++++++++++- checksetup.pl | 11 +++ editproducts.cgi | 35 +++---- globals.pl | 3 +- skins/standard/editusers.css | 5 + t/008filter.t | 2 +- .../account/prefs/permissions.html.tmpl | 8 +- .../default/account/prefs/settings.html.tmpl | 4 +- .../admin/classifications/del.html.tmpl | 2 +- .../admin/classifications/edit.html.tmpl | 4 +- .../classifications/reclassify.html.tmpl | 2 +- .../admin/classifications/select.html.tmpl | 2 +- .../admin/components/confirm-delete.html.tmpl | 4 +- .../admin/components/updated.html.tmpl | 2 +- .../en/default/admin/groups/delete.html.tmpl | 2 +- .../en/default/admin/groups/edit.html.tmpl | 2 +- .../en/default/admin/groups/list.html.tmpl | 2 +- .../en/default/admin/keywords/list.html.tmpl | 3 +- .../admin/products/confirm-delete.html.tmpl | 28 +++--- .../en/default/admin/settings/edit.html.tmpl | 6 +- template/en/default/admin/table.html.tmpl | 15 +-- .../en/default/admin/users/edit.html.tmpl | 2 +- .../en/default/admin/users/list.html.tmpl | 22 +---- .../en/default/bug/create/create.html.tmpl | 2 +- template/en/default/bug/edit.html.tmpl | 2 +- template/en/default/filterexceptions.pl | 47 --------- .../global/choose-classification.html.tmpl | 2 +- .../default/global/choose-product.html.tmpl | 2 +- .../en/default/list/edit-multiple.html.tmpl | 7 +- .../en/default/list/list-simple.html.tmpl | 4 +- .../en/default/reports/components.html.tmpl | 4 +- .../en/default/reports/keywords.html.tmpl | 4 +- 34 files changed, 204 insertions(+), 142 deletions(-) diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index a5cea32923..4f0f819f93 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -89,6 +89,8 @@ use base qw(Exporter); FULLTEXT_BUGLIST_LIMIT SENDMAIL_EXE + + SAFE_PROTOCOLS ); @Bugzilla::Constants::EXPORT_OK = qw(contenttypes); @@ -238,4 +240,9 @@ use constant FULLTEXT_BUGLIST_LIMIT => 200; # Path to sendmail.exe (Windows only) use constant SENDMAIL_EXE => '/usr/lib/sendmail.exe'; +# Protocols which are considered as safe. +use constant SAFE_PROTOCOLS => ('afs', 'cid', 'ftp', 'gopher', 'http', 'https', + 'irc', 'mid', 'news', 'nntp', 'prospero', 'telnet', + 'view-source', 'wais'); + 1; diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm index f4bcc7105c..26a0aebdcd 100644 --- a/Bugzilla/Template.pm +++ b/Bugzilla/Template.pm @@ -375,7 +375,9 @@ sub create { $var =~ s/\@/\@/g; return $var; }, - + + html_light => \&Bugzilla::Util::html_light_quote, + # iCalendar contentline filter ics => [ sub { my ($context, @args) = @_; diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm index 9d5f40ffb6..e5cffd29f7 100644 --- a/Bugzilla/Util.pm +++ b/Bugzilla/Util.pm @@ -32,7 +32,7 @@ use base qw(Exporter); @Bugzilla::Util::EXPORT = qw(is_tainted trick_taint detaint_natural detaint_signed html_quote url_quote value_quote xml_quote - css_class_quote + css_class_quote html_light_quote i_am_cgi lsearch max min diff_arrays diff_strings @@ -91,6 +91,93 @@ sub html_quote { return $var; } +sub html_light_quote { + my ($text) = @_; + + # List of allowed HTML elements having no attributes. + my @allow = qw(b strong em i u p br abbr acronym ins del cite code var + dfn samp kbd big small sub sup tt dd dt dl ul li ol); + + # Are HTML::Scrubber and HTML::Parser installed? + eval { require HTML::Scrubber; + require HTML::Parser; + }; + + # We need utf8_mode() from HTML::Parser 3.40 if running Perl >= 5.8. + if ($@ || ($] >= 5.008 && $HTML::Parser::VERSION < 3.40)) { # Package(s) not installed. + my $safe = join('|', @allow); + my $chr = chr(1); + + # First, escape safe elements. + $text =~ s#<($safe)>#$chr$1$chr#go; + $text =~ s##$chr/$1$chr#go; + # Now filter < and >. + $text =~ s#<#<#g; + $text =~ s#>#>#g; + # Restore safe elements. + $text =~ s#$chr/($safe)$chr##go; + $text =~ s#$chr($safe)$chr#<$1>#go; + return $text; + } + else { # Packages installed. + # We can be less restrictive. We can accept elements with attributes. + push(@allow, qw(a blockquote q span)); + + # Allowed protocols. + my $safe_protocols = join('|', SAFE_PROTOCOLS); + my $protocol_regexp = qr{(^(?:$safe_protocols):|^[^:]+$)}i; + + # Deny all elements and attributes unless explicitly authorized. + my @default = (0 => { + id => 1, + name => 1, + class => 1, + '*' => 0, # Reject all other attributes. + } + ); + + # Specific rules for allowed elements. If no specific rule is set + # for a given element, then the default is used. + my @rules = (a => { + href => $protocol_regexp, + title => 1, + id => 1, + name => 1, + class => 1, + '*' => 0, # Reject all other attributes. + }, + blockquote => { + cite => $protocol_regexp, + id => 1, + name => 1, + class => 1, + '*' => 0, # Reject all other attributes. + }, + 'q' => { + cite => $protocol_regexp, + id => 1, + name => 1, + class => 1, + '*' => 0, # Reject all other attributes. + }, + ); + + my $scrubber = HTML::Scrubber->new(default => \@default, + allow => \@allow, + rules => \@rules, + comment => 0, + process => 0); + + # Avoid filling the web server error log with Perl 5.8.x. + # In HTML::Scrubber 0.08, the HTML::Parser object is stored in + # the "_p" key, but this may change in future versions. + if ($] >= 5.008 && ref($scrubber->{_p}) eq 'HTML::Parser') { + $scrubber->{_p}->utf8_mode(1); + } + return $scrubber->scrub($text); + } +} + # This orignally came from CGI.pm, by Lincoln D. Stein sub url_quote { my ($toencode) = (@_); @@ -473,6 +560,12 @@ be done in the template where possible. Returns a value quoted for use in HTML, with &, E, E, and E<34> being replaced with their appropriate HTML entities. +=item C + +Returns a string where only explicitly allowed HTML elements and attributes +are kept. All HTML elements and attributes not being in the whitelist are either +escaped (if HTML::Scrubber is not installed) or removed. + =item C Quotes characters so that they may be included as part of a url. diff --git a/checksetup.pl b/checksetup.pl index 3a09b73993..cee1ee8a3f 100755 --- a/checksetup.pl +++ b/checksetup.pl @@ -371,6 +371,8 @@ my $xmlparser = have_vers("XML::Parser",0); my $gdgraph = have_vers("GD::Graph",0); my $gdtextalign = have_vers("GD::Text::Align",0); my $patchreader = have_vers("PatchReader","0.9.4"); +my $html_parser = have_vers("HTML::Parser", ($] >= 5.008) ? "3.40" : 0); +my $scrubber = have_vers("HTML::Scrubber", 0); print "\n" unless $silent; @@ -413,6 +415,15 @@ if (!$patchreader && !$silent) { print "install the \nPatchReader module:\n"; print "PatchReader: " . install_command("PatchReader") . "\n"; } +if ((!$scrubber || !$html_parser) && !$silent) { + print "If you want additional HTML tags within product and group "; + print "descriptions,\nyou should install:\n"; + print "HTML::Scrubber: " . install_command("HTML::Scrubber") . "\n" + if !$scrubber; + print "HTML::Parser: " . install_command("HTML::Parser") . "\n" + if !$html_parser; + print "\n"; +} if (%missing) { print "\n\n"; diff --git a/editproducts.cgi b/editproducts.cgi index e8bca53504..b5f62ccb52 100755 --- a/editproducts.cgi +++ b/editproducts.cgi @@ -23,7 +23,7 @@ # Dawn Endico # Joe Robins # Gavin Shelley -# Frédéric Buclin +# Frédéric Buclin # Greg Hendricks # # Direct any questions on this source code to @@ -78,7 +78,7 @@ sub CheckProduct ($) } unless (TestProduct $prod) { - print "Sorry, product '$prod' does not exist."; + print "Sorry, product '" . html_quote($prod) . "' does not exist."; PutTrailer(); exit; } @@ -110,7 +110,7 @@ sub CheckClassification ($) } unless (TestClassification $cl) { - print "Sorry, classification '$cl' does not exist."; + print "Sorry, classification '" . html_quote($cl) . "' does not exist."; PutTrailer(); exit; } @@ -157,7 +157,8 @@ sub CheckClassificationProduct ($$) my $res = $dbh->selectrow_array($query, undef, ($prod, $cl)); unless ($res) { - print "Sorry, classification->product '$cl'->'$prod' does not exist."; + print "Sorry, classification->product '" . html_quote($cl) . + "'->'" . html_quote($prod) . "' does not exist."; PutTrailer(); exit; } @@ -473,7 +474,7 @@ if ($action eq 'new') { # Check for exact case sensitive match: if ($existing_product eq $product) { - print "The product '$product' already exists. Please press\n"; + print "The product '" . html_quote($product) . "' already exists. Please press\n"; print "Back and try again.\n"; PutTrailer($localtrailer); exit; @@ -481,8 +482,8 @@ if ($action eq 'new') { # Next check for a case-insensitive match: if (lc($existing_product) eq lc($product)) { - print "The new product '$product' differs from existing product "; - print "'$existing_product' only in case. Please press\n"; + print "The new product '" . html_quote($product) . "' differs from existing product '"; + print html_quote($existing_product) . "' only in case. Please press\n"; print "Back and try again.\n"; PutTrailer($localtrailer); exit; @@ -492,7 +493,7 @@ if ($action eq 'new') { my $version = trim($cgi->param('version') || ''); if ($version eq '') { - print "You must enter a version for product '$product'. Please press\n"; + print "You must enter a version for product '" . html_quote($product) . "'. Please press\n"; print "Back and try again.\n"; PutTrailer($localtrailer); exit; @@ -501,7 +502,7 @@ if ($action eq 'new') { my $description = trim($cgi->param('description') || ''); if ($description eq '') { - print "You must enter a description for product '$product'. Please press\n"; + print "You must enter a description for product '" . html_quote($product) . "'. Please press\n"; print "Back and try again.\n"; PutTrailer($localtrailer); exit; @@ -840,8 +841,8 @@ if ($action eq 'edit' || (!$action && $product)) { while ( MoreSQLData() ) { my ($component, $description) = FetchSQLData(); $description ||= "description missing"; - print "$component:"; - print "$description\n"; + print "" . html_quote($component) . ":"; + print "" . html_light_quote($description) . "\n"; } print "\n"; } else { @@ -861,7 +862,7 @@ if ($action eq 'edit' || (!$action && $product)) { while ( MoreSQLData() ) { my ($version) = FetchSQLData(); print "
" if $br; - print $version; + print html_quote($version); $br = 1; } } else { @@ -884,7 +885,7 @@ if ($action eq 'edit' || (!$action && $product)) { while ( MoreSQLData() ) { my ($milestone) = FetchSQLData(); print "
" if $br; - print $milestone; + print html_quote($milestone); $br = 1; } } else { @@ -1317,7 +1318,7 @@ if ($action eq 'update') { if (lc($product) ne lc($productold) && TestProduct($product)) { - print "Sorry, product name '$product' is already in use."; + print "Sorry, product name '" . html_quote($product) . "' is already in use."; $dbh->bz_unlock_tables(UNLOCK_ABORT); PutTrailer($localtrailer); exit; @@ -1347,7 +1348,8 @@ if ($action eq 'update') { my ($who, $id) = (@$ref); RemoveVotes($id, $who, "The rules for voting on this product has changed;\nyou had too many votes for a single bug."); my $name = DBID_to_name($who); - print qq{
Removed votes for bug $id from $name\n}; + print "
Removed votes for bug $id from " . + html_quote($name) . "\n"; } } @@ -1379,7 +1381,8 @@ if ($action eq 'update') { RemoveVotes($id, $who, "The rules for voting on this product has changed; you had too many\ntotal votes, so all votes have been removed."); my $name = DBID_to_name($who); - print qq{
Removed votes for bug $id from $name\n}; + print "
Removed votes for bug $id from " . + html_quote($name) . "\n"; } } } diff --git a/globals.pl b/globals.pl index 437667e073..f3073703bd 100644 --- a/globals.pl +++ b/globals.pl @@ -786,7 +786,8 @@ sub quoteUrls { my $tmp; # non-mailto protocols - my $protocol_re = qr/(afs|cid|ftp|gopher|http|https|irc|mid|news|nntp|prospero|telnet|view-source|wais)/i; + my $safe_protocols = join('|', SAFE_PROTOCOLS); + my $protocol_re = qr/($safe_protocols)/i; $text =~ s~\b(${protocol_re}: # The protocol: [^\s<>\"]+ # Any non-whitespace diff --git a/skins/standard/editusers.css b/skins/standard/editusers.css index a5bf4581fd..55eb5c307f 100644 --- a/skins/standard/editusers.css +++ b/skins/standard/editusers.css @@ -50,3 +50,8 @@ table.groups td.checkbox { text-align: center; white-space: nowrap; } + +.missing { + color: red; + border-color: inherit; +} diff --git a/t/008filter.t b/t/008filter.t index a3a56e0053..244ff993af 100644 --- a/t/008filter.t +++ b/t/008filter.t @@ -222,7 +222,7 @@ sub directive_ok { # Note: If a single directive prints two things, and only one is # filtered, we may not catch that case. return 1 if $directive =~ /FILTER\ (html|csv|js|url_quote|css_class_quote| - ics|quoteUrls|time|uri|xml|lower| + ics|quoteUrls|time|uri|xml|lower|html_light| obsolete|inactive|closed|unitconvert| none)\b/x; diff --git a/template/en/default/account/prefs/permissions.html.tmpl b/template/en/default/account/prefs/permissions.html.tmpl index 46d4559ec8..77cc70999f 100644 --- a/template/en/default/account/prefs/permissions.html.tmpl +++ b/template/en/default/account/prefs/permissions.html.tmpl @@ -42,8 +42,8 @@ [% FOREACH bit_description = has_bits %] - - + + [% END %]
[% bit_description.name %][% bit_description.desc %][% bit_description.name FILTER html %][% bit_description.desc FILTER html_light %]
@@ -63,8 +63,8 @@ [% FOREACH bit_description = set_bits %] - - + + [% END %]
[% bit_description.name %][% bit_description.desc %][% bit_description.name FILTER html %][% bit_description.desc FILTER html_light %]
diff --git a/template/en/default/account/prefs/settings.html.tmpl b/template/en/default/account/prefs/settings.html.tmpl index a425dcac15..31b1d745b1 100644 --- a/template/en/default/account/prefs/settings.html.tmpl +++ b/template/en/default/account/prefs/settings.html.tmpl @@ -38,8 +38,8 @@ [% setting_descs.$name OR name FILTER html %] - + Edit products @@ -43,7 +43,7 @@ [% product.name FILTER html %] [% IF product.description %] - [% product.description FILTER none %] + [% product.description FILTER html_light %] [% ELSE %] description missing [% END %] diff --git a/template/en/default/admin/classifications/reclassify.html.tmpl b/template/en/default/admin/classifications/reclassify.html.tmpl index 5d4cb73e4a..82e5d0d656 100644 --- a/template/en/default/admin/classifications/reclassify.html.tmpl +++ b/template/en/default/admin/classifications/reclassify.html.tmpl @@ -35,7 +35,7 @@ Description: [% IF description %] - [% description %] + [% description FILTER html_light %] [% ELSE %] description missing [% END %] diff --git a/template/en/default/admin/classifications/select.html.tmpl b/template/en/default/admin/classifications/select.html.tmpl index 3cfa3fcaaa..b61cdfe86d 100644 --- a/template/en/default/admin/classifications/select.html.tmpl +++ b/template/en/default/admin/classifications/select.html.tmpl @@ -38,7 +38,7 @@ [% cl.classification FILTER html %] [% IF cl.description %] - [% cl.description %] + [% cl.description FILTER html_light %] [% ELSE %] none [% END %] diff --git a/template/en/default/admin/components/confirm-delete.html.tmpl b/template/en/default/admin/components/confirm-delete.html.tmpl index 5e108e7a88..e6dfc70b6b 100644 --- a/template/en/default/admin/components/confirm-delete.html.tmpl +++ b/template/en/default/admin/components/confirm-delete.html.tmpl @@ -60,7 +60,7 @@ Component Description: - [% description FILTER html %] + [% description FILTER html_light %] Default assignee: @@ -82,7 +82,7 @@ Product Description: - [% product_description FILTER html %] + [% product_description FILTER html_light %] [% END %] [% IF Param('usetargetmilestone') %] diff --git a/template/en/default/admin/components/updated.html.tmpl b/template/en/default/admin/components/updated.html.tmpl index b4c4fea3cf..e4501a850c 100644 --- a/template/en/default/admin/components/updated.html.tmpl +++ b/template/en/default/admin/components/updated.html.tmpl @@ -48,7 +48,7 @@ - +
Updated description to:'[% description FILTER html %]''[% description FILTER html_light %]'
[% END %] diff --git a/template/en/default/admin/groups/delete.html.tmpl b/template/en/default/admin/groups/delete.html.tmpl index d720ecddc5..ddeedf75ce 100644 --- a/template/en/default/admin/groups/delete.html.tmpl +++ b/template/en/default/admin/groups/delete.html.tmpl @@ -47,7 +47,7 @@ [% gid FILTER html %] [% name FILTER html %] - [% description FILTER html %] + [% description FILTER html_light %] diff --git a/template/en/default/admin/groups/edit.html.tmpl b/template/en/default/admin/groups/edit.html.tmpl index 610d3102ef..bb513cf12e 100644 --- a/template/en/default/admin/groups/edit.html.tmpl +++ b/template/en/default/admin/groups/edit.html.tmpl @@ -165,7 +165,7 @@ [% group.grpnam FILTER html %] - [% group.grpdesc FILTER html %] + [% group.grpdesc FILTER html_light %] [% END %] diff --git a/template/en/default/admin/groups/list.html.tmpl b/template/en/default/admin/groups/list.html.tmpl index ee1eced11e..0d850ddc32 100644 --- a/template/en/default/admin/groups/list.html.tmpl +++ b/template/en/default/admin/groups/list.html.tmpl @@ -54,7 +54,7 @@ [% group.name FILTER html %] - [% group.description FILTER html %] + [% group.description FILTER html_light %] [% group.regexp FILTER html %]  diff --git a/template/en/default/admin/keywords/list.html.tmpl b/template/en/default/admin/keywords/list.html.tmpl index 113b904335..eaa96fd90c 100755 --- a/template/en/default/admin/keywords/list.html.tmpl +++ b/template/en/default/admin/keywords/list.html.tmpl @@ -43,7 +43,8 @@ }, { name => "description" - heading => "Description" + heading => "Description" + allow_html_content => 1 }, { name => "bug_count" diff --git a/template/en/default/admin/products/confirm-delete.html.tmpl b/template/en/default/admin/products/confirm-delete.html.tmpl index dc18eb512f..4b3c406790 100644 --- a/template/en/default/admin/products/confirm-delete.html.tmpl +++ b/template/en/default/admin/products/confirm-delete.html.tmpl @@ -56,13 +56,6 @@ [% classification_url_part = "" %] [% END %] -[% UNLESS class_description %] - [% class_description = 'missing' %] -[% END %] -[% UNLESS prod_description %] - [% prod_description = 'missing' %] -[% END %] - [% IF disallownew %] [% disallownew = "closed" %] [% ELSE %] @@ -82,8 +75,13 @@ Classification Description: - [%# descriptions are intentionally not filtered to allow html content %] - [% class_description FILTER none %] + + [% IF class_description %] + [% class_description FILTER html_light %] + [% ELSE %] + missing + [% END %] + [% END %] @@ -98,8 +96,13 @@ Description: - [%# descriptions are intentionally not filtered to allow html content %] - [% prod_description FILTER none %] + + [% IF prod_description %] + [% prod_description FILTER html_light %] + [% ELSE %] + missing + [% END %] + [% IF Param('usetargetmilestone') %] @@ -140,10 +143,9 @@ [% FOREACH c = components %] [% c.name FILTER html %]: - [%# descriptions are intentionally not filtered to allow html content %] [% IF c.description %] - [% c.description FILTER none %] + [% c.description FILTER html_light %] [% ELSE %] missing [% END %] diff --git a/template/en/default/admin/settings/edit.html.tmpl b/template/en/default/admin/settings/edit.html.tmpl index b5377a2410..85663f19b1 100644 --- a/template/en/default/admin/settings/edit.html.tmpl +++ b/template/en/default/admin/settings/edit.html.tmpl @@ -64,7 +64,7 @@ page, and the Default Value will automatically apply to everyone. [% setting_descs.$name OR name FILTER html %] - [% FOREACH x = settings.${name}.legal_values %]