[Rt-commit] rt branch, 4.0.8-releng, updated. rt-4.0.8rc2-38-g048ac13
Kevin Falcone
falcone at bestpractical.com
Thu Oct 25 18:37:40 EDT 2012
The branch, 4.0.8-releng has been updated
via 048ac133c6aa7af528a84dc1aece81042ed79c4f (commit)
via 9b5d6534dc158b4449e6c6587fde19f51ff02721 (commit)
via 73c3c162183b756bb33378f3def60033de776836 (commit)
via f350d859467e04d391491c0129eca052a90059bc (commit)
via 0b3d6861f99269a40dce601263bba1145e61bd1d (commit)
via 496e93fe0360954c7d36cecbc2219791752c58c7 (commit)
via e6c6ae922252e36b510264472820ea8df72e4b7e (commit)
via dba758b4353ed506e1d7527a0aad22b0124c8786 (commit)
via 42623930c7a4eb524dcd5d702912faae418acab0 (commit)
via e0c1ec5163da68ddaf477010b6219d41a7be40bc (commit)
via 0bd9510437d7adbeb330313eaebc6103eb917026 (commit)
via c964114ca495830f70ec8789d4d9de878fbf84b2 (commit)
via 4cda0e47df5431c59d2a03f61917ab35c0cdbfaf (commit)
via ecbdf95e4d4bebf71152fdfa18b11524d364c9a4 (commit)
via 72ee4047c9855b3c8e1a7e2a0e7635c3a3ed230b (commit)
via 7e3782deb299becc9221cc83ee2d9cab09721d74 (commit)
via 5d569351cabe49819641fec936bdcf0e87580922 (commit)
via e8d0554663854b857025e7211bd0cc51d09e6223 (commit)
via 03ff7a5c4492815c749d8245546b4be35cb8b3e1 (commit)
via cbdc50385da30d56b996ad33a7e08117f98dc64c (commit)
via 666d7a4f14e1065a520062b89cccb34932fa4567 (commit)
via 4c40dd6b6523977abb8ac77276bbeff45df73027 (commit)
via e3c2880e6526f9c78079df2097483fea30ba910e (commit)
via e50f6496cfd15a3b9e1e24b67c9b1b721107d6a5 (commit)
via 08555f51ff8a4105aaaf34725d6b8df356203a86 (commit)
via 8f242072bb196e155ec9a5fbbf06bc8a2d3c2813 (commit)
via 1f213d6989e646e0163c901b0567ba1c94a311a8 (commit)
via 8ce033b38ced865622a2823aa266ae9bbc251811 (commit)
via cd0235bda39972fabecd837d982dd9f1619296c4 (commit)
via b32d9f5ffe7b60816942c91a6b7cb62992910369 (commit)
via 10e4a18a61b07b505bae8e9ffff65708a97fc659 (commit)
via aeaeb9cf58240603bb8a7d6d951518d32e8512a6 (commit)
via df1c01d5563883abfcde5c5ed1675d6617256ebe (commit)
via 46f44f7c2147f674ab0b54e9a80d70076558330a (commit)
from e4916c6fd66a4812f47531b865bffca6398bff5d (commit)
Summary of changes:
lib/RT/Action/SendEmail.pm | 54 +++++--------
lib/RT/Article.pm | 2 +-
lib/RT/Attachment.pm | 49 ++++++++----
lib/RT/Crypt/GnuPG.pm | 14 +++-
lib/RT/Interface/Email.pm | 52 +++++++++----
lib/RT/Interface/Email/Auth/GnuPG.pm | 3 +-
lib/RT/Interface/Web.pm | 117 +++++++++++++++++++++++-----
lib/RT/Queue.pm | 40 ++++++++--
lib/RT/Template.pm | 1 +
lib/RT/User.pm | 1 +
share/html/Admin/Queues/Modify.html | 6 +-
share/html/Admin/Users/GnuPG.html | 15 +++-
share/html/Elements/CSRF | 6 +-
share/html/Elements/GnuPG/SignEncryptWidget | 10 ++-
share/html/Elements/Login | 2 +
share/html/Elements/LoginRedirectWarning | 20 +++++
share/html/NoAuth/css/base/login.css | 8 ++
t/web/crypt-gnupg.t | 27 +++++--
t/web/ticket_forward.t | 6 +-
19 files changed, 314 insertions(+), 119 deletions(-)
create mode 100644 share/html/Elements/LoginRedirectWarning
- Log -----------------------------------------------------------------
commit 46f44f7c2147f674ab0b54e9a80d70076558330a
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Aug 27 19:59:54 2012 -0400
Fix a typo, preventing emails from setting internal encryption header
X-RT-Incoming-Encryption, along with a number of other headers, is
removed from all parts of incoming emails because they are used to
annotate GPG-related properties of the message. Fix a typo, which
allowed the properly-spelled header to pass through under specific
circumstances.
diff --git a/lib/RT/Interface/Email/Auth/GnuPG.pm b/lib/RT/Interface/Email/Auth/GnuPG.pm
index e508908..5fe8759 100644
--- a/lib/RT/Interface/Email/Auth/GnuPG.pm
+++ b/lib/RT/Interface/Email/Auth/GnuPG.pm
@@ -77,7 +77,7 @@ sub GetCurrentUser {
foreach my $p ( $args{'Message'}->parts_DFS ) {
$p->head->delete($_) for qw(
- X-RT-GnuPG-Status X-RT-Incoming-Encrypton
+ X-RT-GnuPG-Status X-RT-Incoming-Encryption
X-RT-Incoming-Signature X-RT-Privacy
);
}
commit df1c01d5563883abfcde5c5ed1675d6617256ebe
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Aug 27 21:24:25 2012 -0400
Remove internal signing and encryption hints from incoming mail
RT uses the existance of the X-RT-Sign and X-RT-Encrypt headers to
decide if the outgoing mails it sends as a result of the transaction
should be signed and/or encrypted. It standardly inserts these headers
based on checkboxes visable in the web UI; however, it fails to remove
them from incoming mails, allowing external users to create nearly
arbitrary messages signed by the queue's key.
diff --git a/lib/RT/Interface/Email/Auth/GnuPG.pm b/lib/RT/Interface/Email/Auth/GnuPG.pm
index 5fe8759..87a523d 100644
--- a/lib/RT/Interface/Email/Auth/GnuPG.pm
+++ b/lib/RT/Interface/Email/Auth/GnuPG.pm
@@ -79,6 +79,7 @@ sub GetCurrentUser {
$p->head->delete($_) for qw(
X-RT-GnuPG-Status X-RT-Incoming-Encryption
X-RT-Incoming-Signature X-RT-Privacy
+ X-RT-Sign X-RT-Encrypt
);
}
commit aeaeb9cf58240603bb8a7d6d951518d32e8512a6
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Aug 30 17:16:41 2012 -0400
Restrict users to only signing with queue or their own personal keys
While the UI only presents two options for signing, the underlying form
accepted any key ID or email address, and dutifully signed the mail with
the appropriate secret key. Restrict the accepted values of SignUsing
to the two values that we displayed.
diff --git a/share/html/Elements/GnuPG/SignEncryptWidget b/share/html/Elements/GnuPG/SignEncryptWidget
index 0ae0f84..2f3f103 100644
--- a/share/html/Elements/GnuPG/SignEncryptWidget
+++ b/share/html/Elements/GnuPG/SignEncryptWidget
@@ -129,12 +129,16 @@ if ( $self->{'Sign'} ) {
$QueueObj ||= $TicketObj->QueueObj
if $TicketObj;
- my $address = $self->{'SignUsing'};
- $address ||= ($self->{'UpdateType'} && $self->{'UpdateType'} eq "private")
+ my $private = $session{'CurrentUser'}->UserObj->PrivateKey || '';
+ my $queue = ($self->{'UpdateType'} && $self->{'UpdateType'} eq "private")
? ( $QueueObj->CommentAddress || RT->Config->Get('CommentAddress') )
: ( $QueueObj->CorrespondAddress || RT->Config->Get('CorrespondAddress') );
- unless ( RT::Crypt::GnuPG::DrySign( $address ) ) {
+ my $address = $self->{'SignUsing'} || $queue;
+ if ($address ne $private and $address ne $queue) {
+ push @{ $self->{'GnuPGCanNotSignAs'} ||= [] }, $address;
+ $checks_failure = 1;
+ } elsif ( not RT::Crypt::GnuPG::DrySign( $address ) ) {
push @{ $self->{'GnuPGCanNotSignAs'} ||= [] }, $address;
$checks_failure = 1;
} else {
commit 10e4a18a61b07b505bae8e9ffff65708a97fc659
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Aug 30 18:42:55 2012 -0400
Don't propose any secret keys to users with no email address
GetKeysForSigning, when called with 'force', returns all keys if no
email address is provided. This caused users with no email address to
have all secret keys offered to them. Remove the 'force' to ensure that
empty email address are shown an empty list.
diff --git a/share/html/Admin/Users/GnuPG.html b/share/html/Admin/Users/GnuPG.html
index 90408e4..8925c9f 100644
--- a/share/html/Admin/Users/GnuPG.html
+++ b/share/html/Admin/Users/GnuPG.html
@@ -64,7 +64,7 @@
<& /Widgets/Form/Select,
Name => 'PrivateKey',
Description => loc('Private Key'),
- Values => [ map $_->{'Key'}, @{ $keys_meta{'info'} } ],
+ Values => \@potential_keys,
CurrentValue => $UserObj->PrivateKey,
DefaultLabel => loc('No private key'),
&>
@@ -91,7 +91,8 @@ unless ( $UserObj->id ) {
$id = $ARGS{'id'} = $UserObj->id;
my $email = $UserObj->EmailAddress;
-my %keys_meta = RT::Crypt::GnuPG::GetKeysForSigning( $email, 'force' );
+my %keys_meta = RT::Crypt::GnuPG::GetKeysForSigning( $email );
+my @potential_keys = map $_->{'Key'}, @{ $keys_meta{'info'} || [] };
$ARGS{'PrivateKey'} = $m->comp('/Widgets/Form/Select:Process',
Name => 'PrivateKey',
commit b32d9f5ffe7b60816942c91a6b7cb62992910369
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Aug 30 22:42:22 2012 -0400
Explicitly restrict private keys to ones offered
diff --git a/share/html/Admin/Users/GnuPG.html b/share/html/Admin/Users/GnuPG.html
index 8925c9f..373e314 100644
--- a/share/html/Admin/Users/GnuPG.html
+++ b/share/html/Admin/Users/GnuPG.html
@@ -101,8 +101,12 @@ $ARGS{'PrivateKey'} = $m->comp('/Widgets/Form/Select:Process',
);
if ( $Update ) {
- my ($status, $msg) = $UserObj->SetPrivateKey( $ARGS{'PrivateKey'} );
- push @results, $msg;
+ if (not $ARGS{'PrivateKey'} or grep {$_ eq $ARGS{'PrivateKey'}} @potential_keys) {
+ my ($status, $msg) = $UserObj->SetPrivateKey( $ARGS{'PrivateKey'} );
+ push @results, $msg;
+ } else {
+ push @results, loc("Invalid key [_1] for address '[_2]'", $ARGS{'PrivateKey'}, $email);
+ }
}
my $title = loc("[_1]'s GnuPG keys",$UserObj->Name);
commit cd0235bda39972fabecd837d982dd9f1619296c4
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Aug 30 22:46:32 2012 -0400
Avoid spurious update and warning messages on key update
This avoids an ominous-looking warning about failure to remove the key,
when no key was previously set but no key was newly selected.
diff --git a/share/html/Admin/Users/GnuPG.html b/share/html/Admin/Users/GnuPG.html
index 373e314..ee58c44 100644
--- a/share/html/Admin/Users/GnuPG.html
+++ b/share/html/Admin/Users/GnuPG.html
@@ -102,8 +102,10 @@ $ARGS{'PrivateKey'} = $m->comp('/Widgets/Form/Select:Process',
if ( $Update ) {
if (not $ARGS{'PrivateKey'} or grep {$_ eq $ARGS{'PrivateKey'}} @potential_keys) {
- my ($status, $msg) = $UserObj->SetPrivateKey( $ARGS{'PrivateKey'} );
- push @results, $msg;
+ if (($ARGS{'PrivateKey'}||'') ne ($UserObj->PrivateKey||'')) {
+ my ($status, $msg) = $UserObj->SetPrivateKey( $ARGS{'PrivateKey'} );
+ push @results, $msg;
+ }
} else {
push @results, loc("Invalid key [_1] for address '[_2]'", $ARGS{'PrivateKey'}, $email);
}
commit 8ce033b38ced865622a2823aa266ae9bbc251811
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Aug 30 23:58:40 2012 -0400
Require AdminUser to set PGP private key IDs, not merely ModifySelf
Be explicit that modification of the PrivateKey pseudo-column requires
admin rights -- namely, AdminUser, not just ModifySelf. While the page
that controlls that property is in /Admin/Users/GnuPG.html, accessing
that page requires only the ShowConfigTab right, and modifying the
property on yourself required only ModifySelf. Combined with the
ability (also granted by ModifySelf) for users to change their own email
addresses, this opened the possibility for users to claim arbitrary
private keys as their own, for use in signing.
diff --git a/lib/RT/User.pm b/lib/RT/User.pm
index 56a22cd..72d00f3 100644
--- a/lib/RT/User.pm
+++ b/lib/RT/User.pm
@@ -102,6 +102,7 @@ sub _OverlayAccessible {
AuthSystem => { public => 1, admin => 1 },
Gecos => { public => 1, admin => 1 },
PGPKey => { public => 1, admin => 1 },
+ PrivateKey => { admin => 1 },
}
}
commit 1f213d6989e646e0163c901b0567ba1c94a311a8
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Aug 31 00:17:08 2012 -0400
Ensure that no --arguments can be snuck to GPG commands as arguments
GnuPG::Interface 0.46 adds code to prefix command_args with a "--" if it
doesn't have one already. Rather than bump the dependency, explicitly
add the "--" in the two places that we pass in command_args.
diff --git a/lib/RT/Crypt/GnuPG.pm b/lib/RT/Crypt/GnuPG.pm
index 8df1c1b..77f9a07 100644
--- a/lib/RT/Crypt/GnuPG.pm
+++ b/lib/RT/Crypt/GnuPG.pm
@@ -2120,7 +2120,9 @@ sub GetKeysInfo {
eval {
local $SIG{'CHLD'} = 'DEFAULT';
my $method = $type eq 'private'? 'list_secret_keys': 'list_public_keys';
- my $pid = safe_run_child { $gnupg->$method( handles => $handles, $email? (command_args => $email) : () ) };
+ my $pid = safe_run_child { $gnupg->$method( handles => $handles, $email
+ ? (command_args => [ "--", $email])
+ : () ) };
close $handle{'stdin'};
waitpid $pid, 0;
};
@@ -2314,7 +2316,7 @@ sub DeleteKey {
my $pid = safe_run_child { $gnupg->wrap_call(
handles => $handles,
commands => ['--delete-secret-and-public-key'],
- command_args => [$key],
+ command_args => ["--", $key],
) };
close $handle{'stdin'};
while ( my $str = readline $handle{'status'} ) {
commit 8f242072bb196e155ec9a5fbbf06bc8a2d3c2813
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Aug 31 21:12:02 2012 -0400
Refactor shared code controlling if a message will be encrypted or signed
diff --git a/lib/RT/Action/SendEmail.pm b/lib/RT/Action/SendEmail.pm
index 4ae1a8b..c301870 100644
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -106,23 +106,10 @@ sub Commit {
if ( RT->Config->Get('RecordOutgoingEmail')
&& RT->Config->Get('GnuPG')->{'Enable'} )
{
-
- # it's hacky, but we should know if we're going to crypt things
- my $attachment = $self->TransactionObj->Attachments->First;
-
- my %crypt;
- foreach my $argument (qw(Sign Encrypt)) {
- if ( $attachment
- && defined $attachment->GetHeader("X-RT-$argument") )
- {
- $crypt{$argument} = $attachment->GetHeader("X-RT-$argument");
- } else {
- $crypt{$argument} = $self->TicketObj->QueueObj->$argument();
- }
- }
- if ( $crypt{'Sign'} || $crypt{'Encrypt'} ) {
- $orig_message = $message->dup;
- }
+ $orig_message = $message->dup if RT::Interface::Email::WillSignEncrypt(
+ Attachment => $self->TransactionObj->Attachments->First,
+ Ticket => $self->TicketObj,
+ );
}
my ($ret) = $self->SendMessage($message);
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index 9270462..6fec643 100644
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -318,6 +318,24 @@ header field then it's value is used
=cut
+sub WillSignEncrypt {
+ my %args = @_;
+ my $attachment = delete $args{Attachment};
+ my $ticket = delete $args{Ticket};
+
+ for my $argument ( qw(Sign Encrypt) ) {
+ next if defined $args{ $argument };
+
+ if ( $attachment and defined $attachment->GetHeader("X-RT-$argument") ) {
+ $args{$argument} = $attachment->GetHeader("X-RT-$argument");
+ } elsif ( $ticket ) {
+ $args{$argument} = $ticket->QueueObj->$argument();
+ }
+ }
+
+ return wantarray ? %args : ($args{Sign} || $args{Encrypt});
+}
+
sub SendEmail {
my (%args) = (
Entity => undef,
@@ -366,23 +384,12 @@ sub SendEmail {
}
if ( RT->Config->Get('GnuPG')->{'Enable'} ) {
- my %crypt;
-
- my $attachment;
- $attachment = $TransactionObj->Attachments->First
- if $TransactionObj;
-
- foreach my $argument ( qw(Sign Encrypt) ) {
- next if defined $args{ $argument };
-
- if ( $attachment && defined $attachment->GetHeader("X-RT-$argument") ) {
- $crypt{$argument} = $attachment->GetHeader("X-RT-$argument");
- } elsif ( $TicketObj ) {
- $crypt{$argument} = $TicketObj->QueueObj->$argument();
- }
- }
-
- my $res = SignEncrypt( %args, %crypt );
+ %args = WillSignEncrypt(
+ %args,
+ Attachment => $TransactionObj ? $TransactionObj->Attachments->First : undef,
+ Ticket => $TicketObj,
+ );
+ my $res = SignEncrypt( %args );
return $res unless $res > 0;
}
commit 08555f51ff8a4105aaaf34725d6b8df356203a86
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Aug 31 21:15:26 2012 -0400
Refactor RT::Action::SendEmail->Commit to consolidate RecordOutgoingEmail path
Instead of interweaving the logic around the case where
RecordOutgoingEmail is disabled, resolve that simple case first.
diff --git a/lib/RT/Action/SendEmail.pm b/lib/RT/Action/SendEmail.pm
index c301870..2a7a2e3 100644
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -99,34 +99,31 @@ activated in the config.
sub Commit {
my $self = shift;
- $self->DeferDigestRecipients() if RT->Config->Get('RecordOutgoingEmail');
+ return abs $self->SendMessage( $self->TemplateObj->MIMEObj )
+ unless RT->Config->Get('RecordOutgoingEmail');
+
+ $self->DeferDigestRecipients();
my $message = $self->TemplateObj->MIMEObj;
my $orig_message;
- if ( RT->Config->Get('RecordOutgoingEmail')
- && RT->Config->Get('GnuPG')->{'Enable'} )
- {
- $orig_message = $message->dup if RT::Interface::Email::WillSignEncrypt(
- Attachment => $self->TransactionObj->Attachments->First,
- Ticket => $self->TicketObj,
- );
- }
+ $orig_message = $message->dup if RT::Interface::Email::WillSignEncrypt(
+ Attachment => $self->TransactionObj->Attachments->First,
+ Ticket => $self->TicketObj,
+ );
my ($ret) = $self->SendMessage($message);
- if ( $ret > 0 && RT->Config->Get('RecordOutgoingEmail') ) {
- if ($orig_message) {
- $message->attach(
- Type => 'application/x-rt-original-message',
- Disposition => 'inline',
- Data => $orig_message->as_string,
- );
- }
- $self->RecordOutgoingMailTransaction($message);
- $self->RecordDeferredRecipients();
- }
-
+ return abs( $ret ) if $ret <= 0;
- return ( abs $ret );
+ if ($orig_message) {
+ $message->attach(
+ Type => 'application/x-rt-original-message',
+ Disposition => 'inline',
+ Data => $orig_message->as_string,
+ );
+ }
+ $self->RecordOutgoingMailTransaction($message);
+ $self->RecordDeferredRecipients();
+ return 1;
}
=head2 Prepare
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index 6fec643..d7655bf 100644
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -323,6 +323,11 @@ sub WillSignEncrypt {
my $attachment = delete $args{Attachment};
my $ticket = delete $args{Ticket};
+ if ( not RT->Config->Get('GnuPG')->{'Enable'} ) {
+ $args{Sign} = $args{Encrypt} = 0;
+ return wantarray ? %args : 0;
+ }
+
for my $argument ( qw(Sign Encrypt) ) {
next if defined $args{ $argument };
commit e50f6496cfd15a3b9e1e24b67c9b1b721107d6a5
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Aug 31 23:25:27 2012 -0400
When creating tickets via the UI, always set signing/encryption headers
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 99aa7b6..b100a11 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1632,9 +1632,8 @@ sub CreateTicket {
}
}
- foreach my $argument (qw(Encrypt Sign)) {
- $MIMEObj->head->replace( "X-RT-$argument" => $ARGS{$argument} ? 1 : 0 )
- if defined $ARGS{$argument};
+ for my $argument (qw(Encrypt Sign)) {
+ $MIMEObj->head->replace( "X-RT-$argument" => $ARGS{$argument} ? 1 : 0 );
}
my %create_args = (
commit e3c2880e6526f9c78079df2097483fea30ba910e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Aug 31 23:27:10 2012 -0400
Differentiate "always sign" from "default to signing when composing"
Signing incorporates two related but distinct concepts: first,
verification that the message was transmitted without corruption
(integrity), and second, proof that the message was indeed sent by the
sender (authentication).
When the "Sign by default" queue option is disabled, the only messages
which are signed are those triggered by an action where the sender has
explicitly checked the "Sign" option. Consequently, messages which are
not generated via the web UI, such as auto-replies, or requestors'
messages which are forwarded to owners, are not signed. In this
configuration, the signing conveys authenticity, but does not provide
integrity for all messages..
When the "Sign by default" queue option is enabled, all messages default
to being signed, even ones generated automatically. Thus, emails from
the requestor (which may have arbitrary content!) will be signed by the
queue's key when redistributed to owners and AdminCcs. In this
configuration, the signing ensures integrity, but (because signing
happens automatically) means that no authenticity should be attached to
messages thus signed.
Because the simple configuration option fundamentally changed the
meaning of the signature, exchanging integrity for authenticity, with no
warning, alter it to instead provide an additional option ("Sign
auto-generated messages") which takes the burden of this change in
meaning. Degrade the "Sign by default" option to describing the default
state of the checkbox on the ticket create or reply page.
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index d7655bf..38157c2 100644
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -333,8 +333,14 @@ sub WillSignEncrypt {
if ( $attachment and defined $attachment->GetHeader("X-RT-$argument") ) {
$args{$argument} = $attachment->GetHeader("X-RT-$argument");
- } elsif ( $ticket ) {
- $args{$argument} = $ticket->QueueObj->$argument();
+ } elsif ( $ticket and $argument eq "Encrypt" ) {
+ $args{Encrypt} = $ticket->QueueObj->Encrypt();
+ } elsif ( $ticket and $argument eq "Sign" ) {
+ # Note that $queue->Sign is UI-only, and that all
+ # UI-generated messages explicitly set the X-RT-Crypt header
+ # to 0 or 1; thus this path is only taken for messages
+ # generated _not_ via the web UI.
+ $args{Sign} = $ticket->QueueObj->SignAuto();
}
}
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 406df92..a942bb6 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -394,6 +394,7 @@ sub Create {
FinalPriority => 0,
DefaultDueIn => 0,
Sign => undef,
+ SignAuto => undef,
Encrypt => undef,
_RecordTransaction => 1,
@_
@@ -436,14 +437,11 @@ sub Create {
}
$RT::Handle->Commit;
- if ( defined $args{'Sign'} ) {
- my ($status, $msg) = $self->SetSign( $args{'Sign'} );
- $RT::Logger->error("Couldn't set attribute 'Sign': $msg")
- unless $status;
- }
- if ( defined $args{'Encrypt'} ) {
- my ($status, $msg) = $self->SetEncrypt( $args{'Encrypt'} );
- $RT::Logger->error("Couldn't set attribute 'Encrypt': $msg")
+ for my $attr (qw/Sign SignAuto Encrypt/) {
+ next unless defined $args{$attr};
+ my $set = "Set" . $attr;
+ my ($status, $msg) = $self->$set( $args{$attr} );
+ $RT::Logger->error("Couldn't set attribute '$attr': $msg")
unless $status;
}
@@ -595,6 +593,32 @@ sub SetSign {
return ($status, $self->loc('Signing disabled'));
}
+sub SignAuto {
+ my $self = shift;
+ my $value = shift;
+
+ return undef unless $self->CurrentUserHasRight('SeeQueue');
+ my $attr = $self->FirstAttribute('SignAuto') or return 0;
+ return $attr->Content;
+}
+
+sub SetSignAuto {
+ my $self = shift;
+ my $value = shift;
+
+ return ( 0, $self->loc('Permission Denied') )
+ unless $self->CurrentUserHasRight('AdminQueue');
+
+ my ($status, $msg) = $self->SetAttribute(
+ Name => 'SignAuto',
+ Description => 'Sign auto-generated outgoing messages',
+ Content => $value,
+ );
+ return ($status, $msg) unless $status;
+ return ($status, $self->loc('Signing enabled')) if $value;
+ return ($status, $self->loc('Signing disabled'));
+}
+
sub Encrypt {
my $self = shift;
my $value = shift;
diff --git a/share/html/Admin/Queues/Modify.html b/share/html/Admin/Queues/Modify.html
index 85cd62f..c2cf094 100755
--- a/share/html/Admin/Queues/Modify.html
+++ b/share/html/Admin/Queues/Modify.html
@@ -119,6 +119,8 @@
<td align="right"><input type="checkbox" class="checkbox" name="Encrypt" value="1" <% $QueueObj->Encrypt? 'checked="checked"': '' |n%> /></td>
<td><&|/l&>Encrypt by default</&></td>
</tr>
+<tr><td align="right"><input type="checkbox" class="checkbox" name="SignAuto" value="1" <% $QueueObj->SignAuto? 'checked="checked"': '' |n%> /></td>
+<td colspan="3"><&|/l_unsafe, "<b>","</b>","<i>","</i>"&>Sign all auto-generated mail. [_1]Caution[_2]: Enabling this option alters the signature from providing [_3]authentication[_4] to providing [_3]integrity[_4].</&></td></tr>
% }
<tr><td align="right"><input type="checkbox" class="checkbox" name="Enabled" value="1" <%$EnabledChecked|n%> /></td>
@@ -181,13 +183,13 @@ unless ($Create) {
if ( $QueueObj->Id ) {
$title = loc('Configuration for queue [_1]', $QueueObj->Name );
my @attribs= qw(Description CorrespondAddress CommentAddress Name
- InitialPriority FinalPriority DefaultDueIn Sign Encrypt Lifecycle SubjectTag Disabled);
+ InitialPriority FinalPriority DefaultDueIn Sign SignAuto Encrypt Lifecycle SubjectTag Disabled);
# we're asking about enabled on the web page but really care about disabled
if ( $SetEnabled ) {
$Disabled = $ARGS{'Disabled'} = $Enabled? 0: 1;
$ARGS{$_} = 0 foreach grep !defined $ARGS{$_} || !length $ARGS{$_},
- qw(Sign Encrypt Disabled);
+ qw(Sign SignAuto Encrypt Disabled);
}
$m->callback(
commit 4c40dd6b6523977abb8ac77276bbeff45df73027
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Sep 26 22:14:42 2012 -0700
Blacklist components from automatic, argument-based CSRF whitelisting
Not all pages which take only an id query parameter are idempotent. The
ticket bookmark toggle — generally used via ajax — requires only id to
toggle bookmark state.
Start a blacklist, which should remain small, for these components.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 99aa7b6..8ae93e8 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1189,6 +1189,13 @@ our %is_whitelisted_component = (
'/m/tickets/search' => 1,
);
+# Components which are blacklisted from automatic, argument-based whitelisting.
+# These pages are not idempotent when called with just an id.
+our %is_blacklisted_component = (
+ # Takes only id and toggles bookmark state
+ '/Helpers/Toggle/TicketBookmark' => 1,
+);
+
sub IsCompCSRFWhitelisted {
my $comp = shift;
my $ARGS = shift;
@@ -1211,6 +1218,10 @@ sub IsCompCSRFWhitelisted {
delete $args{pass};
}
+ # Some pages aren't idempotent even with safe args like id; blacklist
+ # them from the automatic whitelisting below.
+ return 0 if $is_blacklisted_component{$comp};
+
# Eliminate arguments that do not indicate an effectful request.
# For example, "id" is acceptable because that is how RT retrieves a
# record.
commit 666d7a4f14e1065a520062b89cccb34932fa4567
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Sep 28 14:07:38 2012 -0700
Intuit the next page when logging in at the RT web root
Forcing the next page to WebURL, or even WebPath, loses the potential
query parameters to index.html. Perhaps most common is the URL
/?q=foo
for a quick search. It was previously lost during the login flow.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 99aa7b6..1ed6766 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -304,7 +304,7 @@ sub HandleRequest {
}
# Specially handle /index.html so that we get a nicer URL
elsif ( $m->request_comp->path eq '/index.html' ) {
- my $next = SetNextPage(RT->Config->Get('WebURL'));
+ my $next = SetNextPage();
$m->comp('/NoAuth/Login.html', next => $next, actions => [$msg]);
$m->abort;
}
commit cbdc50385da30d56b996ad33a7e08117f98dc64c
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Sep 28 11:11:19 2012 -0700
Abstract away reading $session{NextPage} into two functions
Setting is already wrapped, but reading was direct. Following commits
will use FetchNextPage for display.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 1ed6766..ebf306f 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -379,6 +379,27 @@ sub SetNextPage {
return $hash;
}
+=head2 FetchNextPage HASHKEY
+
+Returns the stashed next page URL for the given hash.
+
+=cut
+
+sub FetchNextPage {
+ my $hash = shift || "";
+ return $HTML::Mason::Commands::session{'NextPage'}->{$hash};
+}
+
+=head2 RemoveNextPage HASHKEY
+
+Removes the stashed next page URL for the given hash and returns it.
+
+=cut
+
+sub RemoveNextPage {
+ my $hash = shift || "";
+ return delete $HTML::Mason::Commands::session{'NextPage'}->{$hash};
+}
=head2 TangentForLogin [HASH]
@@ -600,7 +621,7 @@ sub AttemptExternalAuth {
$user =~ s/^\Q$NodeName\E\\//i;
}
- my $next = delete $HTML::Mason::Commands::session{'NextPage'}->{$ARGS->{'next'} || ''};
+ my $next = RemoveNextPage($ARGS->{'next'});
InstantiateNewSession() unless _UserLoggedIn;
$HTML::Mason::Commands::session{'CurrentUser'} = RT::CurrentUser->new();
$HTML::Mason::Commands::session{'CurrentUser'}->$load_method($user);
@@ -699,7 +720,7 @@ sub AttemptPasswordAuthentication {
# It's important to nab the next page from the session before we blow
# the session away
- my $next = delete $HTML::Mason::Commands::session{'NextPage'}->{$ARGS->{'next'} || ''};
+ my $next = RemoveNextPage($ARGS->{'next'});
InstantiateNewSession();
$HTML::Mason::Commands::session{'CurrentUser'} = $user_obj;
commit 03ff7a5c4492815c749d8245546b4be35cb8b3e1
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Sep 28 12:51:39 2012 -0700
Anticipate storing more information about the next page in the session
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index ebf306f..623d2d1 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -374,14 +374,14 @@ sub SetNextPage {
my $next = shift || IntuitNextPage();
my $hash = Digest::MD5::md5_hex($next . $$ . rand(1024));
- $HTML::Mason::Commands::session{'NextPage'}->{$hash} = $next;
+ $HTML::Mason::Commands::session{'NextPage'}->{$hash}{'url'} = $next;
$HTML::Mason::Commands::session{'i'}++;
return $hash;
}
=head2 FetchNextPage HASHKEY
-Returns the stashed next page URL for the given hash.
+Returns the stashed next page hashref for the given hash.
=cut
@@ -392,7 +392,7 @@ sub FetchNextPage {
=head2 RemoveNextPage HASHKEY
-Removes the stashed next page URL for the given hash and returns it.
+Removes the stashed next page for the given hash and returns it.
=cut
@@ -622,6 +622,7 @@ sub AttemptExternalAuth {
}
my $next = RemoveNextPage($ARGS->{'next'});
+ $next = $next->{'url'} if $next;
InstantiateNewSession() unless _UserLoggedIn;
$HTML::Mason::Commands::session{'CurrentUser'} = RT::CurrentUser->new();
$HTML::Mason::Commands::session{'CurrentUser'}->$load_method($user);
@@ -721,6 +722,7 @@ sub AttemptPasswordAuthentication {
# It's important to nab the next page from the session before we blow
# the session away
my $next = RemoveNextPage($ARGS->{'next'});
+ $next = $next->{'url'} if $next;
InstantiateNewSession();
$HTML::Mason::Commands::session{'CurrentUser'} = $user_obj;
commit e8d0554663854b857025e7211bd0cc51d09e6223
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Sep 28 11:15:15 2012 -0700
Check the original request for side-effects before prompting for login
Sets the foundation for informing users about to login what will happen
after they do so in order to help prevent phishing.
Note that while this shares some code and config with CSRF, it _isn't_
CSRF since the user is not yet logged in and it requires a user to fill
in valid credentials. Sharing config allows administrators to whitelist
external sites, such as corporate portals, which deep-link into RT, thus
preventing RT from crying wolf to the end user about legitimate
requests.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 623d2d1..1b37455 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -304,12 +304,12 @@ sub HandleRequest {
}
# Specially handle /index.html so that we get a nicer URL
elsif ( $m->request_comp->path eq '/index.html' ) {
- my $next = SetNextPage();
+ my $next = SetNextPage($ARGS);
$m->comp('/NoAuth/Login.html', next => $next, actions => [$msg]);
$m->abort;
}
else {
- TangentForLogin(results => ($msg ? LoginError($msg) : undef));
+ TangentForLogin($ARGS, results => ($msg ? LoginError($msg) : undef));
}
}
}
@@ -362,7 +362,7 @@ sub LoginError {
return $key;
}
-=head2 SetNextPage [PATH]
+=head2 SetNextPage ARGSRef [PATH]
Intuits and stashes the next page in the sesssion hash. If PATH is
specified, uses that instead of the value of L<IntuitNextPage()>. Returns
@@ -371,10 +371,30 @@ the hash value.
=cut
sub SetNextPage {
- my $next = shift || IntuitNextPage();
+ my $ARGS = shift;
+ my $next = $_[0] ? $_[0] : IntuitNextPage();
my $hash = Digest::MD5::md5_hex($next . $$ . rand(1024));
+ my $page = { url => $next };
+
+ # If an explicit URL was passed and we didn't IntuitNextPage, then
+ # IsPossibleCSRF below is almost certainly unrelated to the actual
+ # destination. Currently explicit next pages aren't used in RT, but the
+ # API is available.
+ if (not $_[0] and RT->Config->Get("RestrictReferrer")) {
+ # This isn't really CSRF, but the CSRF heuristics are useful for catching
+ # requests which may have unintended side-effects.
+ my ($is_csrf, $msg, @loc) = IsPossibleCSRF($ARGS);
+ if ($is_csrf) {
+ RT->Logger->notice(
+ "Marking original destination as having side-effects before redirecting for login.\n"
+ ."Request: $next\n"
+ ."Reason: " . HTML::Mason::Commands::loc($msg, @loc)
+ );
+ $page->{'HasSideEffects'} = [$msg, @loc];
+ }
+ }
- $HTML::Mason::Commands::session{'NextPage'}->{$hash}{'url'} = $next;
+ $HTML::Mason::Commands::session{'NextPage'}->{$hash} = $page;
$HTML::Mason::Commands::session{'i'}++;
return $hash;
}
@@ -401,15 +421,18 @@ sub RemoveNextPage {
return delete $HTML::Mason::Commands::session{'NextPage'}->{$hash};
}
-=head2 TangentForLogin [HASH]
+=head2 TangentForLogin ARGSRef [HASH]
Redirects to C</NoAuth/Login.html>, setting the value of L<IntuitNextPage> as
-the next page. Optionally takes a hash which is dumped into query params.
+the next page. Takes a hashref of request %ARGS as the first parameter.
+Optionally takes all other parameters as a hash which is dumped into query
+params.
=cut
sub TangentForLogin {
- my $hash = SetNextPage();
+ my $ARGS = shift;
+ my $hash = SetNextPage($ARGS);
my %query = (@_, next => $hash);
my $login = RT->Config->Get('WebURL') . 'NoAuth/Login.html?';
$login .= $HTML::Mason::Commands::m->comp('/Elements/QueryString', %query);
@@ -424,8 +447,9 @@ calls L<TangentForLogin> with the appropriate results key.
=cut
sub TangentForLoginWithError {
- my $key = LoginError(HTML::Mason::Commands::loc(@_));
- TangentForLogin( results => $key );
+ my $ARGS = shift;
+ my $key = LoginError(HTML::Mason::Commands::loc(@_));
+ TangentForLogin( $ARGS, results => $key );
}
=head2 IntuitNextPage
@@ -661,7 +685,7 @@ sub AttemptExternalAuth {
delete $HTML::Mason::Commands::session{'CurrentUser'};
if (RT->Config->Get('WebFallbackToInternalAuth')) {
- TangentForLoginWithError('Cannot create user: [_1]', $msg);
+ TangentForLoginWithError($ARGS, 'Cannot create user: [_1]', $msg);
} else {
$m->abort();
}
@@ -684,13 +708,13 @@ sub AttemptExternalAuth {
$user = $orig_user;
unless ( RT->Config->Get('WebFallbackToInternalAuth') ) {
- TangentForLoginWithError('You are not an authorized user');
+ TangentForLoginWithError($ARGS, 'You are not an authorized user');
}
}
} elsif ( RT->Config->Get('WebFallbackToInternalAuth') ) {
unless ( defined $HTML::Mason::Commands::session{'CurrentUser'} ) {
# XXX unreachable due to prior defaulting in HandleRequest (check c34d108)
- TangentForLoginWithError('You are not an authorized user');
+ TangentForLoginWithError($ARGS, 'You are not an authorized user');
}
} else {
commit 5d569351cabe49819641fec936bdcf0e87580922
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Sep 28 12:44:18 2012 -0700
Inform the user logging in about potential side-effects
This helps prevent phishing where the user follows a malicious link and
isn't logged in yet. Previously there was no indication of what would
happen after login. The CSRF protection does somewhat double duty and
provides the same measure of phishing protection when the user is
already logged in, but it is never triggered by unauthenticated
requests.
The warnings on login are controlled by the same configuration as CSRF.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 1b37455..c8667c1 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1453,6 +1453,30 @@ sub MaybeShowInterstitialCSRFPage {
# Calls abort, never gets here
}
+our @POTENTIAL_PAGE_ACTIONS = (
+ qr'/Ticket/Create.html' => "create a ticket", # loc
+ qr'/Ticket/' => "update a ticket", # loc
+ qr'/Admin/' => "modify RT's configuration", # loc
+ qr'/Approval/' => "update an approval", # loc
+ qr'/Articles/' => "update an article", # loc
+ qr'/Dashboards/' => "modify a dashboard", # loc
+ qr'/m/ticket/' => "update a ticket", # loc
+ qr'Prefs' => "modify your preferences", # loc
+ qr'/Search/' => "modify or access a search", # loc
+ qr'/SelfService/Create' => "create a ticket", # loc
+ qr'/SelfService/' => "update a ticket", # loc
+);
+
+sub PotentialPageAction {
+ my $page = shift;
+ my @potentials = @POTENTIAL_PAGE_ACTIONS;
+ while (my ($pattern, $result) = splice @potentials, 0, 2) {
+ return HTML::Mason::Commands::loc($result)
+ if $page =~ $pattern;
+ }
+ return "";
+}
+
package HTML::Mason::Commands;
use vars qw/$r $m %session/;
diff --git a/share/html/Elements/Login b/share/html/Elements/Login
index b86bfef..b3f1a24 100755
--- a/share/html/Elements/Login
+++ b/share/html/Elements/Login
@@ -61,6 +61,8 @@
<div id="login-box">
<&| /Widgets/TitleBox, title => loc('Login'), titleright => $RT::VERSION, hideable => 0 &>
+<& LoginRedirectWarning, %ARGS &>
+
% unless (RT->Config->Get('WebExternalAuth') and !RT->Config->Get('WebFallbackToInternalAuth')) {
<form id="login" name="login" method="post" action="<% RT->Config->Get('WebPath') %>/NoAuth/Login.html">
diff --git a/share/html/Elements/LoginRedirectWarning b/share/html/Elements/LoginRedirectWarning
new file mode 100644
index 0000000..af3f0a1
--- /dev/null
+++ b/share/html/Elements/LoginRedirectWarning
@@ -0,0 +1,20 @@
+<%args>
+$next => undef
+</%args>
+<%init>
+return unless $next;
+
+my $destination = RT::Interface::Web::FetchNextPage($next);
+return unless $destination and $destination->{'HasSideEffects'};
+
+my $consequence = RT::Interface::Web::PotentialPageAction($destination->{'url'}) || loc("perform actions");
+ $consequence = $m->interp->apply_escapes($consequence => "h");
+</%init>
+<div class="redirect-warning">
+ <p>
+ <&|/l&>After logging in you'll be sent to your original destination:</&>
+ <tt title="<% $destination->{'url'} %>"><% $destination->{'url'} %></tt>
+ <&|/l_unsafe, "<strong>$consequence</strong>" &>which may [_1] on your behalf.</&>
+ </p>
+ <p><&|/l&>If this is not what you expect, leave this page now without logging in.</&></p>
+</div>
diff --git a/share/html/NoAuth/css/base/login.css b/share/html/NoAuth/css/base/login.css
index bd05a28..608ebf8 100644
--- a/share/html/NoAuth/css/base/login.css
+++ b/share/html/NoAuth/css/base/login.css
@@ -100,3 +100,11 @@ margin-right:auto;margin-left:auto;
padding-left: 1em;
}
+.redirect-warning tt {
+ display: block;
+ margin: 0.5em 0 0.5em 1em;
+ white-space: nowrap;
+ overflow: hidden;
+ text-overflow: ellipsis;
+ width: 90%;
+}
commit 7e3782deb299becc9221cc83ee2d9cab09721d74
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Sep 28 13:06:29 2012 -0700
Include the potential request's action in the CSRF interstitial
We now have the means to be slightly less vague than "perform actions".
diff --git a/share/html/Elements/CSRF b/share/html/Elements/CSRF
index 4893c12..a3c1943 100644
--- a/share/html/Elements/CSRF
+++ b/share/html/Elements/CSRF
@@ -52,11 +52,11 @@
% my $strong_start = "<strong>";
% my $strong_end = "</strong>";
-<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>
+<p><&|/l_unsafe, $strong_start, $strong_end, $Reason, $action &>RT has detected a possible [_1]cross-site request forgery[_2] for this request, because [_3]. A malicious attacker may be trying to [_1][_4][_2] 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_unsafe, $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, $action, $start, $end &>If you really intended to visit [_1] and [_2], then [_3]click here to resume your request[_4].</&></p>
<& /Elements/Footer, %ARGS &>
% $m->abort;
@@ -71,4 +71,6 @@ $escaped_path = "<tt>$escaped_path</tt>";
my $url_with_token = URI->new($OriginalURL);
$url_with_token->query_form([CSRF_Token => $Token]);
+
+my $action = RT::Interface::Web::PotentialPageAction($OriginalURL) || loc("perform actions");
</%INIT>
commit 72ee4047c9855b3c8e1a7e2a0e7635c3a3ed230b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Aug 24 11:31:26 2012 -0700
Load the Class as the current user when creating Articles
We use the loaded class to check the CreateArticle right, which is
always true for the system user. This enabled any privileged user to
create articles in any class that existed given a crafted URL. Only the
core article fields were usable (Name, Summary, Class), since custom
fields are protected by independent rights.
Resolves CVE-2012-4731.
diff --git a/lib/RT/Article.pm b/lib/RT/Article.pm
index 24b952a..678aa11 100644
--- a/lib/RT/Article.pm
+++ b/lib/RT/Article.pm
@@ -102,7 +102,7 @@ sub Create {
@_
);
- my $class = RT::Class->new($RT::SystemUser);
+ my $class = RT::Class->new( $self->CurrentUser );
$class->Load( $args{'Class'} );
unless ( $class->Id ) {
return ( 0, $self->loc('Invalid Class') );
commit ecbdf95e4d4bebf71152fdfa18b11524d364c9a4
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Oct 3 12:29:15 2012 -0700
Headers in the parsed MIME entities of Templates are modifiable
This allows reformatting of inserted headers by canonicalizing tag case
and, crucially, folding (or refolding) lines.
When the header object is not explicitly marked modifiable — such as
when generated via parsing a raw MIME message — Mail::Header assumes
that header values you set should be inserted as-is. This means
newlines are not stripped or validated as you're expected to construct
proper continuations yourself.
RT incorrectly assumed newlines in header values would be stripped,
leaving open the possibility of header injection via various
user-controlled inputs. This commit resolves CVE-2012-4730.
Fixes failing tests by removing the assumptions that:
1) Case of header names is preserved
2) Header values are always on a single line
diff --git a/lib/RT/Template.pm b/lib/RT/Template.pm
index 117cc3f..e509454 100644
--- a/lib/RT/Template.pm
+++ b/lib/RT/Template.pm
@@ -390,6 +390,7 @@ sub _Parse {
# Unfold all headers
$self->{'MIMEObj'}->head->unfold;
+ $self->{'MIMEObj'}->head->modify(1);
return ( 1, $self->loc("Template parsed") );
diff --git a/t/web/crypt-gnupg.t b/t/web/crypt-gnupg.t
index 8c0eb57..b30edc3 100644
--- a/t/web/crypt-gnupg.t
+++ b/t/web/crypt-gnupg.t
@@ -8,6 +8,7 @@ use RT::Test::GnuPG
'trust-model' => 'always',
};
use Test::Warn;
+use MIME::Head;
use RT::Action::SendEmail;
@@ -70,8 +71,7 @@ $user->SetEmailAddress('general at example.com');
for my $mail (@mail) {
unlike $mail, qr/Some content/, "outgoing mail was encrypted";
- my ($content_type) = $mail =~ /^(Content-Type: .*)/m;
- my ($mime_version) = $mail =~ /^(MIME-Version: .*)/m;
+ my ($content_type, $mime_version) = get_headers($mail, "Content-Type", "MIME-Version");
my $body = strip_headers($mail);
$mail = << "MAIL";
@@ -139,8 +139,7 @@ for my $mail (@mail) {
like $mail, qr/Some other content/, "outgoing mail was not encrypted";
like $mail, qr/-----BEGIN PGP SIGNATURE-----[\s\S]+-----END PGP SIGNATURE-----/, "data has some kind of signature";
- my ($content_type) = $mail =~ /^(Content-Type: .*)/m;
- my ($mime_version) = $mail =~ /^(MIME-Version: .*)/m;
+ my ($content_type, $mime_version) = get_headers($mail, "Content-Type", "MIME-Version");
my $body = strip_headers($mail);
$mail = << "MAIL";
@@ -212,8 +211,7 @@ ok(@mail, "got some mail");
for my $mail (@mail) {
unlike $mail, qr/Some other content/, "outgoing mail was encrypted";
- my ($content_type) = $mail =~ /^(Content-Type: .*)/m;
- my ($mime_version) = $mail =~ /^(MIME-Version: .*)/m;
+ my ($content_type, $mime_version) = get_headers($mail, "Content-Type", "MIME-Version");
my $body = strip_headers($mail);
$mail = << "MAIL";
@@ -279,8 +277,7 @@ ok(@mail, "got some mail");
for my $mail (@mail) {
like $mail, qr/Thought you had me figured out didya/, "outgoing mail was unencrypted";
- my ($content_type) = $mail =~ /^(Content-Type: .*)/m;
- my ($mime_version) = $mail =~ /^(MIME-Version: .*)/m;
+ my ($content_type, $mime_version) = get_headers($mail, "Content-Type", "MIME-Version");
my $body = strip_headers($mail);
$mail = << "MAIL";
@@ -326,6 +323,20 @@ MAIL
like($attachments[0]->Content, qr/$RT::rtname/, "RT's mail includes this instance's name");
}
+sub get_headers {
+ my $mail = shift;
+ open my $fh, "<", \$mail or die $!;
+ my $head = MIME::Head->read($fh);
+ return @{[
+ map {
+ my $hdr = "$_: " . $head->get($_);
+ chomp $hdr;
+ $hdr;
+ }
+ @_
+ ]};
+}
+
sub strip_headers
{
my $mail = shift;
diff --git a/t/web/ticket_forward.t b/t/web/ticket_forward.t
index 1d74673..0c411b9 100644
--- a/t/web/ticket_forward.t
+++ b/t/web/ticket_forward.t
@@ -49,7 +49,7 @@ diag "Forward Ticket" if $ENV{TEST_VERBOSE};
my ($mail) = RT::Test->fetch_caught_mails;
like( $mail, qr!Subject: test forward!, 'Subject field' );
like( $mail, qr!To: rt-test, rt-to\@example.com!, 'To field' );
- like( $mail, qr!Cc: rt-cc\@example.com!, 'Cc field' );
+ like( $mail, qr!Cc: rt-cc\@example.com!i, 'Cc field' );
like( $mail, qr!This is a forward of ticket!, 'content' );
like( $mail, qr!this is an attachment!, 'att content' );
like( $mail, qr!$att_name!, 'att file name' );
@@ -75,8 +75,8 @@ qr/Forwarded Transaction #\d+ to rt-test, rt-to\@example.com, rt-cc\@example.com
my ($mail) = RT::Test->fetch_caught_mails;
like( $mail, qr!Subject: test forward!, 'Subject field' );
like( $mail, qr!To: rt-test, rt-to\@example.com!, 'To field' );
- like( $mail, qr!Cc: rt-cc\@example.com!, 'Cc field' );
- like( $mail, qr!Bcc: rt-bcc\@example.com!, 'Bcc field' );
+ like( $mail, qr!Cc: rt-cc\@example.com!i, 'Cc field' );
+ like( $mail, qr!Bcc: rt-bcc\@example.com!i, 'Bcc field' );
like( $mail, qr!This is a forward of transaction!, 'content' );
like( $mail, qr!$att_name!, 'att file name' );
like( $mail, qr!this is an attachment!, 'att content' );
commit 4cda0e47df5431c59d2a03f61917ab35c0cdbfaf
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Oct 4 10:16:08 2012 -0700
Comment on our invalid pattern for splitting headers
And why we can't fix it now.
diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index fb17da3..d46a991 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -676,6 +676,12 @@ sub _SplitHeaders {
my $self = shift;
my $headers = (shift || $self->_Value('Headers'));
my @headers;
+ # XXX TODO: splitting on \n\w is _wrong_ as it treats \n[ as a valid
+ # continuation, which it isn't. The correct split pattern, per RFC 2822,
+ # is /\n(?=[^ \t]|\z)/. That is, only "\n " or "\n\t" is a valid
+ # continuation. Older values of X-RT-GnuPG-Status contain invalid
+ # continuations and rely on this bogus split pattern, however, so it is
+ # left as-is for now.
for (split(/\n(?=\w|\z)/,$headers)) {
push @headers, $_;
commit c964114ca495830f70ec8789d4d9de878fbf84b2
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Oct 4 14:56:33 2012 -0700
Perltidy only before updating the SetHeader method
diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index d46a991..3209a9f 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -601,7 +601,7 @@ sub DelHeader {
my $newheader = '';
foreach my $line ($self->_SplitHeaders) {
next if $line =~ /^\Q$tag\E:\s+(.*)$/is;
- $newheader .= "$line\n";
+ $newheader .= "$line\n";
}
return $self->__Set( Field => 'Headers', Value => $newheader);
}
@@ -636,14 +636,13 @@ sub SetHeader {
my $tag = shift;
my $newheader = '';
- foreach my $line ($self->_SplitHeaders) {
- if (defined $tag and $line =~ /^\Q$tag\E:\s+(.*)$/i) {
- $newheader .= "$tag: $_[0]\n";
- undef $tag;
+ foreach my $line ( $self->_SplitHeaders ) {
+ if ( defined $tag and $line =~ /^\Q$tag\E:\s+(.*)$/i ) {
+ $newheader .= "$tag: $_[0]\n";
+ undef $tag;
+ } else {
+ $newheader .= "$line\n";
}
- else {
- $newheader .= "$line\n";
- }
}
$newheader .= "$tag: $_[0]\n" if defined $tag;
commit 0bd9510437d7adbeb330313eaebc6103eb917026
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Oct 4 15:14:36 2012 -0700
No need to match on the rest of the header line(s), just the tag
Simpler, less prone to a mistake (like the one in SetHeader to be fixed
next).
diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index 3209a9f..1566e87 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -600,7 +600,7 @@ sub DelHeader {
my $newheader = '';
foreach my $line ($self->_SplitHeaders) {
- next if $line =~ /^\Q$tag\E:\s+(.*)$/is;
+ next if $line =~ /^\Q$tag\E:\s+/i;
$newheader .= "$line\n";
}
return $self->__Set( Field => 'Headers', Value => $newheader);
commit e0c1ec5163da68ddaf477010b6219d41a7be40bc
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Oct 4 15:17:43 2012 -0700
Don't require a \r before the \n when forcing header continuations
A bare \n needs to be continued just as much as \r\n.
This doesn't fix the bug of adding an extra space to existing valid
continuations.
diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index 1566e87..039d5e8 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -619,7 +619,7 @@ sub AddHeader {
while ( my ($tag, $value) = splice @_, 0, 2 ) {
$value = '' unless defined $value;
$value =~ s/\s+$//s;
- $value =~ s/\r+\n/\n /g;
+ $value =~ s/\r*\n/\n /g;
$newheader .= "$tag: $value\n";
}
return $self->__Set( Field => 'Headers', Value => $newheader);
commit 42623930c7a4eb524dcd5d702912faae418acab0
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Oct 4 15:19:35 2012 -0700
Refactor header value canonicalization for use by other methods
SetHeader in particular does no canonicalization (yet).
diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index 039d5e8..a464004 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -617,9 +617,7 @@ sub AddHeader {
my $newheader = $self->__Value( 'Headers' );
while ( my ($tag, $value) = splice @_, 0, 2 ) {
- $value = '' unless defined $value;
- $value =~ s/\s+$//s;
- $value =~ s/\r*\n/\n /g;
+ $value = $self->_CanonicalizeHeaderValue($value);
$newheader .= "$tag: $value\n";
}
return $self->__Set( Field => 'Headers', Value => $newheader);
@@ -649,6 +647,17 @@ sub SetHeader {
$self->__Set( Field => 'Headers', Value => $newheader);
}
+sub _CanonicalizeHeaderValue {
+ my $self = shift;
+ my $value = shift;
+
+ $value = '' unless defined $value;
+ $value =~ s/\s+$//s;
+ $value =~ s/\r*\n/\n /g;
+
+ return $value;
+}
+
=head2 SplitHeaders
Returns an array of this attachment object's headers, with one header
commit dba758b4353ed506e1d7527a0aad22b0124c8786
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Oct 4 15:21:08 2012 -0700
Fix three bugs in SetHeader
SetHeader now:
• Canonicalizes the inserted header value to handle continuations
• Removes all existing occurrences of the header instead of only the first.
• Removes header continuations instead of leaving the continued lines
behind (rendering them continuations of the previous header!)
diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index a464004..f1d9a63 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -630,20 +630,25 @@ Replace or add a Header to the attachment's headers.
=cut
sub SetHeader {
- my $self = shift;
- my $tag = shift;
+ my $self = shift;
+ my $tag = shift;
+ my $value = $self->_CanonicalizeHeaderValue(shift);
+ my $replaced = 0;
my $newheader = '';
foreach my $line ( $self->_SplitHeaders ) {
- if ( defined $tag and $line =~ /^\Q$tag\E:\s+(.*)$/i ) {
- $newheader .= "$tag: $_[0]\n";
- undef $tag;
+ if ( $line =~ /^\Q$tag\E:\s+/i ) {
+ # replace first instance, skip all the rest
+ unless ($replaced) {
+ $newheader .= "$tag: $value\n";
+ $replaced = 1;
+ }
} else {
$newheader .= "$line\n";
}
}
- $newheader .= "$tag: $_[0]\n" if defined $tag;
+ $newheader .= "$tag: $value\n" unless $replaced;
$self->__Set( Field => 'Headers', Value => $newheader);
}
commit e6c6ae922252e36b510264472820ea8df72e4b7e
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Oct 4 21:59:42 2012 -0700
Let MIME::Head modify the X-RT-GnuPg-Status header to handle continuations
X-RT-GnuPG-Status is a set to a many-line string and previously
continuations weren't handled properly.
Temporarily letting the inserted headers be modified fixes the known
problem. If instead we marked the entire entity as modifiable early
after parsing and left it that way, the possibility of unforeseen
breakage would be too high for a stable release series. It would also
lead to even further deviation of RT's stored message from the original
source message.
diff --git a/lib/RT/Crypt/GnuPG.pm b/lib/RT/Crypt/GnuPG.pm
index 8df1c1b..3ebb7d7 100644
--- a/lib/RT/Crypt/GnuPG.pm
+++ b/lib/RT/Crypt/GnuPG.pm
@@ -1077,9 +1077,13 @@ sub VerifyDecrypt {
}
if ( $args{'SetStatus'} || $args{'AddStatus'} ) {
my $method = $args{'AddStatus'} ? 'add' : 'set';
+ # Let the header be modified so continuations are handled
+ my $modify = $status_on->head->modify;
+ $status_on->head->modify(1);
$status_on->head->$method(
'X-RT-GnuPG-Status' => $res[-1]->{'status'}
);
+ $status_on->head->modify($modify);
}
}
foreach my $item( grep $_->{'Type'} eq 'encrypted', @protected ) {
@@ -1096,9 +1100,13 @@ sub VerifyDecrypt {
}
if ( $args{'SetStatus'} || $args{'AddStatus'} ) {
my $method = $args{'AddStatus'} ? 'add' : 'set';
+ # Let the header be modified so continuations are handled
+ my $modify = $status_on->head->modify;
+ $status_on->head->modify(1);
$status_on->head->$method(
'X-RT-GnuPG-Status' => $res[-1]->{'status'}
);
+ $status_on->head->modify($modify);
}
}
return @res;
commit 496e93fe0360954c7d36cecbc2219791752c58c7
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Oct 5 15:03:20 2012 -0700
Don't 500 if we come across a session with NextPage of the old variety
Unlikely, but possible if the session got a login form before the
security upgrade and submitted the same form after the upgrade.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index c8667c1..dee87fe 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -646,7 +646,7 @@ sub AttemptExternalAuth {
}
my $next = RemoveNextPage($ARGS->{'next'});
- $next = $next->{'url'} if $next;
+ $next = $next->{'url'} if ref $next;
InstantiateNewSession() unless _UserLoggedIn;
$HTML::Mason::Commands::session{'CurrentUser'} = RT::CurrentUser->new();
$HTML::Mason::Commands::session{'CurrentUser'}->$load_method($user);
@@ -746,7 +746,7 @@ sub AttemptPasswordAuthentication {
# It's important to nab the next page from the session before we blow
# the session away
my $next = RemoveNextPage($ARGS->{'next'});
- $next = $next->{'url'} if $next;
+ $next = $next->{'url'} if ref $next;
InstantiateNewSession();
$HTML::Mason::Commands::session{'CurrentUser'} = $user_obj;
diff --git a/share/html/Elements/LoginRedirectWarning b/share/html/Elements/LoginRedirectWarning
index af3f0a1..891e381 100644
--- a/share/html/Elements/LoginRedirectWarning
+++ b/share/html/Elements/LoginRedirectWarning
@@ -5,7 +5,7 @@ $next => undef
return unless $next;
my $destination = RT::Interface::Web::FetchNextPage($next);
-return unless $destination and $destination->{'HasSideEffects'};
+return unless ref $destination and $destination->{'HasSideEffects'};
my $consequence = RT::Interface::Web::PotentialPageAction($destination->{'url'}) || loc("perform actions");
$consequence = $m->interp->apply_escapes($consequence => "h");
commit 0b3d6861f99269a40dce601263bba1145e61bd1d
Merge: e4916c6 72ee404
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 14:37:03 2012 -0400
Merge branch 'security/4.0/create-article' into 4.0.8-releng
commit f350d859467e04d391491c0129eca052a90059bc
Merge: 0b3d686 4c40dd6
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 14:37:08 2012 -0400
Merge branch 'security/4.0/csrf-blacklist' into 4.0.8-releng
commit 73c3c162183b756bb33378f3def60033de776836
Merge: f350d85 e6c6ae9
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 14:37:11 2012 -0400
Merge branch 'security/4.0/email-header-injection' into 4.0.8-releng
commit 9b5d6534dc158b4449e6c6587fde19f51ff02721
Merge: 73c3c16 e3c2880
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 14:37:15 2012 -0400
Merge branch 'security/4.0/signing' into 4.0.8-releng
commit 048ac133c6aa7af528a84dc1aece81042ed79c4f
Merge: 9b5d653 496e93f
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 14:37:20 2012 -0400
Merge branch 'security/4.0/warn-about-redirect-after-login' into 4.0.8-releng
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list