1286290 - (bmo_csp_modal) CSP compliant bug modal

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: User Interface, defect, P1)

Description

[Dylan Hardison [:dylan] (he/him)]

We should remove all inline JS from the show_bug.cgi modal and set a CSP header that disallows all inline JS on that page. 

Bug Modal is under active development so it is the most likely place for a mistake to be made w.r.t. js injection.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286290_1.patch

This adjusts the CSP directives for index.cgi and show_bug.cgi when format is modal.

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286290_2.patch

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 8796165 [details] [diff] [review]
1286290_2.patch

Review of attachment 8796165 [details] [diff] [review]:
-----------------------------------------------------------------

Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://docker 'nonce-hxDZGvZyKEb8lWTN44dtLkK1KLHztzy0TfpnFZfAglStO4ZL' https://login.persona.org  'unsafe-eval'”). Source: onsubmit attribute on DIV element
--

Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://docker 'nonce-IIyi04PIdMvpr86UPcU1b4cLJZvXlT4KWJ7Ju9MF5srMkGLH' https://login.persona.org  'unsafe-eval'”). Source: onchange attribute on DIV element.
--

Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://docker 'nonce-hxDZGvZyKEb8lWTN44dtLkK1KLHztzy0TfpnFZfAglStO4ZL' https://login.persona.org  'unsafe-eval'”). Source: 
      <!--
      function update_text()....

Need to add ~  18   | <script [% script_nonce FILTER html %] type="text/javascript"> to template/en/default/global/per-bug-queries.html.tmpl.
--

Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://docker 'nonce-hxDZGvZyKEb8lWTN44dtLkK1KLHztzy0TfpnFZfAglStO4ZL' https://login.persona.org  'unsafe-eval'”). Source: onchange attribute on SELECT element.

template/en/default/global/per-bug-queries.html.tmpl: line 61:

    <select id="lob_action" name="action" onchange="update_text();">
--
Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://docker 'nonce-hxDZGvZyKEb8lWTN44dtLkK1KLHztzy0TfpnFZfAglStO4ZL' https://login.persona.org  'unsafe-eval'”). Source: onkeyup attribute on INPUT element.

template/en/default/global/per-bug-queries.html.tmpl: line 63:

    <input class="txt" type="text" id="lob_newqueryname"
           size="20" maxlength="64" name="newqueryname"
           onkeyup="manage_old_lists();">
--
template/en/default/global/select-menu.html.tmpl needs to also block onchange from being used once we work on other parts of the site.

::: extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
@@ +138,3 @@
>    $(function() {
>      needinfo_init();
> +    $(".needinfo_form_changed").on("change", function (event) {

s/needinfo_form_changed/needinfo_from_changed/

@@ +224,4 @@
>                value       => ""
>                size        => 30
>                multiple    => 5
> +              classes     => ["needinfo_form_changed"]

s/needinfo_form_changed/needinfo_from_changed/

::: index.cgi
@@ +34,5 @@
>  }
>  
> +$cgi->content_security_policy(
> +    script_src  => ['self', 'nonce', 'https://login.persona.org', 'unsafe-inline', 'unsafe-eval' ],
> +);

disable => 0 needs to be added.

::: js/global.js
@@ +62,4 @@
>      return false;
>  }
>  
> +

remove extra newline

::: show_bug.cgi
@@ +38,5 @@
>  
> +if ($format_params->{format} eq 'modal') {
> +    $cgi->content_security_policy(
> +        script_src  => ['self', 'nonce', 'https://login.persona.org', 'unsafe-inline', 'unsafe-eval' ],
> +    );

disabled => 0 needs to be added.

::: template/en/default/global/userselect.html.tmpl
@@ +24,4 @@
>    # mandatory: optional; if true, the field cannot be empty.
>    #%]
>  
> +[% THROW "onchange is not allowed" IF onchange %]

Remove interface documentation line about 'onchange'. Also remove he other place in the template 

 [% IF onchange %] onchange="[% onchange FILTER html %]" [% END %]

Note: We use userselect.html.tmpl and many other places besides bug modal so we may need skip this for now.

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286290_3.patch

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 8800001 [details] [diff] [review]
1286290_3.patch

Review of attachment 8800001 [details] [diff] [review]:
-----------------------------------------------------------------

remove nytprof.out from final commit.

Also need to add csp headers to post_bug.cgi and process_bug.cgi if user preferences are set to display updated bug or next bug in their list.

Otherwise looking pretty done.

dkl

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8800001 [details] [diff] [review]
1286290_3.patch

Review of attachment 8800001 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/CGI.pm
@@ +146,4 @@
>          return $self->{Bugzilla_csp} if $self->{Bugzilla_csp};
>          my %params = DEFAULT_CSP;
>          if (%add_params) {
> +            $add_params{disable} = 0;

Also, why would you just not add disable => 0 to DEFAULT_CSP? Seem to accomplish same. And the current way overrides even if a script wants to disable making it not possible.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

(In reply to David Lawrence [:dkl] from comment #6)
> Comment on attachment 8800001 [details] [diff] [review]
> 1286290_3.patch
> 
> Review of attachment 8800001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Bugzilla/CGI.pm
> @@ +146,4 @@
> >          return $self->{Bugzilla_csp} if $self->{Bugzilla_csp};
> >          my %params = DEFAULT_CSP;
> >          if (%add_params) {
> > +            $add_params{disable} = 0;
> 
> Also, why would you just not add disable => 0 to DEFAULT_CSP? Seem to
> accomplish same. And the current way overrides even if a script wants to
> disable making it not possible.

I can explain this. Basically if you override anything with $cgi->content_security_policy(), we turn off disable 0, which is on by default to prevent breaking pages that arn't ready yet.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

(In reply to David Lawrence [:dkl] from comment #5)
> Also need to add csp headers to post_bug.cgi and process_bug.cgi if user
> preferences are set to display updated bug or next bug in their list.

I totally forgot about that! thanks.

Comment 9

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286290_4.patch

it took some digging for post_bug.cgi. The decision to switch to modal
is actually deep in the hook. I think I would move that up to post_bug.cgi in the future... this seems to work though.

post_bug.cgi is really different than process_bug or show_bug.

Note this SHOW_BUG_MODAL_CSP is a function, not a constant because we can't call correct_urlbase() at compile time.

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8801355 [details] [diff] [review]
1286290_4.patch

Review of attachment 8801355 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I missed couple things on the previous review

These need to be fixed. Not limited to bug modal though:
template/en/default/bug/process/bugmail.html.tmpl:    (<a href="#" onclick="return toggleBugmailRecipients([% mailing_bugid FILTER none %], false)">hide</a>)
template/en/default/bug/process/bugmail.html.tmpl:    (<a href="#" onclick="return toggleBugmailRecipients([% mailing_bugid FILTER none %], true)">show</a>)

And move <script>toggleBugmailRecipients...</script> out of the template. convert to jquery while you're in there :)

Also I am getting these errors which seem harmless but are being logged each time regardless:

* Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://fedora 'nonce-xyN2np50prfYmEYACObefmjmvGa4ZgYHlUyw0swCjt0S88GG' https://login.persona.org  'unsafe-eval'”). Source: onsubmit attribute on DIV element.show_bug.cgi
* Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://fedora 'nonce-xyN2np50prfYmEYACObefmjmvGa4ZgYHlUyw0swCjt0S88GG' https://login.persona.org  'unsafe-eval'”). Source: onchange attribute on DIV element.show_bug.cgi
* Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://fedora 'nonce-xyN2np50prfYmEYACObefmjmvGa4ZgYHlUyw0swCjt0S88GG' https://login.persona.org  'unsafe-eval'”). Source: onsubmit attribute on DIV element.show_bug.cgi
* Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://fedora 'nonce-xyN2np50prfYmEYACObefmjmvGa4ZgYHlUyw0swCjt0S88GG' https://login.persona.org  'unsafe-eval'”). Source: onchange attribute on DIV element.

Will see if I can find where they are coming from.

dkl

::: index.cgi
@@ +33,5 @@
>      $cgi->delete('logout');
>  }
>  
> +$cgi->content_security_policy(
> +    script_src  => ['self', 'nonce', 'https://login.persona.org', 'unsafe-inline', 'unsafe-eval' ],

if we are using 'nonce', can we just eliminate 'unsafe-inline' and 'unsafe-eval'?

::: js/global.js
@@ +62,4 @@
>      return false;
>  }
>  
> +

nit: remove extra new-line.

::: post_bug.cgi
@@ +264,5 @@
>  # don't leak the enter_bug format param to show_bug
>  $cgi->delete('format');
>  
> +if ($user->setting('ui_experiments') eq 'on') {
> +    Bugzilla->cgi->content_security_policy(Bugzilla::CGI::SHOW_BUG_MODAL_CSP());

$cgi->content_security_policy($cgi->SHOW_BUG_MODAL_CSP());

::: process_bug.cgi
@@ +420,5 @@
>              ctype  => scalar $cgi->param('ctype'),
>          };
>          Bugzilla::Hook::process('show_bug_format', $format_params);
> +        if ($format_params->{format} eq 'modal') {
> +            $cgi->content_security_policy(Bugzilla::CGI::SHOW_BUG_MODAL_CSP());

$cgi->content_security_policy($cgi->SHOW_BUG_MODAL_CSP());

@@ +421,5 @@
>          };
>          Bugzilla::Hook::process('show_bug_format', $format_params);
> +        if ($format_params->{format} eq 'modal') {
> +            $cgi->content_security_policy(Bugzilla::CGI::SHOW_BUG_MODAL_CSP());
> +        }

Also this section doesn't do anything since $cgi->header was already printed on line 114.

::: template/en/default/global/per-bug-queries.html.tmpl
@@ +48,5 @@
>            old_lists.disabled = false;
>          }
>        }
> +      $(function() {
> +        $("#lob_action").on("change", update_text);  

nit: whitespace

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286290_5.patch

I can't trigger those other CSP errors, but the bugmail recipients one is done.

Comment 12

[David Lawrence [:dkl]]

kldsfasdfsajdhfk

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 8804025 [details] [diff] [review]
1286290_5.patch

Review of attachment 8804025 [details] [diff] [review]:
-----------------------------------------------------------------

process_bug.cgi is still not adding CSP headers for this patch properly. If I make the following change it does do it:

diff --git a/process_bug.cgi b/process_bug.cgi
index 80018cf..06f1dde 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -111,6 +111,10 @@ my $user_match_fields = {
 Bugzilla::Hook::process('bug_user_match_fields', { fields => $user_match_fields });
 Bugzilla::User::match_field($user_match_fields);
 
+if ($cgi->param('format') eq 'modal') {
+    $cgi->content_security_policy(Bugzilla::CGI::SHOW_BUG_MODAL_CSP());
+}
+
 print $cgi->header() unless Bugzilla->usage_mode == USAGE_MODE_EMAIL;
 
 # Check for a mid-air collision. Currently this only works when updating

Once I do this I see the following errors which will need to be addressed:

process_bug.cgi:2608 Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://docker 'nonce-8ElNWMz4WHtGXDV91jbGOkOGEgSo601YO98iJdvfdpDyRy49' https://login.persona.org  'unsafe-eval'”). Source: 
  init_module_visibility();

process_bug.cgi:2650 Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://docker 'nonce-8ElNWMz4WHtGXDV91jbGOkOGEgSo601YO98iJdvfdpDyRy49' https://login.persona.org  'unsafe-eval'”). Source: 

  function needinfo_init() {
    $('#n....

process_bug.cgi:3245 Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://docker 'nonce-8ElNWMz4WHtGXDV91jbGOkOGEgSo601YO98iJdvfdpDyRy49' https://login.persona.org  'unsafe-eval'”). Source: 
      <!--
      function update_text()....

::: template/en/default/bug/process/bugmail.html.tmpl
@@ +36,5 @@
>  [% recipient_count = sent_bugmail.sent.size %]
>  
> +<script [% script_nonce %]>
> +  function toggleBugmailRecipients(bug_id, show) {
> +    console.log(bug_id, show);

remove before committing

@@ +81,1 @@
>       class="[% show_recipients ? "bz_default_hidden" : "" %]">

for both bugmail_summary_XXX and similar for bugmail_summary_XXX_short, you need to change:

class="[% show_recipients ? "bz_default_hidden" : "" %]">

to 

style="[% show_recipients ? "display:none;" : "" %]">

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Bug 687787 introduced this, by adding support for onfocusin. so < 52 is still fine.

I'll update to the latest patch level 1.x jquery, 1.12.1 but remove the offending platform check.

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286290_6.patch

There should be one warning about onfocusin, but it is harmless. :-)

Comment 16

[David Lawrence [:dkl]]

Comment on attachment 8820434 [details] [diff] [review]
1286290_6.patch

Review of attachment 8820434 [details] [diff] [review]:
-----------------------------------------------------------------

* onlick in extensions/BMO/template/en/default/hook/bug_modal/edit-top_actions.html.tmpl needs to be fixed

* Does not load CSP header for process_bug.cgi due to $cgi->header already being output in line 427.

* Also does not load CSP header or attachment.cgi when creating a new attachment or updating an existing template. The above two points were using modal form.

::: process_bug.cgi
@@ +452,2 @@
>                  $vars->{'nextbug'} = $bug->id;
>              }

Missing closing curly brace above

::: show_bug.cgi
@@ +19,5 @@
>  use Bugzilla::Keyword;
>  use Bugzilla::Bug;
>  use Bugzilla::Hook;
> +use Bugzilla::CGI;
> +use Bugzilla::Util qw(correct_urlbase);

correct_urlbase not used.

::: template/en/default/bug/process/bugmail.html.tmpl
@@ +35,4 @@
>  %]
>  [% recipient_count = sent_bugmail.sent.size %]
>  
> +<script [% script_nonce %]>

FILTER none

::: template/en/default/global/per-bug-queries.html.tmpl
@@ +48,5 @@
>            old_lists.disabled = false;
>          }
>        }
> +      $(function() {
> +        $("#lob_action").on("change", update_text);  

remove trailing whitespace

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286290_8.patch

Comment 18

[Daniel Veditz [:dveditz]]

I see you're actively working on this, but can we make this formally block bug 1150541 (making the modal UI the site default)?

Comment 19

[Dylan Hardison [:dylan] (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #18)
> I see you're actively working on this, but can we make this formally block
> bug 1150541 (making the modal UI the site default)?

Absolutely not. Every day this doesn't land is another chance for code to be added that will become an error once it is turned on.

We could, however, prevent users from using the old UI if they have security creds...

Comment 20

[Daniel Veditz [:dveditz]]

The _old_ UI has been pretty thoroughly beat on by web bug bounty seekers. If you're blocking anyone from anything I'm more worried about the new modal UI (as were you in comment 0) as long as the CSP still allows 'unsafe-inline'. Are we saying the same thing? I would like this to land before you switch everyone to the new UI by default. You would like this to land before people make more changes to the bugzilla code--and making the modal UI the default is apparently still depending on a few bugs (i.e. more changes to bugzilla code).

Maybe this bug has morphed and I'm confused? I know it's changed at least somewhat because comment 0 says "remove inline javascript" but the patches add nonces (which is fine--'unsafe-inline' gets disabled when nonces are required).

Comment 21

[Dylan Hardison [:dylan] (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #20)
> The _old_ UI has been pretty thoroughly beat on by web bug bounty seekers.
> If you're blocking anyone from anything I'm more worried about the new modal
> UI (as were you in comment 0) as long as the CSP still allows
> 'unsafe-inline'. 

That was before I read https://w3c.github.io/webappsec-csp/2/


> Are we saying the same thing? I would like this to land
> before you switch everyone to the new UI by default. You would like this to
> land before people make more changes to the bugzilla code--and making the
> modal UI the default is apparently still depending on a few bugs (i.e. more
> changes to bugzilla code).

Probably. I thought you were implying that this should block on modal becoming the default,
rather than modal depending on this bug.

> Maybe this bug has morphed and I'm confused? I know it's changed at least
> somewhat because comment 0 says "remove inline javascript" but the patches
> add nonces (which is fine--'unsafe-inline' gets disabled when nonces are
> required).

the goal of this bug is to have a strict policy for the modal form.
it's one of the more invasive frontend changes we've ever done. :-)

Comment 22

[David Lawrence [:dkl]]

Comment on attachment 8823287 [details] [diff] [review]
1286290_8.patch

Review of attachment 8823287 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good except for doesn't load the headers when inserting/updating an attachment. The following works for me:

diff --git a/attachment.cgi b/attachment.cgi                                                                                                                                                                                           [5/1793]
index d5a69f1..5c5b87e 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -628,6 +628,14 @@ sub insert {
   my $recipients =  { 'changer' => $user, 'owner' => $owner };
   $vars->{'sent_bugmail'} = Bugzilla::BugMail::Send($bugid, $recipients);

+  # BMO: add show_bug_format hook for experimental UI work
+  my $show_bug_format = {};
+  Bugzilla::Hook::process('show_bug_format', $show_bug_format);
+
+  if ($show_bug_format->{format} eq 'modal') {
+      $cgi->content_security_policy(Bugzilla::CGI::SHOW_BUG_MODAL_CSP());
+  }
+
   print $cgi->header();
   # Generate and return the UI (HTML page) from the appropriate template.
   $template->process("attachment/created.html.tmpl", $vars)
@@ -784,6 +792,14 @@ sub update {
     $vars->{'sent_bugmail'} =
         Bugzilla::BugMail::Send($bug->id, { 'changer' => $user });

+    # BMO: add show_bug_format hook for experimental UI work
+    my $show_bug_format = {};
+    Bugzilla::Hook::process('show_bug_format', $show_bug_format);
+
+    if ($show_bug_format->{format} eq 'modal') {
+        $cgi->content_security_policy(Bugzilla::CGI::SHOW_BUG_MODAL_CSP());
+    }
+
     print $cgi->header();

     # Generate and return the UI (HTML page) from the appropriate template.
@@ -796,8 +812,6 @@ sub delete_attachment {
     my $user = Bugzilla->login(LOGIN_REQUIRED);
     my $dbh = Bugzilla->dbh;

-    print $cgi->header();
-
     $user->in_group('admin')
       || ThrowUserError('auth_failure', {group  => 'admin',
                                          action => 'delete',
@@ -853,6 +867,11 @@ sub delete_attachment {
         $vars->{'sent_bugmail'} =
             Bugzilla::BugMail::Send($bug->id, { 'changer' => $user });

+        if ($show_bug_format->{format} eq 'modal') {
+            $cgi->content_security_policy(Bugzilla::CGI::SHOW_BUG_MODAL_CSP());
+        }
+
+        print $cgi->header();
         $template->process("attachment/updated.html.tmpl", $vars)
           || ThrowTemplateError($template->error());
     }
@@ -863,6 +882,7 @@ sub delete_attachment {
         $vars->{'a'} = $attachment;
         $vars->{'token'} = $token;

+        print $cgi->header();
         $template->process("attachment/confirm-delete.html.tmpl", $vars)

::: index.cgi
@@ +33,5 @@
>      $cgi->delete('logout');
>  }
>  
> +$cgi->content_security_policy(
> +    script_src  => ['self', 'nonce', 'http://bugzilla.vm/1286290' ],

remove 'http://bugzilla.vm/1286290'

::: template/en/default/global/header.html.tmpl
@@ +189,1 @@
>            YAHOO.util.Event.addListener = function (el, sType, fn, obj, overrideContext) {

Was this debugging and needs to be removed?

Comment 23

[Dylan Hardison [:dylan] (he/him)]

Yep, that's debugging.

I guess that's a good fix, but we don't require CSP headers to go out for attachments per-se, based on the summary of this bug. I'll apply that fix and re-request review.

Comment 24

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286290_9.patch

Had some trouble with the disabling logic, and had to do some additional testing of attachment.cgi. This seems to pass both now.

Comment 25

[David Lawrence [:dkl]]

Comment on attachment 8829740 [details] [diff] [review]
1286290_9.patch

Review of attachment 8829740 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 26

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   ec96366..6a727b7  master -> master

Comment 27

[Ed Morley [:emorley]]

I just got a "Undefined subroutine Bugzilla::CGI::SHOW_BUG_MODAL_CSP " when making the change in bug 1320996 comment 5. (The bug was updated, even though I was initially shown just the server error)

Comment 28

[Dylan Hardison [:dylan] (he/him)]

I don't see a code path where that function wouldn't be defined
mod_perl doesn't preload the CGIs, but it does preload the modules, so that must've been an apache process that was running with the old modules loaded. 

Should be fine now.