[Rt-commit] rt branch, 3.8-trunk, updated. rt-3.8.8-247-g390a444

Kevin Falcone falcone at bestpractical.com
Wed Jan 19 19:24:19 EST 2011


The branch, 3.8-trunk has been updated
       via  390a444165c03cbfb7974fcbb8315b3654ca9029 (commit)
       via  a620b8bb60eee12c0fe6c988417b7090eaabfbd8 (commit)
       via  63abc24227975ec7f31ff103355bb874d7e805b0 (commit)
       via  b4073b3172b0a3165ef5ca07f6aa0a2918a2edfd (commit)
       via  5d5c2218b8ce4d6053009113fa29fe3af60f1475 (commit)
       via  bbe970f1ef6299eadc5827c182f83aa744809ec9 (commit)
       via  33739de717f4bfd11d0e2067d7f75c6641d95498 (commit)
      from  404c473d367ebb6219cd4cc2da914b8c42d01a2f (commit)

Summary of changes:
 .gitignore                          |    1 +
 UPGRADING                           |   10 ++++
 configure.ac                        |    1 +
 etc/upgrade/vulnerable-passwords.in |   93 +++++++++++++++++++++++++++++++++++
 lib/RT/User_Overlay.pm              |   64 +++++++++++++++---------
 sbin/rt-test-dependencies.in        |    1 +
 t/api/password-types.t              |   31 ++++++++++++
 7 files changed, 178 insertions(+), 23 deletions(-)
 create mode 100755 etc/upgrade/vulnerable-passwords.in
 create mode 100644 t/api/password-types.t

- Log -----------------------------------------------------------------
commit 33739de717f4bfd11d0e2067d7f75c6641d95498
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Dec 15 10:18:49 2010 -0500

    Move to a SHA-256 based password authentication scheme
    
    This continues to support all three previous password storage formats
    that RT had, while changing the default format to a more secure salted
    SHA-256 hash.  Most importly, the previous MD5-based implementation
    failed to use a salt, making rainbow table attacks on leaked RT
    databases nearly trivial.
    
    Though the Password column is only 40 characters wide, pack 4 bytes of
    salt and 26 bytes of SHA-512 into the column.  To enable scripted
    upgrades from unsalted MD5 passwords, we store:
    
        salt . (first 26 bytes of SHA-256( salt . binary-MD5( password ) ) )

diff --git a/lib/RT/User_Overlay.pm b/lib/RT/User_Overlay.pm
index d64ba54..6af1117 100755
--- a/lib/RT/User_Overlay.pm
+++ b/lib/RT/User_Overlay.pm
@@ -69,6 +69,7 @@ package RT::User;
 use strict;
 no warnings qw(redefine);
 
+use Digest::SHA;
 use Digest::MD5;
 use RT::Principals;
 use RT::ACE;
@@ -988,20 +989,28 @@ sub SetPassword {
 
 }
 
-=head3 _GeneratePassword PASSWORD
+=head3 _GeneratePassword PASSWORD [, SALT]
 
-returns an MD5 hash of the password passed in, in hexadecimal encoding.
+Returns a salted SHA-256 hash of the password passed in, in base64
+encoding.
 
 =cut
 
 sub _GeneratePassword {
     my $self = shift;
-    my $password = shift;
-
-    my $md5 = Digest::MD5->new();
-    $md5->add(encode_utf8($password));
-    return ($md5->hexdigest);
-
+    my ($password, $salt) = @_;
+
+    # Generate a random 4-byte salt
+    $salt ||= pack("C4",map{int rand(256)} 1..4);
+
+    # Encode the salt, and a truncated SHA256 of the MD5 of the
+    # password The additional, un-necessary level of MD5 allows for
+    # transparent upgrading to this scheme, from the previous unsalted
+    # MD5 one.
+    return MIME::Base64::encode_base64(
+        $salt
+      . substr(Digest::SHA::sha256($salt . Digest::MD5::md5($password)),0,26)
+    );
 }
 
 =head3 _GeneratePasswordBase64 PASSWORD
@@ -1064,23 +1073,32 @@ sub IsPassword {
         return(undef);
      }
 
-    # generate an md5 password 
-    if ($self->_GeneratePassword($value) eq $self->__Value('Password')) {
-        return(1);
-    }
-
-    #  if it's a historical password we say ok.
-    if ($self->__Value('Password') eq crypt(encode_utf8($value), $self->__Value('Password'))
-        or $self->_GeneratePasswordBase64($value) eq $self->__Value('Password'))
-    {
-        # ...but upgrade the legacy password inplace.
-        $self->SUPER::SetPassword( $self->_GeneratePassword($value) );
-        return(1);
+    my $stored = $self->__Value('Password');
+    if (length $stored == 40) {
+        # The truncated SHA256(salt,MD5(passwd)) form from 2010/12 is 40 characters long
+        my $hash = MIME::Base64::decode_base64($stored);
+        # The first 4 bytes are the salt, the rest is substr(SHA256,0,26)
+        my $salt = substr($hash, 0, 4, "");
+        return 0 unless substr(Digest::SHA::sha256($salt . Digest::MD5::md5($value)), 0, 26) eq $hash;
+    } elsif (length $stored == 32) {
+        # Hex nonsalted-md5
+        return 0 unless Digest::MD5::md5_hex(encode_utf8($value)) eq $stored;
+    } elsif (length $stored == 22) {
+        # Base64 nonsalted-md5
+        return 0 unless Digest::MD5::md5_base64(encode_utf8($value)) eq $stored;
+    } elsif (length $stored == 13) {
+        # crypt() output
+        return 0 unless crypt(encode_utf8($value), $stored) eq $stored;
+    } else {
+        $RT::Logger->warn("Unknown password form");
+        return 0;
     }
 
-    # no password check has succeeded. get out
-
-    return (undef);
+    # We got here by validating successfully, but with a legacy
+    # password form.  Update to the most recent form.
+    my $obj = $self->isa("RT::CurrentUser") ? $self->UserObj : $self;
+    $obj->_Set(Field => 'Password', Value =>  $self->_GeneratePassword($value) );
+    return 1;
 }
 
 sub CurrentUserRequireToSetPassword {
diff --git a/sbin/rt-test-dependencies.in b/sbin/rt-test-dependencies.in
index aaebb0d..a8513fb 100755
--- a/sbin/rt-test-dependencies.in
+++ b/sbin/rt-test-dependencies.in
@@ -208,6 +208,7 @@ sub text_to_hash {
 $deps{'CORE'} = [ text_to_hash( << '.') ];
 Digest::base
 Digest::MD5 2.27
+Digest::SHA
 DBI 1.37
 Class::ReturnValue 0.40
 DBIx::SearchBuilder 1.54

commit bbe970f1ef6299eadc5827c182f83aa744809ec9
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Dec 15 10:37:40 2010 -0500

    Provide a tool to upgrade weak hashes to the new 40-char SHA-256 form

diff --git a/UPGRADING b/UPGRADING
index 8f18073..78099f0 100644
--- a/UPGRADING
+++ b/UPGRADING
@@ -20,6 +20,16 @@ well.
 *******
 UPGRADING FROM 3.8.8 and earlier - Changes:
 
+Previous versions of RT used a password hashing scheme which was too
+easy to reverse, which could allow attackers with read access to the
+RT database to possibly compromise users' passwords.  Even if RT does
+no password authentication itself, it may still store these weak
+password hashes -- using ExternalAuth does not guarantee that you are
+not vulnerable!  To upgrade stored passwords to a stronger hash, run:
+
+    perl etc/upgrade/vulnerable-passwords
+
+
 We've proved that it's possible to delete set of records
 from Transactions table without losing functionality. To delete
 record run the following script:
diff --git a/configure.ac b/configure.ac
index a108d56..5104d44 100755
--- a/configure.ac
+++ b/configure.ac
@@ -398,6 +398,7 @@ AC_CONFIG_FILES([
                  etc/upgrade/3.8-ical-extension
                  etc/upgrade/split-out-cf-categories
                  etc/upgrade/generate-rtaddressregexp
+                 etc/upgrade/vulnerable-passwords
                  sbin/rt-attributes-viewer
                  sbin/rt-dump-database
                  sbin/rt-setup-database
diff --git a/etc/upgrade/vulnerable-passwords.in b/etc/upgrade/vulnerable-passwords.in
new file mode 100755
index 0000000..a12b80e
--- /dev/null
+++ b/etc/upgrade/vulnerable-passwords.in
@@ -0,0 +1,93 @@
+#!@PERL@
+
+use strict;
+use warnings;
+
+use lib "@LOCAL_LIB_PATH@";
+use lib "@RT_LIB_PATH@";
+
+use RT;
+RT::LoadConfig;
+RT::Init;
+
+$| = 1;
+
+use Getopt::Long;
+use Digest::SHA;
+my $fix;
+GetOptions("fix!" => \$fix);
+
+use RT::Users;
+my $users = RT::Users->new( $RT::SystemUser );
+$users->Limit(
+    FIELD => 'Password',
+    OPERATOR => 'IS NOT',
+    VALUE => 'NULL',
+    ENTRYAGGREGATOR => 'AND',
+);
+$users->Limit(
+    FIELD => 'Password',
+    OPERATOR => '!=',
+    VALUE => '*NO-PASSWORD*',
+    ENTRYAGGREGATOR => 'AND',
+);
+$users->Limit(
+    FIELD => 'Password',
+    OPERATOR => 'NOT STARTSWITH',
+    VALUE => '!',
+    ENTRYAGGREGATOR => 'AND',
+);
+push @{$users->{'restrictions'}{ "main.Password" }}, "AND", {
+    field => 'LENGTH(main.Password)',
+    op => '<',
+    value => '40',
+};
+
+my $count = $users->Count;
+if ($count == 0) {
+    print "No users with unsalted or weak cryptography found.\n";
+    exit 0;
+}
+
+if ($fix) {
+    print "Upgrading $count users...\n";
+    while (my $u = $users->Next) {
+        my $stored = $u->__Value("Password");
+        my $raw;
+        if (length $stored == 32) {
+            $raw = pack("H*",$stored);
+        } elsif (length $stored == 22) {
+            $raw = MIME::Base64::decode_base64($stored);
+        } elsif (length $stored == 13) {
+            printf "%20s => Old crypt() format, cannot upgrade\n", $u->Name;
+        } else {
+            printf "%20s => Unknown password format!\n", $u->Name;
+        }
+        next unless $raw;
+
+        my $salt = pack("C4",map{int rand(256)} 1..4);
+        my $sha = Digest::SHA::sha256(
+            $salt . $raw
+        );
+        $u->_Set(
+            Field => "Password",
+            Value => MIME::Base64::encode_base64(
+                $salt . substr($sha,0,26)),
+        );
+    }
+    print "Done.\n";
+    exit 0;
+} else {
+    if ($count < 20) {
+        print "$count users found with unsalted or weak-cryptography passwords:\n";
+        print "      Id | Name\n", "-"x9, "+", "-"x9, "\n";
+        while (my $u = $users->Next) {
+            printf "%8d | %s\n", $u->Id, $u->Name;
+        }
+    } else {
+        print "$count users found with unsalted or weak-cryptography passwords\n";
+    }
+
+    print "\n", "Run again with --fix to upgrade.\n";
+    exit 1;
+}

commit 5d5c2218b8ce4d6053009113fa29fe3af60f1475
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Dec 15 18:09:39 2010 -0500

    Ignore the new generated file

diff --git a/.gitignore b/.gitignore
index adae6e8..ca6e4e9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -12,6 +12,7 @@ etc/upgrade/3.8-branded-queues-extension
 etc/upgrade/3.8-ical-extension
 etc/upgrade/split-out-cf-categories
 etc/upgrade/generate-rtaddressregexp
+etc/upgrade/vulnerable-passwords
 lib/RT.pm
 Makefile
 t/data/gnupg/keyrings/random_seed

commit b4073b3172b0a3165ef5ca07f6aa0a2918a2edfd
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Jan 3 21:31:56 2011 -0500

    Ensure that we remove the trailing newline from base64
    
    Otherwise, this generates 41-character-long strings, which
    DBIx::SearchBuilder has to truncate to 40 characters for us.

diff --git a/etc/upgrade/vulnerable-passwords.in b/etc/upgrade/vulnerable-passwords.in
index a12b80e..c28d2b8 100755
--- a/etc/upgrade/vulnerable-passwords.in
+++ b/etc/upgrade/vulnerable-passwords.in
@@ -72,7 +72,7 @@ if ($fix) {
         $u->_Set(
             Field => "Password",
             Value => MIME::Base64::encode_base64(
-                $salt . substr($sha,0,26)),
+                $salt . substr($sha,0,26), ""),
         );
     }
     print "Done.\n";
diff --git a/lib/RT/User_Overlay.pm b/lib/RT/User_Overlay.pm
index 6af1117..b24fe7a 100755
--- a/lib/RT/User_Overlay.pm
+++ b/lib/RT/User_Overlay.pm
@@ -1004,12 +1004,12 @@ sub _GeneratePassword {
     $salt ||= pack("C4",map{int rand(256)} 1..4);
 
     # Encode the salt, and a truncated SHA256 of the MD5 of the
-    # password The additional, un-necessary level of MD5 allows for
+    # password.  The additional, un-necessary level of MD5 allows for
     # transparent upgrading to this scheme, from the previous unsalted
     # MD5 one.
     return MIME::Base64::encode_base64(
-        $salt
-      . substr(Digest::SHA::sha256($salt . Digest::MD5::md5($password)),0,26)
+        $salt . substr(Digest::SHA::sha256($salt . Digest::MD5::md5($password)),0,26),
+        "" # No newline
     );
 }
 

commit 63abc24227975ec7f31ff103355bb874d7e805b0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Jan 3 21:38:28 2011 -0500

    Always return from the 40-char case, so we don't keep changing the password
    
    Without this, every successful login will store the password with a new salt.

diff --git a/lib/RT/User_Overlay.pm b/lib/RT/User_Overlay.pm
index b24fe7a..a96cbd9 100755
--- a/lib/RT/User_Overlay.pm
+++ b/lib/RT/User_Overlay.pm
@@ -1079,7 +1079,7 @@ sub IsPassword {
         my $hash = MIME::Base64::decode_base64($stored);
         # The first 4 bytes are the salt, the rest is substr(SHA256,0,26)
         my $salt = substr($hash, 0, 4, "");
-        return 0 unless substr(Digest::SHA::sha256($salt . Digest::MD5::md5($value)), 0, 26) eq $hash;
+        return substr(Digest::SHA::sha256($salt . Digest::MD5::md5($value)), 0, 26) eq $hash;
     } elsif (length $stored == 32) {
         # Hex nonsalted-md5
         return 0 unless Digest::MD5::md5_hex(encode_utf8($value)) eq $stored;

commit a620b8bb60eee12c0fe6c988417b7090eaabfbd8
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Jan 3 21:28:51 2011 -0500

    Add tests for the various password forms, and automatic upgrading

diff --git a/t/api/password-types.t b/t/api/password-types.t
new file mode 100644
index 0000000..267a6ed
--- /dev/null
+++ b/t/api/password-types.t
@@ -0,0 +1,31 @@
+#!/usr/bin/perl -w
+use strict;
+use warnings;
+
+use RT::Test;
+use Digest::MD5;
+
+my $root = RT::User->new(RT->SystemUser);
+$root->Load("root");
+
+# Salted truncated SHA-256
+my $old = $root->__Value("Password");
+is(length($old), 40, "Stored as truncated salted SHA-256");
+ok($root->IsPassword("password"));
+is($root->__Value("Password"), $old, "Unchanged after password check");
+
+# Crypt
+$root->_Set( Field => "Password", Value => crypt("something", "salt"));
+ok($root->IsPassword("something"), "crypt()ed password works");
+is(length($root->__Value("Password")), 40, "And is now upgraded to truncated salted SHA-256");
+
+# MD5, hex
+$root->_Set( Field => "Password", Value => Digest::MD5::md5_hex("changed"));
+ok($root->IsPassword("changed"), "Unsalted MD5 hex works");
+is(length($root->__Value("Password")), 40, "And is now upgraded to truncated salted SHA-256");
+
+# MD5, base64
+$root->_Set( Field => "Password", Value => Digest::MD5::md5_base64("new"));
+ok($root->IsPassword("new"), "Unsalted MD5 base64 works");
+is(length($root->__Value("Password")), 40, "And is now upgraded to truncated salted SHA-256");
+

commit 390a444165c03cbfb7974fcbb8315b3654ca9029
Merge: 404c473 a620b8b
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Wed Jan 19 16:55:49 2011 -0500

    Merge branch '3.8/salted-passwords' into 3.8-trunk


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


More information about the Rt-commit mailing list