[Rt-commit] rt branch, 4.2/csrf-whitelist, created. rt-4.2.11-40-g5bdaa0f

[Dustin Graves]

The branch, 4.2/csrf-whitelist has been created
        at  5bdaa0f92f0ff2b4d68fb957b66faa558222aa59 (commit)

- Log -----------------------------------------------------------------
commit 5bdaa0f92f0ff2b4d68fb957b66faa558222aa59
Author: Dustin Graves <dustin at bestpractical.com>
Date:   Mon Aug 3 16:40:24 2015 -0400

    add CSRF whitelist for component parameters
    
    in particular, /Search/Build.html param SavedSearchLoad is whitelisted,
    but not other parameters
    
    move parameter whitelist logic into own sub
    add CSRF tests to test this new behavior
    
    Fixes: I#31090

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 4d267f4..a878dbe 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1376,6 +1376,31 @@ our %is_whitelisted_component = (
     '/Ticket/ShowEmailRecord.html' => 1,
 );
 
+# Whitelist arguments that do not indicate an effectful request.
+our @global_whitelisted_args = (
+    # For example, "id" is acceptable because that is how RT retrieves a
+    # record.
+    'id',
+
+    # If they have a results= from MaybeRedirectForResults, that's also fine.
+    'results',
+
+    # The homepage refresh, which uses the Refresh header, doesn't send
+    # a referer in most browsers; whitelist the one parameter it reloads
+    # with, HomeRefreshInterval, which is safe
+    'HomeRefreshInterval',
+
+    # The NotMobile flag is fine for any page; it's only used to toggle a flag
+    # in the session related to which interface you get.
+    'NotMobile',
+);
+
+our %whitelisted_component_args = (
+    # This happens when you middle-(or ⌘ )-click "Edit" for a saved search on
+    # the homepage. It's not going to do any damage
+    '/Search/Build.html' => ['SavedSearchLoad'],
+);
+
 # Components which are blacklisted from automatic, argument-based whitelisting.
 # These pages are not idempotent when called with just an id.
 our %is_blacklisted_component = (
@@ -1420,28 +1445,23 @@ sub IsCompCSRFWhitelisted {
         }
     }
 
-    # Eliminate arguments that do not indicate an effectful request.
-    # For example, "id" is acceptable because that is how RT retrieves a
-    # record.
-    delete $args{id};
+    return AreCompCSRFParametersWhitelisted($comp, \%args);
+}
 
-    # If they have a results= from MaybeRedirectForResults, that's also fine.
-    delete $args{results};
+sub AreCompCSRFParametersWhitelisted {
+    my $sub = shift;
+    my $ARGS = shift;
 
-    # The homepage refresh, which uses the Refresh header, doesn't send
-    # a referer in most browsers; whitelist the one parameter it reloads
-    # with, HomeRefreshInterval, which is safe
-    delete $args{HomeRefreshInterval};
+    my %leftover_args = %{ $ARGS };
 
-    # The NotMobile flag is fine for any page; it's only used to toggle a flag
-    # in the session related to which interface you get.
-    delete $args{NotMobile};
+    # Join global whitelist and component-specific whitelist
+    my @whitelisted_args = (@global_whitelisted_args, @{ $whitelisted_component_args{$sub} || [] });
 
-    # If there are no arguments, then it's likely to be an idempotent
-    # request, which are not susceptible to CSRF
-    return 1 if !%args;
+    for my $arg (@whitelisted_args) {
+        delete $leftover_args{$arg};
+    }
 
-    return 0;
+    return !%leftover_args;
 }
 
 sub IsRefererCSRFWhitelisted {
diff --git a/t/web/csrf.t b/t/web/csrf.t
index 9d95d06..9b30bd2 100644
--- a/t/web/csrf.t
+++ b/t/web/csrf.t
@@ -34,6 +34,34 @@ $m->get_ok("$test_page&user=root&pass=password");
 $m->content_lacks("Possible cross-site request forgery");
 $m->title_is('Create a new ticket');
 
+# CSRF parameter whitelist tests
+my $searchBuildPath = '/Search/Build.html';
+
+# CSRF whitelist for /Search/Build.html param SavedSearchLoad
+$m->add_header(Referer => undef);
+$m->get_ok("$searchBuildPath?SavedSearchLoad=foo");
+$m->content_lacks('Possible cross-site request forgery');
+$m->title_is('Query Builder');
+
+# CSRF pass for /Search/Build.html no param
+$m->add_header(Referer => undef);
+$m->get_ok("$searchBuildPath");
+$m->content_lacks('Possible cross-site request forgery');
+$m->title_is('Query Builder');
+
+# CSRF fail for /Search/Build.html arbitrary param only
+$m->add_header(Referer => undef);
+$m->get_ok("$searchBuildPath?foo=bar");
+$m->content_contains('Possible cross-site request forgery');
+$m->title_is('Possible cross-site request forgery');
+
+# CSRF fail for /Search/Build.html arbitrary param with SavedSearchLoad
+$m->add_header(Referer => undef);
+$m->get_ok("$searchBuildPath?SavedSearchLoad=foo&foo=bar");
+$m->content_contains('Possible cross-site request forgery');
+$m->title_is('Possible cross-site request forgery');
+
+
 # now send a referer from an attacker
 $m->add_header(Referer => 'http://example.net');
 $m->get_ok($test_page);

-----------------------------------------------------------------------