From 35ef43cf382dbbdaeeeb9a33484873f893e00a51 Mon Sep 17 00:00:00 2001 From: Mikal Kolbein Gule Date: Mon, 18 Jun 2012 14:04:25 +0200 Subject: [PATCH] Master -> 4.0.5-5 --- etc/RT_Config.pm | 19 ++++++++- etc/RT_Config.pm.orig | 25 +++++++++++ lib/RT/Action/SendEmail.pm | 2 +- lib/RT/Interface/Web.pm | 75 +++++++++++++++++++++++---------- lib/RT/Interface/Web/Handler.pm | 2 +- share/html/Elements/Header | 2 +- share/html/Elements/Tabs | 46 ++++++++++---------- share/html/NoAuth/Logout.html | 2 +- share/html/Search/Results.html | 12 +++++- 9 files changed, 134 insertions(+), 51 deletions(-) diff --git a/etc/RT_Config.pm b/etc/RT_Config.pm index c34575a..cec5a7b 100644 --- a/etc/RT_Config.pm +++ b/etc/RT_Config.pm @@ -1775,13 +1775,30 @@ Set($RestrictReferrer, 1); If set to a false value, RT will allow the user to log in from any link or request, merely by passing in C and C parameters; setting it to a true value forces all logins to come from the login box, so the -user us aware that they are being logged in. The default is off, for +user is aware that they are being logged in. The default is off, for backwards compatability. =cut Set($RestrictLoginReferrer, 0); +=item C<$ReferrerWhitelist> + +This is a list of hostname:port combinations that RT will treat as being +part of RT's domain. This is particularly useful if you access RT as +multiple hostnames or have an external auth system that needs to +redirect back to RT once authentication is complete. + + Set(@ReferrerWhitelist, qw(www.example.com:443 www3.example.com:80)); + +If the "RT has detected a possible cross-site request forgery" error is triggered +by a host:port sent by your browser that you believe should be valid, you can copy +the host:port from the error message into this list. + +=cut + +Set(@ReferrerWhitelist, qw()); + =back diff --git a/etc/RT_Config.pm.orig b/etc/RT_Config.pm.orig index 90cf72c..c34575a 100644 --- a/etc/RT_Config.pm.orig +++ b/etc/RT_Config.pm.orig @@ -1759,8 +1759,33 @@ This disables RT's clickjacking protection. Set($Framebusting, 1); +=item C<$RestrictReferrer> + +If set to a false value, the HTTP C (sic) header will not be +checked to ensure that requests come from RT's own domain. As RT allows +for GET requests to alter state, disabling this opens RT up to +cross-site request forgery (CSRF) attacks. + +=cut + +Set($RestrictReferrer, 1); + +=item C<$RestrictLoginReferrer> + +If set to a false value, RT will allow the user to log in from any link +or request, merely by passing in C and C parameters; setting +it to a true value forces all logins to come from the login box, so the +user us aware that they are being logged in. The default is off, for +backwards compatability. + +=cut + +Set($RestrictLoginReferrer, 0); + =back + + =head1 Authorization and user configuration =over 4 diff --git a/lib/RT/Action/SendEmail.pm b/lib/RT/Action/SendEmail.pm index 94686b8..4ae1a8b 100644 --- a/lib/RT/Action/SendEmail.pm +++ b/lib/RT/Action/SendEmail.pm @@ -348,7 +348,7 @@ sub AddAttachments { $MIMEObj->head->delete('RT-Attach-Message'); - my $attachments = RT::Attachments->new( $self->TransactionObj->CreatorObj ); + my $attachments = RT::Attachments->new( RT->SystemUser ); $attachments->Limit( FIELD => 'TransactionId', VALUE => $self->TransactionObj->Id diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm index f04d0cc..3cc35b0 100644 --- a/lib/RT/Interface/Web.pm +++ b/lib/RT/Interface/Web.pm @@ -253,6 +253,7 @@ sub HandleRequest { ValidateWebConfig(); DecodeARGS($ARGS); + local $HTML::Mason::Commands::DECODED_ARGS = $ARGS; PreprocessTimeUpdates($ARGS); InitializeMenu(); @@ -539,6 +540,10 @@ sub ShowRequestedPage { my $m = $HTML::Mason::Commands::m; + # Ensure that the cookie that we send is up-to-date, in case the + # session-id has been modified in any way + SendSessionCookie(); + # precache all system level rights for the current user $HTML::Mason::Commands::session{CurrentUser}->PrincipalObj->HasRights( Object => RT->System ); @@ -690,7 +695,6 @@ sub AttemptPasswordAuthentication { InstantiateNewSession(); $HTML::Mason::Commands::session{'CurrentUser'} = $user_obj; - SendSessionCookie(); $m->callback( %$ARGS, CallbackName => 'SuccessfulLogin', CallbackPage => '/autohandler' ); @@ -745,6 +749,7 @@ sub LoadSessionFromCookie { sub InstantiateNewSession { tied(%HTML::Mason::Commands::session)->delete if tied(%HTML::Mason::Commands::session); tie %HTML::Mason::Commands::session, 'RT::Interface::Web::Session', undef; + SendSessionCookie(); } sub SendSessionCookie { @@ -1161,7 +1166,7 @@ sub ComponentRoots { return @roots; } -my %is_whitelisted_path = ( +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. @@ -1172,7 +1177,7 @@ sub IsCompCSRFWhitelisted { my $comp = shift; my $ARGS = shift; - return 1 if $is_whitelisted_path{$comp}; + return 1 if $is_whitelisted_component{$comp}; my %args = %{ $ARGS }; @@ -1200,6 +1205,11 @@ sub IsCompCSRFWhitelisted { delete $args{results} if $args{results} and $HTML::Mason::Commands::session{"Actions"}->{$args{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 + delete $args{HomeRefreshInterval}; + # If there are no arguments, then it's likely to be an idempotent # request, which are not susceptible to CSRF return 1 if !%args; @@ -1209,11 +1219,16 @@ sub IsCompCSRFWhitelisted { sub IsRefererCSRFWhitelisted { my $referer = _NormalizeHost(shift); - my $config = _NormalizeHost(RT->Config->Get('WebBaseURL')); + my $base_url = _NormalizeHost(RT->Config->Get('WebBaseURL')); + $base_url = $base_url->host_port; - return (1,$referer,$config) if $referer->host_port eq $config->host_port; + my $configs; + for my $config ( $base_url, RT->Config->Get('ReferrerWhitelist') ) { + push @$configs,$config; + return 1 if $referer->host_port eq $config; + } - return (0,$referer,$config); + return (0,$referer,$configs); } =head3 _NormalizeHost @@ -1268,12 +1283,21 @@ EOT "your browser did not supply a Referrer header", # loc ) if !$ENV{HTTP_REFERER}; - my ($whitelisted, $browser, $config) = IsRefererCSRFWhitelisted($ENV{HTTP_REFERER}); + my ($whitelisted, $browser, $configs) = IsRefererCSRFWhitelisted($ENV{HTTP_REFERER}); return 0 if $whitelisted; + if ( @$configs > 1 ) { + return (1, + "the Referrer header supplied by your browser ([_1]) is not allowed by RT's configured hostname ([_2]) or whitelisted hosts ([_3])", # loc + $browser->host_port, + shift @$configs, + join(', ', @$configs) ); + } + return (1, "the Referrer header supplied by your browser ([_1]) is not allowed by RT's configured hostname ([_2])", # loc - $browser->host_port, $config->host_port); + $browser->host_port, + $configs->[0]); } sub ExpandCSRFToken { @@ -1290,6 +1314,7 @@ sub ExpandCSRFToken { return unless $user->ValidateAuthString( $data->{auth}, $token ); %{$ARGS} = %{$data->{args}}; + $HTML::Mason::Commands::DECODED_ARGS = $ARGS; # We explicitly stored file attachments with the request, but not in # the session yet, as that would itself be an attack. Put them into @@ -1304,21 +1329,9 @@ sub ExpandCSRFToken { return 1; } -sub MaybeShowInterstitialCSRFPage { +sub StoreRequestToken { my $ARGS = shift; - return unless RT->Config->Get('RestrictReferrer'); - - # Deal with the form token provided by the interstitial, which lets - # browsers which never set referer headers still use RT, if - # painfully. This blows values into ARGS - return if ExpandCSRFToken($ARGS); - - my ($is_csrf, $msg, @loc) = IsPossibleCSRF($ARGS); - return if !$is_csrf; - - $RT::Logger->notice("Possible CSRF: ".RT::CurrentUser->new->loc($msg, @loc)); - my $token = Digest::MD5::md5_hex(time . {} . $$ . rand(1024)); my $user = $HTML::Mason::Commands::session{'CurrentUser'}->UserObj; my $data = { @@ -1337,10 +1350,28 @@ sub MaybeShowInterstitialCSRFPage { $HTML::Mason::Commands::session{'CSRF'}->{$token} = $data; $HTML::Mason::Commands::session{'i'}++; + return $token; +} + +sub MaybeShowInterstitialCSRFPage { + my $ARGS = shift; + + return unless RT->Config->Get('RestrictReferrer'); + + # Deal with the form token provided by the interstitial, which lets + # browsers which never set referer headers still use RT, if + # painfully. This blows values into ARGS + return if ExpandCSRFToken($ARGS); + + my ($is_csrf, $msg, @loc) = IsPossibleCSRF($ARGS); + return if !$is_csrf; + + $RT::Logger->notice("Possible CSRF: ".RT::CurrentUser->new->loc($msg, @loc)); + my $token = StoreRequestToken($ARGS); $HTML::Mason::Commands::m->comp( '/Elements/CSRF', - OriginalURL => $HTML::Mason::Commands::r->path_info, + OriginalURL => RT->Config->Get('WebPath') . $HTML::Mason::Commands::r->path_info, Reason => HTML::Mason::Commands::loc( $msg, @loc ), Token => $token, ); diff --git a/lib/RT/Interface/Web/Handler.pm b/lib/RT/Interface/Web/Handler.pm index f96f66e..a740167 100644 --- a/lib/RT/Interface/Web/Handler.pm +++ b/lib/RT/Interface/Web/Handler.pm @@ -69,7 +69,7 @@ sub DefaultHandlerArgs { ( ], default_escape_flags => 'h', data_dir => "$RT::MasonDataDir", - allow_globals => [qw(%session)], + allow_globals => [qw(%session $DECODED_ARGS)], # Turn off static source if we're in developer mode. static_source => (RT->Config->Get('DevelMode') ? '0' : '1'), use_object_files => (RT->Config->Get('DevelMode') ? '0' : '1'), diff --git a/share/html/Elements/Header b/share/html/Elements/Header index 4b0c2b8..4a6ac26 100644 --- a/share/html/Elements/Header +++ b/share/html/Elements/Header @@ -57,7 +57,7 @@ <& /Elements/Framekiller &> % if ($Refresh && $Refresh =~ /^(\d+)/ && $1 > 0) { -% my $URL = $m->notes->{LogoutURL}; $URL = $URL ? ";URL=$URL" : ""; +% my $URL = $m->notes->{RefreshURL}; $URL = $URL ? ";URL=$URL" : ""; " /> % } diff --git a/share/html/Elements/Tabs b/share/html/Elements/Tabs index 3e66bfb..4c09bf0 100644 --- a/share/html/Elements/Tabs +++ b/share/html/Elements/Tabs @@ -245,7 +245,7 @@ my $build_admin_menu = sub { my $section; if ( $request_path =~ m|^/Admin/$type/?(?:index.html)?$| || ( $request_path =~ m|^/Admin/$type/(?:Modify.html)$| - && $m->request_args->{'Create'} ) + && $DECODED_ARGS->{'Create'} ) ) { $section = $tabs; @@ -260,11 +260,11 @@ my $build_admin_menu = sub { } if ( $request_path =~ m{^/Admin/Queues} ) { - if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/ + if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/ || - $m->request_args->{'Queue'} && $m->request_args->{'Queue'} =~ /^\d+$/ + $DECODED_ARGS->{'Queue'} && $DECODED_ARGS->{'Queue'} =~ /^\d+$/ ) { - my $id = $m->request_args->{'Queue'} || $m->request_args->{'id'}; + my $id = $DECODED_ARGS->{'Queue'} || $DECODED_ARGS->{'id'}; my $queue_obj = RT::Queue->new( $session{'CurrentUser'} ); $queue_obj->Load($id); @@ -294,8 +294,8 @@ my $build_admin_menu = sub { } } if ( $request_path =~ m{^/Admin/Users} ) { - if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/ ) { - my $id = $m->request_args->{'id'}; + if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/ ) { + my $id = $DECODED_ARGS->{'id'}; my $obj = RT::User->new( $session{'CurrentUser'} ); $obj->Load($id); @@ -312,8 +312,8 @@ my $build_admin_menu = sub { } if ( $request_path =~ m{^/Admin/Groups} ) { - if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/ ) { - my $id = $m->request_args->{'id'}; + if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/ ) { + my $id = $DECODED_ARGS->{'id'}; my $obj = RT::Group->new( $session{'CurrentUser'} ); $obj->Load($id); @@ -327,8 +327,8 @@ my $build_admin_menu = sub { } if ( $request_path =~ m{^/Admin/CustomFields/} ) { - if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/ ) { - my $id = $m->request_args->{'id'}; + if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/ ) { + my $id = $DECODED_ARGS->{'id'}; my $obj = RT::CustomField->new( $session{'CurrentUser'} ); $obj->Load($id); @@ -353,7 +353,7 @@ my $build_admin_menu = sub { if ( $request_path =~ m{^/Admin/Articles/Classes/} ) { my $tabs = PageMenu(); - if ( my $id = $m->request_args->{'id'} ) { + if ( my $id = $DECODED_ARGS->{'id'} ) { my $obj = RT::CustomField->new( $session{'CurrentUser'} ); $obj->Load($id); @@ -490,7 +490,7 @@ my $build_main_nav = sub { $about_me->child( logout => title => loc('Logout'), path => '/NoAuth/Logout.html' ); } if ( $request_path =~ m{^/Dashboards/(\d+)?}) { - if ( my $id = ( $1 || $m->request_args->{'id'} ) ) { + if ( my $id = ( $1 || $DECODED_ARGS->{'id'} ) ) { my $obj = RT::Dashboard->new( $session{'CurrentUser'} ); $obj->LoadById($id); if ( $obj and $obj->id ) { @@ -506,7 +506,7 @@ my $build_main_nav = sub { if ( $request_path =~ m{^/Ticket/} ) { - if ( ( $m->request_args->{'id'} || '' ) =~ /^(\d+)$/ ) { + if ( ( $DECODED_ARGS->{'id'} || '' ) =~ /^(\d+)$/ ) { my $id = $1; my $obj = RT::Ticket->new( $session{'CurrentUser'} ); $obj->Load($id); @@ -654,17 +654,17 @@ my $build_main_nav = sub { && $request_path !~ m{^/Search/Simple\.html} ) || ( $request_path =~ m{^/Search/Simple\.html} - && $m->request_args->{'q'} ) + && $DECODED_ARGS->{'q'} ) ) { my $search = Menu()->child('search'); my $args = ''; my $has_query = ''; my $current_search = $session{"CurrentSearchHash"} || {}; - my $search_id = $m->request_args->{'SavedSearchLoad'} || $m->request_args->{'SavedSearchId'} || $current_search->{'SearchId'} || ''; - my $chart_id = $m->request_args->{'SavedChartSearchId'} || $current_search->{SavedChartSearchId}; + my $search_id = $DECODED_ARGS->{'SavedSearchLoad'} || $DECODED_ARGS->{'SavedSearchId'} || $current_search->{'SearchId'} || ''; + my $chart_id = $DECODED_ARGS->{'SavedChartSearchId'} || $current_search->{SavedChartSearchId}; - $has_query = 1 if ( $m->request_args->{'Query'} or $current_search->{'Query'} ); + $has_query = 1 if ( $DECODED_ARGS->{'Query'} or $current_search->{'Query'} ); my %query_args; my %fallback_query_args = ( @@ -673,12 +673,12 @@ my $build_main_nav = sub { ( map { my $p = $_; - $p => $m->request_args->{$p} || $current_search->{$p} + $p => $DECODED_ARGS->{$p} || $current_search->{$p} } qw(Query Format OrderBy Order Page) ), RowsPerPage => ( - defined $m->request_args->{'RowsPerPage'} - ? $m->request_args->{'RowsPerPage'} + defined $DECODED_ARGS->{'RowsPerPage'} + ? $DECODED_ARGS->{'RowsPerPage'} : $current_search->{'RowsPerPage'} ), ); @@ -773,8 +773,8 @@ my $build_main_nav = sub { } if ( $request_path =~ m{^/Article/} ) { - if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/ ) { - my $id = $m->request_args->{'id'}; + if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/ ) { + my $id = $DECODED_ARGS->{'id'}; my $tabs = PageMenu(); $tabs->child( display => title => loc('Display'), path => "/Articles/Article/Display.html?id=".$id ); @@ -788,7 +788,7 @@ my $build_main_nav = sub { my $tabs = PageMenu(); $tabs->child( search => title => loc("Search"), path => "/Articles/Article/Search.html" ); $tabs->child( create => title => loc("New Article" ), path => "/Articles/Article/PreCreate.html" ); - if ( $request_path =~ m{^/Articles/Article/} and ( $m->request_args->{'id'} || '' ) =~ /^(\d+)$/ ) { + if ( $request_path =~ m{^/Articles/Article/} and ( $DECODED_ARGS->{'id'} || '' ) =~ /^(\d+)$/ ) { my $id = $1; my $obj = RT::Article->new( $session{'CurrentUser'} ); $obj->Load($id); diff --git a/share/html/NoAuth/Logout.html b/share/html/NoAuth/Logout.html index b8e119a..20024cc 100644 --- a/share/html/NoAuth/Logout.html +++ b/share/html/NoAuth/Logout.html @@ -81,5 +81,5 @@ if (keys %session) { } $m->callback( %ARGS, CallbackName => 'AfterSessionDelete' ); -$m->notes->{LogoutURL} = $URL; +$m->notes->{RefreshURL} = $URL; diff --git a/share/html/Search/Results.html b/share/html/Search/Results.html index 0040d2a..171b38d 100644 --- a/share/html/Search/Results.html +++ b/share/html/Search/Results.html @@ -46,7 +46,7 @@ %# %# END BPS TAGGED BLOCK }}} <& /Elements/Header, Title => $title, - Refresh => $session{'tickets_refresh_interval'} || RT->Config->Get('SearchResultsRefreshInterval', $session{'CurrentUser'} ), + Refresh => $refresh, LinkRel => \%link_rel &> <& /Elements/Tabs &> <& /Elements/CollectionList, @@ -148,6 +148,16 @@ if ($ARGS{'TicketsRefreshInterval'}) { $session{'tickets_refresh_interval'} = $ARGS{'TicketsRefreshInterval'}; } +my $refresh = $session{'tickets_refresh_interval'} + || RT->Config->Get('SearchResultsRefreshInterval', $session{'CurrentUser'} ); + +if (RT->Config->Get('RestrictReferrer') and $refresh and not $m->request_args->{CSRF_Token}) { + my $token = RT::Interface::Web::StoreRequestToken( $session{'CurrentSearchHash'} ); + $m->notes->{RefreshURL} = RT->Config->Get('WebURL') + . "Search/Results.html?CSRF_Token=" + . $token; +} + my %link_rel; my $genpage = sub { return $m->comp( -- 2.39.3