[Rt-commit] rt branch, 3.8-trunk, updated. rt-3.8.11-125-g8c6406d
Alex Vandiver
alexmv at bestpractical.com
Tue May 22 12:00:16 EDT 2012
The branch, 3.8-trunk has been updated
via 8c6406d12958a0c6db67676e00522ccfd9f7824b (commit)
via 488f351cb105ef21f6952b14fb8ec1a1aa630967 (commit)
via 42503976eced9fa0d71fe56e792758f876d6e491 (commit)
via 14b8b16c3036ae9a46725cefece768e929a15a4e (commit)
via a5346d0ba4122e989d771a093de79e8b3bdd3024 (commit)
via 65d63b0a9b36b81044d7b164606a415f65876c9d (commit)
via 1286ac125218937c2fe1ea50e874aaa384090774 (commit)
via cd32279d153f4d61a677562f7c96b208157d7a7d (commit)
via 809ea27ee626d2cd00c0635a73a5e4a55f01e423 (commit)
via fec1b72e821c2c9d28996eaec0ca21a7be9cf4e7 (commit)
via 997d5b0fc029ff0b209ec7e47f253958be334d8b (commit)
via 001014997bd8a5c21cbb36eb50505bea14456f1b (commit)
via ff3a3e187d64deac5930877fee8527d32f59406a (commit)
via eada947f53da77f93e91aa27dd0fd30c144a3c5e (commit)
via dd11af8915ce12942ffd7c08607a847d1967be47 (commit)
via 912e2385a38cce818244bbe4197897b190217d1b (commit)
via 07890edf334ce7a238fb65af0ab4689566bff027 (commit)
via 542b80d1fff77dd14e65fbb494eba5118cbb26a6 (commit)
via 6b4e33882d0eac0c8ea5b416b4edd692bdd69e71 (commit)
via 52159bcadff35afb38fbf0ed749f32f213cf537d (commit)
via ad3ab788779fb1f8047bfe190d1d7c439d615c01 (commit)
via 5daf828da793b49df4aa06cce03df9ee9fbcffca (commit)
via 96cde5748cd553256ba6def8a8353d5a6baf054b (commit)
via 5ad63908f7e589534d41d93bf68fd64c3817f156 (commit)
via 3f5531887c5934995688434cfdff752c27573c23 (commit)
via 1d3432b2a14434c775a2bf637e7ec2bde4448bd5 (commit)
via eb44f1060ac3e78a5063aa6982c033d7bbf783aa (commit)
via 02325246190c18f11b1f4056d7c6e7c3fa1f6a9b (commit)
via 9b6e230856538dc8f3801a21f2261fe93a4f493b (commit)
via bf574b144f70e287d5750dc70735f93a4bfb69e5 (commit)
via b6a06ebea3dac5915979055f0a0508a846829033 (commit)
via f4513aee9e19bef089b5aa0586b033e291b8c509 (commit)
via 7040ff301c762d7b30335f3808b06b4ebfe3523b (commit)
via c01f5852cc56e056198a9bb6110842d7553856a8 (commit)
via 0597e05c4e1ea5d954c4cd9ec60c909464571380 (commit)
via 013ee73c2444201435755e924f195fdcfdbb8249 (commit)
via 9bf2265ce80bdb979e5c2b0c90263792fd302d42 (commit)
via b3d3b2f30b574a961b9a9fcccae66c34da4a5eb5 (commit)
via 3a705a092526cd2106bb88fce134e06f855e52ca (commit)
via e915622b841a522d5595e53e2d38ff404e8e17e8 (commit)
via 5aa4f5ceab5ef0bf5263e8b3a7bcd9b0e86c27c7 (commit)
via f8770f8538f28113989d067fdf62b08b0b121727 (commit)
via be375067d28f2fa10f112da4a51f8b87b787f07c (commit)
via 0d977a01e9524922c58ac31e345c2696e91efc26 (commit)
via aa6923451b824192828867b388e49dd46971c13d (commit)
via baa7f1e1bf952194fd39ea95884184a756039c23 (commit)
via e5399ceb1ae24cea0e18800a004ac1ac8d3539f8 (commit)
via 290b46b20d2d2fb84fea2a707e51cc049617469a (commit)
via 56f24489b5f7a43015c528dd305f775e49911e79 (commit)
via 05a6e45a448b0f2712a2356829ef78a1e7385d60 (commit)
via cd180c1f57602555614ef0d57e128f1cad544e87 (commit)
via 87ac5328d5a13f8e99e2ca7783e28d97c15912cc (commit)
via 82c9189f529dc65c1874a15e5379f5f9d11593f7 (commit)
via 6a9a41d6dc2908c34c80333bb507457aec058e7d (commit)
via 1ef24b15d3fc42131bf29f888279de55a9fd01a0 (commit)
via 9feb75b8ee903273c6f708e6d52ab10ae3774b64 (commit)
via 02adabdd6ca2c5df6ee3e13e742b38934cc89447 (commit)
via ee8717368f42f083cfd900170201f7a3d73e2f35 (commit)
via 8fbe5b518a3898229de7f7717231d175d4d33e6f (commit)
via c83b3488e33eba887ae20a6f192f2c5dc4311d01 (commit)
via 64c6ecd431388d7c81c5f94ee4f0c526325ba9c0 (commit)
via 19369ba8f67eec572a992f4bdb22d756872ccc37 (commit)
via ad5d6ed2d2b80fe2426c36d40b70dd6cf2264a6b (commit)
via cdcc2b65b2b361b362bc0fa86e9dc6f60fd65784 (commit)
via e2233e032012c4286d4afecfd0f4d84da497f97b (commit)
via 0b5f3d82e2ee5721208685fe6d2ede4e0ebdaf29 (commit)
via 80b14f90de3eed0a64f9318850c515fb855d2261 (commit)
via a325ac0d049ef4a0e58c8744ae6c61fa193c800f (commit)
via ad312089ab65778fafe6b625f2b796a5b79da843 (commit)
via 3570e453da31c9cc29ef32aff4c10df5987eeb27 (commit)
via f923dbce924c5f3bfc1fc27560fcffe924f07b1c (commit)
via 7c9cd7c92f7672bcf6b100aa2913d6d0e0e33753 (commit)
via 4c5657837b3b5972e0a85da0607ff20bfb72892b (commit)
via 189b322aa22fa68d45a52504fa6f32ab0e1a2b57 (commit)
via 951add5ab10a12b0d40cd3a7edf812b524db6ff9 (commit)
via d17991d80002d65b2d9e98366a550d49ad5232c7 (commit)
via cfd4d893e92e4fe23615d4cd4724803c0a0804cf (commit)
via 01cecdeeca3375402ee29e92683100f6b24e139d (commit)
via 5170d9057c4060a8a9be422f947ad450d5db100e (commit)
via b784bcb5779ca6315717a5bbb9c554f0a28ecb6b (commit)
via 00593b893332714d7288ab683e270003471e35a8 (commit)
via cccfb9c04fa271ff05128749ce99713e50b2da23 (commit)
via d0a37b0ee1bc38f7eda0ae0f155c52fef5996f73 (commit)
via 90650ebd9e5316b3a8f6b6b8992a1b810f8db09b (commit)
via 1f71f5df36140e2239ae82bebfc7237eab34dc0f (commit)
via bb917e0b5a3a5797cc0929211db808e6d9303f9a (commit)
via f96ce669d98ce016f2340fd2286fd14dd6edc80b (commit)
via 8283903fdb1984c9d04722a8e0f9539e00ebf53f (commit)
via d9c47864b8068e8f524118a8a698f23eb0523c8d (commit)
via 40be851ec9b9ecdb48cc9bf250a2832de8ddf1d0 (commit)
via 9243676f7ffbfe9d6b2c614ca604a515893a8e54 (commit)
via d2d451591a7622e96cf3052e591029cdaa890419 (commit)
via 26a2816316b4883529f9d22175b8be2cd58271ff (commit)
via e8268e46e5de3529370c4bf23256ee3331595485 (commit)
via adc0df31fa1427d596294ab61f6b81e8f7d9033f (commit)
via efd243054470431d4e6297630b712e01a02bbef0 (commit)
via 52c4d4c0fe723869a94c3a3292c17238ed83c14f (commit)
via 488cdbd10f68e4d8b5c52934268de3c65d7e0a57 (commit)
via eaa7ba63829b129d2ddae95983ff81883d149bb9 (commit)
via 329e14ec5f581c7e9a490ca3a1b4b1f204cdd419 (commit)
via 02ff6818763e3c5c1ccc7d1fe3854a25cda74a50 (commit)
via 71007612cf3ab9409ab250f0e53b21be86c75780 (commit)
via 864c8193f45fb2733f3e6a148ec1aae7d95d155f (commit)
via 31d1728a9034e1cbcd394449a292a248cad0126e (commit)
via ef35fe55305d97233f74dd75720f15d385a3432f (commit)
via 147c38f3740ef4be7c37a74982aaa1505145e59e (commit)
via 3ddacb63683ed572ff2f1d369974bd4b3fb8d6c6 (commit)
via 0abb5479c11e85c5cbc9e4046a6b678bcce723ab (commit)
from 6928fac63094935a68438f0d6608cbc351c37cb4 (commit)
Summary of changes:
.gitignore | 1 +
bin/rt-mailgate.in | 4 +-
docs/Security | 12 -
docs/security.pod | 82 +++++
etc/RT_Config.pm.in | 40 +++
etc/upgrade/3.8.12/content | 17 +
etc/upgrade/vulnerable-passwords.in | 3 +
lib/RT.pm.in | 20 +-
lib/RT/ACL_Overlay.pm | 3 +
lib/RT/Action/CreateTickets.pm | 1 +
lib/RT/Action/SendEmail.pm | 7 +-
lib/RT/Attachments_Overlay.pm | 11 +-
lib/RT/CustomField_Overlay.pm | 80 ++++-
lib/RT/Date.pm | 29 +-
lib/RT/Graph/Tickets.pm | 10 +-
lib/RT/Group_Overlay.pm | 10 +
lib/RT/Groups_Overlay.pm | 8 +
lib/RT/Handle.pm | 5 +-
lib/RT/Interface/Email.pm | 28 +-
lib/RT/Interface/Web.pm | 345 +++++++++++++++++++-
lib/RT/Interface/Web/Handler.pm | 9 +-
lib/RT/ObjectCustomFieldValue_Overlay.pm | 9 +-
lib/RT/ObjectCustomField_Overlay.pm | 12 +
lib/RT/Queue_Overlay.pm | 13 +
lib/RT/Scrip_Overlay.pm | 30 ++
lib/RT/SearchBuilder.pm | 13 +
lib/RT/Shredder.pm | 2 +
lib/RT/Shredder/Plugin.pm | 1 +
lib/RT/Shredder/Queue.pm | 1 +
lib/RT/Template_Overlay.pm | 24 ++
lib/RT/Ticket_Overlay.pm | 17 +-
lib/RT/Tickets_Overlay.pm | 19 ++
lib/RT/Transaction_Overlay.pm | 18 +-
lib/RT/URI.pm | 2 +-
lib/RT/User_Overlay.pm | 71 ++--
lib/RT/Users_Overlay.pm | 8 +
sbin/rt-email-dashboards.in | 3 +
share/html/Admin/Elements/EditCustomFields | 3 +
share/html/Admin/Tools/Shredder/Dumps/dhandler | 5 +-
.../Admin/Tools/Shredder/Elements/Error/NoStorage | 2 +-
share/html/Approvals/Elements/PendingMyApproval | 4 +-
.../Elements/DashboardsForObject => Elements/CSRF} | 53 ++-
share/html/Elements/CollectionAsTable/Header | 4 +-
share/html/Elements/CollectionListPaging | 12 +-
share/html/Elements/ColumnMap | 10 +-
share/html/Elements/CreateTicket | 2 +-
share/html/Elements/EditCustomField | 2 +-
share/html/Elements/EditCustomFieldAutocomplete | 21 +-
share/html/Elements/EditCustomFieldSelect | 6 +-
share/html/Elements/Error | 2 +-
share/html/Elements/Footer | 4 +-
share/html/Elements/Header | 2 +-
share/html/Elements/HeaderJavascript | 6 +-
share/html/Elements/MessageBox | 2 +-
share/html/Elements/PersonalQuickbar | 2 +-
share/html/Elements/RT__CustomField/ColumnMap | 8 +-
share/html/Elements/ScrubHTML | 26 +-
share/html/Elements/SelectDate | 2 +-
share/html/Elements/ShowCustomFields | 12 +-
share/html/Elements/ShowUser | 2 +-
share/html/Elements/Submit | 4 +-
share/html/Helpers/Autocomplete/CustomFieldValues | 41 ++-
share/html/Helpers/CalPopup.html | 2 +-
share/html/Install/DatabaseType.html | 2 +-
share/html/NoAuth/Logout.html | 2 +-
share/html/NoAuth/js/titlebox-state.js | 2 +-
share/html/NoAuth/js/util.js | 4 +-
share/html/REST/1.0/Forms/transaction/default | 3 -
share/html/Search/Chart.html | 2 +-
share/html/Search/Elements/ResultViews | 2 +-
share/html/Search/Elements/ResultsRSSView | 2 +-
share/html/Search/Results.html | 12 +-
share/html/SelfService/Elements/MyRequests | 22 +-
share/html/SelfService/index.html | 2 +
share/html/Ticket/Elements/Bookmark | 2 +-
share/html/Ticket/Elements/UpdateCc | 6 +-
.../Ticket/Graphs/Elements/EditGraphProperties | 2 +-
share/html/Ticket/Graphs/Elements/ShowGraph | 1 +
share/html/Ticket/Graphs/dhandler | 1 +
share/html/Widgets/ComboBox | 4 +-
share/html/Widgets/TitleBoxStart | 2 +-
share/html/index.html | 2 +-
share/html/l | 2 +-
share/html/{l => l_unsafe} | 0
t/web/csrf-rest.t | 76 +++++
t/web/csrf.t | 181 ++++++++++
t/web/owner_disabled_group_19221.t | 189 +++++++++++
t/web/redirect-after-login.t | 6 +-
88 files changed, 1485 insertions(+), 251 deletions(-)
delete mode 100755 docs/Security
create mode 100644 docs/security.pod
create mode 100644 etc/upgrade/3.8.12/content
copy share/html/{Dashboards/Elements/DashboardsForObject => Elements/CSRF} (62%)
copy share/html/{l => l_unsafe} (100%)
create mode 100644 t/web/csrf-rest.t
create mode 100644 t/web/csrf.t
create mode 100644 t/web/owner_disabled_group_19221.t
- Log -----------------------------------------------------------------
commit 0abb5479c11e85c5cbc9e4046a6b678bcce723ab
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Nov 15 00:43:25 2011 -0500
Ignore the local directory which contains additional, temporarily non-public tests
diff --git a/.gitignore b/.gitignore
index ca6e4e9..a44e6a9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -18,6 +18,7 @@ Makefile
t/data/gnupg/keyrings/random_seed
t/data/configs/apache2.2+fastcgi.conf
t/data/configs/apache2.2+mod_perl.conf
+t/security/embargo/
t/tmp/
sbin/rt-attributes-viewer
sbin/rt-clean-sessions
commit 3ddacb63683ed572ff2f1d369974bd4b3fb8d6c6
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Nov 15 14:15:43 2011 -0500
Pull back docs/security.pod from 4.0-trunk
diff --git a/docs/Security b/docs/Security
deleted file mode 100755
index 51ddfab..0000000
--- a/docs/Security
+++ /dev/null
@@ -1,12 +0,0 @@
-Security tips for running RT3
-
-0 Protect your RT installation by making it only accessible via SSL
-
-1 Be sure to change the password for the root user of RT. The default password is "password". This can be changed via the RT web interface at: Preferences > About me
-
-2 Be sure to protect your RT_SiteConfig.pm file if it contains database credentials or other sensitive information. This file only needs to be readable by RT and your web server. One way to accomplish this is to make the file readable only by root and the group that RT runs as, and then make sure your web server is a member of that group. Advanced configuration may be required if other users have the ability to run CGIs or access the server where RT is running. Otherwise, those users may have access to become RT superusers.
-
-3 Be sure to protect your database. If it does not need to talk to the world, then don't allow it to listen for remote connections. With MySQL this can be accomplished via "skip-networking". If you use your database for other things and must allow remote connections, be sure to use a strong, hard to guess password for RT.
-
-4 Apache, lighttpd, and most other web servers support name based virtual hosts. When possible, configure RT as a name based virtual host to raise the bar against DNS rebinding attacks. Note: If when you visit http://your.servers.ipaddress.here you see RT, it means you are not likely getting this additional protection.
-
diff --git a/docs/security.pod b/docs/security.pod
new file mode 100644
index 0000000..8bf12bd
--- /dev/null
+++ b/docs/security.pod
@@ -0,0 +1,67 @@
+=head1 RT Security
+
+=head2 Reporting security vulnerabilities in RT
+
+If you believe you've discovered a security issue in RT, please send an
+email to <security at bestpractical.com> with a detailed description of the
+issue, and a secure means to respond to you (such as your PGP public
+key).
+
+More information is available at L<http://bestpractical.com/security/>.
+
+=head2 Security tips for running RT
+
+=over
+
+=item *
+
+Protect your RT installation by making it only accessible via SSL. This
+will protect against users' passwords being sniffed as they go over the
+wire, as well as helping prevent phishing attacks.
+
+
+=item *
+
+Be sure to change the password for the C<root> user of RT. The default
+password is C<password>. This can be changed via the RT web interface
+at: Preferences > About me
+
+
+=item *
+
+Be sure to protect your F<RT_SiteConfig.pm> file if it contains database
+credentials or other sensitive information. This file only needs to be
+readable by RT and your web server. One way to accomplish this is to
+make the file readable only by root and the group that RT runs as, and
+then make sure your web server is a member of that group. Advanced
+configuration may be required if other users have the ability to run
+CGIs or access the server where RT is running.
+
+
+=item *
+
+Be sure to protect your database. If it does not need to talk to the
+world, then don't allow it to listen for remote connections. With MySQL
+this can be accomplished via C<skip-networking>. If you use your
+database for other things and must allow remote connections, be sure to
+use a strong, hard to guess password for RT.
+
+
+=item *
+
+Apache, lighttpd, and most other web servers support name based virtual
+hosts. When possible, configure RT as a name based virtual host to
+raise the bar against DNS rebinding attacks. If you see RT when you
+visit http://your.servers.ipaddress.here, it means you are likely not
+getting this additional protection.
+
+
+=item *
+
+Use groups to organize RT permissions. Granting permissions per-user
+makes them, in general, more easily over-granted and forgotten, and more
+likely to diverge from each other, forming a maintenance hassle.
+
+=back
+
+=cut
commit 147c38f3740ef4be7c37a74982aaa1505145e59e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Nov 15 14:50:28 2011 -0500
Add a note about the timeline on public announcements, tests, etc
diff --git a/docs/security.pod b/docs/security.pod
index 8bf12bd..eaecc34 100644
--- a/docs/security.pod
+++ b/docs/security.pod
@@ -9,6 +9,21 @@ key).
More information is available at L<http://bestpractical.com/security/>.
+
+=head2 RT's security process
+
+After a security vulnerability is reported to Best Practical and
+verified, we attempt to resolve it in as timely a fashion as possible.
+Best Practical support customers will be notified before we disclose the
+information to the public. All security announcements will be sent to
+C<rt-announce at bestpractical.com>, which includes
+C<rt-users at bestpractical.com> and C<rt-devel at bestpractical.com>.
+
+As the tests for security vulnerabilities are often nearly identical to
+working exploits, sensitive tests will be embargoed for a period of six
+months before being added to the public RT repository.
+
+
=head2 Security tips for running RT
=over
commit ef35fe55305d97233f74dd75720f15d385a3432f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 17 20:14:07 2011 -0500
Avoid shell interpolation when calling sendmailpipe
Use IPC::Open2 rather than three-arg open with "|-", to prevent shell
interpolation. This allows not only fine control of the parameters to
the sendmail command, but also allows the STDOUT content to be dropped,
as was previously handled using a shell output redirect. For backwards
compatability, use Text::ParseWords to split the configuration
parameters which are strings, rather than argument lists.
This prevents a vulnerability where specially-crafted emails could allow
code execution if VERP was enabled.
This resolves CVE-2011-2082.
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index 401e970..6e48019 100755
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -57,6 +57,7 @@ use RT::EmailParser;
use File::Temp;
use UNIVERSAL::require;
use Mail::Mailer ();
+use Text::ParseWords qw/shellwords/;
BEGIN {
use base 'Exporter';
@@ -404,7 +405,7 @@ sub SendEmail {
if ( $mail_command eq 'sendmailpipe' ) {
my $path = RT->Config->Get('SendmailPath');
- my $args = RT->Config->Get('SendmailArguments');
+ my @args = shellwords(RT->Config->Get('SendmailArguments'));
# SetOutgoingMailFrom
if ( RT->Config->Get('SetOutgoingMailFrom') ) {
@@ -423,12 +424,13 @@ sub SendEmail {
$OutgoingMailAddress ||= RT->Config->Get('OverrideOutgoingMailFrom')->{'Default'};
- $args .= " -f $OutgoingMailAddress"
+ push @args, "-f", $OutgoingMailAddress
if $OutgoingMailAddress;
}
# Set Bounce Arguments
- $args .= ' '. RT->Config->Get('SendmailBounceArguments') if $args{'Bounce'};
+ push @args, shellwords(RT->Config->Get('SendmailBounceArguments'))
+ if $args{'Bounce'};
# VERP
if ( $TransactionObj and
@@ -438,32 +440,36 @@ sub SendEmail {
my $from = $TransactionObj->CreatorObj->EmailAddress;
$from =~ s/@/=/g;
$from =~ s/\s//g;
- $args .= " -f $prefix$from\@$domain";
+ push @args, "-f", "$prefix$from\@$domain";
}
eval {
# don't ignore CHLD signal to get proper exit code
local $SIG{'CHLD'} = 'DEFAULT';
- open( my $mail, '|-', "$path $args >/dev/null" )
- or die "couldn't execute program: $!";
-
# if something wrong with $mail->print we will get PIPE signal, handle it
local $SIG{'PIPE'} = sub { die "program unexpectedly closed pipe" };
+
+ require IPC::Open2;
+ my ($mail, $stdout);
+ my $pid = IPC::Open2::open2( $stdout, $mail, $path, @args )
+ or die "couldn't execute program: $!";
+
$args{'Entity'}->print($mail);
+ close $mail or die "close pipe failed: $!";
- unless ( close $mail ) {
- die "close pipe failed: $!" if $!; # system error
+ waitpid($pid, 0);
+ if ($?) {
# sendmail exit statuses mostly errors with data not software
# TODO: status parsing: core dump, exit on signal or EX_*
- my $msg = "$msgid: `$path $args` exitted with code ". ($?>>8);
+ my $msg = "$msgid: `$path @args` exited with code ". ($?>>8);
$msg = ", interrupted by signal ". ($?&127) if $?&127;
$RT::Logger->error( $msg );
die $msg;
}
};
if ( $@ ) {
- $RT::Logger->crit( "$msgid: Could not send mail with command `$path $args`: " . $@ );
+ $RT::Logger->crit( "$msgid: Could not send mail with command `$path @args`: " . $@ );
if ( $TicketObj ) {
_RecordSendEmailFailure( $TicketObj );
}
commit 31d1728a9034e1cbcd394449a292a248cad0126e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Nov 14 17:22:35 2011 -0500
Prevent storing the old or new hashed password in the transaction table
diff --git a/lib/RT/User_Overlay.pm b/lib/RT/User_Overlay.pm
index b86a924..2cf31c3 100755
--- a/lib/RT/User_Overlay.pm
+++ b/lib/RT/User_Overlay.pm
@@ -1646,7 +1646,9 @@ sub _Set {
if ( $ret == 0 ) { return ( 0, $msg ); }
if ( $args{'RecordTransaction'} == 1 ) {
-
+ if ($args{'Field'} eq "Password") {
+ $args{'Value'} = $Old = '********';
+ }
my ( $Trans, $Msg, $TransObj ) = $self->_NewTransaction(
Type => $args{'TransactionType'},
Field => $args{'Field'},
commit 864c8193f45fb2733f3e6a148ec1aae7d95d155f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Nov 14 17:29:39 2011 -0500
Clean out sensitive user transactions
diff --git a/etc/upgrade/3.8.12/content b/etc/upgrade/3.8.12/content
new file mode 100644
index 0000000..dc1a009
--- /dev/null
+++ b/etc/upgrade/3.8.12/content
@@ -0,0 +1,17 @@
+ at Initial = (
+ sub {
+ my $txns = RT::Transactions->new( $RT::SystemUser );
+ $txns->Limit(
+ FIELD => "ObjectType",
+ VALUE => "RT::User",
+ );
+ $txns->Limit(
+ FIELD => "Field",
+ VALUE => "Password",
+ );
+ while (my $txn = $txns->Next) {
+ $txn->__Set( Field => $_, Value => '********' )
+ for qw/OldValue NewValue/;
+ }
+ },
+);
commit 71007612cf3ab9409ab250f0e53b21be86c75780
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 16:57:56 2011 -0500
Add a consistent CurrentUserCanSee right
diff --git a/lib/RT/Group_Overlay.pm b/lib/RT/Group_Overlay.pm
index 09f3082..34f8c0b 100755
--- a/lib/RT/Group_Overlay.pm
+++ b/lib/RT/Group_Overlay.pm
@@ -1335,8 +1335,18 @@ sub CurrentUserHasRight {
# }}}
+=head2 CurrentUserCanSee
+Always returns 1; unfortunately, for historical reasons, users have
+always been able to examine groups they have indirect access to, even if
+they do not have SeeGroup explicitly.
+=cut
+
+sub CurrentUserCanSee {
+ my $self = shift;
+ return 1;
+}
# {{{ Principal related routines
diff --git a/lib/RT/Queue_Overlay.pm b/lib/RT/Queue_Overlay.pm
index c7ab7f3..1d7c9c8 100755
--- a/lib/RT/Queue_Overlay.pm
+++ b/lib/RT/Queue_Overlay.pm
@@ -1204,6 +1204,18 @@ sub CurrentUserHasRight {
# }}}
+=head2 CurrentUserCanSee
+
+Returns true if the current user can see the queue, using SeeQueue
+
+=cut
+
+sub CurrentUserCanSee {
+ my $self = shift;
+
+ return $self->CurrentUserHasRight('SeeQueue');
+}
+
# {{{ sub HasRight
=head2 HasRight
diff --git a/lib/RT/Ticket_Overlay.pm b/lib/RT/Ticket_Overlay.pm
index 9f1e26c..43731d2 100755
--- a/lib/RT/Ticket_Overlay.pm
+++ b/lib/RT/Ticket_Overlay.pm
@@ -3446,6 +3446,17 @@ sub CurrentUserHasRight {
# }}}
+=head2 CurrentUserCanSee
+
+Returns true if the current user can see the ticket, using ShowTicket
+
+=cut
+
+sub CurrentUserCanSee {
+ my $self = shift;
+ return $self->CurrentUserHasRight('ShowTicket');
+}
+
# {{{ sub HasRight
=head2 HasRight
diff --git a/lib/RT/User_Overlay.pm b/lib/RT/User_Overlay.pm
index 2cf31c3..2f8545e 100755
--- a/lib/RT/User_Overlay.pm
+++ b/lib/RT/User_Overlay.pm
@@ -1360,6 +1360,37 @@ sub HasRight {
return $self->PrincipalObj->HasRight(@_);
}
+=head2 CurrentUserCanSee [FIELD]
+
+Returns true if the current user can see the user, based on if it is
+public, ourself, or we have AdminUsers
+
+=cut
+
+sub CurrentUserCanSee {
+ my $self = shift;
+ my ($what) = @_;
+
+ # If it's public, fine. Note that $what may be "transaction", which
+ # doesn't have an Accessible value, and thus falls through below.
+ if ( $self->_Accessible( $what, 'public' ) ) {
+ return 1;
+ }
+
+ # Users can see their own properties
+ elsif ( defined($self->Id) && $self->CurrentUser->Id == $self->Id ) {
+ return 1;
+ }
+
+ # If the user has the admin users right, that's also enough
+ elsif ( $self->CurrentUser->HasRight( Right => 'AdminUsers', Object => $RT::System) ) {
+ return 1;
+ }
+ else {
+ return 0;
+ }
+}
+
=head2 CurrentUserCanModify RIGHT
If the user has rights for this object, either because
@@ -1675,33 +1706,9 @@ sub _Value {
my $self = shift;
my $field = shift;
- #If the current user doesn't have ACLs, don't let em at it.
-
- my @PublicFields = qw( Name EmailAddress Organization Disabled
- RealName NickName Gecos ExternalAuthId
- AuthSystem ExternalContactInfoId
- ContactInfoSystem );
-
- #if the field is public, return it.
- if ( $self->_Accessible( $field, 'public' ) ) {
- return ( $self->SUPER::_Value($field) );
-
- }
-
- #If the user wants to see their own values, let them
- # TODO figure ouyt a better way to deal with this
- elsif ( defined($self->Id) && $self->CurrentUser->Id == $self->Id ) {
- return ( $self->SUPER::_Value($field) );
- }
-
- #If the user has the admin users right, return the field
- elsif ( $self->CurrentUser->HasRight(Right =>'AdminUsers', Object => $RT::System) ) {
- return ( $self->SUPER::_Value($field) );
- }
- else {
- return (undef);
- }
-
+ # Defer to the abstraction above to know if the field can be read
+ return $self->SUPER::_Value($field) if $self->CurrentUserCanSee($field);
+ return undef;
}
=head2 FriendlyName
commit 02ff6818763e3c5c1ccc7d1fe3854a25cda74a50
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 16:59:52 2011 -0500
Enable ACL checks for non-Ticket transactions
Use CurrentUserCanSee on relevant object to prevent access to
Transactions that the user shouldn't be able to see.
This partially resolves CVE-2011-2084.
diff --git a/lib/RT/Transaction_Overlay.pm b/lib/RT/Transaction_Overlay.pm
index 4870973..9a68ac4 100755
--- a/lib/RT/Transaction_Overlay.pm
+++ b/lib/RT/Transaction_Overlay.pm
@@ -1062,14 +1062,8 @@ sub CurrentUserCanSee {
$cf->Load( $cf_id );
return 0 unless $cf->CurrentUserHasRight('SeeCustomField');
}
- #if they ain't got rights to see, don't let em
- elsif ( $self->__Value('ObjectType') eq "RT::Ticket" ) {
- unless ( $self->CurrentUserHasRight('ShowTicket') ) {
- return 0;
- }
- }
-
- return 1;
+ # Defer to the object in question
+ return $self->Object->CurrentUserCanSee("Transaction");
}
# }}}
commit 329e14ec5f581c7e9a490ca3a1b4b1f204cdd419
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 20:47:21 2011 -0500
Remove unused $args and @arglist variables
diff --git a/share/html/REST/1.0/Forms/transaction/default b/share/html/REST/1.0/Forms/transaction/default
index 46488d2..5d7f024 100644
--- a/share/html/REST/1.0/Forms/transaction/default
+++ b/share/html/REST/1.0/Forms/transaction/default
@@ -49,7 +49,6 @@
%#
<%ARGS>
$id
-$args => undef
$format => undef
$fields => undef
</%ARGS>
@@ -57,8 +56,6 @@ $fields => undef
my $trans = new RT::Transactions $session{CurrentUser};
my ($c, $o, $k, $e) = ("", [], {} , "");
-chomp $args;
-my @arglist = split('/', $args);
my $tid = $id;
$trans->Limit(FIELD => 'Id', OPERATOR => '=', VALUE => $tid);
commit eaa7ba63829b129d2ddae95983ff81883d149bb9
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 20:48:07 2011 -0500
Explicitly ACL ObjectCustomFieldValue content, based on the custon field object
diff --git a/lib/RT/ObjectCustomFieldValue_Overlay.pm b/lib/RT/ObjectCustomFieldValue_Overlay.pm
index 403d216..a1284cb 100644
--- a/lib/RT/ObjectCustomFieldValue_Overlay.pm
+++ b/lib/RT/ObjectCustomFieldValue_Overlay.pm
@@ -175,6 +175,9 @@ content, try "LargeContent"
sub Content {
my $self = shift;
my $content = $self->SUPER::Content;
+
+ return undef unless $self->CustomFieldObj->CurrentUserHasRight('SeeCustomField');
+
if ( !(defined $content && length $content) && $self->ContentType && $self->ContentType eq 'text/plain' ) {
return $self->LargeContent;
} else {
commit 488cdbd10f68e4d8b5c52934268de3c65d7e0a57
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 20:49:25 2011 -0500
There is no reason for ->NewValue and ->OldValue to skip ACLs via __Value
diff --git a/lib/RT/Transaction_Overlay.pm b/lib/RT/Transaction_Overlay.pm
index 9a68ac4..97925cd 100755
--- a/lib/RT/Transaction_Overlay.pm
+++ b/lib/RT/Transaction_Overlay.pm
@@ -1088,7 +1088,7 @@ sub OldValue {
return $Object->Content;
}
else {
- return $self->__Value('OldValue');
+ return $self->_Value('OldValue');
}
}
@@ -1102,7 +1102,7 @@ sub NewValue {
return $Object->Content;
}
else {
- return $self->__Value('NewValue');
+ return $self->_Value('NewValue');
}
}
commit 52c4d4c0fe723869a94c3a3292c17238ed83c14f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Jan 5 22:40:38 2012 -0500
Prevent actual error messages from propagating to the user
With DevelMode off, Mason formats runtime errors using the 'brief'
format, which includes the full path on disk to where the error ocurred.
This is a potential information leak. Instead, provide a generic error
message to the user, but log the actual error in the logs.
diff --git a/lib/RT/Interface/Web/Handler.pm b/lib/RT/Interface/Web/Handler.pm
index 4bb6484..52bb37f 100644
--- a/lib/RT/Interface/Web/Handler.pm
+++ b/lib/RT/Interface/Web/Handler.pm
@@ -75,7 +75,7 @@ sub DefaultHandlerArgs { (
static_source => (RT->Config->Get('DevelMode') ? '0' : '1'),
use_object_files => (RT->Config->Get('DevelMode') ? '0' : '1'),
autoflush => 0,
- error_format => (RT->Config->Get('DevelMode') ? 'html': 'brief'),
+ error_format => (RT->Config->Get('DevelMode') ? 'html': 'rt_error'),
request_class => 'RT::Interface::Web::Request',
named_component_subs => $INC{'Devel/Cover.pm'} ? 1 : 0,
) };
@@ -271,4 +271,10 @@ sub CleanupRequest {
}
# }}}
+sub HTML::Mason::Exception::as_rt_error {
+ my ($self) = @_;
+ $RT::Logger->error( $self->full_message );
+ return "An internal RT error has occurred.";
+}
+
1;
commit efd243054470431d4e6297630b712e01a02bbef0
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Mar 27 17:36:41 2012 -0400
Escape all arguments passed to /l
Escaping arguments prevents one of the easiest ways to accidentally
write XSS vectors. For /l calls which pass HTML in their arguments,
/l_unsafe is provided which preserves the previous behaviour. Note that
the text to be localized itself is not escaped, only the placeholder
values. Hopefully seeing "unsafe" in the name will also make folks
pause and consider what they're doing.
Partially resolves CVE-2011-2083.
diff --git a/share/html/Approvals/Elements/PendingMyApproval b/share/html/Approvals/Elements/PendingMyApproval
index e3cdff6..fa84a17 100755
--- a/share/html/Approvals/Elements/PendingMyApproval
+++ b/share/html/Approvals/Elements/PendingMyApproval
@@ -63,9 +63,9 @@
<input type="checkbox" class="checkbox" value="1" name="ShowRejected" <% defined($ARGS{'ShowRejected'}) && $ARGS{'ShowRejected'} && qq[checked="checked"] |n%> /> <&|/l&>Show denied requests</&><br />
<input type="checkbox" class="checkbox" value="1" name="ShowDependent" <% defined($ARGS{'ShowDependent'}) && $ARGS{'ShowDependent'} && qq[checked="checked"] |n%> /> <&|/l&>Show requests awaiting other approvals</&><br />
-<&|/l,"<input size='15' value='".($created_before->Unix > 0 &&$created_before->ISO(Timezone => 'user'))."' name='CreatedBefore' id='CreatedBefore' />"&>Only show approvals for requests created before [_1]</&><br />
+<&|/l_unsafe,"<input size='15' value='".($created_before->Unix > 0 &&$created_before->ISO(Timezone => 'user'))."' name='CreatedBefore' id='CreatedBefore' />"&>Only show approvals for requests created before [_1]</&><br />
-<&|/l, "<input size='15' value='".( $created_after->Unix >0 && $created_after->ISO(Timezone => 'user'))."' name='CreatedAfter' id='CreatedAfter' />"&>Only show approvals for requests created after [_1]</&>
+<&|/l_unsafe, "<input size='15' value='".( $created_after->Unix >0 && $created_after->ISO(Timezone => 'user'))."' name='CreatedAfter' id='CreatedAfter' />"&>Only show approvals for requests created after [_1]</&>
</&>
<%init>
diff --git a/share/html/Elements/CreateTicket b/share/html/Elements/CreateTicket
index 02275ef..c8287e0 100755
--- a/share/html/Elements/CreateTicket
+++ b/share/html/Elements/CreateTicket
@@ -46,7 +46,7 @@
%#
%# END BPS TAGGED BLOCK }}}
<form action="<% RT->Config->Get('WebPath') %><% $SendTo %>" name="CreateTicketInQueue" id="CreateTicketInQueue">
-<&|/l, $m->scomp('/Elements/SelectNewTicketQueue', OnChange => 'document.CreateTicketInQueue.submit()', SendTo => $SendTo ) &><input type="submit" class="button" value="New ticket in" /> [_1]</&>
+<&|/l_unsafe, $m->scomp('/Elements/SelectNewTicketQueue', OnChange => 'document.CreateTicketInQueue.submit()', SendTo => $SendTo ) &><input type="submit" class="button" value="New ticket in" /> [_1]</&>
</form>
<%ARGS>
$SendTo => '/Ticket/Create.html',
diff --git a/share/html/Elements/Footer b/share/html/Elements/Footer
index 2796213..f688110 100755
--- a/share/html/Elements/Footer
+++ b/share/html/Elements/Footer
@@ -56,13 +56,13 @@
%}
<p id="bpscredits">
<span>
-<&|/l, '»|«', $RT::VERSION, '2010', '<a href="http://www.bestpractical.com?rt='.$RT::VERSION.'">Best Practical Solutions, LLC</a>', &>[_1] RT [_2] Copyright 1996-[_3] [_4].</&>
+<&|/l_unsafe, '»|«', $RT::VERSION, '2010', '<a href="http://www.bestpractical.com?rt='.$RT::VERSION.'">Best Practical Solutions, LLC</a>', &>[_1] RT [_2] Copyright 1996-[_3] [_4].</&>
</span>
</p>
% if (!$Menu) {
<p id="legal">
<&|/l&>Distributed under version 2 <a href="http://www.gnu.org/copyleft/gpl.html"> of the GNU GPL.</a></&><br />
-<&|/l, '<a href="mailto:sales at bestpractical.com">sales at bestpractical.com</a>' &>To inquire about support, training, custom development or licensing, please contact [_1].</&><br />
+<&|/l_unsafe, '<a href="mailto:sales at bestpractical.com">sales at bestpractical.com</a>' &>To inquire about support, training, custom development or licensing, please contact [_1].</&><br />
</p>
% }
diff --git a/share/html/Elements/PersonalQuickbar b/share/html/Elements/PersonalQuickbar
index 993c457..baac704 100644
--- a/share/html/Elements/PersonalQuickbar
+++ b/share/html/Elements/PersonalQuickbar
@@ -51,7 +51,7 @@ $Prefs => '/Prefs/Other.html'
<div id="quick-personal">
<span class="hide"><a href="#skipnav"><&|/l&>Skip Menu</&></a> | </span>
% if ($session{'CurrentUser'}->Name) {
- <&|/l, "<span>".$session{'CurrentUser'}->Name."</span>" &>Logged in as [_1]</&>
+ <&|/l_unsafe, "<span>".$m->interp->apply_escapes($session{'CurrentUser'}->Name, 'h')."</span>" &>Logged in as [_1]</&>
% if ( $session{'CurrentUser'}->HasRight( Right => 'ModifySelf', Object => $RT::System ) ) {
| <a href="<%RT->Config->Get('WebPath')%><%$Prefs%>"><&|/l&>Preferences</&></a>
% }
diff --git a/share/html/Install/DatabaseType.html b/share/html/Install/DatabaseType.html
index 60bf79c..67f2c1b 100644
--- a/share/html/Install/DatabaseType.html
+++ b/share/html/Install/DatabaseType.html
@@ -58,7 +58,7 @@
<&|/l&>SQLite is a database that doesn't need a server or any configuration whatsoever. RT's authors recommend it for testing, demoing and development, but it's not quite right for a high-volume production RT server.</&>
</b></p>
<p>
-<&|/l, '<a href="http://search.cpan.org" target="_new">CPAN</a>' &>If your preferred database isn't listed in the dropdown below, that means RT couldn't find a <i>database driver</i> for it installed locally. You may be able to remedy this by using [_1] to download and install DBD::MySQL, DBD::Oracle or DBD::Pg.</&>
+<&|/l_unsafe, '<a href="http://search.cpan.org" target="_new">CPAN</a>' &>If your preferred database isn't listed in the dropdown below, that means RT couldn't find a <i>database driver</i> for it installed locally. You may be able to remedy this by using [_1] to download and install DBD::MySQL, DBD::Oracle or DBD::Pg.</&>
</p>
</div>
diff --git a/share/html/Search/Chart.html b/share/html/Search/Chart.html
index 1a80ee3..dd782c3 100644
--- a/share/html/Search/Chart.html
+++ b/share/html/Search/Chart.html
@@ -90,7 +90,7 @@ my @actions = $m->comp( '/Widgets/SavedSearch:process', args => \%ARGS, self =>
<form method="get" action="<%RT->Config->Get('WebPath')%>/Search/Chart.html">
<input type="hidden" class="hidden" name="Query" value="<% $ARGS{Query} %>" />
<input type="hidden" class="hidden" name="SavedChartSearchId" value="<% $saved_search->{SearchId} || 'new' %>" />
-<&|/l, $m->scomp('Elements/SelectChartType', Name => 'ChartStyle', Default => $ChartStyle), $m->scomp('Elements/SelectGroupBy', Name => 'PrimaryGroupBy', Query => $ARGS{Query}, Default => $PrimaryGroupBy)
+<&|/l_unsafe, $m->scomp('Elements/SelectChartType', Name => 'ChartStyle', Default => $ChartStyle), $m->scomp('Elements/SelectGroupBy', Name => 'PrimaryGroupBy', Query => $ARGS{Query}, Default => $PrimaryGroupBy)
&>[_1] chart by [_2]</&><input type="submit" class="button" value="<%loc('Update Graph')%>" />
</form>
</&>
diff --git a/share/html/Search/Elements/ResultViews b/share/html/Search/Elements/ResultViews
index c146e67..7a08d61 100644
--- a/share/html/Search/Elements/ResultViews
+++ b/share/html/Search/Elements/ResultViews
@@ -69,7 +69,7 @@ $ShortQueryString => undef
% foreach my $key (keys(%hiddens)) {
<input type="hidden" class="hidden" name="<%$key%>" value="<%defined($hiddens{$key})?$hiddens{$key}:''%>" />
% }
-<&|/l, $m->scomp('SelectChartType', Name => 'ChartStyle'), $m->scomp('SelectGroupBy', Name => 'PrimaryGroupBy', Query => $Query)
+<&|/l_unsafe, $m->scomp('SelectChartType', Name => 'ChartStyle'), $m->scomp('SelectGroupBy', Name => 'PrimaryGroupBy', Query => $Query)
&>[_1] chart by [_2]</&><input type="submit" class="button" value="<%loc('Go')%>" />
</form>
<%init>
diff --git a/share/html/l b/share/html/l
index 771c5a8..960fd53 100755
--- a/share/html/l
+++ b/share/html/l
@@ -47,6 +47,6 @@
%# END BPS TAGGED BLOCK }}}
<%init>
my $hand = ($session{'CurrentUser'} ||= RT::CurrentUser->new)->LanguageHandle;
- $m->print($hand->maketext($m->content, at _));
+ $m->print($hand->maketext($m->content,map { $m->interp->apply_escapes($_, 'h') } @_));
return(1);
</%init>
diff --git a/share/html/l b/share/html/l_unsafe
similarity index 100%
copy from share/html/l
copy to share/html/l_unsafe
commit adc0df31fa1427d596294ab61f6b81e8f7d9033f
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Mar 22 19:10:14 2012 -0400
We did not find and upgrade passwords for disabled users.
This script can be safely re-run on RT installations which were
previously upgraded.
diff --git a/etc/upgrade/vulnerable-passwords.in b/etc/upgrade/vulnerable-passwords.in
index 0af2b64..7948de3 100755
--- a/etc/upgrade/vulnerable-passwords.in
+++ b/etc/upgrade/vulnerable-passwords.in
@@ -89,6 +89,9 @@ push @{$users->{'restrictions'}{ "main.Password" }}, "AND", {
value => '40',
};
+# we want to update passwords on disabled users
+$users->{'find_disabled_rows'} = 1;
+
my $count = $users->Count;
if ($count == 0) {
print "No users with unsalted or weak cryptography found.\n";
commit e8268e46e5de3529370c4bf23256ee3331595485
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:07:31 2012 -0400
Remove extra SendSessionCookie() calls
Calling SendSessionCookie() is only necessary after the cookie value has
changed, which only happens in two places -- initially, after the
current cookie is read, or after successful authentication, when we
change the session identifier to avoid session fixation. Additional
calls to SendSessionCookie only confuse when it is necessary.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b3f593a..68c159b 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -291,8 +291,6 @@ sub SetNextPage {
$HTML::Mason::Commands::session{'NextPage'}->{$hash} = $next;
$HTML::Mason::Commands::session{'i'}++;
-
- SendSessionCookie();
return $hash;
}
@@ -409,7 +407,6 @@ sub MaybeShowNoAuthPage {
if $m->base_comp->path eq '/NoAuth/Login.html' and _UserLoggedIn();
# If it's a noauth file, don't ask for auth.
- SendSessionCookie();
$m->comp( { base_comp => $m->request_comp }, $m->fetch_next, %$ARGS );
$m->abort;
}
@@ -458,8 +455,6 @@ sub ShowRequestedPage {
my $m = $HTML::Mason::Commands::m;
- SendSessionCookie();
-
# If the user isn't privileged, they can only see SelfService
unless ( $HTML::Mason::Commands::session{'CurrentUser'}->Privileged ) {
commit 26a2816316b4883529f9d22175b8be2cd58271ff
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:14:14 2012 -0400
Add basic HTTP_REFERER checking to prevent cross-site request forgery
RT has had a long-standing policy of accepting query parameters in both
POST and GET, and taking action based on them. Unfortunately, this
makes RT susceptible to cross-site request forgery (CSRF), wherein the
browser is tricked into making a request on behalf of the user, which
includes the user's cached credentials, and the remote server runs an
malicious action with the user's identity.
Prevent this by enforcing that all requests to RT come with a "Referer"
(sic) header of RT itself. Studies have shown that while the incidence
of suppressed or incorrect Referer headers is non-trivial over unsecured
HTTP (3% - 11%), it is very uncommon (0.05% - %0.22%) when the server is
secured using HTTPS[1]. As running RT without SSL protection already
presents a host of other vulnerabilities, including man-in-the-middle
attacks and password sniffing, we have judged that disabling CSRF
protections in HTTP-only deployments should be an acceptable solution.
Enforcing that all requests have such a header is entirely too
restrictive, as it prevents a large number of idempotent requests which
are not abusable, but provides the foundation of the CSRF protection.
This resolves CVE-2011-2085.
[1] http://seclab.stanford.edu/websec/csrf/csrf.pdf
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index 1df9c66..ce37aca 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1248,6 +1248,17 @@ requirements.
Set($WebHttpOnlyCookies, 1);
+=item C<$RestrictReferrer>
+
+If set to a false value, the HTTP C<Referer> (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<$WebFlushDbCacheEveryRequest>
By default, RT clears its database cache after every page view.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 68c159b..64b40e3 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -235,6 +235,8 @@ sub HandleRequest {
}
}
+ MaybeDenyCSRF($ARGS);
+
# now it applies not only to home page, but any dashboard that can be used as a workspace
$HTML::Mason::Commands::session{'home_refresh_interval'} = $ARGS->{'HomeRefreshInterval'}
if ( $ARGS->{'HomeRefreshInterval'} );
@@ -981,6 +983,69 @@ sub LogRecordedSQLStatements {
}
+sub IsCompCSRFWhitelisted {
+ my $comp = shift;
+ my $ARGS = shift;
+
+ my %args = %{ $ARGS };
+
+ # 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};
+
+ # If they have a valid results= from MaybeRedirectForResults, that's
+ # also fine.
+ delete $args{results} if $args{results}
+ and $HTML::Mason::Commands::session{"Actions"}->{$args{results}};
+
+ # If there are no arguments, then it's likely to be an idempotent
+ # request, which are not susceptible to CSRF
+ return 1 if !%args;
+
+ return 0;
+}
+
+sub IsRefererCSRFWhitelisted {
+ my $referer = shift;
+
+ my $site = URI->new(RT->Config->Get('WebBaseURL'));
+ $site->host('127.0.0.1') if $site->host eq 'localhost';
+ return 1 if $referer->host_port eq $site->host_port;
+
+ return 0;
+}
+
+sub IsPossibleCSRF {
+ my $ARGS = shift;
+
+ return 0 if IsCompCSRFWhitelisted(
+ $HTML::Mason::Commands::m->request_comp->path,
+ $ARGS
+ );
+
+ # if there is no Referer header then assume the worst
+ return (1, "No Referer header. Perhaps your web browser is configured to never send the Referer header?") if !$ENV{HTTP_REFERER};
+
+ my $referer = URI->new($ENV{HTTP_REFERER});
+ $referer->host('127.0.0.1') if $referer->host eq 'localhost';
+
+ return 0 if IsRefererCSRFWhitelisted($referer);
+
+ return (1, "Referer is unknown site ".$referer->host_port);
+}
+
+sub MaybeDenyCSRF {
+ my $ARGS = shift;
+
+ return unless RT->Config->Get('RestrictReferrer');
+
+ my ($is_csrf, $msg) = IsPossibleCSRF($ARGS);
+ return if !$is_csrf;
+
+ HTML::Mason::Commands::Abort( $msg );
+}
+
package HTML::Mason::Commands;
use vars qw/$r $m %session/;
diff --git a/t/web/csrf.t b/t/web/csrf.t
new file mode 100644
index 0000000..d170354
--- /dev/null
+++ b/t/web/csrf.t
@@ -0,0 +1,58 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+
+use RT::Test tests => undef;
+
+my $ticket = RT::Ticket->new(RT::CurrentUser->new('root'));
+my ($ok, $msg) = $ticket->Create(Queue => 1, Owner => 'nobody', Subject => 'bad music');
+ok($ok);
+
+my ($baseurl, $m) = RT::Test->started_ok;
+
+my $test_page = "/Ticket/Create.html?Queue=1";
+
+$m->get_ok($baseurl);
+ok $m->login, 'logged in';
+
+# valid referer
+$m->add_header(Referer => $baseurl);
+$m->get_ok($test_page);
+$m->content_lacks("Referer is unknown site");
+$m->title_is('Create a new ticket');
+
+# now send a referer from an attacker
+$m->add_header(Referer => 'http://example.net');
+$m->get_ok($test_page);
+$m->content_contains("Referer is unknown site");
+$m->warning_like(qr/Referer is unknown site/);
+
+# try a whitelisted argument from an attacker
+$m->add_header(Referer => 'http://example.net');
+$m->get_ok("/Ticket/Display.html?id=1");
+$m->content_lacks("Referer is unknown site");
+$m->title_is('#1: bad music');
+
+# now a non-whitelisted argument
+$m->get_ok("/Ticket/Display.html?id=1&Action=Take");
+$m->content_contains("Referer is unknown site");
+$m->warning_like(qr/Referer is unknown site/);
+
+
+# now try self-service with CSRF
+my $user = RT::User->new(RT->SystemUser);
+$user->Create(Name => "SelfService", Password => "chops", Privileged => 0);
+
+$m = RT::Test::Web->new;
+$m->get_ok($baseurl);
+$m->get_ok("$baseurl/index.html?user=SelfService&pass=chops");
+$m->title_is("Open tickets", "got self-service interface");
+$m->content_contains("My open tickets", "got self-service interface");
+
+# post without referer
+$m->add_header(Referer => undef);
+$m->get_ok("/SelfService/Create.html?Queue=1");
+$m->content_contains("No Referer header");
+$m->warning_like(qr/No Referer header/);
+
+undef $m;
commit d2d451591a7622e96cf3052e591029cdaa890419
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:25:46 2012 -0400
Whitelist some component (not request!) paths
The RSS feed, despite being requested with no Referer header, and having
query parameters, modifies no state on the server, and as such should be
exempt from CSRF checks.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 64b40e3..b148905 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -983,10 +983,19 @@ sub LogRecordedSQLStatements {
}
+my %is_whitelisted_path = (
+ # 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.
+ '/NoAuth/rss/dhandler' => 1,
+);
+
sub IsCompCSRFWhitelisted {
my $comp = shift;
my $ARGS = shift;
+ return 1 if $is_whitelisted_path{$comp};
+
my %args = %{ $ARGS };
# Eliminate arguments that do not indicate an effectful request.
commit 9243676f7ffbfe9d6b2c614ca604a515893a8e54
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:28:37 2012 -0400
Redirect to an interstitial page on CSRF attacks, rather than denying
As some browsers never supply Referer headers, and to maintain better
integration with other sites which may link into RT, simply denying the
a potential CSRF request with no recourse is unfortunate, and
frustrating to the user.
Instead, store the request parameters in a unique key on the session,
and provide a page with a link which will extract them again and re-run
the request. This allows intercepted requests to continue with only one
extra click.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b148905..b236751 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -235,7 +235,7 @@ sub HandleRequest {
}
}
- MaybeDenyCSRF($ARGS);
+ MaybeShowInterstitialCSRFPage($ARGS);
# now it applies not only to home page, but any dashboard that can be used as a workspace
$HTML::Mason::Commands::session{'home_refresh_interval'} = $ARGS->{'HomeRefreshInterval'}
@@ -1034,25 +1034,70 @@ sub IsPossibleCSRF {
);
# if there is no Referer header then assume the worst
- return (1, "No Referer header. Perhaps your web browser is configured to never send the Referer header?") if !$ENV{HTTP_REFERER};
+ return (1,
+ "your browser did not supply a Referrer header", # loc
+ ) if !$ENV{HTTP_REFERER};
my $referer = URI->new($ENV{HTTP_REFERER});
$referer->host('127.0.0.1') if $referer->host eq 'localhost';
-
return 0 if IsRefererCSRFWhitelisted($referer);
- return (1, "Referer is unknown site ".$referer->host_port);
+ return (1,
+ "the Referrer header supplied by your browser ([_1]) is not allowed", # loc
+ $referer->host_port);
}
-sub MaybeDenyCSRF {
+sub ExpandCSRFToken {
+ my $ARGS = shift;
+
+ my $token = delete $ARGS->{CSRF_Token};
+ return unless $token;
+
+ my $data = $HTML::Mason::Commands::session{'CSRF'}{$token};
+ return unless $data;
+ return unless $data->{path} eq $HTML::Mason::Commands::r->path_info;
+
+ my $user = $HTML::Mason::Commands::session{'CurrentUser'}->UserObj;
+ return unless $user->ValidateAuthString( $data->{auth}, $token );
+
+ %{$ARGS} = %{$data->{args}};
+
+ return 1;
+}
+
+sub MaybeShowInterstitialCSRFPage {
my $ARGS = shift;
return unless RT->Config->Get('RestrictReferrer');
- my ($is_csrf, $msg) = IsPossibleCSRF($ARGS);
+ # 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;
- HTML::Mason::Commands::Abort( $msg );
+ $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 = {
+ auth => $user->GenerateAuthString( $token ),
+ path => $HTML::Mason::Commands::r->path_info,
+ args => $ARGS,
+ };
+
+ $HTML::Mason::Commands::session{'CSRF'}->{$token} = $data;
+ $HTML::Mason::Commands::session{'i'}++;
+
+ $HTML::Mason::Commands::m->comp(
+ '/Elements/CSRF',
+ OriginalURL => $HTML::Mason::Commands::r->path_info,
+ Reason => HTML::Mason::Commands::loc( $msg, @loc ),
+ Token => $token,
+ );
+ # Calls abort, never gets here
}
package HTML::Mason::Commands;
diff --git a/share/html/Elements/CSRF b/share/html/Elements/CSRF
new file mode 100644
index 0000000..891d4f8
--- /dev/null
+++ b/share/html/Elements/CSRF
@@ -0,0 +1,74 @@
+%# BEGIN BPS TAGGED BLOCK {{{
+%#
+%# COPYRIGHT:
+%#
+%# This software is Copyright (c) 1996-2012 Best Practical Solutions, LLC
+%# <sales at bestpractical.com>
+%#
+%# (Except where explicitly superseded by other copyright notices)
+%#
+%#
+%# LICENSE:
+%#
+%# This work is made available to you under the terms of Version 2 of
+%# the GNU General Public License. A copy of that license should have
+%# been provided with this software, but in any event can be snarfed
+%# from www.gnu.org.
+%#
+%# This work is distributed in the hope that it will be useful, but
+%# WITHOUT ANY WARRANTY; without even the implied warranty of
+%# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+%# General Public License for more details.
+%#
+%# You should have received a copy of the GNU General Public License
+%# along with this program; if not, write to the Free Software
+%# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+%# 02110-1301 or visit their web page on the internet at
+%# http://www.gnu.org/licenses/old-licenses/gpl-2.0.html.
+%#
+%#
+%# CONTRIBUTION SUBMISSION POLICY:
+%#
+%# (The following paragraph is not intended to limit the rights granted
+%# to you to modify and distribute this software under the terms of
+%# the GNU General Public License and is only of importance to you if
+%# you choose to contribute your changes and enhancements to the
+%# community by submitting them to Best Practical Solutions, LLC.)
+%#
+%# By intentionally submitting any modifications, corrections or
+%# derivatives to this work, or any other work intended for use with
+%# Request Tracker, to Best Practical Solutions, LLC, you confirm that
+%# you are the copyright holder for those contributions and you grant
+%# Best Practical Solutions, LLC a nonexclusive, worldwide, irrevocable,
+%# royalty-free, perpetual, license to use, copy, create derivative
+%# works based on those contributions, and sublicense and distribute
+%# those contributions and any derivatives thereof.
+%#
+%# END BPS TAGGED BLOCK }}}
+<& /Elements/Header, Title => loc('Possible cross-site request forgery') &>
+<& /Elements/Tabs, Title => loc('Possible cross-site request forgery') &>
+
+<h1><&|/l&>Possible cross-site request forgery</&></h1>
+
+% my $strong_start = "<strong>";
+% my $strong_end = "</strong>";
+<p><&|/l, $strong_start, $strong_end, $Reason &>RT has detected a possible [_1]cross-site request forgery[_2] for this request, because [_3]. This is possibly caused by a malicious attacker trying to perform actions against RT on your behalf. If you did not initiate this request, then you should alert your security team.</&></p>
+
+% my $start = qq|<strong><a href="$url_with_token">|;
+% my $end = qq|</a></strong>|;
+<p><&|/l, $escaped_path, $start, $end &>If you really intended to visit [_1], then [_2]click here to resume your request[_3].</&></p>
+
+<& /Elements/Footer, %ARGS &>
+% $m->abort;
+<%ARGS>
+$OriginalURL => ''
+$Reason => ''
+$Token => ''
+</%ARGS>
+<%INIT>
+my $escaped_path = $m->interp->apply_escapes($OriginalURL, 'h');
+$escaped_path = "<tt>$escaped_path</tt>";
+
+my $url_with_token = URI->new($OriginalURL);
+$url_with_token->query_form([CSRF_Token => $Token]);
+</%INIT>
diff --git a/t/web/csrf.t b/t/web/csrf.t
index d170354..6704269 100644
--- a/t/web/csrf.t
+++ b/t/web/csrf.t
@@ -7,10 +7,13 @@ use RT::Test tests => undef;
my $ticket = RT::Ticket->new(RT::CurrentUser->new('root'));
my ($ok, $msg) = $ticket->Create(Queue => 1, Owner => 'nobody', Subject => 'bad music');
ok($ok);
+my $other = RT::Test->load_or_create_queue(Name => "Other queue", Disabled => 0);
+my $other_queue_id = $other->id;
my ($baseurl, $m) = RT::Test->started_ok;
my $test_page = "/Ticket/Create.html?Queue=1";
+my $test_path = "/Ticket/Create.html";
$m->get_ok($baseurl);
ok $m->login, 'logged in';
@@ -18,25 +21,102 @@ ok $m->login, 'logged in';
# valid referer
$m->add_header(Referer => $baseurl);
$m->get_ok($test_page);
-$m->content_lacks("Referer is unknown site");
+$m->content_lacks("Possible cross-site request forgery");
$m->title_is('Create a new ticket');
# now send a referer from an attacker
$m->add_header(Referer => 'http://example.net');
$m->get_ok($test_page);
-$m->content_contains("Referer is unknown site");
-$m->warning_like(qr/Referer is unknown site/);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Create.html</tt>");
+$m->content_contains("the Referrer header supplied by your browser (example.net:80) is not allowed");
+$m->title_is('Possible cross-site request forgery');
+
+# reinstate mech's usual header policy
+$m->delete_header('Referer');
+
+# clicking the resume request button gets us to the test page
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+like($m->response->request->uri, qr{^http://[^/]+\Q$test_path\E\?CSRF_Token=\w+$});
+$m->title_is('Create a new ticket');
# try a whitelisted argument from an attacker
$m->add_header(Referer => 'http://example.net');
$m->get_ok("/Ticket/Display.html?id=1");
-$m->content_lacks("Referer is unknown site");
+$m->content_lacks("Possible cross-site request forgery");
$m->title_is('#1: bad music');
# now a non-whitelisted argument
$m->get_ok("/Ticket/Display.html?id=1&Action=Take");
-$m->content_contains("Referer is unknown site");
-$m->warning_like(qr/Referer is unknown site/);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Display.html</tt>");
+$m->content_contains("the Referrer header supplied by your browser (example.net:80) is not allowed");
+$m->title_is('Possible cross-site request forgery');
+
+$m->delete_header('Referer');
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+like($m->response->request->uri, qr{^http://[^/]+\Q/Ticket/Display.html});
+$m->title_is('#1: bad music');
+$m->content_contains('Owner changed from Nobody to root');
+
+# force mech to never set referer
+$m->add_header(Referer => undef);
+$m->get_ok($test_page);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Create.html</tt>");
+$m->content_contains("your browser did not supply a Referrer header");
+$m->title_is('Possible cross-site request forgery');
+
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+is($m->response->redirects, 0, "no redirection");
+like($m->response->request->uri, qr{^http://[^/]+\Q$test_path\E\?CSRF_Token=\w+$});
+$m->title_is('Create a new ticket');
+
+# try sending the wrong csrf token, then the right one
+$m->add_header(Referer => undef);
+$m->get_ok($test_page);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Create.html</tt>");
+$m->content_contains("your browser did not supply a Referrer header");
+$m->title_is('Possible cross-site request forgery');
+
+# Sending a wrong CSRF is just a normal request. We'll make a request
+# with just an invalid token, which means no Queue=, which means
+# Create.html errors out.
+my $link = $m->find_link(text_regex => qr{resume your request});
+(my $broken_url = $link->url) =~ s/(CSRF_Token)=\w+/$1=crud/;
+$m->get_ok($broken_url);
+$m->content_contains("Queue could not be loaded");
+$m->title_is('RT Error');
+$m->warning_like(qr/Queue could not be loaded/);
+
+# The token doesn't work for other pages, or other arguments to the same page.
+$m->add_header(Referer => undef);
+$m->get_ok($test_page);
+$m->content_contains("Possible cross-site request forgery");
+my ($token) = $m->content =~ m{CSRF_Token=(\w+)};
+
+$m->add_header(Referer => undef);
+$m->get_ok("/Admin/Queues/Modify.html?id=new&Name=test&CSRF_Token=$token");
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Admin/Queues/Modify.html</tt>");
+$m->content_contains("your browser did not supply a Referrer header");
+$m->title_is('Possible cross-site request forgery');
+
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+$m->title_is('Editing Configuration for queue test');
+
+# Try the same page, but different query parameters, which are blatted by the token
+$m->get_ok("/Ticket/Create.html?Queue=$other_queue_id&CSRF_Token=$token");
+$m->content_lacks("Possible cross-site request forgery");
+$m->title_is('Create a new ticket');
+$m->text_unlike(qr/Queue:\s*Other queue/);
+$m->text_like(qr/Queue:\s*General/);
+
# now try self-service with CSRF
@@ -52,7 +132,16 @@ $m->content_contains("My open tickets", "got self-service interface");
# post without referer
$m->add_header(Referer => undef);
$m->get_ok("/SelfService/Create.html?Queue=1");
-$m->content_contains("No Referer header");
-$m->warning_like(qr/No Referer header/);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/SelfService/Create.html</tt>");
+$m->content_contains("your browser did not supply a Referrer header");
+$m->title_is('Possible cross-site request forgery');
+
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+is($m->response->redirects, 0, "no redirection");
+like($m->response->request->uri, qr{^http://[^/]+\Q/SelfService/Create.html\E\?CSRF_Token=\w+$});
+$m->title_is('Create a ticket');
+$m->content_contains('Describe the issue below:');
undef $m;
commit 40be851ec9b9ecdb48cc9bf250a2832de8ddf1d0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 17 17:23:44 2011 -0500
Ensure that publicly cachable content does not contain Set-Cookie headers
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b3f593a..e8ce3d8 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -734,6 +734,10 @@ sub StaticFileHeaders {
# make cache public
$HTML::Mason::Commands::r->headers_out->{'Cache-Control'} = 'max-age=259200, public';
+ # remove any cookie headers -- if it is cached publicly, it
+ # shouldn't include anyone's cookie!
+ delete $HTML::Mason::Commands::r->err_headers_out->{'Set-Cookie'};
+
# Expire things in a month.
$date->Set( Value => time + 30 * 24 * 60 * 60 );
$HTML::Mason::Commands::r->headers_out->{'Expires'} = $date->RFC2616;
commit d9c47864b8068e8f524118a8a698f23eb0523c8d
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Nov 16 14:19:19 2011 -0500
Only run known formatters in RT::Date
To make a formatter known to RT, you should:
push @RT::Date::FORMATTERS, 'YourFormatter';
This resolves part of CVE-2011-4458.
diff --git a/lib/RT/Date.pm b/lib/RT/Date.pm
index 2b6a3e3..fe62c30 100755
--- a/lib/RT/Date.pm
+++ b/lib/RT/Date.pm
@@ -545,6 +545,10 @@ sub Get
my $self = shift;
my %args = (Format => 'ISO', @_);
my $formatter = $args{'Format'};
+ unless ( $self->ValidFormatter($formatter) ) {
+ RT->Logger->warning("Invalid date formatter '$formatter', falling back to ISO");
+ $formatter = 'ISO';
+ }
$formatter = 'ISO' unless $self->can($formatter);
return $self->$formatter( %args );
}
@@ -583,6 +587,20 @@ sub Formatters
return @FORMATTERS;
}
+=head3 ValidFormatter FORMAT
+
+Returns a true value if C<FORMAT> is a known formatter. Otherwise returns
+false.
+
+=cut
+
+sub ValidFormatter {
+ my $self = shift;
+ my $format = shift;
+ return (grep { $_ eq $format } $self->Formatters and $self->can($format))
+ ? 1 : 0;
+}
+
=head3 DefaultFormat
=cut
commit 8283903fdb1984c9d04722a8e0f9539e00ebf53f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Nov 17 15:21:10 2011 -0500
Require valid names for the format methods called by LocalizedDateTime
This only matches valid method names, so you can still shoot yourself in
the foot by passing something like time_format_bogus. There are too
many aliases in DateTime::Locale::Base to implement better sanity
checks.
This resolves part of CVE-2011-4458.
diff --git a/lib/RT/Date.pm b/lib/RT/Date.pm
index fe62c30..e02d631 100755
--- a/lib/RT/Date.pm
+++ b/lib/RT/Date.pm
@@ -663,8 +663,8 @@ sub LocalizedDateTime
my %args = ( Date => 1,
Time => 1,
Timezone => '',
- DateFormat => 'date_format_full',
- TimeFormat => 'time_format_medium',
+ DateFormat => '',
+ TimeFormat => '',
AbbrDay => 1,
AbbrMonth => 1,
@_,
@@ -675,9 +675,12 @@ sub LocalizedDateTime
return $self->loc("DateTime doesn't support format_cldr, you must upgrade to use this feature")
unless can DateTime::('format_cldr');
+ # Require valid names for the format methods
+ my $date_format = $args{DateFormat} =~ /^\w+$/
+ ? $args{DateFormat} : 'date_format_full';
- my $date_format = $args{'DateFormat'};
- my $time_format = $args{'TimeFormat'};
+ my $time_format = $args{TimeFormat} =~ /^\w+$/
+ ? $args{TimeFormat} : 'time_format_medium';
my $lang = $self->CurrentUser->UserObj->Lang;
unless ($lang) {
commit f96ce669d98ce016f2340fd2286fd14dd6edc80b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Nov 17 17:30:08 2011 -0500
Validate the requested link types when graphing relationships
Otherwise you can call bogus methods on the ticket that don't result in
an RT::Links object.
This resolves part of CVE-2011-4458.
diff --git a/lib/RT/Graph/Tickets.pm b/lib/RT/Graph/Tickets.pm
index cab4299..9902467 100644
--- a/lib/RT/Graph/Tickets.pm
+++ b/lib/RT/Graph/Tickets.pm
@@ -282,6 +282,14 @@ sub TicketLinks {
ShowLinkDescriptions => 0,
@_
);
+
+ my %valid_links = map { $_ => 1 }
+ qw(Members MemberOf RefersTo ReferredToBy DependsOn DependedOnBy);
+
+ # Validate our link types
+ $args{ShowLinks} = [ grep { $valid_links{$_} } @{$args{ShowLinks}} ];
+ $args{LeadingLink} = 'Members' unless $valid_links{ $args{LeadingLink} };
+
unless ( $args{'Graph'} ) {
$args{'Graph'} = GraphViz->new(
name => 'ticket_links_'. $args{'Ticket'}->id,
commit bb917e0b5a3a5797cc0929211db808e6d9303f9a
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 11:07:13 2012 -0500
Explicitly override any Graph parameter passed into RT::Graph::Tickets
Specifying a defined Graph argument to RT::Graph::Tickets->TicketLinks
is only used internally when it is called recursively. Since Graph is
expected to be an existing GraphViz object if defined, it never makes
sense to start with anything but an undefined Graph parameter.
This prevents a user-supplied Graph parameter from having ->add_node
called on it. Since the Graph parameter could contain a Perl package
name, it previously provided a means to call to ->add_node on arbitrary
Perl packages already loaded into memory. While of unlikely utility,
there's no reason to allow such behaviour.
Fixes part of CVE-2011-4458.
diff --git a/share/html/Ticket/Graphs/Elements/ShowGraph b/share/html/Ticket/Graphs/Elements/ShowGraph
index 1d905c7..f4c07d5 100644
--- a/share/html/Ticket/Graphs/Elements/ShowGraph
+++ b/share/html/Ticket/Graphs/Elements/ShowGraph
@@ -66,6 +66,7 @@ $ARGS{'id'} = $id = $ticket->id;
require RT::Graph::Tickets;
my $graph = RT::Graph::Tickets->TicketLinks(
%ARGS,
+ Graph => undef,
Ticket => $ticket,
);
</%INIT>
diff --git a/share/html/Ticket/Graphs/dhandler b/share/html/Ticket/Graphs/dhandler
index a1dfebe..1335ed5 100644
--- a/share/html/Ticket/Graphs/dhandler
+++ b/share/html/Ticket/Graphs/dhandler
@@ -65,6 +65,7 @@ unless ( $ticket->id ) {
require RT::Graph::Tickets;
my $graph = RT::Graph::Tickets->TicketLinks(
%ARGS,
+ Graph => undef,
Ticket => $ticket,
);
commit 1f71f5df36140e2239ae82bebfc7237eab34dc0f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Jan 4 14:51:26 2012 -0500
Prevent user-controlled partial component paths from walking up directories
Mason didn't let through paths outside of it's component roots, but it
did mean private components were accessible semi-directly.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b3f593a..9498fdf 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -745,6 +745,22 @@ sub StaticFileHeaders {
# $HTML::Mason::Commands::r->headers_out->{'Last-Modified'} = $date->RFC2616;
}
+=head2 ComponentPathIsSafe PATH
+
+Takes C<PATH> and returns a boolean indicating that the user-specified partial
+component path is safe.
+
+Currently "safe" means that the path does not start with a dot (C<.>) and does
+not contain a slash-dot C</.>.
+
+=cut
+
+sub ComponentPathIsSafe {
+ my $self = shift;
+ my $path = shift;
+ return $path !~ m{(?:^|/)\.};
+}
+
=head2 PathIsSafe
Takes a C<< Path => path >> and returns a boolean indicating that
diff --git a/share/html/Elements/ShowUser b/share/html/Elements/ShowUser
index 6381594..27f2358 100644
--- a/share/html/Elements/ShowUser
+++ b/share/html/Elements/ShowUser
@@ -51,7 +51,7 @@
# $Address is Email::Address object
my $comp = '/Elements/ShowUser'. ucfirst lc $style;
-unless ( $m->comp_exists( $comp ) ) {
+unless ( RT::Interface::Web->ComponentPathIsSafe($comp) and $m->comp_exists( $comp ) ) {
$RT::Logger->error(
'Either system config or user #'
. $session{'CurrentUser'}->id
commit 90650ebd9e5316b3a8f6b6b8992a1b810f8db09b
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:29:37 2012 -0400
Allow file uploads to persist across CSRF interstitial
While storing request parameters into the session is sufficient for most
requests, it does not cover the case of file uploads, when the request
arguments contain a filehandle, and thus cannot be cleanly serialized.
Deal with this, in the case of ticket attachments, by parsing and
storing the attachment in the CSRF session data using MakeMIMEEntity, as
the request would usually have done. On the event of the CSRF token
being used, extract the parsed attachment and insert it into the
canonical session storage for attachments.
This does not cover the case of custom field file uploads, as the latter
are never stored in the session, and are much harder to isolate.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b236751..fb962c0 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1062,6 +1062,16 @@ sub ExpandCSRFToken {
%{$ARGS} = %{$data->{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
+ # the session now, so they'll be visible.
+ if ($data->{attach}) {
+ my $filename = $data->{attach}{filename};
+ my $mime = $data->{attach}{mime};
+ $HTML::Mason::Commands::session{'Attachments'}{$filename}
+ = $mime;
+ }
+
return 1;
}
@@ -1087,6 +1097,14 @@ sub MaybeShowInterstitialCSRFPage {
path => $HTML::Mason::Commands::r->path_info,
args => $ARGS,
};
+ if ($ARGS->{Attach}) {
+ my $attachment = HTML::Mason::Commands::MakeMIMEEntity( AttachmentFieldName => 'Attach' );
+ my $file_path = delete $ARGS->{'Attach'};
+ $data->{attach} = {
+ filename => Encode::decode_utf8("$file_path"),
+ mime => $attachment,
+ };
+ }
$HTML::Mason::Commands::session{'CSRF'}->{$token} = $data;
$HTML::Mason::Commands::session{'i'}++;
diff --git a/t/web/csrf.t b/t/web/csrf.t
index 6704269..fe8a83e 100644
--- a/t/web/csrf.t
+++ b/t/web/csrf.t
@@ -3,6 +3,7 @@ use strict;
use warnings;
use RT::Test tests => undef;
+$RT::Test::SKIP_REQUEST_WORK_AROUND = 1;
my $ticket = RT::Ticket->new(RT::CurrentUser->new('root'));
my ($ok, $msg) = $ticket->Create(Queue => 1, Owner => 'nobody', Subject => 'bad music');
@@ -117,6 +118,29 @@ $m->title_is('Create a new ticket');
$m->text_unlike(qr/Queue:\s*Other queue/);
$m->text_like(qr/Queue:\s*General/);
+# Ensure that file uploads work across the interstitial
+$m->delete_header('Referer');
+$m->get_ok($test_page);
+$m->content_contains("Create a new ticket", 'ticket create page');
+$m->form_name('TicketCreate');
+$m->field('Subject', 'Attachments test');
+
+my $logofile = "$RT::MasonComponentRoot/NoAuth/images/bplogo.gif";
+open LOGO, "<", $logofile or die "Can't open logo file: $!";
+binmode LOGO;
+my $logo_contents = do {local $/; <LOGO>};
+close LOGO;
+$m->field('Attach', $logofile);
+
+# Lose the referer before the POST
+$m->add_header(Referer => undef);
+$m->submit;
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Create.html</tt>");
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_contains('Download bplogo.gif', 'page has file name');
+$m->follow_link_ok({text => "Download bplogo.gif"});
+is($m->content, $logo_contents, "Binary content matches");
# now try self-service with CSRF
commit d0a37b0ee1bc38f7eda0ae0f155c52fef5996f73
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:31:19 2012 -0400
Add optional CSRF login protection
RT has historically allowed logins from any location by simply providing
'user' and 'pass' parameters, which were used to log the user in. This
opens RT to "login CSRF," wherein a user is tricked into logging in
using the _attacker_'s credentials, which allows the attacker to later
examine their own account and possibly extract information about the
user's browsing history.
As RT does not track or display such information about the user, and the
ability to provide URLs which log a user into RT is central to many
workflows, explicitly whitelist 'user' and 'pass' parameters to RT for
CSRF purposes, if (and only if) they authenticate correctly. If so, it
is also sufficient to whitelist the entire request, as it proves that
the requesting entity possesses the user's full credentials, and would
be able to take any action regardless.
This exception will not function if RT is configured to authenticate
against a source other than its internal database, such as any of those
provided by RT::Authen::ExternalAuth, or via the $ExternalAuth
configuration setting, as RT has no way of verifying the credentials
provided via the query parameters.
If login CSRF is deemed a potential issue locally, we provide a
configuration option ($RestrictLoginReferrer) to enable login CSRF
protection.
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index ce37aca..b9cedc7 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1259,6 +1259,18 @@ cross-site request forgery (CSRF) attacks.
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<user> and C<pass> 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);
+
=item C<$WebFlushDbCacheEveryRequest>
By default, RT clears its database cache after every page view.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index fb962c0..e780fdb 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -998,6 +998,20 @@ sub IsCompCSRFWhitelisted {
my %args = %{ $ARGS };
+ # If the user specifies a *correct* user and pass then they are
+ # golden. This acts on the presumption that external forms may
+ # hardcode a username and password -- if a malicious attacker knew
+ # both already, CSRF is the least of your problems.
+ my $AllowLoginCSRF = not RT->Config->Get('RestrictReferrerLogin');
+ if ($AllowLoginCSRF and defined($args{user}) and defined($args{pass})) {
+ my $user_obj = RT::CurrentUser->new();
+ $user_obj->Load($args{user});
+ return 1 if $user_obj->id && $user_obj->IsPassword($args{pass});
+
+ delete $args{user};
+ delete $args{pass};
+ }
+
# Eliminate arguments that do not indicate an effectful request.
# For example, "id" is acceptable because that is how RT retrieves a
# record.
diff --git a/t/web/csrf.t b/t/web/csrf.t
index fe8a83e..eb34168 100644
--- a/t/web/csrf.t
+++ b/t/web/csrf.t
@@ -16,7 +16,6 @@ my ($baseurl, $m) = RT::Test->started_ok;
my $test_page = "/Ticket/Create.html?Queue=1";
my $test_path = "/Ticket/Create.html";
-$m->get_ok($baseurl);
ok $m->login, 'logged in';
# valid referer
@@ -25,6 +24,18 @@ $m->get_ok($test_page);
$m->content_lacks("Possible cross-site request forgery");
$m->title_is('Create a new ticket');
+# off-site referer BUT provides auth
+$m->add_header(Referer => 'http://example.net');
+$m->get_ok("$test_page&user=root&pass=password");
+$m->content_lacks("Possible cross-site request forgery");
+$m->title_is('Create a new ticket');
+
+# explicitly no referer BUT provides auth
+$m->add_header(Referer => undef);
+$m->get_ok("$test_page&user=root&pass=password");
+$m->content_lacks("Possible cross-site request forgery");
+$m->title_is('Create a new ticket');
+
# now send a referer from an attacker
$m->add_header(Referer => 'http://example.net');
$m->get_ok($test_page);
@@ -148,7 +159,6 @@ my $user = RT::User->new(RT->SystemUser);
$user->Create(Name => "SelfService", Password => "chops", Privileged => 0);
$m = RT::Test::Web->new;
-$m->get_ok($baseurl);
$m->get_ok("$baseurl/index.html?user=SelfService&pass=chops");
$m->title_is("Open tickets", "got self-service interface");
$m->content_contains("My open tickets", "got self-service interface");
commit cccfb9c04fa271ff05128749ce99713e50b2da23
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:38:01 2012 -0400
Allow REST requests to function regardless of Referer header
The REST interface, and existing clients to it, expect to be able to
make queries without supplying a Referer parameter -- which CSRF
protections should rightly deny. While the simple solution would be to
include a Referer header in such requests, we cannot assume that all
REST clients can be so updated.
Create a secondary class of session, the REST session, which is allowed
to make Referer-less state-modifying requests, but not access the rest
of the UI. We recognize such sessions by if their initial request is to
a REST URL; if so, we mark the session as a REST session. For clients
with existing sessions which are not flagged as REST or not, we assign
them into the appropriate class based on the first request we observe;
this will allow existing clients with long-running sessions to continue
unmodified.
This also removes a SessionType check which became obsolete in 54f0f73,
and re-implements the functionality using the new REST flag on the
session.
diff --git a/bin/rt-mailgate.in b/bin/rt-mailgate.in
index c1a57cb..d2daa95 100755
--- a/bin/rt-mailgate.in
+++ b/bin/rt-mailgate.in
@@ -81,9 +81,7 @@ unless ( $opts{'url'} ) {
my $ua = new LWP::UserAgent;
$ua->cookie_jar( { file => $opts{'jar'} } ) if $opts{'jar'};
-my %args = (
- SessionType => 'REST', # Surpress login box
-);
+my %args;
foreach ( qw(queue action) ) {
$args{$_} = $opts{$_} if defined $opts{$_};
};
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index e780fdb..9515ea1 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1042,6 +1042,29 @@ sub IsRefererCSRFWhitelisted {
sub IsPossibleCSRF {
my $ARGS = shift;
+ # If first request on this session is to a REST endpoint, then
+ # whitelist the REST endpoints -- and explicitly deny non-REST
+ # endpoints. We do this because using a REST cookie in a browser
+ # would open the user to CSRF attacks to the REST endpoints.
+ my $path = $HTML::Mason::Commands::r->path_info;
+ $HTML::Mason::Commands::session{'REST'} = $path =~ m{^/+REST/\d+\.\d+(/|$)}
+ unless defined $HTML::Mason::Commands::session{'REST'};
+
+ if ($HTML::Mason::Commands::session{'REST'}) {
+ return 0 if $path =~ m{^/+REST/\d+\.\d+(/|$)};
+ my $why = <<EOT;
+This login session belongs to a REST client, and cannot be used to
+access non-REST interfaces of RT for security reasons.
+EOT
+ my $details = <<EOT;
+Please log out and back in to obtain a session for normal browsing. If
+you understand the security implications, disabling RT's CSRF protection
+will remove this restriction.
+EOT
+ chomp $details;
+ HTML::Mason::Commands::Abort( $why, Details => $details );
+ }
+
return 0 if IsCompCSRFWhitelisted(
$HTML::Mason::Commands::m->request_comp->path,
$ARGS
diff --git a/share/html/Elements/Error b/share/html/Elements/Error
index 8459373..14eb2c4 100755
--- a/share/html/Elements/Error
+++ b/share/html/Elements/Error
@@ -81,7 +81,7 @@ Encode::_utf8_off($error);
$RT::Logger->error($error);
-if ( defined $session{'SessionType'} && $session{'SessionType'} eq 'REST' ) {
+if ( $session{'REST'} ) {
$r->content_type('text/plain');
$m->out( "Error: " . $Why . "\n" );
$m->out( $Details . "\n" ) if defined $Details && length $Details;
diff --git a/t/web/csrf-rest.t b/t/web/csrf-rest.t
new file mode 100644
index 0000000..a2b6448
--- /dev/null
+++ b/t/web/csrf-rest.t
@@ -0,0 +1,76 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+
+use RT::Test tests => undef;
+
+my ($baseurl, $m) = RT::Test->started_ok;
+
+# Get a non-REST session
+diag "Standard web session";
+ok $m->login, 'logged in';
+$m->content_contains("RT at a glance", "Get full UI content");
+
+# Requesting a REST page should be fine, as we have a Referer
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request with referrer");
+
+# Removing the Referer header gets us an interstitial
+$m->add_header(Referer => undef);
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ format => 'l',
+ foo => 'bar',
+]);
+$m->content_contains("Possible cross-site request forgery",
+ "REST request without referrer is blocked");
+
+# But passing username and password lets us though
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ user => 'root',
+ pass => 'password',
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request without referrer, but username/password supplied, is OK");
+
+# And we can still access non-REST urls
+$m->get("$baseurl");
+$m->content_contains("RT at a glance", "Full UI is still available");
+
+
+# Now go get a REST session
+diag "REST session";
+$m = RT::Test::Web->new;
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ user => 'root',
+ pass => 'password',
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request to log in");
+
+# Requesting that page again, with a username/password but no referrer,
+# is fine
+$m->add_header(Referer => undef);
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ user => 'root',
+ pass => 'password',
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request with no referrer, but username/pass");
+
+# And it's still fine without both referer and username and password,
+# because REST is special-cased
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request with no referrer or username/pass is special-cased for REST sessions");
+
+# But the REST page can't request normal pages
+$m->get("$baseurl");
+$m->content_lacks("RT at a glance", "Full UI is denied for REST sessions");
+$m->content_contains("This login session belongs to a REST client", "Tells you why");
+$m->warning_like(qr/This login session belongs to a REST client/, "Logs a warning");
+
+undef $m;
+
diff --git a/t/web/redirect-after-login.t b/t/web/redirect-after-login.t
index d39bb58..f97f126 100644
--- a/t/web/redirect-after-login.t
+++ b/t/web/redirect-after-login.t
@@ -205,16 +205,17 @@ for my $path (qw(Prefs/Other.html /Prefs/Other.html)) {
# test REST login response
{
+ $agent = RT::Test::Web->new;
my $requested = $url."REST/1.0/?user=root;pass=password";
$agent->get($requested);
is($agent->status, 200, "Loaded a page");
is($agent->uri, $requested, "didn't redirect to /NoAuth/Login.html for REST");
- $agent->get_ok($url);
- $agent->logout();
+ $agent->get_ok($url."REST/1.0");
}
# test REST login response for wrong pass
{
+ $agent = RT::Test::Web->new;
my $requested = $url."REST/1.0/?user=root;pass=passwrong";
$agent->get_ok($requested);
is($agent->status, 200, "Loaded a page");
@@ -229,6 +230,7 @@ for my $path (qw(Prefs/Other.html /Prefs/Other.html)) {
# test REST login response for no creds
{
+ $agent = RT::Test::Web->new;
my $requested = $url."REST/1.0/";
$agent->get_ok($requested);
is($agent->status, 200, "Loaded a page");
commit 00593b893332714d7288ab683e270003471e35a8
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Jan 4 16:51:05 2012 -0500
Make CheckIntegrity idempotent on a running install
Avoid reconnecting to the database if we have a handle and stop trying
to initialize the logger. These changes make it safe to call as many
times as you'd like on a running instance without affecting the app.
Specifically, CheckIntegrity is now safely callable from places other
than server initialization.
Note that any calls to CheckIntegrity must now come _after_
InitLogging() is called. In RT 3, CheckIntegrity is called in
rt-server and standalone_httpd.
diff --git a/lib/RT/Handle.pm b/lib/RT/Handle.pm
index 38905de..bb61429 100755
--- a/lib/RT/Handle.pm
+++ b/lib/RT/Handle.pm
@@ -239,8 +239,9 @@ sub CheckIntegrity {
return (0, 'no connection', "Failed to connect to $dsn as user '$user': ". $DBI::errstr);
}
- RT::ConnectToDatabase();
- RT::InitLogging();
+ unless ($RT::Handle and $RT::Handle->dbh) {
+ RT::ConnectToDatabase();
+ }
require RT::CurrentUser;
my $test_user = new RT::CurrentUser;
commit b784bcb5779ca6315717a5bbb9c554f0a28ecb6b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Jan 4 16:56:59 2012 -0500
Refuse to turn on InstallMode when we have database integrity
This prevents install mode from activating on a working install and
resolves part of CVE-2011-4458.
diff --git a/lib/RT.pm.in b/lib/RT.pm.in
index 623de66..5275ac8 100755
--- a/lib/RT.pm.in
+++ b/lib/RT.pm.in
@@ -678,11 +678,21 @@ sub InitPlugins {
sub InstallMode {
my $self = shift;
if (@_) {
- $_INSTALL_MODE = shift;
- if($_INSTALL_MODE) {
- require RT::CurrentUser;
- $SystemUser = RT::CurrentUser->new();
- }
+ my ($integrity, $state, $msg) = RT::Handle->CheckIntegrity;
+ if ($_[0] and $integrity) {
+ # Trying to turn install mode on but we have a good DB!
+ require Carp;
+ $RT::Logger->error(
+ Carp::longmess("Something tried to turn on InstallMode but we have DB integrity!")
+ );
+ }
+ else {
+ $_INSTALL_MODE = shift;
+ if($_INSTALL_MODE) {
+ require RT::CurrentUser;
+ $SystemUser = RT::CurrentUser->new();
+ }
+ }
}
return $_INSTALL_MODE;
}
commit 5170d9057c4060a8a9be422f947ad450d5db100e
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Nov 14 19:00:35 2011 -0500
Iterate attachments as the creator of the current transaction when sending mail
Check CurrentUserCanSee before trying to add an attachment since it
could otherwise end up empty now that we have the correct current user.
Additionally, simply check RT::Transaction's CurrentUserCanSee when
iterating inside an RT::Attachments object rather than maintaining a
different but similar conditional tree. CurrentUserCanSee correctly
access checks transaction types like EmailRecord, for example.
This resolves part of CVE-2011-2084.
diff --git a/lib/RT/Action/SendEmail.pm b/lib/RT/Action/SendEmail.pm
index 9e93e4a..b61e3f3 100755
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -349,7 +349,7 @@ sub AddAttachments {
$MIMEObj->head->delete('RT-Attach-Message');
- my $attachments = RT::Attachments->new($RT::SystemUser);
+ my $attachments = RT::Attachments->new( RT::CurrentUser->new($self->TransactionObj->Creator) );
$attachments->Limit(
FIELD => 'TransactionId',
VALUE => $self->TransactionObj->Id
@@ -409,6 +409,10 @@ sub AddAttachment {
my $attach = shift;
my $MIMEObj = shift || $self->TemplateObj->MIMEObj;
+ # $attach->TransactionObj may not always be $self->TransactionObj
+ return unless $attach->Id
+ and $attach->TransactionObj->CurrentUserCanSee;
+
$MIMEObj->attach(
Type => $attach->ContentType,
Charset => $attach->OriginalEncoding,
@@ -467,8 +471,7 @@ sub AddTicket {
my $self = shift;
my $tid = shift;
- # XXX: we need a current user here, but who is current user?
- my $attachs = RT::Attachments->new($RT::SystemUser);
+ my $attachs = RT::Attachments->new( RT::CurrentUser->new($self->TransactionObj->Creator) );
my $txn_alias = $attachs->TransactionAlias;
$attachs->Limit( ALIAS => $txn_alias, FIELD => 'Type', VALUE => 'Create' );
$attachs->Limit(
diff --git a/lib/RT/Attachments_Overlay.pm b/lib/RT/Attachments_Overlay.pm
index d758c76..12fc88b 100755
--- a/lib/RT/Attachments_Overlay.pm
+++ b/lib/RT/Attachments_Overlay.pm
@@ -227,15 +227,12 @@ sub Next {
my $Attachment = $self->SUPER::Next;
return $Attachment unless $Attachment;
- my $txn = $Attachment->TransactionObj;
- if ( $txn->__Value('Type') eq 'Comment' ) {
- return $Attachment if $txn->CurrentUserHasRight('ShowTicketComments');
- } elsif ( $txn->CurrentUserHasRight('ShowTicket') ) {
+ if ( $Attachment->TransactionObj->CurrentUserCanSee ) {
return $Attachment;
+ } else {
+ # If the user doesn't have the right to show this ticket
+ return $self->Next;
}
-
- # If the user doesn't have the right to show this ticket
- return $self->Next;
}
# }}}
commit 01cecdeeca3375402ee29e92683100f6b24e139d
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 26 14:58:20 2012 -0400
Forbid javascript: and data: ticket links to avoid clickable XSS vectors
This partially resolves CVE-2011-2083. (#58605)
diff --git a/lib/RT/URI.pm b/lib/RT/URI.pm
index facce04..03489ad 100755
--- a/lib/RT/URI.pm
+++ b/lib/RT/URI.pm
@@ -132,7 +132,7 @@ sub FromURI {
# Special case: integers passed in as URIs must be ticket ids
if ($uri =~ /^(\d+)$/) {
$scheme = "fsck.com-rt";
- } elsif ($uri =~ /^((?:\w|\.|-)+?):/) {
+ } elsif ($uri =~ /^((?!javascript|data)(?:\w|\.|-)+?):/i) {
$scheme = $1;
}
else {
commit cfd4d893e92e4fe23615d4cd4724803c0a0804cf
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Thu May 5 14:23:24 2011 -0400
Explicitly pass the type of escaping we want to apply_escapes
diff --git a/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage b/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
index bae4685..b864724 100644
--- a/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
+++ b/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
@@ -55,5 +55,5 @@ $Path => ''
Title => 'Error',
&>
<div class="error">
-<% loc('Shredder needs a directory to write dumps to. Please check that you have <span class="file-path">[_1]</span> and it is writable by your web server.', $m->interp->apply_escapes( $Path ) ) |n%>
+<% loc('Shredder needs a directory to write dumps to. Please check that you have <span class="file-path">[_1]</span> and it is writable by your web server.', $m->interp->apply_escapes( $Path, 'h' ) ) |n%>
</div>
diff --git a/share/html/index.html b/share/html/index.html
index 49d524a..39459af 100755
--- a/share/html/index.html
+++ b/share/html/index.html
@@ -125,7 +125,7 @@ if ( $ARGS{'QuickCreate'} ) {
if ( $ARGS{'q'} ) {
- RT::Interface::Web::Redirect(RT->Config->Get('WebURL')."Search/Simple.html?q=".$m->interp->apply_escapes($ARGS{q}));
+ RT::Interface::Web::Redirect(RT->Config->Get('WebURL')."Search/Simple.html?q=".$m->interp->apply_escapes($ARGS{q}, 'u'));
}
my $actions;
commit d17991d80002d65b2d9e98366a550d49ad5232c7
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Mar 30 18:36:02 2012 -0400
Ensure that the new /l_unsafe is protected from direct access as well
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b3f593a..5aa6adf 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -436,7 +436,7 @@ sub MaybeRejectPrivateComponentRequest {
_elements | # mobile UI
Widgets |
autohandler | # requesting this directly is suspicious
- l ) # loc component
+ l (_unsafe)? ) # loc component
( $ | / ) # trailing slash or end of path
}xi) {
$m->abort(403);
commit 951add5ab10a12b0d40cd3a7edf812b524db6ff9
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 11:22:40 2012 -0500
Escape backslashes in text used for GraphViz input
Previously you could avoid the double quote escaping by proceeding your
double quote with a backslash. The backslash naively added by gv_escape
would itself be escaped by your original backslash, leaving the quote
unescaped. This most often simply broke GraphViz output rendering it
unusable.
While injecting arbitrary input into GraphViz is of unlikely utility,
GraphViz did have a buffer overflow vulnerability in versions 2.20.2 and
earlier as described in CVE-2008-4555.
diff --git a/lib/RT/Graph/Tickets.pm b/lib/RT/Graph/Tickets.pm
index cab4299..f51acdf 100644
--- a/lib/RT/Graph/Tickets.pm
+++ b/lib/RT/Graph/Tickets.pm
@@ -104,7 +104,7 @@ EOT
sub gv_escape($) {
my $value = shift;
- $value =~ s{(?=")}{\\}g;
+ $value =~ s{(?=["\\])}{\\}g;
return $value;
}
commit 189b322aa22fa68d45a52504fa6f32ab0e1a2b57
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 26 17:42:23 2012 -0400
Check ACLs on the receiving end when modifying a scrip's Queue or Template
Users with ModifyScrips in Queue A must also have ModifyScrips in the
receiving queue when moving a scrip from one queue to another. When
making a scrip global, the actor must have ModifyScrips globally.
Similarly, users must be able to see the template they're updating the
scrip to use.
This stricter ACL checking prevents queue admins from moving arbitrary
scrips into other queues in which they have no permissions.
Partially resolves CVE-2011-2084. Ticket #50901.
diff --git a/lib/RT/Scrip_Overlay.pm b/lib/RT/Scrip_Overlay.pm
index 3c89f84..db39d95 100755
--- a/lib/RT/Scrip_Overlay.pm
+++ b/lib/RT/Scrip_Overlay.pm
@@ -503,12 +503,42 @@ sub Commit {
# does an acl check and then passes off the call
sub _Set {
my $self = shift;
+ my %args = (
+ Field => undef,
+ Value => undef,
+ @_,
+ );
unless ( $self->CurrentUserHasRight('ModifyScrips') ) {
$RT::Logger->debug(
"CurrentUser can't modify Scrips for " . $self->Queue . "\n" );
return ( 0, $self->loc('Permission Denied') );
}
+
+ if (exists $args{Value}) {
+ if ($args{Field} eq 'Queue') {
+ if ($args{Value}) {
+ # moving to another queue
+ my $queue = RT::Queue->new( $self->CurrentUser );
+ $queue->Load($args{Value});
+ unless ($queue->Id and $queue->CurrentUserHasRight('ModifyScrips')) {
+ return ( 0, $self->loc('Permission Denied') );
+ }
+ } else {
+ # moving to global
+ unless ($self->CurrentUser->HasRight( Object => RT->System, Right => 'ModifyScrips' )) {
+ return ( 0, $self->loc('Permission Denied') );
+ }
+ }
+ }
+ elsif ($args{Field} eq 'Template') {
+ my $template = RT::Template->new( $self->CurrentUser );
+ $template->Load($args{Value});
+ unless ($template->Id and $template->CurrentUserHasQueueRight('ShowTemplate')) {
+ return ( 0, $self->loc('Permission Denied') );
+ }
+ }
+ }
return $self->__Set(@_);
}
commit 4c5657837b3b5972e0a85da0607ff20bfb72892b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Mar 27 12:28:41 2012 -0400
Check ACLs on the receiving end when modifying a Template's Queue
Users with ModifyTemplate in Queue A must also have ModifyTemplate in the
receiving queue when moving a template from one queue to another. When
making a template global, the actor must have ModifyTemplate globally.
This stricter ACL checking prevents queue admins from moving arbitrary
templates into other queues in which they have no permissions.
Partially resolves CVE-2011-2084. Ticket #50901.
diff --git a/lib/RT/Template_Overlay.pm b/lib/RT/Template_Overlay.pm
index e6f6374..21cb97a 100755
--- a/lib/RT/Template_Overlay.pm
+++ b/lib/RT/Template_Overlay.pm
@@ -94,10 +94,34 @@ sub _Accessible {
sub _Set {
my $self = shift;
+ my %args = (
+ Field => undef,
+ Value => undef,
+ @_,
+ );
unless ( $self->CurrentUserHasQueueRight('ModifyTemplate') ) {
return ( 0, $self->loc('Permission Denied') );
}
+
+ if (exists $args{Value}) {
+ if ($args{Field} eq 'Queue') {
+ if ($args{Value}) {
+ # moving to another queue
+ my $queue = RT::Queue->new( $self->CurrentUser );
+ $queue->Load($args{Value});
+ unless ($queue->Id and $queue->CurrentUserHasRight('ModifyTemplate')) {
+ return ( 0, $self->loc('Permission Denied') );
+ }
+ } else {
+ # moving to global
+ unless ($self->CurrentUser->HasRight( Object => RT->System, Right => 'ModifyTemplate' )) {
+ return ( 0, $self->loc('Permission Denied') );
+ }
+ }
+ }
+ }
+
return $self->SUPER::_Set( @_ );
}
commit 7c9cd7c92f7672bcf6b100aa2913d6d0e0e33753
Author: Jesse Vincent <jesse at bestpractical.com>
Date: Tue Aug 17 20:00:26 2010 -0400
Move the meat of ScrubHTML into RT::Interface::Web::ScrubHTML
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b3f593a..e81c53f 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2300,6 +2300,56 @@ sub _parse_saved_search {
return ( _load_container_object( $obj_type, $obj_id ), $search_id );
}
+=head2 ScrubHTML content
+
+Removes unsafe and undesired HTML from the passed content
+
+=cut
+
+my $SCRUBBER;
+sub ScrubHTML {
+ my $Content = shift;
+ $SCRUBBER = _NewScrubber() unless $SCRUBBER;
+
+ $Content = '' if !defined($Content);
+ return $SCRUBBER->scrub($Content);
+}
+
+=head2 _NewScrubber
+
+Returns a new L<HTML::Scrubber> object. Override this if you insist on
+letting more HTML through.
+
+=cut
+
+sub _NewScrubber {
+ require HTML::Scrubber;
+ my $scrubber = HTML::Scrubber->new();
+ $scrubber->default(
+ 0,
+ {
+ '*' => 0,
+ id => 1,
+ class => 1,
+ # Match http, ftp and relative urls
+ # XXX: we also scrub format strings with this module then allow simple config options
+ href => qr{^(?:http:|ftp:|https:|/|__Web(?:Path|BaseURL|URL)__)}i,
+ face => 1,
+ size => 1,
+ target => 1,
+ style => qr{^(?:(?:color:\s*rgb\(\d+,\s*\d+,\s*\d+\))|
+ (?:text-align:\s*))}ix,
+ }
+ );
+ $scrubber->deny(qw[*]);
+ $scrubber->allow(
+ qw[A B U P BR I HR BR SMALL EM FONT SPAN STRONG SUB SUP STRIKE H1 H2 H3 H4 H5 H6 DIV UL OL LI DL DT DD PRE]
+ );
+ $scrubber->comment(0);
+
+ return $scrubber;
+}
+
package RT::Interface::Web;
RT::Base->_ImportOverlays();
diff --git a/share/html/Elements/ScrubHTML b/share/html/Elements/ScrubHTML
index 87aaaf3..5f72d24 100644
--- a/share/html/Elements/ScrubHTML
+++ b/share/html/Elements/ScrubHTML
@@ -45,32 +45,8 @@
%# those contributions and any derivatives thereof.
%#
%# END BPS TAGGED BLOCK }}}
-<%ONCE>
-my $scrubber = new HTML::Scrubber;
-$scrubber->default(
- 0,
- {
- '*' => 0,
- id => 1,
- class => 1,
- # Match http, ftp and relative urls
- # XXX: we also scrub format strings with this module then allow simple config options
- href => qr{^(?:http:|ftp:|https:|/|__Web(?:Path|BaseURL|URL)__)}i,
- face => 1,
- size => 1,
- target => 1,
- style => qr{^(?:(?:color:\s*rgb\(\d+,\s*\d+,\s*\d+\))|
- (?:text-align:\s*))}ix,
- }
-);
-$scrubber->deny(qw[*]);
-$scrubber->allow(
- qw[A B U P BR I HR BR SMALL EM FONT SPAN STRONG SUB SUP STRIKE H1 H2 H3 H4 H5 H6 DIV UL OL LI DL DT DD PRE]
-);
-$scrubber->comment(0);
-</%ONCE>
<%init>
-return $scrubber->scrub($Content);
+return ScrubHTML($Content);
</%init>
<%args>
$Content => undef
diff --git a/share/html/Search/Elements/ResultsRSSView b/share/html/Search/Elements/ResultsRSSView
index f3b416a..e79c51b 100644
--- a/share/html/Search/Elements/ResultsRSSView
+++ b/share/html/Search/Elements/ResultsRSSView
@@ -102,7 +102,7 @@ $r->content_type('application/rss+xml');
# create an RSS 1.0 file (http://purl.org/rss/1.0/)
use XML::RSS;
- my $rss = new XML::RSS (version => '1.0');
+ my $rss = XML::RSS->new(version => '1.0');
$rss->channel(
title => RT->Config->Get('rtname').": Search " . $ARGS{'Query'},
link => RT->Config->Get('WebURL'),
commit f923dbce924c5f3bfc1fc27560fcffe924f07b1c
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Mar 30 19:03:29 2012 -0400
Overhaul what CSS we allow in style attributes to be safer *and* more useful
The regex takes care to accomodate Outlook's idiosyncrasies and ensure
that the features of the rich text editor work.
This is still a large hammer. If there's any forbidden CSS property
specified the entire style attribute will be stripped (including
otherwise allowed properties). The fix for this is parsing the CSS
itself with something like CSS::Adaptor::Whitelist, but that's for
another day.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index e81c53f..566fd3f 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2337,8 +2337,24 @@ sub _NewScrubber {
face => 1,
size => 1,
target => 1,
- style => qr{^(?:(?:color:\s*rgb\(\d+,\s*\d+,\s*\d+\))|
- (?:text-align:\s*))}ix,
+ style => qr{
+ ^(?:\s*
+ (?:(?:background-)?color: \s*
+ (?:rgb\(\s* \d+, \s* \d+, \s* \d+ \s*\) | # rgb(d,d,d)
+ \#[a-f0-9]{3,6} | # #fff or #ffffff
+ [\w\-]+ # green, light-blue, etc.
+ ) |
+ text-align: \s* \w+ |
+ font-size: \s* [\w.\-]+ |
+ font-family: \s* [\w\s"',.\-]+ |
+ font-weight: \s* [\w\-]+ |
+
+ # MS Office styles, which are probably fine. If we don't, then any
+ # associated styles in the same attribute get stripped.
+ mso-[\w\-]+?: \s* [\w\s"',.\-]+
+ )\s* ;? \s*)
+ +$ # one or more of these allowed properties from here 'till sunset
+ }ix,
}
);
$scrubber->deny(qw[*]);
commit 3570e453da31c9cc29ef32aff4c10df5987eeb27
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Mar 8 15:07:06 2011 -0500
Allow blockquotes in our HTML so quote folding works
Blockquotes should be harmless, and both Gmail and Thunderbird use them
when quoting HTML messages.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 566fd3f..a69c81a 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2359,7 +2359,7 @@ sub _NewScrubber {
);
$scrubber->deny(qw[*]);
$scrubber->allow(
- qw[A B U P BR I HR BR SMALL EM FONT SPAN STRONG SUB SUP STRIKE H1 H2 H3 H4 H5 H6 DIV UL OL LI DL DT DD PRE]
+ qw[A B U P BR I HR BR SMALL EM FONT SPAN STRONG SUB SUP STRIKE H1 H2 H3 H4 H5 H6 DIV UL OL LI DL DT DD PRE BLOCKQUOTE]
);
$scrubber->comment(0);
commit ad312089ab65778fafe6b625f2b796a5b79da843
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Mar 29 14:12:37 2012 -0400
RowsPerPage and FirstRow only accept natural numbers and undef
This prevents user provided page sizes from injecting SQL after the
LIMIT. GotoPage is covered by FirstRow.
Resolves CVE-2011-4460. Ticket #69239.
diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index e4a17f4..62a2369 100755
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -96,6 +96,19 @@ sub OrderByCols {
return $self->SUPER::OrderByCols( @sort );
}
+# If we're setting RowsPerPage or FirstRow, ensure we get a natural number or undef.
+sub RowsPerPage {
+ my $self = shift;
+ return if @_ and defined $_[0] and $_[0] =~ /\D/;
+ return $self->SUPER::RowsPerPage(@_);
+}
+
+sub FirstRow {
+ my $self = shift;
+ return if @_ and defined $_[0] and $_[0] =~ /\D/;
+ return $self->SUPER::FirstRow(@_);
+}
+
=head2 LimitToEnabled
Only find items that haven't been disabled
commit a325ac0d049ef4a0e58c8744ae6c61fa193c800f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 16:56:17 2012 -0500
Refactor HTML scrubbing to make it easier to customize what is allowed
This is in preparation for stricter rules that may want slight loosening
for some installs (i.e. a regex rather than flat out denial).
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index a69c81a..3bff80b 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2317,50 +2317,68 @@ sub ScrubHTML {
=head2 _NewScrubber
-Returns a new L<HTML::Scrubber> object. Override this if you insist on
-letting more HTML through.
+Returns a new L<HTML::Scrubber> object.
+
+If you need to be more lax about what HTML tags and attributes are allowed,
+create C</opt/rt4/local/lib/RT/Interface/Web_Local.pm> with something like the
+following:
+
+ package HTML::Mason::Commands;
+ # Let tables through
+ push @SCRUBBER_ALLOWED_TAGS, qw(TABLE THEAD TBODY TFOOT TR TD TH);
+ 1;
=cut
+our @SCRUBBER_ALLOWED_TAGS = qw(
+ A B U P BR I HR BR SMALL EM FONT SPAN STRONG SUB SUP STRIKE H1 H2 H3 H4 H5
+ H6 DIV UL OL LI DL DT DD PRE BLOCKQUOTE
+);
+
+our %SCRUBBER_ALLOWED_ATTRIBUTES = (
+ id => 1,
+ class => 1,
+ # Match http, ftp and relative urls
+ # XXX: we also scrub format strings with this module then allow simple config options
+ href => qr{^(?:http:|ftp:|https:|/|__Web(?:Path|BaseURL|URL)__)}i,
+ face => 1,
+ size => 1,
+ target => 1,
+ style => qr{
+ ^(?:\s*
+ (?:(?:background-)?color: \s*
+ (?:rgb\(\s* \d+, \s* \d+, \s* \d+ \s*\) | # rgb(d,d,d)
+ \#[a-f0-9]{3,6} | # #fff or #ffffff
+ [\w\-]+ # green, light-blue, etc.
+ ) |
+ text-align: \s* \w+ |
+ font-size: \s* [\w.\-]+ |
+ font-family: \s* [\w\s"',.\-]+ |
+ font-weight: \s* [\w\-]+ |
+
+ # MS Office styles, which are probably fine. If we don't, then any
+ # associated styles in the same attribute get stripped.
+ mso-[\w\-]+?: \s* [\w\s"',.\-]+
+ )\s* ;? \s*)
+ +$ # one or more of these allowed properties from here 'till sunset
+ }ix,
+);
+
sub _NewScrubber {
require HTML::Scrubber;
my $scrubber = HTML::Scrubber->new();
$scrubber->default(
0,
{
- '*' => 0,
- id => 1,
- class => 1,
- # Match http, ftp and relative urls
- # XXX: we also scrub format strings with this module then allow simple config options
- href => qr{^(?:http:|ftp:|https:|/|__Web(?:Path|BaseURL|URL)__)}i,
- face => 1,
- size => 1,
- target => 1,
- style => qr{
- ^(?:\s*
- (?:(?:background-)?color: \s*
- (?:rgb\(\s* \d+, \s* \d+, \s* \d+ \s*\) | # rgb(d,d,d)
- \#[a-f0-9]{3,6} | # #fff or #ffffff
- [\w\-]+ # green, light-blue, etc.
- ) |
- text-align: \s* \w+ |
- font-size: \s* [\w.\-]+ |
- font-family: \s* [\w\s"',.\-]+ |
- font-weight: \s* [\w\-]+ |
-
- # MS Office styles, which are probably fine. If we don't, then any
- # associated styles in the same attribute get stripped.
- mso-[\w\-]+?: \s* [\w\s"',.\-]+
- )\s* ;? \s*)
- +$ # one or more of these allowed properties from here 'till sunset
- }ix,
- }
+ %SCRUBBER_ALLOWED_ATTRIBUTES,
+ '*' => 0, # require attributes be explicitly allowed
+ },
);
$scrubber->deny(qw[*]);
- $scrubber->allow(
- qw[A B U P BR I HR BR SMALL EM FONT SPAN STRONG SUB SUP STRIKE H1 H2 H3 H4 H5 H6 DIV UL OL LI DL DT DD PRE BLOCKQUOTE]
- );
+ $scrubber->allow(@SCRUBBER_ALLOWED_TAGS);
+
+ # Scrubbing comments is vital since IE conditional comments can contain
+ # arbitrary HTML and we'd pass it right on through.
$scrubber->comment(0);
return $scrubber;
commit 80b14f90de3eed0a64f9318850c515fb855d2261
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 17:28:34 2012 -0500
Add a way to specify tag-specific attribute rules for scrubbing
Currently unused, but immensely useful.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 3bff80b..100dfb3 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2364,6 +2364,8 @@ our %SCRUBBER_ALLOWED_ATTRIBUTES = (
}ix,
);
+our %SCRUBBER_RULES = ();
+
sub _NewScrubber {
require HTML::Scrubber;
my $scrubber = HTML::Scrubber->new();
@@ -2376,6 +2378,7 @@ sub _NewScrubber {
);
$scrubber->deny(qw[*]);
$scrubber->allow(@SCRUBBER_ALLOWED_TAGS);
+ $scrubber->rules(%SCRUBBER_RULES);
# Scrubbing comments is vital since IE conditional comments can contain
# arbitrary HTML and we'd pass it right on through.
commit 0b5f3d82e2ee5721208685fe6d2ede4e0ebdaf29
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 17:46:25 2012 -0500
Scrub class and id attributes from HTML instead of passing them through
A large part of RT's layout and enhancement with Javascript depends on
classes and ids. Passing through those attributes means ticket history
can trivially piggyback on RT's core CSS and JS to do malicious things
like hide content or style and move fake elements on top of core UI.
Since classes and ids are useful, however, for site-specific RT
customizations around correspondence, the scrubbing configuration was
refactored in previously commits to make it easier to tweak what tags
and attributes are allowed.
Resolves part of CVE-2011-2083.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 100dfb3..de504fe 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2336,8 +2336,6 @@ our @SCRUBBER_ALLOWED_TAGS = qw(
);
our %SCRUBBER_ALLOWED_ATTRIBUTES = (
- id => 1,
- class => 1,
# Match http, ftp and relative urls
# XXX: we also scrub format strings with this module then allow simple config options
href => qr{^(?:http:|ftp:|https:|/|__Web(?:Path|BaseURL|URL)__)}i,
commit e2233e032012c4286d4afecfd0f4d84da497f97b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 14:06:36 2012 -0500
Inherit from the normal autohandler chain when serving Shredder backups
The normal inheritance chain of:
/autohandler
/Admin/autohandler
/Admin/Tools/Shredder/autohandler
provides appropriate access control at each level. By inheriting from
nothing, the shredder dump dhandler was called directly, making a
complete end-run around all ACLs. Anyone who could guess (i.e. brute
force) the very predictable dump filenames would be treated to a SQL
dump of shredded data, no login required.
Instead of inheriting from nothing to avoid the footer, simply abort the
request at the end of the dhandler.
Fixes part of CVE-2011-2084.
diff --git a/share/html/Admin/Tools/Shredder/Dumps/dhandler b/share/html/Admin/Tools/Shredder/Dumps/dhandler
index e742001..53b8065 100644
--- a/share/html/Admin/Tools/Shredder/Dumps/dhandler
+++ b/share/html/Admin/Tools/Shredder/Dumps/dhandler
@@ -48,9 +48,6 @@
<%ATTR>
AutoFlush => 0
</%ATTR>
-<%FLAGS>
-inherit => undef
-</%FLAGS>
<%INIT>
my $arg = $m->dhandler_arg;
$m->abort(404) if $arg =~ m{\.\.|/|\\};
@@ -64,5 +61,5 @@ my $buf;
while( read $fh, $buf, 1024*1024 ) {
$m->out($buf);
}
-return 0;
+$m->abort;
</%INIT>
commit cdcc2b65b2b361b362bc0fa86e9dc6f60fd65784
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:05:08 2011 -0400
Remove unused GenericQueryArgs parameter
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index 8bca076..9325fdf 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -52,7 +52,6 @@
Order => @Order,
OrderBy => @OrderBy,
BaseURL => $BaseURL,
- GenericQueryArgs => $GenericQueryArgs,
AllowSorting => $AllowSorting,
Class => 'RT::Tickets',
Rows => $Rows,
@@ -79,7 +78,6 @@ $title => loc("My [_1] tickets", $friendly_status)
@status => RT::Queue->ActiveStatusArray()
$BaseURL => undef
$Page => 1
-$GenericQueryArgs => undef
$AllowSorting => 1
@Order => ('ASC')
@OrderBy => ('Created')
commit ad5d6ed2d2b80fe2426c36d40b70dd6cf2264a6b
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:05:47 2011 -0400
Similarly, there is no reason to configure AllowSorting
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index 9325fdf..2a5af8a 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -52,7 +52,7 @@
Order => @Order,
OrderBy => @OrderBy,
BaseURL => $BaseURL,
- AllowSorting => $AllowSorting,
+ AllowSorting => 1,
Class => 'RT::Tickets',
Rows => $Rows,
Page => $Page &>
@@ -78,7 +78,6 @@ $title => loc("My [_1] tickets", $friendly_status)
@status => RT::Queue->ActiveStatusArray()
$BaseURL => undef
$Page => 1
-$AllowSorting => 1
@Order => ('ASC')
@OrderBy => ('Created')
$Rows => 50
commit 19369ba8f67eec572a992f4bdb22d756872ccc37
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:06:50 2011 -0400
Disallow setting arbitrary titles
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index 2a5af8a..6e29e88 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -45,7 +45,7 @@
%# those contributions and any derivatives thereof.
%#
%# END BPS TAGGED BLOCK }}}
-<&| /Widgets/TitleBox, title => $title &>
+<&| /Widgets/TitleBox, title => $title &>
<& /Elements/CollectionList, Title => $title,
Format => $Format,
Query => $Query,
@@ -59,6 +59,7 @@
</&>
<%INIT>
+my $title = loc("My [_1] tickets", $friendly_status);
my $id = $session{'CurrentUser'}->id;
my $Query = "( "
. join( ' OR ', map "$_.id = $id", @roles )
@@ -69,11 +70,9 @@ if ( @status ) {
. " )";
}
my $Format = RT->Config->Get('DefaultSelfServiceSearchResultFormat');
-
</%INIT>
<%ARGS>
$friendly_status => loc('open')
-$title => loc("My [_1] tickets", $friendly_status)
@roles => ('Watcher')
@status => RT::Queue->ActiveStatusArray()
$BaseURL => undef
commit 64c6ecd431388d7c81c5f94ee4f0c526325ba9c0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:08:14 2011 -0400
Disallow setting of roles via query params
This closes a hole wherein unprivileged users could inject TicketSQL.
While this is not in itself dangerous, it might allow unprivileged users
to see unexpected tickets if rights have been mis-applied by the
administrator (such as giving ShowTicket to Everyone).
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index 6e29e88..a7b453d 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -61,9 +61,7 @@
<%INIT>
my $title = loc("My [_1] tickets", $friendly_status);
my $id = $session{'CurrentUser'}->id;
-my $Query = "( "
- . join( ' OR ', map "$_.id = $id", @roles )
- . ")";
+my $Query = "( Watcher.id = $id )";
if ( @status ) {
$Query .= " AND ( "
. join( ' OR ', map "Status = '$_'", @status )
@@ -73,7 +71,6 @@ my $Format = RT->Config->Get('DefaultSelfServiceSearchResultFormat');
</%INIT>
<%ARGS>
$friendly_status => loc('open')
- at roles => ('Watcher')
@status => RT::Queue->ActiveStatusArray()
$BaseURL => undef
$Page => 1
commit c83b3488e33eba887ae20a6f192f2c5dc4311d01
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:12:47 2011 -0400
Always pass in status list to selfservice search
This prevents it from being overridden via query parameters; much like
the previous commit, this allowed TicketSQL injection. At the same
time, properly escape any statuses which contain apostrophes.
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index a7b453d..880b4e3 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -63,15 +63,14 @@ my $title = loc("My [_1] tickets", $friendly_status);
my $id = $session{'CurrentUser'}->id;
my $Query = "( Watcher.id = $id )";
if ( @status ) {
- $Query .= " AND ( "
- . join( ' OR ', map "Status = '$_'", @status )
- . " )";
+ @status = map {s/(['\\])/\\$1/g; "Status = '$_'"} @status;
+ $Query .= " AND ( " . join(' OR ', @status ) . " )";
}
my $Format = RT->Config->Get('DefaultSelfServiceSearchResultFormat');
</%INIT>
<%ARGS>
$friendly_status => loc('open')
- at status => RT::Queue->ActiveStatusArray()
+ at status => ()
$BaseURL => undef
$Page => 1
@Order => ('ASC')
diff --git a/share/html/SelfService/index.html b/share/html/SelfService/index.html
index f57554a..9030804 100755
--- a/share/html/SelfService/index.html
+++ b/share/html/SelfService/index.html
@@ -48,6 +48,8 @@
<& /SelfService/Elements/Header, Title => loc('Open tickets') &>
<& /SelfService/Elements/MyRequests,
%ARGS,
+ status => [ RT::Queue->ActiveStatusArray() ],
+ friendly_status => loc('open'),
BaseURL => RT->Config->Get('WebPath') ."/SelfService/?",
Page => $Page,
&>
commit 8fbe5b518a3898229de7f7717231d175d4d33e6f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Nov 16 12:11:18 2011 -0500
Ensure the empty CFVs collection never returns results after a failed rights check
This is identical to negative limiting we do elsewhere to ensure no
records are returned.
This resolves part of CVE-2011-2084.
diff --git a/lib/RT/CustomField_Overlay.pm b/lib/RT/CustomField_Overlay.pm
index 5db11db..b9368c5 100755
--- a/lib/RT/CustomField_Overlay.pm
+++ b/lib/RT/CustomField_Overlay.pm
@@ -385,6 +385,8 @@ sub Values {
# if the user has no rights, return an empty object
if ( $self->id && $self->CurrentUserHasRight( 'SeeCustomField') ) {
$cf_values->LimitToCustomField( $self->Id );
+ } else {
+ $cf_values->Limit( FIELD => 'id', VALUE => 0 );
}
return ($cf_values);
}
commit ee8717368f42f083cfd900170201f7a3d73e2f35
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Nov 16 12:52:02 2011 -0500
Push id = 0 limits into an ACL subclause
This ensures the id = 0 condition isn't OR'd with a subsequent limit
after we return the collection.
diff --git a/lib/RT/CustomField_Overlay.pm b/lib/RT/CustomField_Overlay.pm
index b9368c5..925a455 100755
--- a/lib/RT/CustomField_Overlay.pm
+++ b/lib/RT/CustomField_Overlay.pm
@@ -386,7 +386,7 @@ sub Values {
if ( $self->id && $self->CurrentUserHasRight( 'SeeCustomField') ) {
$cf_values->LimitToCustomField( $self->Id );
} else {
- $cf_values->Limit( FIELD => 'id', VALUE => 0 );
+ $cf_values->Limit( FIELD => 'id', VALUE => 0, SUBCLAUSE => 'acl' );
}
return ($cf_values);
}
diff --git a/lib/RT/Ticket_Overlay.pm b/lib/RT/Ticket_Overlay.pm
index c3ef5ff..5e9d65d 100755
--- a/lib/RT/Ticket_Overlay.pm
+++ b/lib/RT/Ticket_Overlay.pm
@@ -2220,7 +2220,7 @@ sub _Links {
my $links = $self->{ $cache_key }
= RT::Links->new( $self->CurrentUser );
unless ( $self->CurrentUserHasRight('ShowTicket') ) {
- $links->Limit( FIELD => 'id', VALUE => 0 );
+ $links->Limit( FIELD => 'id', VALUE => 0, SUBCLAUSE => 'acl' );
return $links;
}
diff --git a/lib/RT/Transaction_Overlay.pm b/lib/RT/Transaction_Overlay.pm
index 4870973..88ead8e 100755
--- a/lib/RT/Transaction_Overlay.pm
+++ b/lib/RT/Transaction_Overlay.pm
@@ -506,7 +506,7 @@ sub Attachments {
$self->{'attachments'} = RT::Attachments->new( $self->CurrentUser );
unless ( $self->CurrentUserCanSee ) {
- $self->{'attachments'}->Limit(FIELD => 'id', VALUE => '0');
+ $self->{'attachments'}->Limit(FIELD => 'id', VALUE => '0', SUBCLAUSE => 'acl');
return $self->{'attachments'};
}
commit 02adabdd6ca2c5df6ee3e13e742b38934cc89447
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 19:39:14 2012 -0400
Terminate the request if there isn't a CustomField or Context Argument
diff --git a/share/html/Helpers/Autocomplete/CustomFieldValues b/share/html/Helpers/Autocomplete/CustomFieldValues
index 85323cc..d4af62c 100644
--- a/share/html/Helpers/Autocomplete/CustomFieldValues
+++ b/share/html/Helpers/Autocomplete/CustomFieldValues
@@ -54,6 +54,9 @@
</ul>
% $m->abort;
<%INIT>
+
+$m->abort unless $ARGS{ContextType} and $ARGS{ContextId};
+
my ($CustomField, $Value);
while( my($k, $v) = each %ARGS ) {
next unless $k =~ /^Object-.*?-\d*-CustomField-(\d+)-Values?$/;
@@ -63,6 +66,8 @@ while( my($k, $v) = each %ARGS ) {
$m->abort unless $CustomField;
my $CustomFieldObj = RT::CustomField->new( $session{'CurrentUser'} );
$CustomFieldObj->Load( $CustomField );
+$m->abort unless $CustomFieldObj->Id;
+
my $values = $CustomFieldObj->Values;
$values->Limit(
FIELD => 'Name',
commit 9feb75b8ee903273c6f708e6d52ab10ae3774b64
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 15:56:35 2012 -0400
Load and Validate Custom Field Context Objects
We now require that you pass a ContextId in the AutoComplete URL and we
use that Id to load an object of the appropriate type and check that the
object is actually related to the Custom Field you're looking at (this
prevents you from using Queue A to look at valuds for Custom Field B
which is only applied to Queue C, or using a Class of id 8 to simulate a
Queue of id 8).
Since you might use the Context Object to gain rights to load a
CustomField, we can't just load a random Custom Field and ask "is this a
context object?" which is why this code loads a Custom Field as the
system user and then uses that privileged object to validate the context
object before dropping privileges and loading the CO and CF as the
current user.
diff --git a/lib/RT/CustomField_Overlay.pm b/lib/RT/CustomField_Overlay.pm
index 925a455..ad333f1 100755
--- a/lib/RT/CustomField_Overlay.pm
+++ b/lib/RT/CustomField_Overlay.pm
@@ -732,7 +732,77 @@ sub ContextObject {
my $self = shift;
return $self->{'context_object'};
}
-
+
+sub ValidContextType {
+ my $self = shift;
+ my $class = shift;
+
+ my %valid;
+ $valid{$_}++ for split '-', $self->LookupType;
+ delete $valid{'RT::Transaction'};
+
+ return $valid{$class};
+}
+
+=head2 LoadContextObject
+
+Takes an Id for a Context Object and loads the right kind of RT::Object
+for this particular Custom Field (based on the LookupType) and returns it.
+This is a good way to ensure you don't try to use a Queue as a Context
+Object on a User Custom Field.
+
+=cut
+
+sub LoadContextObject {
+ my $self = shift;
+ my $type = shift;
+ my $contextid = shift;
+
+ unless ( $self->ValidContextType($type) ) {
+ RT->Logger->debug("Invalid ContextType $type for Custom Field ".$self->Id);
+ return;
+ }
+
+ my $context_object = $type->new( $self->CurrentUser );
+ my ($id, $msg) = $context_object->LoadById( $contextid );
+ unless ( $id ) {
+ RT->Logger->debug("Invalid ContextObject id: $msg");
+ return;
+ }
+ return $context_object;
+}
+
+=head2 ValidateContextObject
+
+Ensure that a given ContextObject applies to this Custom Field.
+For custom fields that are assigned to Queues or to Classes, this checks that the Custom
+Field is actually applied to that objects. For Global Custom Fields, it returns true
+as long as the Object is of the right type, because you may be using
+your permissions on a given Queue of Class to see a Global CF.
+For CFs that are only applied Globally, you don't need a ContextObject.
+
+=cut
+
+sub ValidateContextObject {
+ my $self = shift;
+ my $object = shift;
+
+ return 1 if $self->IsApplied(0);
+
+ # global only custom fields don't have objects
+ # that should be used as context objects.
+ return if $self->ApplyGlobally;
+
+ # Otherwise, make sure we weren't passed a user object that we're
+ # supposed to treat as a queue.
+ return unless $self->ValidContextType(ref $object);
+
+ # Check that it is applied correctly
+ my ($applied_to) = grep {ref($_) eq $self->RecordClassFromLookupType} ($object, $object->ACLEquivalenceObjects);
+ return unless $applied_to;
+ return $self->IsApplied($applied_to->id);
+}
+
# {{{ sub _Set
sub _Set {
diff --git a/share/html/Elements/EditCustomFieldAutocomplete b/share/html/Elements/EditCustomFieldAutocomplete
index 13a43ed..c16aa8a 100644
--- a/share/html/Elements/EditCustomFieldAutocomplete
+++ b/share/html/Elements/EditCustomFieldAutocomplete
@@ -52,7 +52,7 @@ new Ajax.Autocompleter(
"<% $name %>-Values",
"<% $name %>-Choices",
"<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues",
- { tokens: [ '\n' ] }
+ { tokens: [ '\n' ], parameters: "<% $Context |n %>" }
);
% } else {
<input type="text" id="<% $name %>-Value" name="<% $name %>-Value" class="CF-<%$CustomField->id%>-Edit" value="<% $Default %>"/><div id="<% $name %>-Choices" class="autocomplete"></div>
@@ -61,7 +61,7 @@ new Ajax.Autocompleter(
"<% $name %>-Value",
"<% $name %>-Choices",
"<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues",
- {}
+ { parameters: "<% $Context |n %>" }
);
% }
</script>
@@ -76,6 +76,11 @@ if ( $Multiple and $Values ) {
$Default .= $value->Content ."\n";
}
}
+my $Context = "";
+if ($CustomField->ContextObject) {
+ $Context .= "ContextId=" . $CustomField->ContextObject->Id . "&";
+ $Context .= "ContextType=". ref($CustomField->ContextObject);
+}
</%INIT>
<%ARGS>
$CustomField => undef
diff --git a/share/html/Helpers/Autocomplete/CustomFieldValues b/share/html/Helpers/Autocomplete/CustomFieldValues
index d4af62c..83d516b 100644
--- a/share/html/Helpers/Autocomplete/CustomFieldValues
+++ b/share/html/Helpers/Autocomplete/CustomFieldValues
@@ -55,7 +55,10 @@
% $m->abort;
<%INIT>
-$m->abort unless $ARGS{ContextType} and $ARGS{ContextId};
+unless ( exists $ARGS{ContextType} and exists $ARGS{ContextId} ) {
+ RT->Logger->debug("No context provided");
+ $m->abort;
+}
my ($CustomField, $Value);
while( my($k, $v) = each %ARGS ) {
@@ -63,10 +66,39 @@ while( my($k, $v) = each %ARGS ) {
($CustomField, $Value) = ($1, $v);
last;
}
-$m->abort unless $CustomField;
+
+unless ( $CustomField ) {
+ RT->Logger->debug("No CustomField provided");
+ $m->abort;
+}
+
+my $SystemCustomFieldObj = RT::CustomField->new( RT->SystemUser );
+my ($id, $msg) = $SystemCustomFieldObj->LoadById( $CustomField ) ;
+unless ( $id ) {
+ RT->Logger->debug("Invalid CustomField provided: $msg");
+ $m->abort;
+}
+
+my $context_object = $SystemCustomFieldObj->LoadContextObject(
+ $ARGS{ContextType}, $ARGS{ContextId} );
+$m->abort unless $context_object;
+
my $CustomFieldObj = RT::CustomField->new( $session{'CurrentUser'} );
-$CustomFieldObj->Load( $CustomField );
-$m->abort unless $CustomFieldObj->Id;
+if ( $SystemCustomFieldObj->ValidateContextObject($context_object) ) {
+ # drop our privileges that came from calling LoadContextObject as the System User
+ $context_object->new($session{'CurrentUser'});
+ $context_object->LoadById($ARGS{ContextId});
+ $CustomFieldObj->SetContextObject( $context_object );
+} else {
+ RT->Logger->debug("Invalid Context Object ".$context_object->id." for Custom Field ".$SystemCustomFieldObj->id);
+ $m->abort;
+}
+
+($id, $msg) = $CustomFieldObj->LoadById( $CustomField );
+unless ( $CustomFieldObj->Name ) {
+ RT->Logger->debug("Current User cannot see this Custom Field, terminating");
+ $m->abort;
+}
my $values = $CustomFieldObj->Values;
$values->Limit(
commit 1ef24b15d3fc42131bf29f888279de55a9fd01a0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 16:00:33 2012 -0400
When loading custom fields by queue, default the context object accordingly
diff --git a/lib/RT/CustomField_Overlay.pm b/lib/RT/CustomField_Overlay.pm
index ad333f1..ecf403e 100755
--- a/lib/RT/CustomField_Overlay.pm
+++ b/lib/RT/CustomField_Overlay.pm
@@ -324,10 +324,12 @@ sub LoadByName {
}
# if we're looking for a queue by name, make it a number
- if ( defined $args{'Queue'} && $args{'Queue'} =~ /\D/ ) {
+ if ( defined $args{'Queue'} && ($args{'Queue'} =~ /\D/ || !$self->ContextObject) ) {
my $QueueObj = RT::Queue->new( $self->CurrentUser );
$QueueObj->Load( $args{'Queue'} );
$args{'Queue'} = $QueueObj->Id;
+ $self->SetContextObject( $QueueObj )
+ unless $self->ContextObject;
}
# XXX - really naive implementation. Slow. - not really. still just one query
commit 6a9a41d6dc2908c34c80333bb507457aec058e7d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 18:29:46 2012 -0400
Set context objects on CFs explicitly whenever possible
diff --git a/lib/RT/Action/CreateTickets.pm b/lib/RT/Action/CreateTickets.pm
index 08763d5..7a13ddc 100755
--- a/lib/RT/Action/CreateTickets.pm
+++ b/lib/RT/Action/CreateTickets.pm
@@ -1147,6 +1147,7 @@ sub UpdateCustomFields {
my $cf = $1;
my $CustomFieldObj = RT::CustomField->new($self->CurrentUser);
+ $CustomFieldObj->SetContextObject( $ticket );
$CustomFieldObj->LoadById($cf);
my @values;
diff --git a/lib/RT/CustomField_Overlay.pm b/lib/RT/CustomField_Overlay.pm
index ecf403e..9cb993f 100755
--- a/lib/RT/CustomField_Overlay.pm
+++ b/lib/RT/CustomField_Overlay.pm
@@ -1486,6 +1486,7 @@ sub SetBasedOn {
unless defined $value and length $value;
my $cf = RT::CustomField->new( $self->CurrentUser );
+ $cf->SetContextObject( $self->ContextObject );
$cf->Load( ref $value ? $value->Id : $value );
return (0, "Permission denied")
@@ -1501,6 +1502,7 @@ sub SetBasedOn {
sub BasedOnObj {
my $self = shift;
my $obj = RT::CustomField->new( $self->CurrentUser );
+ $obj->SetContextObject( $self->ContextObject );
my $attribute = $self->FirstAttribute("BasedOn");
$obj->Load($attribute->Content) if defined $attribute;
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b3f593a..58df091 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1193,6 +1193,7 @@ sub CreateTicket {
my $cfid = $1;
my $cf = RT::CustomField->new( $session{'CurrentUser'} );
+ $cf->SetContextObject( $Queue );
$cf->Load($cfid);
unless ( $cf->id ) {
$RT::Logger->error( "Couldn't load custom field #" . $cfid );
@@ -1801,6 +1802,7 @@ sub ProcessObjectCustomFieldUpdates {
foreach my $cf ( keys %{ $custom_fields_to_mod{$class}{$id} } ) {
my $CustomFieldObj = RT::CustomField->new( $session{'CurrentUser'} );
+ $CustomFieldObj->SetContextObject($Object);
$CustomFieldObj->LoadById($cf);
unless ( $CustomFieldObj->id ) {
$RT::Logger->warning("Couldn't load custom field #$cf");
diff --git a/lib/RT/ObjectCustomField_Overlay.pm b/lib/RT/ObjectCustomField_Overlay.pm
index 689d62f..3b7d879 100644
--- a/lib/RT/ObjectCustomField_Overlay.pm
+++ b/lib/RT/ObjectCustomField_Overlay.pm
@@ -119,7 +119,19 @@ sub Delete {
sub CustomFieldObj {
my $self = shift;
my $id = shift || $self->CustomField;
+
+ # To find out the proper context object to load the CF with, we need
+ # data from the CF -- namely, the record class. Go find that as the
+ # system user first.
+ my $system_CF = RT::CustomField->new( RT->SystemUser );
+ $system_CF->Load( $id );
+ my $class = $system_CF->RecordClassFromLookupType;
+
+ my $obj = $class->new( $self->CurrentUser );
+ $obj->Load( $self->ObjectId );
+
my $CF = RT::CustomField->new( $self->CurrentUser );
+ $CF->SetContextObject( $obj );
$CF->Load( $id );
return $CF;
}
diff --git a/lib/RT/Queue_Overlay.pm b/lib/RT/Queue_Overlay.pm
index c7ab7f3..007dae4 100755
--- a/lib/RT/Queue_Overlay.pm
+++ b/lib/RT/Queue_Overlay.pm
@@ -657,6 +657,7 @@ sub TicketTransactionCustomFields {
my $cfs = RT::CustomFields->new( $self->CurrentUser );
if ( $self->CurrentUserHasRight('SeeQueue') ) {
+ $cfs->SetContextObject( $self );
$cfs->LimitToGlobalOrObjectId( $self->Id );
$cfs->LimitToLookupType( 'RT::Queue-RT::Ticket-RT::Transaction' );
$cfs->ApplySortOrder;
diff --git a/lib/RT/Shredder/Queue.pm b/lib/RT/Shredder/Queue.pm
index 8ee1094..79b67d1 100644
--- a/lib/RT/Shredder/Queue.pm
+++ b/lib/RT/Shredder/Queue.pm
@@ -91,6 +91,7 @@ sub __DependsOn
# Custom Fields
$objs = RT::CustomFields->new( $self->CurrentUser );
+ $objs->SetContextObject( $self );
$objs->LimitToQueue( $self->id );
push( @$list, $objs );
diff --git a/lib/RT/Ticket_Overlay.pm b/lib/RT/Ticket_Overlay.pm
index 5e9d65d..39ed9c1 100755
--- a/lib/RT/Ticket_Overlay.pm
+++ b/lib/RT/Ticket_Overlay.pm
@@ -3564,7 +3564,9 @@ sub Transactions {
sub TransactionCustomFields {
my $self = shift;
- return $self->QueueObj->TicketTransactionCustomFields;
+ my $cfs = $self->QueueObj->TicketTransactionCustomFields;
+ $cfs->SetContextObject( $self );
+ return $cfs;
}
# }}}
diff --git a/lib/RT/Transaction_Overlay.pm b/lib/RT/Transaction_Overlay.pm
index 88ead8e..b564ef5 100755
--- a/lib/RT/Transaction_Overlay.pm
+++ b/lib/RT/Transaction_Overlay.pm
@@ -728,6 +728,7 @@ sub BriefDescription {
if ( $self->Field ) {
my $cf = RT::CustomField->new( $self->CurrentUser );
+ $cf->SetContextObject( $self->Object );
$cf->Load( $self->Field );
$field = $cf->Name();
}
@@ -1197,6 +1198,7 @@ sub CustomFieldValues {
# do we want to cover this situation somehow here?
unless ( defined $field && $field =~ /^\d+$/o ) {
my $CFs = RT::CustomFields->new( $self->CurrentUser );
+ $CFs->SetContextObject( $self->Object );
$CFs->Limit( FIELD => 'Name', VALUE => $field );
$CFs->LimitToLookupType($self->CustomFieldLookupType);
$CFs->LimitToGlobalOrObjectId($self->Object->QueueObj->id);
diff --git a/share/html/Admin/Elements/EditCustomFields b/share/html/Admin/Elements/EditCustomFields
index 91d5cff..8226390 100755
--- a/share/html/Admin/Elements/EditCustomFields
+++ b/share/html/Admin/Elements/EditCustomFields
@@ -128,6 +128,7 @@ if ( $MoveCustomFieldDown ) { {
if ( $UpdateCFs ) {
foreach my $cf_id ( @AddCustomField ) {
my $CF = RT::CustomField->new( $session{'CurrentUser'} );
+ $CF->SetContextObject( $Object );
$CF->Load( $cf_id );
unless ( $CF->id ) {
push @results, loc("Couldn't load CustomField #[_1]", $cf_id);
@@ -138,6 +139,7 @@ if ( $UpdateCFs ) {
}
foreach my $cf_id ( @RemoveCustomField ) {
my $CF = RT::CustomField->new( $session{'CurrentUser'} );
+ $CF->SetContextObject( $Object );
$CF->Load( $cf_id );
unless ( $CF->id ) {
push @results, loc("Couldn't load CustomField #[_1]", $cf_id);
@@ -153,6 +155,7 @@ $m->callback(CallbackName => 'UpdateExtraFields', Results => \@results, Object =
my $applied_cfs = RT::CustomFields->new( $session{'CurrentUser'} );
$applied_cfs->LimitToLookupType($lookup);
$applied_cfs->LimitToGlobalOrObjectId($id);
+$applied_cfs->SetContextObject( $Object );
$applied_cfs->ApplySortOrder;
my $not_applied_cfs = RT::CustomFields->new( $session{'CurrentUser'} );
commit 82c9189f529dc65c1874a15e5379f5f9d11593f7
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 2 22:17:35 2012 -0400
Consistently escape all possibly suspect characters in JS strings
This resolves part of CVE-2011-2083.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b3f593a..4a7e2b0 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -110,6 +110,25 @@ sub EscapeURI {
# }}}
+sub _encode_surrogates {
+ my $uni = $_[0] - 0x10000;
+ return ($uni / 0x400 + 0xD800, $uni % 0x400 + 0xDC00);
+}
+
+sub EscapeJS {
+ my $ref = shift;
+ return unless defined $$ref;
+
+ $$ref = "'" . join('',
+ map {
+ chr($_) =~ /[a-zA-Z0-9]/ ? chr($_) :
+ $_ <= 255 ? sprintf("\\x%02X", $_) :
+ $_ <= 65535 ? sprintf("\\u%04X", $_) :
+ sprintf("\\u%X\\u%X", _encode_surrogates($_))
+ } unpack('U*', $$ref))
+ . "'";
+}
+
# {{{ WebCanonicalizeInfo
=head2 WebCanonicalizeInfo();
diff --git a/lib/RT/Interface/Web/Handler.pm b/lib/RT/Interface/Web/Handler.pm
index 4bb6484..1ea5709 100644
--- a/lib/RT/Interface/Web/Handler.pm
+++ b/lib/RT/Interface/Web/Handler.pm
@@ -205,6 +205,7 @@ sub NewHandler {
$handler->interp->set_escape( h => \&RT::Interface::Web::EscapeUTF8 );
$handler->interp->set_escape( u => \&RT::Interface::Web::EscapeURI );
+ $handler->interp->set_escape( j => \&RT::Interface::Web::EscapeJS );
return($handler);
}
diff --git a/sbin/rt-email-dashboards.in b/sbin/rt-email-dashboards.in
index 50dad2f..0b3686a 100644
--- a/sbin/rt-email-dashboards.in
+++ b/sbin/rt-email-dashboards.in
@@ -384,6 +384,9 @@ sub get_from {
autohandler_name => '', # disable forced login and more
data_dir => $data_dir,
);
+ $mason->interp->set_escape( h => \&RT::Interface::Web::EscapeUTF8 );
+ $mason->interp->set_escape( u => \&RT::Interface::Web::EscapeURI );
+ $mason->interp->set_escape( j => \&RT::Interface::Web::EscapeJS );
}
return $mason;
}
diff --git a/share/html/Elements/ColumnMap b/share/html/Elements/ColumnMap
index 8afe4a1..696a48a 100644
--- a/share/html/Elements/ColumnMap
+++ b/share/html/Elements/ColumnMap
@@ -118,14 +118,16 @@ my $COLUMN_MAP = {
my $name = $_[1] || 'SelectedTickets';
my $checked = $m->request_args->{ $name .'All' }? 'checked="checked"': '';
- return \qq{<input type="checkbox" name="${name}All" value="1" $checked
- onclick="setCheckbox(this.form, '$name', this.checked)" />};
+ return \qq{<input type="checkbox" name="}, $name, \qq{All" value="1" $checked
+ onclick="setCheckbox(this.form, },
+ $m->interp->apply_escapes($name,'j'),
+ \qq{, this.checked)" />};
},
value => sub {
my $id = $_[0]->id;
my $name = $_[2] || 'SelectedTickets';
- return \qq{<input type="checkbox" name="$name" value="$id" checked="checked" />}
+ return \qq{<input type="checkbox" name="}, $name, \qq{" value="$id" checked="checked" />}
if $m->request_args->{ $name . 'All'};
my $arg = $m->request_args->{ $name };
@@ -136,7 +138,7 @@ my $COLUMN_MAP = {
elsif ( $arg ) {
$checked = 'checked="checked"' if $arg == $id;
}
- return \qq{<input type="checkbox" name="$name" value="$id" $checked />}
+ return \qq{<input type="checkbox" name="}, $name, \qq{" value="$id" $checked />}
},
},
RadioButton => {
diff --git a/share/html/Elements/EditCustomFieldAutocomplete b/share/html/Elements/EditCustomFieldAutocomplete
index 13a43ed..25447e7 100644
--- a/share/html/Elements/EditCustomFieldAutocomplete
+++ b/share/html/Elements/EditCustomFieldAutocomplete
@@ -49,18 +49,18 @@
<textarea cols="<% $Cols %>" rows="<% $Rows %>" name="<% $name %>-Values" id="<% $name %>-Values" class="CF-<%$CustomField->id%>-Edit"><% $Default %></textarea><div id="<% $name %>-Choices" class="autocomplete"></div>
<script type="text/javascript">
new Ajax.Autocompleter(
- "<% $name %>-Values",
- "<% $name %>-Choices",
- "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues",
+ <% $name |n,j%>+"-Values",
+ <% $name |n,j%>+"-Choices",
+ <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/CustomFieldValues",
{ tokens: [ '\n' ] }
);
% } else {
<input type="text" id="<% $name %>-Value" name="<% $name %>-Value" class="CF-<%$CustomField->id%>-Edit" value="<% $Default %>"/><div id="<% $name %>-Choices" class="autocomplete"></div>
<script type="text/javascript">
new Ajax.Autocompleter(
- "<% $name %>-Value",
- "<% $name %>-Choices",
- "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues",
+ <% $name |n,j%>+"-Value",
+ <% $name |n,j%>+"-Choices",
+ <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/CustomFieldValues",
{}
);
% }
diff --git a/share/html/Elements/EditCustomFieldSelect b/share/html/Elements/EditCustomFieldSelect
index bf2a828..f106a70 100644
--- a/share/html/Elements/EditCustomFieldSelect
+++ b/share/html/Elements/EditCustomFieldSelect
@@ -55,7 +55,7 @@
% if (!$HideCategory and @category and not $CustomField->BasedOnObj->id) {
<script type="text/javascript" src="<%RT->Config->Get('WebPath')%>/NoAuth/js/cascaded.js"></script>
%# XXX - Hide this select from w3m?
- <select onchange="filter_cascade('<% $id %>-Values', this.value)" name="<% $id %>-Category" class="CF-<%$CustomField->id%>-Edit">
+ <select onchange="filter_cascade(<% "$id-Values" |n,j%>, this.value)" name="<% $id %>-Category" class="CF-<%$CustomField->id%>-Edit">
<option value=""<% !$selected && qq[ selected="selected"] |n %>><&|/l&>-</&></option>
% foreach my $cat (@category) {
% my ($depth, $name) = @$cat;
@@ -66,12 +66,12 @@
<script type="text/javascript" src="<%RT->Config->Get('WebPath')%>/NoAuth/js/cascaded.js"></script>
<script type="text/javascript"><!--
doOnLoad( function () {
- var basedon = document.getElementById('<% $NamePrefix . $CustomField->BasedOnObj->id %>-Values');
+ var basedon = document.getElementById(<% $NamePrefix . $CustomField->BasedOnObj->id . "-Values" |n,j%>);
if (basedon != null) {
var oldchange = basedon.onchange;
basedon.onchange = function () {
filter_cascade(
- '<% $id %>-Values',
+ <% "$id-Values" |n,j%>,
basedon.value,
1
);
diff --git a/share/html/Elements/HeaderJavascript b/share/html/Elements/HeaderJavascript
index ce0b976..95be989 100644
--- a/share/html/Elements/HeaderJavascript
+++ b/share/html/Elements/HeaderJavascript
@@ -60,7 +60,7 @@ $onload => undef
<script type="text/javascript"><!--
doOnLoad(loadTitleBoxStates);
% if ( $focus ) {
- doOnLoad(function () { focusElementById('<% $focus %>') });
+ doOnLoad(function () { focusElementById(<% $focus |n,j%>) });
% }
% if ( $onload ) {
@@ -112,8 +112,8 @@ $onload => undef
typeField.setAttribute('value', 'text/html');
textArea.parentNode.appendChild(typeField);
- var oFCKeditor = new FCKeditor( textArea.name, '100%', <% RT->Config->Get('MessageBoxRichTextHeight', $session{CurrentUser} ) %> );
- oFCKeditor.BasePath = "<%RT->Config->Get('WebPath')%>/NoAuth/RichText/";
+ var oFCKeditor = new FCKeditor( textArea.name, '100%', <% RT->Config->Get('MessageBoxRichTextHeight', $session{CurrentUser} ) |n,j%> );
+ oFCKeditor.BasePath = <%RT->Config->Get('WebPath') |n,j%>+"/NoAuth/RichText/";
oFCKeditor.ReplaceTextarea();
}
}
diff --git a/share/html/Elements/RT__CustomField/ColumnMap b/share/html/Elements/RT__CustomField/ColumnMap
index c0e17f2..ecaa3b7 100644
--- a/share/html/Elements/RT__CustomField/ColumnMap
+++ b/share/html/Elements/RT__CustomField/ColumnMap
@@ -120,8 +120,10 @@ my $COLUMN_MAP = {
my $name = 'RemoveCustomField';
my $checked = $m->request_args->{ $name .'All' }? 'checked="checked"': '';
- return \qq{<input type="checkbox" name="${name}All" value="1" $checked
- onclick="setCheckbox(this.form, '$name', this.checked)" />};
+ return \qq{<input type="checkbox" name="}, $name, \qq{All" value="1" $checked
+ onclick="setCheckbox(this.form, },
+ $m->interp->apply_escapes($name,'j'),
+ \qq{, this.checked)" />};
},
value => sub {
my $id = $_[0]->id;
@@ -137,7 +139,7 @@ my $COLUMN_MAP = {
elsif ( $arg ) {
$checked = 'checked="checked"' if $arg == $id;
}
- return \qq{<input type="checkbox" name="$name" value="$id" $checked />}
+ return \qq{<input type="checkbox" name="}, $name, \qq{" value="$id" $checked />}
},
},
MoveCF => {
diff --git a/share/html/Elements/SelectDate b/share/html/Elements/SelectDate
index df4dc2b..784fad9 100755
--- a/share/html/Elements/SelectDate
+++ b/share/html/Elements/SelectDate
@@ -46,7 +46,7 @@
%#
%# END BPS TAGGED BLOCK }}}
<script type="text/javascript"><!--
- onLoadHook('createCalendarLink("<% $Name %>");');
+ onLoadHook("createCalendarLink(<% $Name |n,j%>);");
--></script>
<input type="text" id="<% $Name %>" name="<% $Name %>" value="<% $Value %>" size="<% $Size %>" />
<%init>
diff --git a/share/html/Elements/ShowCustomFields b/share/html/Elements/ShowCustomFields
index 1bb6143..efbbfa8 100644
--- a/share/html/Elements/ShowCustomFields
+++ b/share/html/Elements/ShowCustomFields
@@ -108,13 +108,13 @@ my $print_value = sub {
if ( $cf->IncludeContentForValue ) {
my $vid = $value->id;
$m->out( '<div class="object_cf_value_include" id="object_cf_value_'. $vid .'">' );
- $m->print( loc("See also:") );
- $m->out( '<a href="'. $value->IncludeContentForValue .'">' );
- $m->print( $value->IncludeContentForValue );
+ $m->out( loc("See also:") );
+ $m->out( '<a href="'. $m->interp->apply_escapes($value->IncludeContentForValue, 'h') .'">' );
+ $m->out( $m->interp->apply_escapes($value->IncludeContentForValue, 'h') );
$m->out( qq{</a></div>\n} );
- $m->out( qq{<script><!--\nahah('} );
- $m->print( $value->IncludeContentForValue );
- $m->out( qq{', 'object_cf_value_$vid');\n--></script>\n} );
+ $m->out( qq{<script><!--\nahah(} );
+ $m->out( $m->interp->apply_escapes($value->IncludeContentForValue, 'j') );
+ $m->out( qq{, 'object_cf_value_$vid');\n--></script>\n} );
}
};
diff --git a/share/html/Elements/Submit b/share/html/Elements/Submit
index fd2ecde..a1970d9 100755
--- a/share/html/Elements/Submit
+++ b/share/html/Elements/Submit
@@ -52,10 +52,10 @@ id="<%$id%>"
>
<div class="extra-buttons">
% if ($CheckAll) {
- <input type="button" value="<%$CheckAllLabel%>" onclick="setCheckbox(this.form, '<% $CheckboxName %>', true);return false;" class="button" />
+ <input type="button" value="<%$CheckAllLabel%>" onclick="setCheckbox(this.form, <% $CheckboxName |n,j%>, true);return false;" class="button" />
% }
% if ($ClearAll) {
- <input type="button" value="<%$ClearAllLabel%>" onclick="setCheckbox(this.form, '<% $CheckboxName %>', false);return false;" class="button" />
+ <input type="button" value="<%$ClearAllLabel%>" onclick="setCheckbox(this.form, <% $CheckboxName |n,j%>, false);return false;" class="button" />
% }
% if ($Reset) {
<input type="reset" value="<%$ResetLabel%>" class="button" />
diff --git a/share/html/Helpers/CalPopup.html b/share/html/Helpers/CalPopup.html
index cd812d3..ca9d3b5 100644
--- a/share/html/Helpers/CalPopup.html
+++ b/share/html/Helpers/CalPopup.html
@@ -74,7 +74,7 @@
% if ( ( $DisplayedYear == $today[5] + 1900 ) && ( $DisplayedMonth == $today[4] + 1 ) && ( $day == $today[3] ) ) {
% $class = 'today';
% }
- <a <% $class && 'class="'.$class.'"' |n%> href="#" onclick="updateParentField('<% $field %>','<% $datestr %>'); return false;"><% $day %></a>
+ <a <% $class && 'class="'.$class.'"' |n%> href="#" onclick="updateParentField(<% $field |n,j%>,<% $datestr |n,j%>); return false;"><% $day %></a>
% } else {
% }
diff --git a/share/html/NoAuth/js/titlebox-state.js b/share/html/NoAuth/js/titlebox-state.js
index ac0c2f0..2d31ec3 100644
--- a/share/html/NoAuth/js/titlebox-state.js
+++ b/share/html/NoAuth/js/titlebox-state.js
@@ -46,7 +46,7 @@
%#
%# END BPS TAGGED BLOCK }}}
function createCookie(name,value,days) {
- var path = "<%RT->Config->Get('WebPath')%>" ? "<%RT->Config->Get('WebPath')%>" : "/";
+ var path = <%RT->Config->Get('WebPath')|n,j%> ? <%RT->Config->Get('WebPath')|n,j%> : "/";
if (days) {
var date = new Date();
diff --git a/share/html/NoAuth/js/util.js b/share/html/NoAuth/js/util.js
index d8ce74c..8f1c52e 100644
--- a/share/html/NoAuth/js/util.js
+++ b/share/html/NoAuth/js/util.js
@@ -192,7 +192,7 @@ function doOnLoad(handler) {
/* calendar functions */
function openCalWindow(field) {
- var objWindow = window.open('<%RT->Config->Get('WebPath')%>/Helpers/CalPopup.html?field='+field,
+ var objWindow = window.open(<%RT->Config->Get('WebPath')|n,j%>+'/Helpers/CalPopup.html?field='+field,
'RT_Calendar',
'height=235,width=285,scrollbars=1');
objWindow.focus();
@@ -206,7 +206,7 @@ function createCalendarLink(input) {
$(link).observe('click', function(ev) { openCalWindow(input); ev.stop(); });
//link.setAttribute('onclick', "openCalWindow('"+input+"'); return false;");
- var text = document.createTextNode('<% loc("Calendar") %>');
+ var text = document.createTextNode(<% loc("Calendar") |n,j%>);
link.appendChild(text);
var space = document.createTextNode(' ');
diff --git a/share/html/Ticket/Elements/Bookmark b/share/html/Ticket/Elements/Bookmark
index 28034c5..6fc1fd3 100644
--- a/share/html/Ticket/Elements/Bookmark
+++ b/share/html/Ticket/Elements/Bookmark
@@ -83,7 +83,7 @@ $Toggle => 0
</%ARGS>
<span id="toggle-<% $id %>" class="toggle-<% $id %>">
% my $url = RT->Config->Get('WebPath') ."/Helpers/Toggle/TicketBookmark?id=". $id;
-<a align="right" href="<% $url %>" onclick="toggleTicketBookmark('<% $id|n %>', '<% $url %>'); return false;">
+<a align="right" href="<% $url %>" onclick="toggleTicketBookmark(<% $id|n,j %>, <% $url|n,j %>); return false;">
% if ( $bookmarked ) {
<img src="<% RT->Config->Get('WebPath') %>/NoAuth/images/star.gif" alt="<% loc('Remove Bookmark') %>" style="border-style: none" />
% } else {
diff --git a/share/html/Ticket/Elements/UpdateCc b/share/html/Ticket/Elements/UpdateCc
index f30fdad..d88a64e 100644
--- a/share/html/Ticket/Elements/UpdateCc
+++ b/share/html/Ticket/Elements/UpdateCc
@@ -58,8 +58,7 @@ id="UpdateCc-<%$addr%>"
name="UpdateCc-<%$addr%>"
type="checkbox"
% my $clean_addr = $txn_addresses{$addr}->format;
-% $clean_addr =~ s/'/\\'/g;
- onClick="checkboxToInput('UpdateCc', 'UpdateCc-<%$addr%>','<%$clean_addr%>' ); $(UpdateIgnoreAddressCheckboxes).value=1"
+ onClick="checkboxToInput('UpdateCc', <% "UpdateCc-$addr" |n,j%>, <%$clean_addr|n,j%> ); $(UpdateIgnoreAddressCheckboxes).value=1"
<% $ARGS{'UpdateCc-'.$addr} ? 'checked="checked"' : ''%> > <& /Elements/ShowUser, Address => $txn_addresses{$addr}&>
%}
</td></tr>
@@ -73,8 +72,7 @@ name="UpdateCc-<%$addr%>"
name="UpdateBcc-<%$addr%>"
type="checkbox"
% my $clean_addr = $txn_addresses{$addr}->format;
-% $clean_addr =~ s/'/\\'/g;
- onClick="checkboxToInput('UpdateBcc', 'UpdateBcc-<%$addr%>','<%$clean_addr%>' ); $(UpdateIgnoreAddressCheckboxes).value=1"
+ onClick="checkboxToInput('UpdateBcc', <% "UpdateBcc-$addr" |n,j%>, <%$clean_addr|n,j%> ); $(UpdateIgnoreAddressCheckboxes).value=1"
<% $ARGS{'UpdateBcc-'.$addr} ? 'checked="checked"' : ''%>>
<& /Elements/ShowUser, Address => $txn_addresses{$addr}&>
%}
diff --git a/share/html/Ticket/Graphs/Elements/EditGraphProperties b/share/html/Ticket/Graphs/Elements/EditGraphProperties
index beb67a2..b1fc1c3 100644
--- a/share/html/Ticket/Graphs/Elements/EditGraphProperties
+++ b/share/html/Ticket/Graphs/Elements/EditGraphProperties
@@ -151,7 +151,7 @@ my $class = '';
$class = 'class="hidden"' if $Level != 1 && !@Default;
</%INIT>
<% loc('Show Tickets Properties on [_1] level', $Level) %>
-(<small><a href="#" onclick="hideshow('<% $id %>'); return false;"><% loc('open/close') %></a></small>):
+(<small><a href="#" onclick="hideshow(<% $id |n,j%>); return false;"><% loc('open/close') %></a></small>):
<table id="<% $id %>" <% $class |n %>>
% while ( my ($group, $list) = (splice @Available, 0, 2) ) {
<tr><td><% loc($group) %>:</td><td>
diff --git a/share/html/Widgets/ComboBox b/share/html/Widgets/ComboBox
index 6d4e9f7..d4e4c2c 100644
--- a/share/html/Widgets/ComboBox
+++ b/share/html/Widgets/ComboBox
@@ -56,7 +56,7 @@ my $z_index = 9999;
<div id="<% $Name %>_Container" class="combobox <%$Class%>" style="z-index: <%$z_index--%>">
<input name="<% $Name %>" id="<% $Name %>" class="combo-text" value="<% $Default || '' %>" type="text" <% $Size ? "size='$Size'" : '' |n %> autocomplete="off" />
-<br style="display: none" /><span id="<% $Name %>_Button" class="combo-button">▼</span><select name="List-<% $Name %>" id="<% $Name %>_List" class="combo-list" onchange="ComboBox_SimpleAttach(this, this.form['<% $Name %>']); " size="<% $Rows %>">
+<br style="display: none" /><span id="<% $Name %>_Button" class="combo-button">▼</span><select name="List-<% $Name %>" id="<% $Name %>_List" class="combo-list" onchange="ComboBox_SimpleAttach(this, this.form[<% $Name |n,j%>]); " size="<% $Rows %>">
<option style="display: none" value="">-</option>
% foreach my $value (@Values) {
<option value="<%$value%>"><% $value%></option>
@@ -64,7 +64,7 @@ my $z_index = 9999;
</select>
</div>
<script language="javascript"><!--
-ComboBox_InitWith('<% $Name %>');
+ComboBox_InitWith(<% $Name |n,j %>);
//--></script>
</nobr>
<%ARGS>
diff --git a/share/html/Widgets/TitleBoxStart b/share/html/Widgets/TitleBoxStart
index 492cfab..0c06129 100755
--- a/share/html/Widgets/TitleBoxStart
+++ b/share/html/Widgets/TitleBoxStart
@@ -49,7 +49,7 @@
<div class="titlebox-title<% $title_class ? " $title_class" : ''%>">
% if ($hideable) {
<span class="widget"><a href="#"
- onclick="return rollup('<%$tid%>');"
+ onclick="return rollup(<%$tid|n,j%>);"
title="Toggle visibility"></a>
</span>
% }
commit 87ac5328d5a13f8e99e2ca7783e28d97c15912cc
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Nov 15 16:23:19 2011 -0500
Prevent linking directly to CF values when the value is a data: URI
You can still create data: URIs in a linked CF with the value as part of
the URI, but the whole value can't be a data: URI itself.
This resolves part of CVE-2011-2083.
diff --git a/lib/RT/ObjectCustomFieldValue_Overlay.pm b/lib/RT/ObjectCustomFieldValue_Overlay.pm
index 403d216..5c925ab 100644
--- a/lib/RT/ObjectCustomFieldValue_Overlay.pm
+++ b/lib/RT/ObjectCustomFieldValue_Overlay.pm
@@ -253,11 +253,11 @@ sub _FillInTemplateURL {
# special case, whole value should be an URL
if ( $url =~ /^__CustomField__/ ) {
my $value = $self->Content;
- # protect from javascript: URLs
- if ( $value =~ /^\s*javascript:/i ) {
+ # protect from potentially malicious URLs
+ if ( $value =~ /^\s*(?:javascript|data):/i ) {
my $object = $self->Object;
$RT::Logger->error(
- "Dangerouse value with JavaScript in custom field '". $self->CustomFieldObj->Name ."'"
+ "Potentially dangerous URL type in custom field '". $self->CustomFieldObj->Name ."'"
." on ". ref($object) ." #". $object->id
);
return undef;
commit cd180c1f57602555614ef0d57e128f1cad544e87
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Nov 16 17:07:47 2011 -0500
Escape wrap parameter when rendering a message box
This resolves part of CVE-2011-2083.
diff --git a/share/html/Elements/MessageBox b/share/html/Elements/MessageBox
index 3bc73eb..3ca8dc8 100755
--- a/share/html/Elements/MessageBox
+++ b/share/html/Elements/MessageBox
@@ -67,7 +67,7 @@ if ( $IncludeSignature and my $text = $session{'CurrentUser'}->UserObj->Signatur
# wrap="something" seems to really break IE + richtext
my $wrap_type = '';
if ( not RT->Config->Get('MessageBoxRichText', $session{'CurrentUser'}) ) {
- $wrap_type = qq(wrap="$Wrap");
+ $wrap_type = 'wrap="' . $m->interp->apply_escapes($Wrap, 'h') . '"';
}
</%INIT>
commit 05a6e45a448b0f2712a2356829ef78a1e7385d60
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Mar 28 18:33:56 2012 -0400
Escape NamePrefix to avoid XSS if it's passed into EditCustomField
This resolves part of CVE-2011-2083. Ticket #42936.
diff --git a/share/html/Elements/EditCustomField b/share/html/Elements/EditCustomField
index 6c5d7f5..32ea59d 100644
--- a/share/html/Elements/EditCustomField
+++ b/share/html/Elements/EditCustomField
@@ -85,7 +85,7 @@ if ($MaxValues == 1 && $Values) {
}
# The "Magic" hidden input causes RT to know that we were trying to edit the field, even if
# we don't see a value later, since browsers aren't compelled to submit empty form fields
-$m->out("\n".'<input type="hidden" class="hidden" name="'.$NamePrefix.$CustomField->Id.'-Values-Magic" value="1" />'."\n");
+$m->out("\n".'<input type="hidden" class="hidden" name="'.$m->interp->apply_escapes($NamePrefix, 'h').$CustomField->Id.'-Values-Magic" value="1" />'."\n");
my $EditComponent = "EditCustomField$Type";
$m->callback( %ARGS, CallbackName => 'EditComponentName', Name => \$EditComponent, CustomField => $CustomField, Object => $Object );
commit 56f24489b5f7a43015c528dd305f775e49911e79
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Mar 29 13:47:42 2012 -0400
Close an XSS vector via BaseURL in collection lists
Most uses of CollectionList don't pass in a user-modifiable BaseURL, so
they're generally safe.
Resolves part of CVE-2011-2083. Ticket #58595.
diff --git a/share/html/Elements/CollectionAsTable/Header b/share/html/Elements/CollectionAsTable/Header
index 878a77e..75aaa3c 100644
--- a/share/html/Elements/CollectionAsTable/Header
+++ b/share/html/Elements/CollectionAsTable/Header
@@ -129,11 +129,11 @@ foreach my $col ( @Format ) {
if $OrderBy[0] && $OrderBy[0] eq $attr;
$m->out(
- '<a href="' . $BaseURL
+ '<a href="' . $m->interp->apply_escapes($BaseURL
. $m->comp( '/Elements/QueryString',
%$generic_query_args,
OrderBy => $attr, Order => $new_order
- )
+ ), 'h')
. '">'. loc($title) .'</a>'
);
}
diff --git a/share/html/Elements/CollectionListPaging b/share/html/Elements/CollectionListPaging
index 7be9ea6..89cf0fa 100644
--- a/share/html/Elements/CollectionListPaging
+++ b/share/html/Elements/CollectionListPaging
@@ -55,22 +55,24 @@ $URLParams => undef
</%ARGS>
<%INIT>
+$BaseURL = $m->interp->apply_escapes($BaseURL, 'h');
+
$m->out(qq{<div class="paging">});
if ($Pages == 1) {
$m->out(loc('Page 1 of 1'));
}
else{
$m->out(loc('Page') . ' ');
-my $prev = $m->comp(
+my $prev = $m->interp->apply_escapes($m->comp(
'/Elements/QueryString',
%$URLParams,
Page => ( $CurrentPage - 1 )
- );
-my $next = $m->comp(
+ ), 'h');
+my $next = $m->interp->apply_escapes($m->comp(
'/Elements/QueryString',
%$URLParams,
Page => ( $CurrentPage + 1 )
- );
+ ), 'h');
my %show;
$show{1} = 1;
$show{$_} = 1 for (($CurrentPage - 2)..($CurrentPage + 2));
@@ -81,7 +83,7 @@ for my $number ( 1 .. $Pages ) {
if ( $show{$number} ) {
$dots = undef;
my $qs =
- $m->comp( '/Elements/QueryString', %$URLParams, Page => $number );
+ $m->interp->apply_escapes($m->comp( '/Elements/QueryString', %$URLParams, Page => $number ), 'h');
$m->out(qq{<span class="pagenum">});
if ( $number == $CurrentPage ) {
$m->out(qq{<span class="currentpage">$number</span> });
commit 290b46b20d2d2fb84fea2a707e51cc049617469a
Merge: 6928fac adc0df3
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:02:18 2012 -0400
Merge branch 'security/3.8/vulnerable-passwords' into security/3.8-trunk
commit e5399ceb1ae24cea0e18800a004ac1ac8d3539f8
Merge: 290b46b cfd4d89
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:02:22 2012 -0400
Merge branch 'security/3.8/escape-flags' into security/3.8-trunk
commit baa7f1e1bf952194fd39ea95884184a756039c23
Merge: e5399ce d17991d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:03:18 2012 -0400
Merge branch 'security/3.8/slash-l-xss' into security/3.8-trunk
commit aa6923451b824192828867b388e49dd46971c13d
Merge: baa7f1e 56f2448
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:04:36 2012 -0400
Merge branch 'security/3.8/xss' into security/3.8-trunk
commit 0d977a01e9524922c58ac31e345c2696e91efc26
Merge: aa69234 01cecde
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:04:51 2012 -0400
Merge branch 'security/3.8/clickable-xss-links' into security/3.8-trunk
commit be375067d28f2fa10f112da4a51f8b87b787f07c
Merge: 0d977a0 52c4d4c
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:05:12 2012 -0400
Merge branch 'security/3.8/mason-runtime-errors' into security/3.8-trunk
commit f8770f8538f28113989d067fdf62b08b0b121727
Merge: be37506 0b5f3d8
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:05:18 2012 -0400
Merge branch 'security/3.8/scrub-class-id' into security/3.8-trunk
commit 5aa4f5ceab5ef0bf5263e8b3a7bcd9b0e86c27c7
Merge: f8770f8 4c56578
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:05:28 2012 -0400
Merge branch 'security/3.8/stricter-scrips-templates-acls' into security/3.8-trunk
commit e915622b841a522d5595e53e2d38ff404e8e17e8
Merge: 5aa4f5c c83b348
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:05:34 2012 -0400
Merge branch 'security/3.8/selfservice' into security/3.8-trunk
commit 3a705a092526cd2106bb88fce134e06f855e52ca
Merge: e915622 e2233e0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:05:39 2012 -0400
Merge branch 'security/3.8/shredder-dumps' into security/3.8-trunk
commit b3d3b2f30b574a961b9a9fcccae66c34da4a5eb5
Merge: 3a705a0 5170d90
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:05:49 2012 -0400
Merge branch 'security/3.8/attachments' into security/3.8-trunk
commit 9bf2265ce80bdb979e5c2b0c90263792fd302d42
Merge: b3d3b2f 40be851
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:05:55 2012 -0400
Merge branch 'security/3.8/cached-set-cookie' into security/3.8-trunk
commit 013ee73c2444201435755e924f195fdcfdbb8249
Merge: 9bf2265 488cdbd
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:11:22 2012 -0400
Merge branch 'security/3.8/transaction-leak' into security/3.8-trunk
commit 0597e05c4e1ea5d954c4cd9ec60c909464571380
Merge: 013ee73 cccfb9c
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:11:24 2012 -0400
Merge branch 'security/3.8/csrf-referer' into security/3.8-trunk
diff --cc share/html/Elements/CSRF
index 8459373,891d4f8..b7c1575
mode 100755,100644..100644
--- a/share/html/Elements/CSRF
+++ b/share/html/Elements/CSRF
@@@ -45,46 -45,30 +45,30 @@@
%# those contributions and any derivatives thereof.
%#
%# END BPS TAGGED BLOCK }}}
- % $m->callback( %ARGS, error => $error );
+ <& /Elements/Header, Title => loc('Possible cross-site request forgery') &>
+ <& /Elements/Tabs, Title => loc('Possible cross-site request forgery') &>
- % unless ($SuppressHeader) {
- <& /Elements/Header, Code => $Code, Why => $Why, Title => $Title &>
- <& /Elements/Tabs, Title => $Title &>
- % }
+ <h1><&|/l&>Possible cross-site request forgery</&></h1>
- <div class="error">
- <%$Why%>
- <br />
- <%$Details%>
- </div>
+ % my $strong_start = "<strong>";
+ % my $strong_end = "</strong>";
-<p><&|/l, $strong_start, $strong_end, $Reason &>RT has detected a possible [_1]cross-site request forgery[_2] for this request, because [_3]. This is possibly caused by a malicious attacker trying to perform actions against RT on your behalf. If you did not initiate this request, then you should alert your security team.</&></p>
++<p><&|/l_unsafe, $strong_start, $strong_end, $Reason &>RT has detected a possible [_1]cross-site request forgery[_2] for this request, because [_3]. This is possibly caused by a malicious attacker trying to perform actions against RT on your behalf. If you did not initiate this request, then you should alert your security team.</&></p>
- <%cleanup>
- $m->comp('/Elements/Footer');
- $m->abort();
- </%cleanup>
-
- <%args>
- $Code => undef
- $Details => ''
- $Title => loc("RT Error")
- $Why => loc("the calling component did not specify why"),
- $SuppressHeader => 0,
- </%args>
+ % my $start = qq|<strong><a href="$url_with_token">|;
+ % my $end = qq|</a></strong>|;
-<p><&|/l, $escaped_path, $start, $end &>If you really intended to visit [_1], then [_2]click here to resume your request[_3].</&></p>
++<p><&|/l_unsafe, $escaped_path, $start, $end &>If you really intended to visit [_1], then [_2]click here to resume your request[_3].</&></p>
+ <& /Elements/Footer, %ARGS &>
+ % $m->abort;
+ <%ARGS>
+ $OriginalURL => ''
+ $Reason => ''
+ $Token => ''
+ </%ARGS>
<%INIT>
- my $error = "WebRT: $Why";
- $error .= " ($Details)" if defined $Details && length $Details;
-
- # TODO: Log::Dispatch isn't UTF-8 safe. Autrijus needs to talk to dave rolsky about getting this fixed
- use Encode ();
- Encode::_utf8_off($error);
-
- $RT::Logger->error($error);
+ my $escaped_path = $m->interp->apply_escapes($OriginalURL, 'h');
+ $escaped_path = "<tt>$escaped_path</tt>";
- if ( defined $session{'SessionType'} && $session{'SessionType'} eq 'REST' ) {
- $r->content_type('text/plain');
- $m->out( "Error: " . $Why . "\n" );
- $m->out( $Details . "\n" ) if defined $Details && length $Details;
- $m->abort();
- }
+ my $url_with_token = URI->new($OriginalURL);
+ $url_with_token->query_form([CSRF_Token => $Token]);
</%INIT>
commit c01f5852cc56e056198a9bb6110842d7553856a8
Merge: 0597e05 bb917e0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:11:41 2012 -0400
Merge branch 'security/3.8/arbitrary-methods' into security/3.8-trunk
commit 7040ff301c762d7b30335f3808b06b4ebfe3523b
Merge: c01f585 ef35fe5
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:11:42 2012 -0400
Merge branch 'security/3.8/verp-code-execution' into security/3.8-trunk
commit f4513aee9e19bef089b5aa0586b033e291b8c509
Merge: 7040ff3 1f71f5d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:11:43 2012 -0400
Merge branch 'security/3.8/private-components' into security/3.8-trunk
commit b6a06ebea3dac5915979055f0a0508a846829033
Merge: f4513ae b784bcb
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:11:43 2012 -0400
Merge branch 'security/3.8/installmode' into security/3.8-trunk
commit bf574b144f70e287d5750dc70735f93a4bfb69e5
Merge: b6a06eb ad31208
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:11:44 2012 -0400
Merge branch 'security/3.8/paging-injection' into security/3.8-trunk
commit 9b6e230856538dc8f3801a21f2261fe93a4f493b
Merge: bf574b14 951add5
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:11:45 2012 -0400
Merge branch 'security/3.8/graphviz-escaping' into security/3.8-trunk
commit 02325246190c18f11b1f4056d7c6e7c3fa1f6a9b
Merge: 9b6e230 6a9a41d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 15:12:01 2012 -0400
Merge branch 'security/3.8/custom-field-values' into security/3.8-trunk
Conflicts:
share/html/Elements/EditCustomFieldAutocomplete
diff --cc share/html/Elements/EditCustomFieldAutocomplete
index 25447e7,c16aa8a..70ff396
--- a/share/html/Elements/EditCustomFieldAutocomplete
+++ b/share/html/Elements/EditCustomFieldAutocomplete
@@@ -49,19 -49,19 +49,19 @@@
<textarea cols="<% $Cols %>" rows="<% $Rows %>" name="<% $name %>-Values" id="<% $name %>-Values" class="CF-<%$CustomField->id%>-Edit"><% $Default %></textarea><div id="<% $name %>-Choices" class="autocomplete"></div>
<script type="text/javascript">
new Ajax.Autocompleter(
- "<% $name %>-Values",
- "<% $name %>-Choices",
- "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues",
- { tokens: [ '\n' ], parameters: "<% $Context |n %>" }
+ <% $name |n,j%>+"-Values",
+ <% $name |n,j%>+"-Choices",
+ <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/CustomFieldValues",
- { tokens: [ '\n' ] }
++ { tokens: [ '\n' ], parameters: <% $Context |n,j %> }
);
% } else {
<input type="text" id="<% $name %>-Value" name="<% $name %>-Value" class="CF-<%$CustomField->id%>-Edit" value="<% $Default %>"/><div id="<% $name %>-Choices" class="autocomplete"></div>
<script type="text/javascript">
new Ajax.Autocompleter(
- "<% $name %>-Value",
- "<% $name %>-Choices",
- "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues",
- { parameters: "<% $Context |n %>" }
+ <% $name |n,j%>+"-Value",
+ <% $name |n,j%>+"-Choices",
+ <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/CustomFieldValues",
- {}
++ { parameters: <% $Context |n,j %> }
);
% }
</script>
commit eb44f1060ac3e78a5063aa6982c033d7bbf783aa
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Jan 9 15:34:04 2012 -0500
Test that RT::Users->WhoHaveRight doesn't pick up disabled groups
This test file currently fails. The bug is noticeable in the owner
lists since SelectOwner is the only place which uses WhoHaveRight.
Fixing it requires checking Disabled on the Principals table joined
against Groups (not Users, which is already joined). Alternatively, we
should be able to check Disabled on CachedGroupMembers as long as we
make sure the check always goes through CGM instead of sometimes
GroupMembers and sometimes CGM.
diff --git a/t/web/owner_disabled_group_19221.t b/t/web/owner_disabled_group_19221.t
new file mode 100644
index 0000000..53ad713
--- /dev/null
+++ b/t/web/owner_disabled_group_19221.t
@@ -0,0 +1,64 @@
+#!/usr/bin/env perl
+use strict;
+use warnings;
+
+use RT::Test tests => undef;
+
+my $queue = RT::Test->load_or_create_queue( Name => 'Test' );
+ok $queue && $queue->id, 'loaded or created queue';
+
+my $user = RT::Test->load_or_create_user(
+ Name => 'ausername',
+ Privileged => 1,
+);
+ok $user && $user->id, 'loaded or created user';
+
+my $group = RT::Group->new(RT->SystemUser);
+my ($ok, $msg) = $group->CreateUserDefinedGroup(Name => 'Disabled Group');
+ok($ok, $msg);
+
+($ok, $msg) = $group->AddMember( $user->PrincipalId );
+ok($ok, $msg);
+
+ok( RT::Test->set_rights({
+ Principal => $group,
+ Object => $queue,
+ Right => [qw(OwnTicket)]
+}), 'set rights');
+
+RT->Config->Set( AutocompleteOwners => 0 );
+my ($base, $m) = RT::Test->started_ok;
+ok $m->login, 'logged in';
+
+diag "user from group shows up in create form";
+{
+ $m->get_ok('/', 'open home page');
+ $m->form_name('CreateTicketInQueue');
+ $m->select( 'Queue', $queue->id );
+ $m->submit;
+
+ $m->content_contains('Create a new ticket', 'opened create ticket page');
+ my $form = $m->form_name('TicketCreate');
+ my $input = $form->find_input('Owner');
+ is $input->value, RT->Nobody->Id, 'correct owner selected';
+ ok((scalar grep { $_ == $user->Id } $input->possible_values), 'user from group is in dropdown');
+}
+
+diag "user from disabled group DOESN'T shows up in create form";
+{
+ ($ok, $msg) = $group->SetDisabled(1);
+ ok($ok, $msg);
+
+ $m->get_ok('/', 'open home page');
+ $m->form_name('CreateTicketInQueue');
+ $m->select( 'Queue', $queue->id );
+ $m->submit;
+
+ $m->content_contains('Create a new ticket', 'opened create ticket page');
+ my $form = $m->form_name('TicketCreate');
+ my $input = $form->find_input('Owner');
+ is $input->value, RT->Nobody->Id, 'correct owner selected';
+ ok((not scalar grep { $_ == $user->Id } $input->possible_values), 'user from disabled group is NOT in dropdown');
+}
+
+undef $m;
commit 1d3432b2a14434c775a2bf637e7ec2bde4448bd5
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Mar 26 13:35:01 2012 -0400
Ensure that all joins through CachedGroupMembers limits to non-disabled rows
When a group becomes disabled in RT, we mark all CGM rows that existed
because of that group as 'Disabled'. Unfortunately, many joins through
CGM neglected to take the Disabled column into account, leading to users
possibly having rights that they should not, due to having them by way
of a disabled group.
This addresses CVE-2011-4459.
diff --git a/lib/RT/ACL_Overlay.pm b/lib/RT/ACL_Overlay.pm
index feef257..a0429a3 100755
--- a/lib/RT/ACL_Overlay.pm
+++ b/lib/RT/ACL_Overlay.pm
@@ -175,6 +175,9 @@ sub LimitToPrincipal {
FIELD1 => 'PrincipalId',
ALIAS2 => $cgm,
FIELD2 => 'GroupId' );
+ $self->Limit( ALIAS => $cgm,
+ FIELD => 'Disabled',
+ VALUE => 0 );
$self->Limit( ALIAS => $cgm,
FIELD => 'MemberId',
OPERATOR => '=',
diff --git a/lib/RT/Groups_Overlay.pm b/lib/RT/Groups_Overlay.pm
index ed18939..aef37b2 100755
--- a/lib/RT/Groups_Overlay.pm
+++ b/lib/RT/Groups_Overlay.pm
@@ -263,6 +263,8 @@ sub WithMember {
ALIAS2 => $members, FIELD2 => 'GroupId');
$self->Limit(ALIAS => $members, FIELD => 'MemberId', OPERATOR => '=', VALUE => $args{'PrincipalId'});
+ $self->Limit(ALIAS => $members, FIELD => 'Disabled', VALUE => 0)
+ if $args{'Recursively'};
}
sub WithoutMember {
@@ -288,6 +290,12 @@ sub WithoutMember {
VALUE => $args{'PrincipalId'},
);
$self->Limit(
+ LEFTJOIN => $members_alias,
+ ALIAS => $members_alias,
+ FIELD => 'Disabled',
+ VALUE => 0
+ ) if $args{'Recursively'};
+ $self->Limit(
ALIAS => $members_alias,
FIELD => 'MemberId',
OPERATOR => 'IS',
diff --git a/lib/RT/Tickets_Overlay.pm b/lib/RT/Tickets_Overlay.pm
index d3a3599..40de7ce 100755
--- a/lib/RT/Tickets_Overlay.pm
+++ b/lib/RT/Tickets_Overlay.pm
@@ -1032,6 +1032,12 @@ sub _GroupMembersJoin {
FIELD2 => 'GroupId',
ENTRYAGGREGATOR => 'AND',
);
+ $self->SUPER::Limit(
+ LEFTJOIN => $alias,
+ ALIAS => $alias,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ );
$self->{'_sql_group_members_aliases'}{ $args{'GroupsAlias'} } = $alias
unless $args{'New'};
@@ -1196,6 +1202,12 @@ sub _WatcherMembershipLimit {
FIELD2 => 'id'
);
+ $self->Limit(
+ ALIAS => $groupmembers,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ );
+
$self->Join(
ALIAS1 => $memberships,
FIELD1 => 'MemberId',
@@ -1203,6 +1215,13 @@ sub _WatcherMembershipLimit {
FIELD2 => 'id'
);
+ $self->Limit(
+ ALIAS => $memberships,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ );
+
+
$self->_CloseParen;
}
diff --git a/lib/RT/User_Overlay.pm b/lib/RT/User_Overlay.pm
index 84bee0e..a7f008d 100755
--- a/lib/RT/User_Overlay.pm
+++ b/lib/RT/User_Overlay.pm
@@ -1538,6 +1538,12 @@ sub WatchedQueues {
FIELD => 'MemberId',
VALUE => $self->PrincipalId,
);
+ $watched_queues->Limit(
+ ALIAS => $queues_alias,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ );
+
$RT::Logger->debug("WatchedQueues got " . $watched_queues->Count . " queues");
diff --git a/lib/RT/Users_Overlay.pm b/lib/RT/Users_Overlay.pm
index 9640925..681c1b3 100755
--- a/lib/RT/Users_Overlay.pm
+++ b/lib/RT/Users_Overlay.pm
@@ -188,6 +188,9 @@ sub MemberOfGroup {
FIELD1 => 'id',
ALIAS2 => $groupalias,
FIELD2 => 'MemberId' );
+ $self->Limit( ALIAS => $groupalias,
+ FIELD => 'Disabled',
+ VALUE => 0 );
$self->Limit( ALIAS => "$groupalias",
FIELD => 'GroupId',
@@ -259,6 +262,11 @@ sub _JoinGroupMembers
ALIAS2 => $principals,
FIELD2 => 'id'
);
+ $self->Limit(
+ ALIAS => $group_members,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ ) if $args{'IncludeSubgroupMembers'};
return $group_members;
}
@@ -277,6 +285,11 @@ sub _JoinGroups
ALIAS2 => $group_members,
FIELD2 => 'GroupId'
);
+ $self->Limit(
+ ALIAS => $groups,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ );
return $groups;
}
diff --git a/t/web/owner_disabled_group_19221.t b/t/web/owner_disabled_group_19221.t
index 53ad713..8327482 100644
--- a/t/web/owner_disabled_group_19221.t
+++ b/t/web/owner_disabled_group_19221.t
@@ -59,6 +59,131 @@ diag "user from disabled group DOESN'T shows up in create form";
my $input = $form->find_input('Owner');
is $input->value, RT->Nobody->Id, 'correct owner selected';
ok((not scalar grep { $_ == $user->Id } $input->possible_values), 'user from disabled group is NOT in dropdown');
+ ($ok, $msg) = $group->SetDisabled(0);
+ ok($ok, $msg);
+}
+
+
+
+diag "Put us in a nested group";
+my $super = RT::Group->new(RT->SystemUser);
+($ok, $msg) = $super->CreateUserDefinedGroup(Name => 'Supergroup');
+ok($ok, $msg);
+
+($ok, $msg) = $super->AddMember( $group->PrincipalId );
+ok($ok, $msg);
+
+ok( RT::Test->set_rights({
+ Principal => $super,
+ Object => $queue,
+ Right => [qw(OwnTicket)]
+}), 'set rights');
+
+
+diag "Disable the middle group";
+{
+ ($ok, $msg) = $group->SetDisabled(1);
+ ok($ok, "Disabled group: $msg");
+
+ $m->get_ok('/', 'open home page');
+ $m->form_name('CreateTicketInQueue');
+ $m->select( 'Queue', $queue->id );
+ $m->submit;
+
+ $m->content_contains('Create a new ticket', 'opened create ticket page');
+ my $form = $m->form_name('TicketCreate');
+ my $input = $form->find_input('Owner');
+ is $input->value, RT->Nobody->Id, 'correct owner selected';
+ ok((not scalar grep { $_ == $user->Id } $input->possible_values), 'user from disabled group is NOT in dropdown');
+ ($ok, $msg) = $group->SetDisabled(0);
+ ok($ok, "Re-enabled group: $msg");
}
+diag "Disable the top group";
+{
+ ($ok, $msg) = $super->SetDisabled(1);
+ ok($ok, "Disabled supergroup: $msg");
+
+ $m->get_ok('/', 'open home page');
+ $m->form_name('CreateTicketInQueue');
+ $m->select( 'Queue', $queue->id );
+ $m->submit;
+
+ $m->content_contains('Create a new ticket', 'opened create ticket page');
+ my $form = $m->form_name('TicketCreate');
+ my $input = $form->find_input('Owner');
+ is $input->value, RT->Nobody->Id, 'correct owner selected';
+ ok((not scalar grep { $_ == $user->Id } $input->possible_values), 'user from disabled group is NOT in dropdown');
+ ($ok, $msg) = $super->SetDisabled(0);
+ ok($ok, "Re-enabled supergroup: $msg");
+}
+
+
+diag "Check WithMember and WithoutMember recursively";
+{
+ my $with = RT::Groups->new( RT->SystemUser );
+ $with->WithMember( PrincipalId => $user->PrincipalObj->Id, Recursively => 1 );
+ $with->Limit( FIELD => 'domain', OPERATOR => '=', VALUE => 'UserDefined' );
+ is_deeply(
+ [map {$_->Name} @{$with->ItemsArrayRef}],
+ ['Disabled Group','Supergroup'],
+ "Get expected recursive memberships",
+ );
+
+ my $without = RT::Groups->new( RT->SystemUser );
+ $without->WithoutMember( PrincipalId => $user->PrincipalObj->Id, Recursively => 1 );
+ $without->Limit( FIELD => 'domain', OPERATOR => '=', VALUE => 'UserDefined' );
+ is_deeply(
+ [map {$_->Name} @{$without->ItemsArrayRef}],
+ [],
+ "And not a member of no groups",
+ );
+
+ ($ok, $msg) = $super->SetDisabled(1);
+ ok($ok, "Disabled supergroup: $msg");
+ $with->RedoSearch;
+ $without->RedoSearch;
+ is_deeply(
+ [map {$_->Name} @{$with->ItemsArrayRef}],
+ ['Disabled Group'],
+ "Recursive check only contains subgroup",
+ );
+ is_deeply(
+ [map {$_->Name} @{$without->ItemsArrayRef}],
+ [],
+ "Doesn't find the currently disabled group",
+ );
+ ($ok, $msg) = $super->SetDisabled(0);
+ ok($ok, "Re-enabled supergroup: $msg");
+
+ ($ok, $msg) = $group->SetDisabled(1);
+ ok($ok, "Disabled intermediate group: $msg");
+ $with->RedoSearch;
+ $without->RedoSearch;
+ is_deeply(
+ [map {$_->Name} @{$with->ItemsArrayRef}],
+ [],
+ "Recursive check finds no groups",
+ );
+ is_deeply(
+ [map {$_->Name} @{$without->ItemsArrayRef}],
+ ['Supergroup'],
+ "Now not a member of the supergroup",
+ );
+ ($ok, $msg) = $group->SetDisabled(0);
+ ok($ok, "Re-enabled intermediate group: $msg");
+}
+
+diag "Check MemberOfGroup";
+{
+ ($ok, $msg) = $group->SetDisabled(1);
+ ok($ok, "Disabled intermediate group: $msg");
+ my $users = RT::Users->new(RT->SystemUser);
+ $users->MemberOfGroup($super->PrincipalObj->id);
+ is($users->Count, 0, "Supergroup claims no members");
+ ($ok, $msg) = $group->SetDisabled(0);
+ ok($ok, "Re-enabled intermediate group: $msg");
+}
+
+
undef $m;
commit 3f5531887c5934995688434cfdff752c27573c23
Merge: 0232524 1d3432b
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 16:12:22 2012 -0400
Merge branch 'security/3.8/disabled-group-members' into security/3.8-trunk
commit 5ad63908f7e589534d41d93bf68fd64c3817f156
Merge: 3f55318 147c38f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 16:12:23 2012 -0400
Merge branch 'security/3.8/infrastructure' into security/3.8-trunk
commit 96cde5748cd553256ba6def8a8353d5a6baf054b
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Apr 13 00:03:30 2012 -0400
Remove an incorrect Disabled limit
diff --git a/lib/RT/Users_Overlay.pm b/lib/RT/Users_Overlay.pm
index 681c1b3..b314d89 100755
--- a/lib/RT/Users_Overlay.pm
+++ b/lib/RT/Users_Overlay.pm
@@ -285,11 +285,6 @@ sub _JoinGroups
ALIAS2 => $group_members,
FIELD2 => 'GroupId'
);
- $self->Limit(
- ALIAS => $groups,
- FIELD => 'Disabled',
- VALUE => 0,
- );
return $groups;
}
commit 5daf828da793b49df4aa06cce03df9ee9fbcffca
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Fri Apr 13 16:45:33 2012 -0400
Tell users and admins what Referrer we wanted
This introduces a normalizing method we could use elsewhere in the Web
code, as well as uses that code to hide the localhost->127.0.0.1
transformations.
It also adds to the error string so that you know what RT was expecting.
"RT's configured hostname" is the best we could do without explicitly
stating WebBaseURL in a user facing error message.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index ebfb2d7..4104dac 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1069,13 +1069,28 @@ sub IsCompCSRFWhitelisted {
}
sub IsRefererCSRFWhitelisted {
- my $referer = shift;
+ my $referer = _NormalizeHost(shift);
+ my $config = _NormalizeHost(RT->Config->Get('WebBaseURL'));
- my $site = URI->new(RT->Config->Get('WebBaseURL'));
- $site->host('127.0.0.1') if $site->host eq 'localhost';
- return 1 if $referer->host_port eq $site->host_port;
+ return (1,$referer,$config) if $referer->host_port eq $config->host_port;
+
+ return (0,$referer,$config);
+}
+
+=head3 _NormalizeHost
+
+Takes a URI and creates a URI object that's been normalized
+to handle common problems such as localhost vs 127.0.0.1
+
+=cut
+
+sub _NormalizeHost {
+
+ my $uri= URI->new(shift);
+ $uri->host('127.0.0.1') if $uri->host eq 'localhost';
+
+ return $uri;
- return 0;
}
sub IsPossibleCSRF {
@@ -1114,13 +1129,12 @@ EOT
"your browser did not supply a Referrer header", # loc
) if !$ENV{HTTP_REFERER};
- my $referer = URI->new($ENV{HTTP_REFERER});
- $referer->host('127.0.0.1') if $referer->host eq 'localhost';
- return 0 if IsRefererCSRFWhitelisted($referer);
+ my ($whitelisted, $browser, $config) = IsRefererCSRFWhitelisted($ENV{HTTP_REFERER});
+ return 0 if $whitelisted;
return (1,
- "the Referrer header supplied by your browser ([_1]) is not allowed", # loc
- $referer->host_port);
+ "the Referrer header supplied by your browser ([_1]) is not allowed by RT's configured hostname ([_2])", # loc
+ $browser->host_port, $config->host_port);
}
sub ExpandCSRFToken {
commit ad3ab788779fb1f8047bfe190d1d7c439d615c01
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 18 16:13:53 2012 -0400
Safety-checking on classes loaded with `eval "require $class"`
While these close an arbitrary execution of code vulnerability, it
required SuperUser privileges to exploit. As SuperUsers already have
the ability to run arbitrary code using Scrips, this vulnerability was
primarily one of CSRF, which is closed by CSRF protection. Regardless,
validate the package names before they are inserted into the string
eval.
diff --git a/lib/RT/Shredder.pm b/lib/RT/Shredder.pm
index 024a50b..477a9f2 100644
--- a/lib/RT/Shredder.pm
+++ b/lib/RT/Shredder.pm
@@ -349,6 +349,8 @@ sub CastObjectsToRecords
} elsif ( UNIVERSAL::isa( $targets, 'SCALAR' ) || !ref $targets ) {
$targets = $$targets if ref $targets;
my ($class, $id) = split /-/, $targets;
+ RT::Shredder::Exception->throw( "Unsupported class $class" )
+ unless $class =~ /^\w+(::\w+)*$/;
$class = 'RT::'. $class unless $class =~ /^RTx?::/i;
eval "require $class";
die "Couldn't load '$class' module" if $@;
diff --git a/lib/RT/Shredder/Plugin.pm b/lib/RT/Shredder/Plugin.pm
index 3438068..725c089 100644
--- a/lib/RT/Shredder/Plugin.pm
+++ b/lib/RT/Shredder/Plugin.pm
@@ -167,6 +167,7 @@ sub LoadByName
{
my $self = shift;
my $name = shift or return (0, "Name not specified");
+ $name =~ /^\w+(::\w+)*$/ or return (0, "Invalid plugin name");
local $@;
my $plugin = "RT::Shredder::Plugin::$name";
commit 52159bcadff35afb38fbf0ed749f32f213cf537d
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Mon Jan 9 13:41:52 2012 -0500
Encourage users to look in the logs when an error happens.
diff --git a/lib/RT/Interface/Web/Handler.pm b/lib/RT/Interface/Web/Handler.pm
index 70e92fd..4f28f02 100644
--- a/lib/RT/Interface/Web/Handler.pm
+++ b/lib/RT/Interface/Web/Handler.pm
@@ -275,7 +275,7 @@ sub CleanupRequest {
sub HTML::Mason::Exception::as_rt_error {
my ($self) = @_;
$RT::Logger->error( $self->full_message );
- return "An internal RT error has occurred.";
+ return "An internal RT error has occurred. Your administrator can find more details in RT's log files.";
}
1;
commit 6b4e33882d0eac0c8ea5b416b4edd692bdd69e71
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue May 1 22:07:42 2012 -0400
$r->path_info is not reliable; use the request_comp's path
$r->path_info is not populated under mod_perl with a non-empty WebPath,
due to the way HTML::Mason::ApacheHandler is implemented. As what we
truely care about is if the component being requested is under the REST
directory, ->path_info can be replaced with ->request_comp->path, which
both simplifies the regex and brings it in line with the one used during
auth.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 4104dac..43cf8cf 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1100,12 +1100,12 @@ sub IsPossibleCSRF {
# whitelist the REST endpoints -- and explicitly deny non-REST
# endpoints. We do this because using a REST cookie in a browser
# would open the user to CSRF attacks to the REST endpoints.
- my $path = $HTML::Mason::Commands::r->path_info;
- $HTML::Mason::Commands::session{'REST'} = $path =~ m{^/+REST/\d+\.\d+(/|$)}
+ my $comp = $HTML::Mason::Commands::m->request_comp->path;
+ $HTML::Mason::Commands::session{'REST'} = $comp =~ m{^/REST/\d+\.\d+/}
unless defined $HTML::Mason::Commands::session{'REST'};
if ($HTML::Mason::Commands::session{'REST'}) {
- return 0 if $path =~ m{^/+REST/\d+\.\d+(/|$)};
+ return 0 if $comp =~ m{^/REST/\d+\.\d+/};
my $why = <<EOT;
This login session belongs to a REST client, and cannot be used to
access non-REST interfaces of RT for security reasons.
@@ -1119,10 +1119,7 @@ EOT
HTML::Mason::Commands::Abort( $why, Details => $details );
}
- return 0 if IsCompCSRFWhitelisted(
- $HTML::Mason::Commands::m->request_comp->path,
- $ARGS
- );
+ return 0 if IsCompCSRFWhitelisted( $comp, $ARGS );
# if there is no Referer header then assume the worst
return (1,
commit 542b80d1fff77dd14e65fbb494eba5118cbb26a6
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue May 1 22:12:03 2012 -0400
$r->path_info is not reliable; use the full URI
$r->path_info is not populated under mod_perl with a non-empty WebPath,
due to the way HTML::Mason::ApacheHandler is implemented. As in this
case we wish to construct a URL that can be used to link back to
ourselves, using ->uri (which includes the WebPath, if any) is the
correct method to use.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 43cf8cf..9d4bce1 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1142,7 +1142,7 @@ sub ExpandCSRFToken {
my $data = $HTML::Mason::Commands::session{'CSRF'}{$token};
return unless $data;
- return unless $data->{path} eq $HTML::Mason::Commands::r->path_info;
+ return unless $data->{uri} eq $HTML::Mason::Commands::r->uri;
my $user = $HTML::Mason::Commands::session{'CurrentUser'}->UserObj;
return unless $user->ValidateAuthString( $data->{auth}, $token );
@@ -1181,7 +1181,7 @@ sub MaybeShowInterstitialCSRFPage {
my $user = $HTML::Mason::Commands::session{'CurrentUser'}->UserObj;
my $data = {
auth => $user->GenerateAuthString( $token ),
- path => $HTML::Mason::Commands::r->path_info,
+ uri => $HTML::Mason::Commands::r->uri,
args => $ARGS,
};
if ($ARGS->{Attach}) {
@@ -1198,7 +1198,7 @@ sub MaybeShowInterstitialCSRFPage {
$HTML::Mason::Commands::m->comp(
'/Elements/CSRF',
- OriginalURL => $HTML::Mason::Commands::r->path_info,
+ OriginalURL => $HTML::Mason::Commands::r->uri,
Reason => HTML::Mason::Commands::loc( $msg, @loc ),
Token => $token,
);
commit 07890edf334ce7a238fb65af0ab4689566bff027
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed May 2 01:05:21 2012 -0400
Switch to our so that extensions can whitelist components
Also rename the variable because you're actually whitelisting a
component, especially something like a dhandler which won't be in the
URL.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 4104dac..2650444 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1022,7 +1022,7 @@ sub LogRecordedSQLStatements {
}
-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.
@@ -1033,7 +1033,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 };
commit 912e2385a38cce818244bbe4197897b190217d1b
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed May 2 01:06:01 2012 -0400
Add a new ReferrerWhitelist config option
This is a list of hostname:port that RT will accept HTTP_REFERER for.
This is helpful if your RT has two hostnames or if you need to have auth
from an external service that redirects back into RT.
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index b9cedc7..7cd97f2 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1743,6 +1743,19 @@ Should rejection notes be sent to the requestors? The default is true.
Set($ApprovalRejectionNotes, 1);
+=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));
+
+=cut
+
+Set(@ReferrerWhitelist, qw());
+
=back
=head1 Miscellaneous Configuration
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 2650444..fd5f8ee 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1070,11 +1070,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
@@ -1129,12 +1134,14 @@ 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;
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);
+ "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) );
}
sub ExpandCSRFToken {
commit dd11af8915ce12942ffd7c08607a847d1967be47
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed May 2 01:06:30 2012 -0400
Document how to pull from the error into the config
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index 7cd97f2..4f3f605 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1752,6 +1752,10 @@ 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());
commit eada947f53da77f93e91aa27dd0fd30c144a3c5e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed May 2 16:42:40 2012 -0400
Fix a simple typo
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index b9cedc7..d82ddd8 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1264,7 +1264,7 @@ 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<user> and C<pass> 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
commit ff3a3e187d64deac5930877fee8527d32f59406a
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu May 3 23:03:29 2012 -0400
Allow the homepage refresh argument as an idempotent query parameter
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 4104dac..9180871 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1061,6 +1061,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;
commit 001014997bd8a5c21cbb36eb50505bea14456f1b
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu May 3 23:16:23 2012 -0400
Abstract out creation of request tokens which bypass CSRF
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 9180871..c418707 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1170,21 +1170,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 = {
@@ -1203,7 +1191,25 @@ 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,
commit 997d5b0fc029ff0b209ec7e47f253958be334d8b
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu May 3 23:38:32 2012 -0400
Rename LogoutURL to the more general-use RefreshURL
diff --git a/share/html/Elements/Header b/share/html/Elements/Header
index f9bd27f..a59602c 100755
--- a/share/html/Elements/Header
+++ b/share/html/Elements/Header
@@ -54,7 +54,7 @@
% if ($Refresh && $Refresh =~ /^(\d+)/ && $1 > 0) {
-% my $URL = $m->notes->{LogoutURL}; $URL = $URL ? ";URL=$URL" : "";
+% my $URL = $m->notes->{RefreshURL}; $URL = $URL ? ";URL=$URL" : "";
<meta http-equiv="refresh" content="<% "$1$URL" %>" />
% }
diff --git a/share/html/NoAuth/Logout.html b/share/html/NoAuth/Logout.html
index fa21100..5563a99 100755
--- 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;
</%INIT>
commit fec1b72e821c2c9d28996eaec0ca21a7be9cf4e7
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu May 3 23:48:36 2012 -0400
Set the refresh URL on ticket results to a CRSF-safe one
Unfortunately, browsers do not always provide a referrer when
redirecting to a page by way of a <meta http-equiv="refresh">. As such,
automatic result refreshes trigger CSRF protections, as they request a
complex URL with many query parameters, and no referrer.
Work around this by generating a CSRF token which encodes the complete
set of query parameters, and redirecting to that. An unfortunate but
unavoidable side effect of this is that the session is bloated by each
of these sets of store query parameters on each search result page that
has a refresh set.
diff --git a/share/html/Search/Results.html b/share/html/Search/Results.html
index 8aea1fc..d9a7b1d 100755
--- 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,
RSSAutoDiscovery => $RSSFeedURL,
LinkRel => \%link_rel &>
<& /Ticket/Elements/Tabs,
@@ -174,6 +174,16 @@ if ($ARGS{'TicketsRefreshInterval'}) {
$session{'tickets_refresh_interval'} = $ARGS{'TicketsRefreshInterval'};
}
+my $refresh = $session{'tickets_refresh_interval'}
+ || RT->Config->Get('SearchResultsRefreshInterval', $session{'CurrentUser'} );
+
+if ($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(
commit 809ea27ee626d2cd00c0635a73a5e4a55f01e423
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri May 4 16:49:31 2012 -0400
Clean up the error message in a common case of no explicit whitelisted hosts
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index fd5f8ee..1b22603 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1137,11 +1137,18 @@ EOT
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]) or whitelisted hosts ([_3])", # loc
+ "the Referrer header supplied by your browser ([_1]) is not allowed by RT's configured hostname ([_2])", # loc
$browser->host_port,
- shift @$configs,
- join(', ', @$configs) );
+ $configs->[0]);
}
sub ExpandCSRFToken {
commit cd32279d153f4d61a677562f7c96b208157d7a7d
Merge: eada947 542b80d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Sun May 6 01:38:06 2012 -0400
Merge branch 'security/3.8/interstitial-path' into security/3.8-trunk
commit 1286ac125218937c2fe1ea50e874aaa384090774
Merge: cd32279 fec1b72
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Sun May 6 01:39:15 2012 -0400
Merge branch 'security/3.8/refresh-csrf' into security/3.8-trunk
diff --cc lib/RT/Interface/Web.pm
index 9d4bce1,c418707..5d92ea5
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@@ -1195,10 -1191,28 +1188,28 @@@ sub StoreRequestToken
$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 => $HTML::Mason::Commands::r->uri,
Reason => HTML::Mason::Commands::loc( $msg, @loc ),
Token => $token,
);
commit 65d63b0a9b36b81044d7b164606a415f65876c9d
Merge: 1286ac1 809ea27
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Sun May 6 01:42:18 2012 -0400
Merge branch 'security/3.8/whitelist-csrf-referrer' into security/3.8-trunk
commit a5346d0ba4122e989d771a093de79e8b3bdd3024
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon May 7 00:29:29 2012 -0400
Only enable CSRF argument stashing in refresh URL if CSRF is enabled
Not only is it only necessary if CSRF protections are on, but RT does
not expand CSRF_Token unless RestrictReferrer is enabled.
diff --git a/share/html/Search/Results.html b/share/html/Search/Results.html
index d9a7b1d..c072d9a 100755
--- a/share/html/Search/Results.html
+++ b/share/html/Search/Results.html
@@ -177,7 +177,7 @@ if ($ARGS{'TicketsRefreshInterval'}) {
my $refresh = $session{'tickets_refresh_interval'}
|| RT->Config->Get('SearchResultsRefreshInterval', $session{'CurrentUser'} );
-if ($refresh and not $m->request_args->{CSRF_Token}) {
+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="
commit 14b8b16c3036ae9a46725cefece768e929a15a4e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue May 15 13:55:15 2012 -0400
AddAttachments must use $RT::SystemUser when searching for attachments to use
c29107c changed AddAttachments to use the transaction's current user to
search for which attachments to add to the outgoing mail.
Unfortunately, this ignored the common case where the transaction's
current user is an unprivileged user who does not have rights to see
their own attachment. This manifested itself as AdminCc emails not
having attachments which were included with the original mail that
triggered them, despite RT-Attach-Message being set.
Revert the CurrentUser on the Attachments search to $RT::SystemUser, as
it was pre- c29107c. This does not re-open the vulnerability, as
(unlike the AddTicket functionality) the transaction creator can only
cause attachments on their own transaction to be distributed. While one
route to fix this would be to modify RT::Attachments->Next to allow
creators to always see their own attachments, such a change might have
broader-reaching implications.
Conflicts:
lib/RT/Action/SendEmail.pm
diff --git a/lib/RT/Action/SendEmail.pm b/lib/RT/Action/SendEmail.pm
index b61e3f3..a98a764 100755
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -349,7 +349,7 @@ sub AddAttachments {
$MIMEObj->head->delete('RT-Attach-Message');
- my $attachments = RT::Attachments->new( RT::CurrentUser->new($self->TransactionObj->Creator) );
+ my $attachments = RT::Attachments->new($RT::SystemUser);
$attachments->Limit(
FIELD => 'TransactionId',
VALUE => $self->TransactionObj->Id
commit 42503976eced9fa0d71fe56e792758f876d6e491
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue May 15 14:00:28 2012 -0400
Ensure that updated session is sent to clients after external auth
While 162cd06 correctly identified that sending the cookie was only
necessary after reading it or re-auth, it failed to notice that
InstantiateNewSession is called elsewhere than
AttemptPasswordAuthentication (notably AttemptExternalAuth), all of
which require SendSessionCookie calls to function correctly.
Ensure that the updated cookie value is always set after it is changed
by InstantiateNewSession, as well as directly before page display (in
case other callbacks change the session id by other means).
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 9ef8a57..fd4be73 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -476,6 +476,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();
+
# If the user isn't privileged, they can only see SelfService
unless ( $HTML::Mason::Commands::session{'CurrentUser'}->Privileged ) {
@@ -614,7 +618,6 @@ sub AttemptPasswordAuthentication {
InstantiateNewSession();
$HTML::Mason::Commands::session{'CurrentUser'} = $user_obj;
- SendSessionCookie();
$m->callback( %$ARGS, CallbackName => 'SuccessfulLogin', CallbackPage => '/autohandler' );
@@ -669,6 +672,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 {
commit 488f351cb105ef21f6952b14fb8ec1a1aa630967
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue May 22 09:11:44 2012 -0400
Version bump for 3.8.12
diff --git a/configure.ac b/configure.ac
index 5104d44..d840f2d 100755
--- a/configure.ac
+++ b/configure.ac
@@ -7,7 +7,7 @@ AC_REVISION($Revision$)dnl
dnl Setup autoconf
AC_PREREQ([2.53])
-AC_INIT(RT, 3.8.HEAD, [rt-bugs at bestpractical.com])
+AC_INIT(RT, 3.8.12, [rt-bugs at bestpractical.com])
AC_CONFIG_SRCDIR([lib/RT.pm.in])
dnl Extract RT version number components
commit 8c6406d12958a0c6db67676e00522ccfd9f7824b
Merge: 6928fac 488f351
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue May 22 09:14:15 2012 -0400
Merge branch '3.8.12-releng' into 3.8-trunk
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list