[Rt-commit] rt branch, 3.8.15-releng, created. rt-3.8.14-42-g9207c2b
Kevin Falcone
falcone at bestpractical.com
Thu Oct 25 18:41:37 EDT 2012
The branch, 3.8.15-releng has been created
at 9207c2bb13f88e07d4863fa3a37b4243d06a5dc1 (commit)
- Log -----------------------------------------------------------------
commit 3c00b4eb1822720f4c3eb457ed4a4979039fdbb2
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Feb 7 15:54:04 2011 -0500
Make ExternalAuth also respect the ?next=hash argument after logins
(cherry picked from commit 5dfa1847869db1040b685293af14858e3804abd1)
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 798d26d..abb764b 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -525,6 +525,7 @@ sub AttemptExternalAuth {
$user =~ s/^\Q$NodeName\E\\//i;
}
+ my $next = delete $HTML::Mason::Commands::session{'NextPage'}->{$ARGS->{'next'} || ''};
InstantiateNewSession() unless _UserLoggedIn;
$HTML::Mason::Commands::session{'CurrentUser'} = RT::CurrentUser->new();
$HTML::Mason::Commands::session{'CurrentUser'}->$load_method($user);
@@ -572,6 +573,7 @@ sub AttemptExternalAuth {
if ( _UserLoggedIn() ) {
$m->callback( %$ARGS, CallbackName => 'ExternalAuthSuccessfulLogin', CallbackPage => '/autohandler' );
+ Redirect($next) if $next;
} else {
delete $HTML::Mason::Commands::session{'CurrentUser'};
$user = $orig_user;
commit f6673a2abd849b4c0d39f6c121fddd1383e7c849
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Feb 11 17:17:44 2011 -0500
Provide some rationale in comments for the convoluted logic
(cherry picked from commit 49bd564ca4a883a37af17711ba584a98bc1ca1e1)
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index abb764b..0595c0b 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -573,7 +573,15 @@ sub AttemptExternalAuth {
if ( _UserLoggedIn() ) {
$m->callback( %$ARGS, CallbackName => 'ExternalAuthSuccessfulLogin', CallbackPage => '/autohandler' );
+ # It is possible that we did a redirect to the login page,
+ # if the external auth allows lack of auth through with no
+ # REMOTE_USER set, instead of forcing a "permission
+ # denied" message. Honor the $next.
Redirect($next) if $next;
+ # Unlike AttemptPasswordAuthentication below, we do not
+ # force a redirect to / if $next is not set -- otherwise,
+ # straight-up external auth would always redirect to /
+ # when you first hit it.
} else {
delete $HTML::Mason::Commands::session{'CurrentUser'};
$user = $orig_user;
commit 673337b97b9a3c81959dd38ab970f44d40a2a6ce
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Jul 11 09:11:40 2012 -0700
WebExternalOnly was renamed to WebFallbackToInternalAuth
Although it was caught in 21874f2c back in 2006, that commit was
accidentally reverted in 3027bf06 during a merge.
This commit applies effectively the same diff as 21874f2c.
(cherry picked from commit 71c59d6157d08be514a00b359290d7f7486a0f92)
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 0595c0b..1ba98fd 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -586,7 +586,7 @@ sub AttemptExternalAuth {
delete $HTML::Mason::Commands::session{'CurrentUser'};
$user = $orig_user;
- if ( RT->Config->Get('WebExternalOnly') ) {
+ unless ( RT->Config->Get('WebFallbackToInternalAuth') ) {
TangentForLoginWithError('You are not an authorized user');
}
}
commit 532719661a10199f03fad962f925ea53a4a313f9
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 1ba98fd..f3657a5 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -244,7 +244,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 c346a1760d7cafe3d42239d4490527ee6d6f0275
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 f3657a5..d969611 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -315,6 +315,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]
@@ -525,7 +546,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);
@@ -624,7 +645,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 fd4a12b5ae2e8ba0022c3b832e1509349d747400
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 d969611..8aba86b 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -310,14 +310,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
@@ -328,7 +328,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
@@ -547,6 +547,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);
@@ -646,6 +647,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 44c383be047b8ab4ebb025931146a116674ede58
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 8aba86b..6c88a43 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -244,12 +244,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));
}
}
}
@@ -298,7 +298,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
@@ -307,10 +307,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;
}
@@ -337,15 +357,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);
@@ -360,8 +383,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
@@ -586,7 +610,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();
}
@@ -609,13 +633,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 cd52f369163b942aeb18ed3b51952abc51be4360
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 6d43b96..cca5751 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 bc9b830951c23a1606d5d47289e5dd576e221597
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 cca5751..846c013 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 d40153f411d9df7e361ebe0d349dbe26f17fbd90
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 8be14af..a26f6ec 100644
--- a/share/html/Elements/GnuPG/SignEncryptWidget
+++ b/share/html/Elements/GnuPG/SignEncryptWidget
@@ -124,12 +124,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 6f369a7b16355c04bac190e4aff1cff8d390cbdd
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 de11993..ab63727 100644
--- a/share/html/Admin/Users/GnuPG.html
+++ b/share/html/Admin/Users/GnuPG.html
@@ -69,7 +69,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'),
&>
@@ -96,7 +96,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 afbe5d7262c7e70f0ea5f2e7869d908e10056264
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 ab63727..b50b22e 100644
--- a/share/html/Admin/Users/GnuPG.html
+++ b/share/html/Admin/Users/GnuPG.html
@@ -106,8 +106,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 3554073b41c3459dccda7770ac13a4096a046ccc
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 b50b22e..63e175e 100644
--- a/share/html/Admin/Users/GnuPG.html
+++ b/share/html/Admin/Users/GnuPG.html
@@ -107,8 +107,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 46092f1cb697acb120800165f2ca0f2b884dfdee
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 798d26d..172ef50 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1044,6 +1044,13 @@ our %is_whitelisted_component = (
'/Search/Simple.html' => 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;
@@ -1066,6 +1073,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 90c62a8b13a0066a3507d019ce850fd48be0e956
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_Overlay.pm b/lib/RT/Template_Overlay.pm
index 21cb97a..963a87b 100755
--- a/lib/RT/Template_Overlay.pm
+++ b/lib/RT/Template_Overlay.pm
@@ -383,6 +383,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 fb28c88..1a317af 100644
--- a/t/web/crypt-gnupg.t
+++ b/t/web/crypt-gnupg.t
@@ -8,6 +8,7 @@ plan skip_all => 'GnuPG required.'
plan skip_all => 'gpg executable is required.'
unless RT::Test->find_executable('gpg');
+use MIME::Head;
use RT::Action::SendEmail;
@@ -95,8 +96,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";
@@ -163,8 +163,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";
@@ -235,8 +234,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";
@@ -301,8 +299,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";
@@ -348,6 +345,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;
commit 6031c17cd51d48a7eb0cad9736b96bc1fe9427d3
Author: Ruslan Zakirov <ruz at bestpractical.com>
Date: Wed Jun 23 23:44:45 2010 +0400
Don't encode folded headers, such as Subject
Now that long Subject headers may be folded during insertion, headers
may contain \r\n, which EncodeToMIME incorrectly then MIME-encodes.
Cherry-pick back 1c1d8a2, which ensures that we do not un-necessarily
MIME-Encode subject headers merely for being folded.
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index a8e2ce8..d85add3 100755
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -905,7 +905,7 @@ sub EncodeToMIME {
return ($value);
}
- return ($value) unless $value =~ /[^\x20-\x7e]/;
+ return ($value) if $value =~ /^(?:[\t\x20-\x7e]|\x0D*\x0A[ \t])+$/s;
$value =~ s/\s+$//;
commit f694f92061fb49a6e2209560dc4f238539e42118
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_Overlay.pm b/lib/RT/Attachment_Overlay.pm
index 45a5ab6..7808cc2 100644
--- a/lib/RT/Attachment_Overlay.pm
+++ b/lib/RT/Attachment_Overlay.pm
@@ -626,6 +626,12 @@ sub _SplitHeaders {
my $self = shift;
my $headers = (shift || $self->SUPER::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 fa1b930f6f017e8c56d7d38c4d05f008d493a90b
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_Overlay.pm b/lib/RT/Attachment_Overlay.pm
index 7808cc2..5f47b48 100644
--- a/lib/RT/Attachment_Overlay.pm
+++ b/lib/RT/Attachment_Overlay.pm
@@ -551,7 +551,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);
}
@@ -586,14 +586,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 4db9fc51d96e47aefb7c3261720c1d7793d6d061
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_Overlay.pm b/lib/RT/Attachment_Overlay.pm
index 5f47b48..a88e86f 100644
--- a/lib/RT/Attachment_Overlay.pm
+++ b/lib/RT/Attachment_Overlay.pm
@@ -550,7 +550,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 a81641fb571467216ae0bf7b4868e11e1a3595d5
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_Overlay.pm b/lib/RT/Attachment_Overlay.pm
index a88e86f..77417d3 100644
--- a/lib/RT/Attachment_Overlay.pm
+++ b/lib/RT/Attachment_Overlay.pm
@@ -569,7 +569,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 7b077390de3b31e7ca303765492c49dbf08854cd
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_Overlay.pm b/lib/RT/Attachment_Overlay.pm
index 77417d3..1b3a2a6 100644
--- a/lib/RT/Attachment_Overlay.pm
+++ b/lib/RT/Attachment_Overlay.pm
@@ -567,9 +567,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);
@@ -599,6 +597,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 18c1c1ae5995c91a1dcf0dc09e947824fddf92d6
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_Overlay.pm b/lib/RT/Attachment_Overlay.pm
index 1b3a2a6..cdc6770 100644
--- a/lib/RT/Attachment_Overlay.pm
+++ b/lib/RT/Attachment_Overlay.pm
@@ -580,20 +580,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 6d24554940028a9c1e2267d46448cf9bdb5a4f12
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 2efee33..b30fd29 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 0e7ae6e14970cc3d88a8fafada65603b393d0cfd
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_Overlay.pm b/lib/RT/User_Overlay.pm
index db31c5e..f168ea4 100755
--- a/lib/RT/User_Overlay.pm
+++ b/lib/RT/User_Overlay.pm
@@ -94,6 +94,7 @@ sub _OverlayAccessible {
AuthSystem => { public => 1, admin => 1 },
Gecos => { public => 1, admin => 1 },
PGPKey => { public => 1, admin => 1 },
+ PrivateKey => { admin => 1 },
}
}
commit 8c04cb4bf33d021e269e23e6aa5e305788c1c4a8
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 2efee33..a13a642 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;
};
@@ -2315,7 +2317,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 97cba99d13e1133a60625f25a779ef3c0653040b
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 a98a764..473a742 100755
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -107,23 +107,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 a8e2ce8..c532d2a 100755
--- 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 b4385a9c2160434c89413b6db6eb36c6e1eae217
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 473a742..189b999 100755
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -100,34 +100,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 c532d2a..1452d1c 100755
--- 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 7a7604ea010d1d5fec226f9df494d38803982577
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 798d26d..4bcae78 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1393,10 +1393,8 @@ sub CreateTicket {
}
}
- foreach my $argument (qw(Encrypt Sign)) {
- $MIMEObj->head->add(
- "X-RT-$argument" => Encode::encode_utf8( $ARGS{$argument} )
- ) if defined $ARGS{$argument};
+ for my $argument (qw(Encrypt Sign)) {
+ $MIMEObj->head->replace( "X-RT-$argument" => $ARGS{$argument} ? 1 : 0 );
}
my %create_args = (
commit d89c737753a562ec410456a035aae7b66a275692
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 1452d1c..824a394 100755
--- 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_Overlay.pm b/lib/RT/Queue_Overlay.pm
index c7b7498..d998adb 100755
--- a/lib/RT/Queue_Overlay.pm
+++ b/lib/RT/Queue_Overlay.pm
@@ -336,6 +336,7 @@ sub Create {
FinalPriority => 0,
DefaultDueIn => 0,
Sign => undef,
+ SignAuto => undef,
Encrypt => undef,
_RecordTransaction => 1,
@_
@@ -370,14 +371,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;
}
@@ -520,6 +518,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 5dd7bf0..5bb5c26 100755
--- a/share/html/Admin/Queues/Modify.html
+++ b/share/html/Admin/Queues/Modify.html
@@ -113,6 +113,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>
@@ -180,13 +182,13 @@ if ($Create) {
if ( $QueueObj->Id ) {
delete $session{'create_in_queues'};
my @attribs= qw(Description CorrespondAddress CommentAddress Name
- InitialPriority FinalPriority DefaultDueIn Sign Encrypt SubjectTag Disabled);
+ InitialPriority FinalPriority DefaultDueIn Sign SignAuto Encrypt 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);
}
push @results, UpdateRecordObject(
commit 4dd863af5f3fd23f5cadf2ae95a5f3f43cf42bb6
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Fri Jun 8 16:28:07 2012 -0400
GPG 1.4.12 tweaked the header on the trustdb
Without this, anyone running rt tests on 1.4.12 rather than 1.4.10 would
end up with changes in the test suite. This should be back-compat to
1.4.10.
(cherry picked from commit 41dc5b44091d3c5ccf01c534823a15143a09ba86)
(cherry picked from commit 19c26e9111264d9ce8d1c0203827131f7edc86c3)
diff --git a/t/data/gnupg/keyrings/trustdb.gpg b/t/data/gnupg/keyrings/trustdb.gpg
index 9f2ae63..331d5dd 100644
Binary files a/t/data/gnupg/keyrings/trustdb.gpg and b/t/data/gnupg/keyrings/trustdb.gpg differ
commit 5c872d1e399a0f4a424af2dd6df08b58fcfbf257
Author: Jim Brandt <jbrandt at bestpractical.com>
Date: Tue May 1 10:45:35 2012 -0400
Add DECRYPTION_INFO to ignore_keywords.
GnuPG added DECRYPTION_INFO in version 1.4.12.
https://gitorious.org/gnupg-org/gnupg/commit/cfb193a1de2f0553ee65a19a417a885938539225
(cherry picked from commit ea2a23c994b19fca0755a25d208b2aba77021530)
diff --git a/lib/RT/Crypt/GnuPG.pm b/lib/RT/Crypt/GnuPG.pm
index a13a642..21fa73b 100644
--- a/lib/RT/Crypt/GnuPG.pm
+++ b/lib/RT/Crypt/GnuPG.pm
@@ -1697,6 +1697,7 @@ my %ignore_keyword = map { $_ => 1 } qw(
BEGIN_ENCRYPTION SIG_ID VALIDSIG
ENC_TO BEGIN_DECRYPTION END_DECRYPTION GOODMDC
TRUST_UNDEFINED TRUST_NEVER TRUST_MARGINAL TRUST_FULLY TRUST_ULTIMATE
+ DECRYPTION_INFO
);
sub ParseStatus {
commit 3194a62ce1d8fab3cde8d10d8a1f5e6d5de8f5ae
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 6c88a43..8b86269 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1302,6 +1302,29 @@ 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'/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 eb645d4..936ad6a 100755
--- a/share/html/Elements/Login
+++ b/share/html/Elements/Login
@@ -65,6 +65,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/misc.css b/share/html/NoAuth/css/base/misc.css
index ede9dae..8670a59 100644
--- a/share/html/NoAuth/css/base/misc.css
+++ b/share/html/NoAuth/css/base/misc.css
@@ -98,3 +98,12 @@ hr.clear {
border: none;
font-size: 1px;
}
+
+.redirect-warning tt {
+ display: block;
+ margin: 0.5em 0 0.5em 1em;
+ white-space: nowrap;
+ overflow: hidden;
+ text-overflow: ellipsis;
+ width: 90%;
+}
commit a4a58b4dd558da4bfbbeeb23e6e1aaf8030656ff
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 b7c1575..21a5306 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 3fde2bcd1bb3b709cb88c7252d3975036e5e0965
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.
(cherry picked from commit 496e93fe0360954c7d36cecbc2219791752c58c7)
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 8b86269..1da4bd4 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -571,7 +571,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);
@@ -671,7 +671,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 b033a5c1e0abea911c165b3766e23cc1adfa3c35
Merge: 70a841a 46092f1
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 15:04:51 2012 -0400
Merge branch 'security/3.8/csrf-blacklist' into 3.8.15-releng
commit 7265102e205962c7fe26f4853de5524e0c44a000
Merge: b033a5c 6d24554
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 15:04:55 2012 -0400
Merge branch 'security/3.8/email-header-injection' into 3.8.15-releng
commit 3eb51e91463180082f38eb73b9a792305e4bfc05
Merge: 7265102 5c872d1
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 15:04:58 2012 -0400
Merge branch 'security/3.8/signing' into 3.8.15-releng
commit 91fd945a5c78c336adc049434898c2129ccc56a3
Merge: 3eb51e9 3fde2bc
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 15:05:01 2012 -0400
Merge branch 'security/3.8/warn-about-redirect-after-login' into 3.8.15-releng
commit 9207c2bb13f88e07d4863fa3a37b4243d06a5dc1
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 25 16:19:10 2012 -0400
bump version for 3.8.15
diff --git a/configure.ac b/configure.ac
index 5104d44..6f5f71d 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.15, [rt-bugs at bestpractical.com])
AC_CONFIG_SRCDIR([lib/RT.pm.in])
dnl Extract RT version number components
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list