From b1ef63e5bfc0d3995245b42154686db1400b2c22 Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Sun, 15 Oct 2006 03:26:50 +0000 Subject: Bug 206037: [SECURITY] Fix escaping/quoting in edit*.cgi scripts - Patch by Frédéric Buclin r=justdave a=justdave MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Bugzilla/Constants.pm | 7 ++ Bugzilla/Install/Requirements.pm | 23 ++++++ Bugzilla/Template.pm | 7 +- Bugzilla/Util.pm | 95 +++++++++++++++++++++- skins/standard/editusers.css | 5 ++ t/008filter.t | 2 +- .../en/default/account/prefs/permissions.html.tmpl | 8 +- .../en/default/account/prefs/settings.html.tmpl | 8 +- .../en/default/admin/classifications/del.html.tmpl | 2 +- .../default/admin/classifications/edit.html.tmpl | 2 +- .../admin/classifications/reclassify.html.tmpl | 2 +- .../default/admin/classifications/select.html.tmpl | 2 +- .../admin/components/confirm-delete.html.tmpl | 4 +- .../en/default/admin/components/updated.html.tmpl | 2 +- template/en/default/admin/groups/delete.html.tmpl | 2 +- template/en/default/admin/groups/edit.html.tmpl | 2 +- template/en/default/admin/groups/list.html.tmpl | 1 + template/en/default/admin/keywords/list.html.tmpl | 3 +- .../admin/products/confirm-delete.html.tmpl | 6 +- .../default/admin/products/edit-common.html.tmpl | 2 +- template/en/default/admin/products/edit.html.tmpl | 2 +- .../en/default/admin/products/updated.html.tmpl | 2 +- template/en/default/admin/settings/edit.html.tmpl | 6 +- template/en/default/admin/table.html.tmpl | 16 ++-- template/en/default/admin/users/edit.html.tmpl | 2 +- template/en/default/admin/users/list.html.tmpl | 46 +++++++---- template/en/default/bug/create/create.html.tmpl | 2 +- template/en/default/bug/edit.html.tmpl | 6 +- template/en/default/bug/show-multiple.html.tmpl | 2 +- template/en/default/filterexceptions.pl | 35 -------- .../default/global/choose-classification.html.tmpl | 2 +- .../en/default/global/choose-product.html.tmpl | 2 +- template/en/default/list/edit-multiple.html.tmpl | 7 +- template/en/default/list/list-simple.html.tmpl | 4 +- template/en/default/reports/components.html.tmpl | 4 +- template/en/default/reports/keywords.html.tmpl | 4 +- 36 files changed, 219 insertions(+), 108 deletions(-) diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index 4ce2cbc09..337405a61 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -123,6 +123,8 @@ use File::Basename; ON_WINDOWS MAX_TOKEN_AGE + + SAFE_PROTOCOLS ); @Bugzilla::Constants::EXPORT_OK = qw(contenttypes); @@ -302,6 +304,11 @@ use constant FIELD_TYPE_SINGLE_SELECT => 2; # The maximum number of days a token will remain valid. use constant MAX_TOKEN_AGE => 3; +# 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'); + # States that are considered to be "open" for bugs. use constant BUG_STATE_OPEN => ('NEW', 'REOPENED', 'ASSIGNED', 'UNCONFIRMED'); diff --git a/Bugzilla/Install/Requirements.pm b/Bugzilla/Install/Requirements.pm index 06c8b557b..7dddefd75 100644 --- a/Bugzilla/Install/Requirements.pm +++ b/Bugzilla/Install/Requirements.pm @@ -125,6 +125,18 @@ use constant OPTIONAL_MODULES => [ name => 'SOAP::Lite', version => 0 }, + { + # Since Perl 5.8, we need the 'utf8_mode' method of HTML::Parser + # which has been introduced in version 3.39_92 and fixed in 3.40 + # to not complain when running Perl 5.6. + # This module is required by HTML::Scrubber. + name => 'HTML::Parser', + version => ($] >= 5.008) ? '3.40' : 0 + }, + { + name => 'HTML::Scrubber', + version => 0 + }, ]; # These are only required if you want to use Bugzilla with @@ -305,6 +317,17 @@ sub check_requirements { " " . install_command('Net::LDAP') . "\n\n"; } + # HTML filtering + if (!$have_mod{'HTML::Parser'} || !$have_mod{'HTML::Scrubber'}) { + print "If you want additional HTML tags within product and group", + " descriptions,\nyou should install:\n\n"; + print " HTML::Scrubber: " . install_command('HTML::Scrubber') . "\n" + if !$have_mod{'HTML::Scrubber'}; + print " HTML::Parser: " . install_command('HTML::Parser') . "\n" + if !$have_mod{'HTML::Parser'}; + print "\n"; + } + # mod_perl if (!$have_mod{'mod_perl2'}) { print "If you would like mod_perl support, you must install at", diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm index 7149828ef..915e3cdc6 100644 --- a/Bugzilla/Template.pm +++ b/Bugzilla/Template.pm @@ -289,7 +289,8 @@ sub quoteUrls { ~egox; # 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 @@ -734,7 +735,9 @@ sub create { } 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 8457c8df8..d346d2547 100644 --- a/Bugzilla/Util.pm +++ b/Bugzilla/Util.pm @@ -34,7 +34,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 get_netaddr correct_urlbase lsearch diff_arrays diff_strings @@ -95,6 +95,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 originally came from CGI.pm, by Lincoln D. Stein sub url_quote { my ($toencode) = (@_); @@ -553,6 +640,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/skins/standard/editusers.css b/skins/standard/editusers.css index a5bf4581f..55eb5c307 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 66f4b7c97..d4053461e 100644 --- a/t/008filter.t +++ b/t/008filter.t @@ -223,7 +223,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|base64|url_quote|css_class_quote| - ics|quoteUrls|time|uri|xml|lower| + ics|quoteUrls|time|uri|xml|lower|html_light| obsolete|inactive|closed|unitconvert| txt|none)\b/x; diff --git a/template/en/default/account/prefs/permissions.html.tmpl b/template/en/default/account/prefs/permissions.html.tmpl index dd6e1785b..77dda1ce4 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 3ef9a5852..568dac0cb 100644 --- a/template/en/default/account/prefs/settings.html.tmpl +++ b/template/en/default/account/prefs/settings.html.tmpl @@ -49,8 +49,8 @@ [% IF settings.${name}.is_enabled %] - [% ELSE %] - + diff --git a/template/en/default/admin/classifications/del.html.tmpl b/template/en/default/admin/classifications/del.html.tmpl index b450548b7..84c3cb197 100644 --- a/template/en/default/admin/classifications/del.html.tmpl +++ b/template/en/default/admin/classifications/del.html.tmpl @@ -36,7 +36,7 @@ Description: [% IF classification.description %] - [% classification.description FILTER none %] + [% classification.description FILTER html_light %] [% ELSE %] description missing [% END %] diff --git a/template/en/default/admin/classifications/edit.html.tmpl b/template/en/default/admin/classifications/edit.html.tmpl index b1fc482c2..b56a401f4 100644 --- a/template/en/default/admin/classifications/edit.html.tmpl +++ b/template/en/default/admin/classifications/edit.html.tmpl @@ -59,7 +59,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 d45b88073..0db2fc265 100644 --- a/template/en/default/admin/classifications/reclassify.html.tmpl +++ b/template/en/default/admin/classifications/reclassify.html.tmpl @@ -33,7 +33,7 @@ Description: [% IF classification.description %] - [% classification.description FILTER none %] + [% classification.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 eaa2149f0..fd3aaf45d 100644 --- a/template/en/default/admin/classifications/select.html.tmpl +++ b/template/en/default/admin/classifications/select.html.tmpl @@ -37,7 +37,7 @@ [% cl.name 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 4c94813fd..e7e00636e 100644 --- a/template/en/default/admin/components/confirm-delete.html.tmpl +++ b/template/en/default/admin/components/confirm-delete.html.tmpl @@ -44,7 +44,7 @@ Component Description: - [% comp.description FILTER html %] + [% comp.description FILTER html_light %] Default assignee: @@ -66,7 +66,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 a6f2c8b9d..a4cbfdf5b 100644 --- a/template/en/default/admin/components/updated.html.tmpl +++ b/template/en/default/admin/components/updated.html.tmpl @@ -56,7 +56,7 @@ - +
Updated description to:'[% comp.description FILTER html %]''[% comp.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 d0c50f69a..f5aa7a9b4 100644 --- a/template/en/default/admin/groups/delete.html.tmpl +++ b/template/en/default/admin/groups/delete.html.tmpl @@ -48,7 +48,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 51aba7ffe..a66e78fde 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 fe32bc53d..ef2c7486b 100644 --- a/template/en/default/admin/groups/list.html.tmpl +++ b/template/en/default/admin/groups/list.html.tmpl @@ -47,6 +47,7 @@ } {name => 'description' heading => 'Description' + allow_html_content => 1 } {name => 'userregexp' heading => 'User RegExp' diff --git a/template/en/default/admin/keywords/list.html.tmpl b/template/en/default/admin/keywords/list.html.tmpl index 999538561..1ffa0f27d 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 e59dd8707..75aeb623a 100644 --- a/template/en/default/admin/products/confirm-delete.html.tmpl +++ b/template/en/default/admin/products/confirm-delete.html.tmpl @@ -56,7 +56,7 @@ [%# descriptions are intentionally not filtered to allow html content %] [% IF classification.description %] - [% classification.description FILTER none %] + [% classification.description FILTER html_light %] [% ELSE %] missing [% END %] @@ -78,7 +78,7 @@ [%# descriptions are intentionally not filtered to allow html content %] [% IF product.description %] - [% product.description FILTER none %] + [% product.description FILTER html_light %] [% ELSE %] missing [% END %] @@ -132,7 +132,7 @@ [%# 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/products/edit-common.html.tmpl b/template/en/default/admin/products/edit-common.html.tmpl index e3edadc9c..afa15d73c 100644 --- a/template/en/default/admin/products/edit-common.html.tmpl +++ b/template/en/default/admin/products/edit-common.html.tmpl @@ -40,7 +40,7 @@ Description: + [% product.description FILTER html %] diff --git a/template/en/default/admin/products/edit.html.tmpl b/template/en/default/admin/products/edit.html.tmpl index 4e8cc7b19..105ec6e74 100644 --- a/template/en/default/admin/products/edit.html.tmpl +++ b/template/en/default/admin/products/edit.html.tmpl @@ -50,7 +50,7 @@ [% FOREACH component = product.components %] [% component.name FILTER html %]:  [% IF component.description %] - [% component.description FILTER none %] + [% component.description FILTER html_light %] [% ELSE %] description missing [% END %] diff --git a/template/en/default/admin/products/updated.html.tmpl b/template/en/default/admin/products/updated.html.tmpl index e74720fed..8a0790d6e 100644 --- a/template/en/default/admin/products/updated.html.tmpl +++ b/template/en/default/admin/products/updated.html.tmpl @@ -75,7 +75,7 @@

Updated description to:

-

[% product.description FILTER html %]

+

[% product.description FILTER html_light %]

[% updated = 1 %] [% END %] diff --git a/template/en/default/admin/settings/edit.html.tmpl b/template/en/default/admin/settings/edit.html.tmpl index 68c8577b0..9ca9226e7 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 %]