From 9244270a7d1ca49e315a98c24d51bf405bfa2880 Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Mon, 24 Jan 2011 19:29:39 +0100 Subject: Bug 619588: (CVE-2010-4567) [SECURITY] Safety checks that disallow clicking for javascript: or data: URLs in the URL field can be evaded with prefixed whitespace and Bug 628034: (CVE-2011-0048) [SECURITY] For not-logged-in users, the URL field doesn't safeguard against javascript: or data: URLs r=dkl a=LpSolit --- Bugzilla/Template.pm | 27 +++++++++++++++++++------ template/en/default/attachment/edit.html.tmpl | 5 +---- template/en/default/bug/edit.html.tmpl | 10 ++++----- template/en/default/bug/show-multiple.html.tmpl | 6 +++--- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm index 453ca5596..4de2e815f 100644 --- a/Bugzilla/Template.pm +++ b/Bugzilla/Template.pm @@ -66,6 +66,12 @@ use constant FORMAT_3_SIZE => [19,28,28]; use constant FORMAT_DOUBLE => '%19s %-55s'; use constant FORMAT_2_SIZE => [19,55]; +# Pseudo-constant. +sub SAFE_URL_REGEXP { + my $safe_protocols = join('|', SAFE_PROTOCOLS); + return qr/($safe_protocols):[^\s<>\"]+[\w\/]/i; +} + # Convert the constants in the Bugzilla::Constants module into a hash we can # pass to the template object for reflection into its "constants" namespace # (which is like its "variables" namespace, but for constants). To do so, we @@ -205,12 +211,8 @@ sub quoteUrls { ~egox; # non-mailto protocols - my $safe_protocols = join('|', SAFE_PROTOCOLS); - my $protocol_re = qr/($safe_protocols)/i; - - $text =~ s~\b(${protocol_re}: # The protocol: - [^\s<>\"]+ # Any non-whitespace - [\w\/]) # so that we end in \w or / + my $safe_protocols = SAFE_URL_REGEXP(); + $text =~ s~\b($safe_protocols) ~($tmp = html_quote($1)) && ($things[$count++] = "$tmp") && ("\0\0" . ($count-1) . "\0\0") @@ -890,6 +892,19 @@ sub create { return $docs_urlbase; }, + # Check whether the URL is safe. + 'is_safe_url' => sub { + my $url = shift; + return 0 unless $url; + + my $safe_url_regexp = SAFE_URL_REGEXP(); + return 1 if $url =~ /^$safe_url_regexp$/; + # Pointing to a local file with no colon in its name is fine. + return 1 if $url =~ /^[^\s<>\":]+[\w\/]$/i; + # If we come here, then we cannot guarantee it's safe. + return 0; + }, + # Allow templates to generate a token themselves. 'issue_hash_token' => \&Bugzilla::Token::issue_hash_token, diff --git a/template/en/default/attachment/edit.html.tmpl b/template/en/default/attachment/edit.html.tmpl index 56d2b8a80..eeebcffae 100644 --- a/template/en/default/attachment/edit.html.tmpl +++ b/template/en/default/attachment/edit.html.tmpl @@ -185,10 +185,7 @@ defaultcontent = (attachment.contenttype.match('^text\/')) ? attachment.data.replace('(.*\n|.+)', '>$1') : undef %] - [%# The regexp is stolen from quoteUrls(), see Template.pm %] - [% safe_protocols = constants.SAFE_PROTOCOLS.join('|') %] - [% IF attachment.contenttype == 'text/plain' - && attachment.data.match("^($safe_protocols):" _ '[^\s<>\"]+[\w\/]$') %] + [% IF attachment.contenttype == 'text/plain' AND is_safe_url(attachment.data) %]

[% IF attachment.datasize < 120 %] diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl index 1ae71b299..0aa5f80af 100644 --- a/template/en/default/bug/edit.html.tmpl +++ b/template/en/default/bug/edit.html.tmpl @@ -555,12 +555,10 @@ [%# Block for URL Keyword and Whiteboard #%] [%############################################################################%] [% BLOCK section_url_keyword_whiteboard %] -[%# *** URL Whiteboard Keywords *** %] URL [% ELSE %] URL @@ -570,8 +568,7 @@ [% IF bug.check_can_change_field("bug_file_loc", 0, 1) %] - [% IF bug.bug_file_loc - AND NOT bug.bug_file_loc.match("^(javascript|data)") %] + [% IF is_safe_url(bug.bug_file_loc) %] [% bug.bug_file_loc FILTER truncate(40) FILTER html %] @@ -582,7 +579,8 @@ [% END %] [% url_output = PROCESS input no_td=1 inputname => "bug_file_loc" size => "40" colspan => 2 %] - [% IF NOT bug.check_can_change_field("bug_file_loc", 0, 1) %] + [% IF NOT bug.check_can_change_field("bug_file_loc", 0, 1) + AND is_safe_url(bug.bug_file_loc) %] [% url_output FILTER none %] [% ELSE %] [% url_output FILTER none %] diff --git a/template/en/default/bug/show-multiple.html.tmpl b/template/en/default/bug/show-multiple.html.tmpl index 56f732667..33dde14a3 100644 --- a/template/en/default/bug/show-multiple.html.tmpl +++ b/template/en/default/bug/show-multiple.html.tmpl @@ -163,11 +163,11 @@ [% field_descs.bug_file_loc FILTER html %]: - [% IF bug.bug_file_loc.match("^(javascript|data)") %] - [% bug.bug_file_loc FILTER html %] - [% ELSE %] + [% IF is_safe_url(bug.bug_file_loc) %] [% bug.bug_file_loc FILTER html %] + [% ELSE %] + [% bug.bug_file_loc FILTER html %] [% END %] -- cgit v1.2.3-24-g4f1b