[Bps-public-commit] rt-authen-externalauth branch, master, updated. 0.25-5-g436255c

[Shawn Moore]

The branch, master has been updated
       via  436255c04b4881bb6d8eec9a57b8593033d863a9 (commit)
      from  94e6194437cf09777319918f514eab947698bcfc (commit)

Summary of changes:
 lib/RT/Authen/ExternalAuth.pm     | 34 ++++++++++++++++++++++++++++++++++
 lib/RT/Authen/ExternalAuth/DBI.pm | 16 ++++++++++++++--
 2 files changed, 48 insertions(+), 2 deletions(-)

- Log -----------------------------------------------------------------
commit 436255c04b4881bb6d8eec9a57b8593033d863a9
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.pm b/lib/RT/Authen/ExternalAuth.pm
index aed6922..fd58a26 100644
--- a/lib/RT/Authen/ExternalAuth.pm
+++ b/lib/RT/Authen/ExternalAuth.pm
@@ -294,6 +294,7 @@ This is free software, licensed under:
 
 use RT::Authen::ExternalAuth::LDAP;
 use RT::Authen::ExternalAuth::DBI;
+use Encode qw/encode/;
 
 use strict;
 
@@ -889,4 +890,37 @@ sub CanonicalizeUserInfo {
     };
 }
 
+=head2 C<constant_time_eq($a, $b)>
+
+Taken verbatim from RT 4.4's RT::Util.
+
+=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;
+}
+
 1;
diff --git a/lib/RT/Authen/ExternalAuth/DBI.pm b/lib/RT/Authen/ExternalAuth/DBI.pm
index 3fe6cda..6b11861 100644
--- a/lib/RT/Authen/ExternalAuth/DBI.pm
+++ b/lib/RT/Authen/ExternalAuth/DBI.pm
@@ -32,6 +32,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);
@@ -123,6 +124,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 this extension 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::Authen::ExternalAuth::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
@@ -250,7 +262,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::Authen::ExternalAuth::constant_time_eq(${encrypt}->($password,$db_p_salt), $pass_from_db)) {
                 $RT::Logger->info(  $service,
                                     "AUTH FAILED",
                                     $username,
@@ -258,7 +270,7 @@ sub GetAuth {
                 return 0;
             }
         } else {
-            if(${encrypt}->($password) ne $pass_from_db){
+            unless (RT::Authen::ExternalAuth::constant_time_eq(${encrypt}->($password), $pass_from_db)) {
                 $RT::Logger->info(  $service,
                                     "AUTH FAILED",
                                     $username,

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