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

[Dustin Graves]

The branch, 4.2/csrf-whitelist has been created
        at  583a6f47013ee172daee71af82523b60ca8eacfe (commit)

- Log -----------------------------------------------------------------
commit 583a6f47013ee172daee71af82523b60ca8eacfe
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 params SavedSearchLoad, NewQuery
      /Ticket/Update.html params QuoteTransaction, Action, DefaultStatus
      /Articles/Article/ExtractIntoClass.html param Ticket
    
    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..9548b0e 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1354,7 +1354,7 @@ sub StaticRoots {
     return grep { $_ and -d $_ } @static;
 }
 
-our %is_whitelisted_component = (
+our %IS_WHITELISTED_COMPONENT = (
     # The RSS feed embeds an auth token in the path, but query
     # information for the search.  Because it's a straight-up read, in
     # addition to embedding its own auth, it's fine.
@@ -1376,9 +1376,40 @@ 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 = (
+    # SavedSearchLoad - This happens when you middle-(or ⌘ )-click "Edit" for a saved search on
+    # the homepage. It's not going to do any damage
+    # NewQuery - This is simply to clear the search query
+    '/Search/Build.html' => ['SavedSearchLoad','NewQuery'],
+    # Happens if you try and reply to a message in the ticket history or click a number
+    # of options on a tickets Action menu
+    '/Ticket/Update.html' => ['QuoteTransaction', 'Action', 'DefaultStatus'],
+    # Action->Extract Article on a ticket's menu
+    '/Articles/Article/ExtractIntoClass.html' => ['Ticket'],
+);
+
 # Components which are blacklisted from automatic, argument-based whitelisting.
 # These pages are not idempotent when called with just an id.
-our %is_blacklisted_component = (
+our %IS_BLACKLISTED_COMPONENT = (
     # Takes only id and toggles bookmark state
     '/Helpers/Toggle/TicketBookmark' => 1,
 );
@@ -1387,7 +1418,7 @@ sub IsCompCSRFWhitelisted {
     my $comp = shift;
     my $ARGS = shift;
 
-    return 1 if $is_whitelisted_component{$comp};
+    return 1 if $IS_WHITELISTED_COMPONENT{$comp};
 
     my %args = %{ $ARGS };
 
@@ -1407,7 +1438,7 @@ sub IsCompCSRFWhitelisted {
 
     # Some pages aren't idempotent even with safe args like id; blacklist
     # them from the automatic whitelisting below.
-    return 0 if $is_blacklisted_component{$comp};
+    return 0 if $IS_BLACKLISTED_COMPONENT{$comp};
 
     if ( my %csrf_config = RT->Config->Get('ReferrerComponents') ) {
         my $value = $csrf_config{$comp};
@@ -1420,28 +1451,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..3fea287 100644
--- a/t/web/csrf.t
+++ b/t/web/csrf.t
@@ -34,6 +34,55 @@ $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');
+
+# CSRF pass for /Search/Build.html param NewQuery
+$m->add_header(Referer => undef);
+$m->get_ok("$searchBuildPath?NewQuery=1");
+$m->content_lacks('Possible cross-site request forgery');
+$m->title_is('Query Builder');
+
+# CSRF pass for /Ticket/Update.html items in ticket action menu
+$m->add_header(Referer => undef);
+$m->get_ok('/Ticket/Update.html?id=1&Action=foo');
+$m->content_lacks('Possible cross-site request forgery');
+
+# CSRF pass for /Ticket/Update.html reply to message in ticket history
+$m->add_header(Referer => undef);
+$m->get_ok('/Ticket/Update.html?id=1&QuoteTransaction=1&Action=Reply');
+$m->content_lacks('Possible cross-site request forgery');
+
+# CSRF pass for /Articles/Article/ExtractIntoClass.html
+# Action->Extract Article on ticket menu
+$m->add_header(Referer => undef);
+$m->get_ok('/Articles/Article/ExtractIntoClass.html?Ticket=1');
+$m->content_lacks('Possible cross-site request forgery');
+
 # now send a referer from an attacker
 $m->add_header(Referer => 'http://example.net');
 $m->get_ok($test_page);

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