[Rt-commit] rt branch, 4.4.2-releng, updated. rt-4.4.2rc1-15-g3f9e376

Shawn Moore shawn at bestpractical.com
Thu Jun 15 14:55:36 EDT 2017


The branch, 4.4.2-releng has been updated
       via  3f9e376a334670e12b5570b9ded1f6f2d7d6c059 (commit)
       via  9880ea1f304f4b860de476220716600dc9af2e38 (commit)
       via  db7ed96ba7b9a3c7722e10935ae7e88fa48e9cd7 (commit)
       via  85c8471fd873b3473adfbd5d1076efe793d20605 (commit)
       via  c4b0d35a0b058711fd42666a3ca03e52aedfb363 (commit)
       via  274e11d4748ee0c5c4b59c43a26a071ce8cb6eaf (commit)
       via  80660a5cc74715c5587d89bee90139fe88137d81 (commit)
       via  f6a8864ca439aa278b2583d460399ceb8830c182 (commit)
       via  c98487eb88e7df552bd555d134f98d2b4bb62f3a (commit)
       via  5e174fe2f2c9bd282b38e6b9c3b353e48143e81d (commit)
       via  3143a4cf1b4f81354a7a47977d3bc2ca4de1c461 (commit)
       via  b9c2968bfcb6383876e399c0604dbe390939c517 (commit)
       via  aebb36bdf32cf1166d945403ab0bae8e034434a9 (commit)
      from  d487efa662db952d29eaa719606c942c2e0a97ea (commit)

Summary of changes:
 lib/RT.pm                               |  4 +++
 lib/RT/Authen/ExternalAuth/DBI.pm       | 17 +++++++++--
 lib/RT/Config.pm                        |  8 +++++
 lib/RT/Interface/Web.pm                 |  4 +--
 lib/RT/User.pm                          | 38 +++++++++++++++++------
 lib/RT/Util.pm                          | 54 +++++++++++++++++++++++++++++++++
 sbin/rt-test-dependencies.in            |  2 +-
 share/html/Dashboards/Subscription.html |  2 +-
 share/html/Ticket/Attachment/dhandler   |  6 ++--
 t/web/csrf.t                            | 14 ++++-----
 10 files changed, 125 insertions(+), 24 deletions(-)

- Log -----------------------------------------------------------------
commit f6a8864ca439aa278b2583d460399ceb8830c182
Merge: ff7a847 b9c2968
Author: Shawn M Moore <shawn at bestpractical.com>
Date:   Thu Jun 1 15:33:53 2017 +0000

    Merge branch 'security/4.0/csrf-leak' into security/4.4.2-releng


commit 80660a5cc74715c5587d89bee90139fe88137d81
Merge: f6a8864 5e174fe
Author: Shawn M Moore <shawn at bestpractical.com>
Date:   Thu Jun 1 15:34:29 2017 +0000

    Merge branch 'security/4.0/email-parse-dos' into security/4.4.2-releng

diff --cc sbin/rt-test-dependencies.in
index ec88168,7882a37..27ed21c
--- a/sbin/rt-test-dependencies.in
+++ b/sbin/rt-test-dependencies.in
@@@ -136,76 -196,85 +136,76 @@@ Devel::StackTrace 1.1
  Digest::base
  Digest::MD5 2.27
  Digest::SHA
- Email::Address 1.897
 -DBI 1.37
 -Class::ReturnValue 0.40
 -DBIx::SearchBuilder 1.59
 -Text::Template 1.44
++Email::Address 1.908
 +Email::Address::List 0.02
 +Encode 2.64
 +Errno
 +File::Glob
  File::ShareDir
  File::Spec 0.8
 +File::Temp 0.19
 +HTML::Entities
 +HTML::FormatText::WithLinks 0.14
 +HTML::FormatText::WithLinks::AndTables 0.06
 +HTML::Mason 1.43
 +HTML::Mason::PSGIHandler 0.52
  HTML::Quoted
 +HTML::RewriteAttributes 0.05
  HTML::Scrubber 0.08
 -HTML::TreeBuilder
 -HTML::FormatText
 -Log::Dispatch 2.23
 -Sys::Syslog 0.16
 +HTTP::Message 6.0
 +IPC::Run3
 +JavaScript::Minifier::XS
 +JSON
 +List::MoreUtils
  Locale::Maketext 1.06
 +Locale::Maketext::Fuzzy 0.11
  Locale::Maketext::Lexicon 0.32
 -Locale::Maketext::Fuzzy
 -MIME::Entity 5.425
 +Log::Dispatch 2.30
 +LWP::Simple
 +Mail::Header 2.12
  Mail::Mailer 1.57
 -Email::Address 1.908
 -Text::Wrapper 
 -Time::ParseDate
 -Time::HiRes 
 -File::Temp 0.19
 -Text::Quoted 2.02
 -Tree::Simple 1.04
 -UNIVERSAL::require
 -Regexp::Common
 -Scalar::Util
 +MIME::Entity 5.504
 +MIME::Types
 +Module::Refresh 0.03
  Module::Versions::Report 1.05
 -Cache::Simple::TimedExpiry
 -Encode 2.64
 -CSS::Squish 0.06
 -File::Glob
 -Devel::StackTrace 1.19
 -Text::Password::Pronounceable
 -Devel::GlobalDestruction
 -List::MoreUtils
  Net::CIDR
 +Net::IP
 +Plack 1.0002
 +Plack::Handler::Starlet
 +Pod::Select
 +Regexp::Common
  Regexp::Common::net::CIDR
  Regexp::IPv6
 -.
 -
 -$deps{'MASON'} = [ text_to_hash( << '.') ];
 -HTML::Mason 1.43
 -Errno
 -Digest::MD5 2.27
 -CGI::Cookie 1.20
 +Role::Basic 0.12
 +Scalar::Util
 +Scope::Upper
  Storable 2.08
 -Apache::Session 1.53
 -XML::RSS 1.05
 +Symbol::Global::Name 0.04
 +Sys::Syslog 0.16
 +Text::Password::Pronounceable
 +Text::Quoted 2.07
 +Text::Template 1.44
  Text::WikiFormat 0.76
 -CSS::Squish 0.06
 -Devel::StackTrace 1.19
 -JSON
 -IPC::Run3
 -.
 -
 -$deps{'PSGI'} = [ text_to_hash( << '.') ];
 -CGI 3.38
 -CGI::PSGI 0.12
 -HTML::Mason::PSGIHandler 0.52
 -Plack 0.9971
 -Plack::Handler::Starlet
 -CGI::Emulate::PSGI
 +Text::Wrapper
 +Time::HiRes
 +Time::ParseDate
 +Tree::Simple 1.04
 +UNIVERSAL::require
 +URI 1.59
 +URI::QueryParam
 +XML::RSS 1.05
  .
 -set_dep( PSGI => CGI => 4.00 ) if $] > 5.019003;
 -
 +set_dep( CORE => 'Symbol::Global::Name' => 0.05 ) if $] >= 5.019003;
 +set_dep( CORE => CGI => 4.00 ) if $] > 5.019003;
  
  $deps{'MAILGATE'} = [ text_to_hash( << '.') ];
 -Getopt::Long
 -LWP::UserAgent
 -Pod::Usage
 -.
 -
 -$deps{'SSL-MAILGATE'} = [ text_to_hash( << '.') ];
  Crypt::SSLeay
 -Net::SSL
 -LWP::UserAgent 6.0
 +Getopt::Long
  LWP::Protocol::https
 +LWP::UserAgent 6.0
  Mozilla::CA
 +Net::SSL
 +Pod::Usage
  .
  
  $deps{'CLI'} = [ text_to_hash( << '.') ];

commit 274e11d4748ee0c5c4b59c43a26a071ce8cb6eaf
Merge: 80660a5 aebb36b
Author: Shawn M Moore <shawn at bestpractical.com>
Date:   Thu Jun 1 15:35:27 2017 +0000

    Merge branch 'security/4.0/attachment-xss-fix' into security/4.4.2-releng

diff --cc share/html/Ticket/Attachment/dhandler
index a711351,1cdedd7..afa471c
--- a/share/html/Ticket/Attachment/dhandler
+++ b/share/html/Ticket/Attachment/dhandler
@@@ -46,58 -46,52 +46,60 @@@
  %#
  %# END BPS TAGGED BLOCK }}}
  <%perl>
 -    my ($ticket, $trans,$attach, $filename);
 -    my $arg = $m->dhandler_arg;                # get rest of path
 -    if ($arg =~ m{^(\d+)/(\d+)}) {
 -        $trans = $1;
 -        $attach = $2;
 -    }
 -    else {
 -        Abort("Corrupted attachment URL.");
 -    }
 -     my $AttachmentObj = RT::Attachment->new($session{'CurrentUser'});
 -     $AttachmentObj->Load($attach) || Abort("Attachment '$attach' could not be loaded");
 +my ( $ticket, $trans, $attach, $filename );
 +my $arg = $m->dhandler_arg;    # get rest of path
 +if ( $arg =~ m{^(\d+)/(\d+)} ) {
 +    $trans  = $1;
 +    $attach = $2;
 +}
 +else {
 +    Abort("Corrupted attachment URL.");
 +}
 +my $AttachmentObj = RT::Attachment->new( $session{'CurrentUser'} );
 +$AttachmentObj->Load($attach) || Abort("Attachment '$attach' could not be loaded");
  
 +unless ( $AttachmentObj->id ) {
 +    Abort("Bad attachment id. Couldn't find attachment '$attach'\n");
 +}
 +unless ( $AttachmentObj->TransactionId() == $trans ) {
 +    Abort("Bad transaction number for attachment. $trans should be". $AttachmentObj->TransactionId() . "\n");
 +}
  
 -     unless ($AttachmentObj->id) {
 -        Abort("Bad attachment id. Couldn't find attachment '$attach'\n");
 -    }
 -     unless ($AttachmentObj->TransactionId() == $trans ) {
 -        Abort("Bad transaction number for attachment. $trans should be".$AttachmentObj->TransactionId() ."\n");
 +my $content = $AttachmentObj->OriginalContent;
 +my $content_type = $AttachmentObj->ContentType || 'text/plain';
  
- if ( RT->Config->Get('AlwaysDownloadAttachments') ) {
 -     }
++my $attachment_regex = qr{^(image/svg\+xml|application/pdf)}i;
++if ( RT->Config->Get('AlwaysDownloadAttachments') || ($content_type =~ $attachment_regex) ) {
 +    $r->headers_out->{'Content-Disposition'} = "attachment";
 +}
 +elsif ( !RT->Config->Get('TrustHTMLAttachments') ) {
-     $content_type = 'text/plain' if ( $content_type =~ /^text\/html/i );
++    my $text_plain_regex = qr{^(text/html|application/xhtml\+xml|text/xml|application/xml)}i;
++    $content_type = 'text/plain' if ( $content_type =~ $text_plain_regex );
 +}
 +elsif (lc $content_type eq 'text/html') {
 +    # If we're trusting and serving HTML for display not download, try to do
 +    # inline <img> rewriting to be extra helpful.
 +    my $count = RT::Interface::Web::RewriteInlineImages(
 +        Content     => \$content,
 +        Attachment  => $AttachmentObj,
 +    );
 +    RT->Logger->debug("Rewrote $count CID images when displaying original HTML attachment #$attach");
 +}
  
 -     my $content_type = $AttachmentObj->ContentType || 'text/plain';
 +my $enc  = $AttachmentObj->OriginalEncoding || 'utf-8';
 +my $iana = Encode::find_encoding($enc);
 +   $iana = $iana ? $iana->mime_name : $enc;
  
 -     my $attachment_regex = qr{^(image/svg\+xml|application/pdf)}i;
 -     if ( RT->Config->Get('AlwaysDownloadAttachments') || ($content_type =~ $attachment_regex) ) {
 -         $r->headers_out->{'Content-Disposition'} = "attachment";
 -     }
 -     elsif (!RT->Config->Get('TrustHTMLAttachments')) {
 -         my $text_plain_regex = qr{^(text/html|application/xhtml\+xml|text/xml|application/xml)}i;
 -         $content_type = 'text/plain' if ( $content_type =~ $text_plain_regex );
 -     }
 +require MIME::Types;
 +my $mimetype = MIME::Types->new->type($content_type);
 +unless ( $mimetype && $mimetype->isBinary ) {
 +    $content_type .= ";charset=$iana";
 +}
  
 -     my $enc = $AttachmentObj->OriginalEncoding || 'utf-8';
 -     my $iana = Encode::find_encoding( $enc );
 -     $iana = $iana? $iana->mime_name : $enc;
 -
 -     require MIME::Types;
 -     my $mimetype = MIME::Types->new->type($content_type);
 -     unless ( $mimetype && $mimetype->isBinary ) {
 -	    $content_type .= ";charset=$iana";
 -     }
 -
 -     $r->content_type( $content_type );
 -     $m->clear_buffer();
 -     $m->out($AttachmentObj->OriginalContent);
 -     $m->abort; 
 +$r->content_type($content_type);
 +$m->clear_buffer();
 +$m->out($content);
 +$m->abort;
  </%perl>
  <%attr>
  AutoFlush => 0

commit c4b0d35a0b058711fd42666a3ca03e52aedfb363
Author: Aaron Kondziela <aaron at bestpractical.com>
Date:   Tue Jan 24 18:20:28 2017 -0500

    Fix timing sidechannel vulnerability in password checking
    
    "eq" operators for comparing against passwords are replaced by a new
    RT::Util::constant_time_eq to resolve a timing sidechannel vulnerability.
    
    This addresses CVE-2017-5361.
    
    Fixes: T#161960

diff --git a/lib/RT/Authen/ExternalAuth/DBI.pm b/lib/RT/Authen/ExternalAuth/DBI.pm
index a7d3c10..2138fec 100644
--- a/lib/RT/Authen/ExternalAuth/DBI.pm
+++ b/lib/RT/Authen/ExternalAuth/DBI.pm
@@ -50,6 +50,7 @@ package RT::Authen::ExternalAuth::DBI;
 
 use DBI;
 use RT::Authen::ExternalAuth::DBI::Cookie;
+use RT::Util;
 
 use warnings;
 use strict;
@@ -81,6 +82,7 @@ Provides the database implementation for L<RT::Authen::ExternalAuth>.
             'p_field'                   =>  'password',
 
             # Example of custom hashed password check
+            # (See below for security concerns with this implementation)
             #'p_check'                   =>  sub {
             #    my ($hash_from_db, $password) = @_;
             #    return $hash_from_db eq function($password);
@@ -170,6 +172,17 @@ An example, where C<FooBar()> is some external hashing function:
 Importantly, the C<p_check> subroutine allows for arbitrarily complex password
 checking unlike C<p_enc_pkg> and C<p_enc_sub>.
 
+Please note, the use of the C<eq> operator in the C<p_check> example above
+introduces a timing sidechannel vulnerability. (It was left there for clarity
+of the example.) There is a comparison function available in RT that is
+hardened against timing attacks. The comparison from the above example could
+be re-written with it like this:
+
+    p_check => sub {
+        my ($hash_from_db, $password) = @_;
+        return RT::Util::constant_time_eq($hash_from_db, FooBar($password));
+    },
+
 =item p_enc_pkg, p_enc_sub
 
 The Perl package and subroutine used to encrypt passwords from the
@@ -298,7 +311,7 @@ sub GetAuth {
         # Jump to the next external authentication service if they don't match
         if(defined($db_p_salt)) {
             $RT::Logger->debug("Using salt:",$db_p_salt);
-            if(${encrypt}->($password,$db_p_salt) ne $pass_from_db){
+            unless (RT::Util::constant_time_eq(${encrypt}->($password,$db_p_salt), $pass_from_db)) {
                 $RT::Logger->info(  $service,
                                     "AUTH FAILED",
                                     $username,
@@ -306,7 +319,7 @@ sub GetAuth {
                 return 0;
             }
         } else {
-            if(${encrypt}->($password) ne $pass_from_db){
+            unless (RT::Util::constant_time_eq(${encrypt}->($password), $pass_from_db)) {
                 $RT::Logger->info(  $service,
                                     "AUTH FAILED",
                                     $username,
diff --git a/lib/RT/User.pm b/lib/RT/User.pm
index 30f9645..8cf787c 100644
--- a/lib/RT/User.pm
+++ b/lib/RT/User.pm
@@ -84,6 +84,7 @@ use RT::Principals;
 use RT::ACE;
 use RT::Interface::Email;
 use Text::Password::Pronounceable;
+use RT::Util;
 
 sub _OverlayAccessible {
     {
@@ -1099,11 +1100,17 @@ sub IsPassword {
         # If it's a new-style (>= RT 4.0) password, it starts with a '!'
         my (undef, $method, @rest) = split /!/, $stored;
         if ($method eq "bcrypt") {
-            return 0 unless $self->_GeneratePassword_bcrypt($value, @rest) eq $stored;
+            return 0 unless RT::Util::constant_time_eq(
+                $self->_GeneratePassword_bcrypt($value, @rest),
+                $stored
+            );
             # Upgrade to a larger number of rounds if necessary
             return 1 unless $rest[0] < RT->Config->Get('BcryptCost');
         } elsif ($method eq "sha512") {
-            return 0 unless $self->_GeneratePassword_sha512($value, @rest) eq $stored;
+            return 0 unless RT::Util::constant_time_eq(
+                $self->_GeneratePassword_sha512($value, @rest),
+                $stored
+            );
         } else {
             $RT::Logger->warn("Unknown hash method $method");
             return 0;
@@ -1113,16 +1120,28 @@ sub IsPassword {
         my $hash = MIME::Base64::decode_base64($stored);
         # Decoding yields 30 byes; first 4 are the salt, the rest are substr(SHA256,0,26)
         my $salt = substr($hash, 0, 4, "");
-        return 0 unless substr(Digest::SHA::sha256($salt . Digest::MD5::md5(Encode::encode( "UTF-8", $value))), 0, 26) eq $hash;
+        return 0 unless RT::Util::constant_time_eq(
+            substr(Digest::SHA::sha256($salt . Digest::MD5::md5(Encode::encode( "UTF-8", $value))), 0, 26),
+            $hash
+        );
     } elsif (length $stored == 32) {
         # Hex nonsalted-md5
-        return 0 unless Digest::MD5::md5_hex(Encode::encode( "UTF-8", $value)) eq $stored;
+        return 0 unless RT::Util::constant_time_eq(
+            Digest::MD5::md5_hex(Encode::encode( "UTF-8", $value)),
+            $stored
+        );
     } elsif (length $stored == 22) {
         # Base64 nonsalted-md5
-        return 0 unless Digest::MD5::md5_base64(Encode::encode( "UTF-8", $value)) eq $stored;
+        return 0 unless RT::Util::constant_time_eq(
+            Digest::MD5::md5_base64(Encode::encode( "UTF-8", $value)),
+            $stored
+        );
     } elsif (length $stored == 13) {
         # crypt() output
-        return 0 unless crypt(Encode::encode( "UTF-8", $value), $stored) eq $stored;
+        return 0 unless RT::Util::constant_time_eq(
+            crypt(Encode::encode( "UTF-8", $value), $stored),
+            $stored
+        );
     } else {
         $RT::Logger->warning("Unknown password form");
         return 0;
@@ -1218,19 +1237,20 @@ sub GenerateAuthString {
 
 =head3 ValidateAuthString
 
-Takes auth string and protected string. Returns true is protected string
+Takes auth string and protected string. Returns true if protected string
 has been protected by user's L</AuthToken>. See also L</GenerateAuthString>.
 
 =cut
 
 sub ValidateAuthString {
     my $self = shift;
-    my $auth_string = shift;
+    my $auth_string_to_validate = shift;
     my $protected = shift;
 
     my $str = Encode::encode( "UTF-8", $self->AuthToken . $protected );
+    my $valid_auth_string = substr(Digest::MD5::md5_hex($str),0,16);
 
-    return $auth_string eq substr(Digest::MD5::md5_hex($str),0,16);
+    return RT::Util::constant_time_eq( $auth_string_to_validate, $valid_auth_string );
 }
 
 =head2 SetDisabled
diff --git a/lib/RT/Util.pm b/lib/RT/Util.pm
index 58eb9fd..317a208 100644
--- a/lib/RT/Util.pm
+++ b/lib/RT/Util.pm
@@ -54,6 +54,8 @@ use warnings;
 use base 'Exporter';
 our @EXPORT = qw/safe_run_child mime_recommended_filename/;
 
+use Encode qw/encode/;
+
 sub safe_run_child (&) {
     my $our_pid = $$;
 
@@ -150,6 +152,58 @@ sub assert_bytes {
 }
 
 
+=head2 C<constant_time_eq($a, $b)>
+
+Compares two strings for equality in constant-time. Replacement for the C<eq>
+operator designed to avoid timing side-channel vulnerabilities. Returns zero
+or one.
+
+This is intended for use in cryptographic subsystems for comparing well-formed
+data such as hashes - not for direct use with user input or as a general
+replacement for the C<eq> operator.
+
+The two string arguments B<MUST> be of equal length. If the lengths differ,
+this function will call C<die()>, as proceeding with execution would create
+a timing vulnerability. Length is defined by characters, not bytes.
+
+This code has been tested to do what it claims. Do not change it without
+thorough statistical timing analysis to validate the changes.
+
+Added to resolve CVE-2017-5361
+
+For more on timing attacks, see this Wikipedia article:
+B<https://en.wikipedia.org/wiki/Timing_attack>
+
+=cut
+
+sub constant_time_eq {
+    my ($a, $b) = @_;
+
+    my $result = 0;
+
+    # generic error message avoids potential information leaks
+    my $generic_error = "Cannot compare values";
+    die $generic_error unless defined $a and defined $b;
+    die $generic_error unless length $a == length $b;
+    die $generic_error if ref($a) or ref($b);
+
+    for (my $i = 0; $i < length($a); $i++) {
+        my $a_char = substr($a, $i, 1);
+        my $b_char = substr($b, $i, 1);
+
+        # encode() is set to die on malformed
+        my @a_octets = unpack("C*", encode('UTF-8', $a_char, Encode::FB_CROAK));
+        my @b_octets = unpack("C*", encode('UTF-8', $b_char, Encode::FB_CROAK));
+        die $generic_error if (scalar @a_octets) != (scalar @b_octets);
+
+        for (my $j = 0; $j < scalar @a_octets; $j++) {
+            $result |= $a_octets[$j] ^ $b_octets[$j];
+        }
+    }
+    return 0 + not $result;
+}
+
+
 RT::Base->_ImportOverlays();
 
 1;

commit 85c8471fd873b3473adfbd5d1076efe793d20605
Merge: 274e11d c4b0d35
Author: Shawn M Moore <shawn at bestpractical.com>
Date:   Thu Jun 1 15:39:27 2017 +0000

    Merge branch 'security/4.4/password-timing-attack' into security/4.4.2-releng


commit db7ed96ba7b9a3c7722e10935ae7e88fa48e9cd7
Merge: 85c8471 3143a4c
Author: Shawn M Moore <shawn at bestpractical.com>
Date:   Thu Jun 1 15:39:35 2017 +0000

    Merge branch 'security/4.0/dash-subscription' into security/4.4.2-releng


commit 9880ea1f304f4b860de476220716600dc9af2e38
Merge: db7ed96 c98487e
Author: Shawn M Moore <shawn at bestpractical.com>
Date:   Thu Jun 1 15:39:42 2017 +0000

    Merge branch 'security/4.2/referrer-login' into security/4.4.2-releng


commit 3f9e376a334670e12b5570b9ded1f6f2d7d6c059
Merge: d487efa 9880ea1
Author: Shawn M Moore <shawn at bestpractical.com>
Date:   Thu Jun 15 13:48:24 2017 -0400

    Merge branch 'security/4.4.2-releng' into 4.4.2-releng


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


More information about the rt-commit mailing list