From: lpsolit%gmail.com <> Date: Sun, 15 Oct 2006 03:30:53 +0000 (+0000) Subject: Bug 206037: [SECURITY] Fix escaping/quoting in edit*.cgi scripts - Patch by Frédéric... X-Git-Tag: bugzilla-2.22.1~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c62e7eac726e0f18e868c5f4b99c7d79527edba9;p=thirdparty%2Fbugzilla.git Bug 206037: [SECURITY] Fix escaping/quoting in edit*.cgi scripts - Patch by Frédéric Buclin r=justdave a=justdave --- diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index c005187320..9baebf065a 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -91,6 +91,8 @@ use base qw(Exporter); ADMIN_GROUP_NAME SENDMAIL_EXE + + SAFE_PROTOCOLS ); @Bugzilla::Constants::EXPORT_OK = qw(contenttypes); @@ -243,4 +245,9 @@ use constant ADMIN_GROUP_NAME => 'admin'; # 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 a6cf8b6fbe..cbefc93b71 100644 --- a/Bugzilla/Template.pm +++ b/Bugzilla/Template.pm @@ -498,7 +498,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 3e3e3c1beb..8ac74a95fe 100644 --- a/Bugzilla/Util.pm +++ b/Bugzilla/Util.pm @@ -33,7 +33,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 correct_urlbase lsearch max min diff_arrays diff_strings @@ -94,6 +94,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) = (@_); @@ -535,6 +622,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 f7d3513e48..776d014389 100755 --- a/checksetup.pl +++ b/checksetup.pl @@ -386,6 +386,8 @@ my $gdgraph = have_vers("GD::Graph",0); my $gdtextalign = have_vers("GD::Text::Align",0); my $patchreader = have_vers("PatchReader","0.9.4"); my $imagemagick = have_vers("Image::Magick",0); +my $html_parser = have_vers("HTML::Parser", ($] >= 5.008) ? "3.40" : 0); +my $scrubber = have_vers("HTML::Scrubber", 0); print "\n" unless $silent; @@ -436,6 +438,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/globals.pl b/globals.pl index c490f6962a..e790cbb171 100644 --- a/globals.pl +++ b/globals.pl @@ -518,7 +518,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 92d4de33a1..cd351525ac 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| none)\b/x; diff --git a/template/en/default/account/prefs/permissions.html.tmpl b/template/en/default/account/prefs/permissions.html.tmpl index dd6e1785b9..77dda1ce42 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 3ef9a5852e..568dac0cbd 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 c32e46b4df..7f54a18ed8 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 24ec9dacfc..60ece58b2b 100644 --- a/template/en/default/admin/classifications/edit.html.tmpl +++ b/template/en/default/admin/classifications/edit.html.tmpl @@ -33,7 +33,7 @@ Description: + [% classification.description FILTER html %] @@ -49,7 +49,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 127aeea877..b89f83d093 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 789c40968b..616f8445b2 100644 --- a/template/en/default/admin/classifications/select.html.tmpl +++ b/template/en/default/admin/classifications/select.html.tmpl @@ -36,7 +36,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 dabace1545..1641884351 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: - [% prod.description FILTER html %] + [% prod.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 d0f66f2ad4..f52ace03c1 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 64e30f9e46..e6d96b142d 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 84eb6e9f19..b0b9096186 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 55fb49fb09..23550f26d7 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 ec64772870..64dd960a52 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 4e8cc7b195..105ec6e741 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 e74720fed8..8a0790d6ec 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 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 %]