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

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++] = "<a href=\"$tmp\">$tmp</a>") &&
                ("\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) %]
               <p>
                 <a href="[% attachment.data FILTER html %]">
                   [% 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 *** %]
   <tr>
     <td class="field_label">
       <label for="bug_file_loc" accesskey="u"><b>
-        [% IF bug.bug_file_loc 
-           AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
+        [% IF is_safe_url(bug.bug_file_loc) %]
           <a href="[% bug.bug_file_loc FILTER html %]"><u>U</u>RL</a>
         [% ELSE %]
           <u>U</u>RL
@@ -570,8 +568,7 @@
     <td>
       [% IF bug.check_can_change_field("bug_file_loc", 0, 1) %]
         <span id="bz_url_edit_container" class="bz_default_hidden"> 
-        [% IF bug.bug_file_loc 
-           AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
+        [% IF is_safe_url(bug.bug_file_loc) %]
            <a href="[% bug.bug_file_loc FILTER html %]" target="_blank"
               title="[% bug.bug_file_loc FILTER html %]">
              [% bug.bug_file_loc FILTER truncate(40) FILTER html %]</a>
@@ -582,7 +579,8 @@
       [% END %]
       <span id="bz_url_input_area">
         [% 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) %]
           <a href="[% bug.bug_file_loc FILTER html %]">[% url_output FILTER none %]</a>
         [% 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 @@
       <tr>
         <th>[% field_descs.bug_file_loc FILTER html %]:</th>
         <td colspan="3">
-          [% IF bug.bug_file_loc.match("^(javascript|data)") %]
-            [% bug.bug_file_loc FILTER html %]
-          [% ELSE %]
+          [% IF is_safe_url(bug.bug_file_loc) %]
             <a href="[% bug.bug_file_loc FILTER html %]">
                      [% bug.bug_file_loc FILTER html %]</a>
+          [% ELSE %]
+            [% bug.bug_file_loc FILTER html %]
           [% END %]
         </td>
       </tr>