Hi,
I noticed today what I belive to be a deficiency in RT’s password
handling that I feel should be looked into. The following code is
from RT/User_Overlay.pm in 3.6.0:
sub _GeneratePassword {
my $self = shift;
my $password = shift;
my $md5 = Digest::MD5->new();
$md5->add($password);
return ($md5->hexdigest);
}
[…]
sub IsPassword {
my $self = shift;
my $value = shift;
[..]
# if it's a historical password we say ok.
if ($self->__Value('Password') eq crypt($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);
}
Unless there’s magic here that I can’t spot, this means the current
password regime is storing passwords without salts. As a consequence,
identical passwords will always have identical hash values. Given
this, finding users with identical passwords is a no-brainer.
Dictionary attacks are also easer to mount, since the required number
of computations (and the amount of space required to store a
pre-compiled dictionary) is much lower when there’s a 1-to-1
relationship between cleartext password and hash.
Granted, you need to be able read the password hashes from the
database to effectively exploit this. Still, in any case it seems
that the “upgrade the legacy password” code path actually reduces the
security of the system. As things stand it could probably just as
well be removed.
Arne.
Granted, you need to be able read the password hashes from the
database to effectively exploit this.
A patch would be much appreciated.
Still, in any case it seems
that the “upgrade the legacy password” code path actually reduces the
security of the system. As things stand it could probably just as
well be removed.
Depends who you ask. A number of sites are using RT as an
authentication source for other services and rely on the fact that
password storage is or becomes MD5.
Best,
Jesse
PGP.sig (186 Bytes)
Jesse Vincent wrote:
A patch would be much appreciated.
Appended. Put together rather quickly, but I believe it to be sound.
Obviously should be reviewed carefully anyway.
Depends who you ask. A number of sites are using RT as an authentication
source for other services and rely on the fact that password storage is
or becomes MD5.
Hm, and they rely on this non-salted md5-format to be used? I that case
they’ll be out of luck if we update the code to use salted md5 as well,
since the format is different.
Arne.
md5-salt.patch (1.73 KB)
Jesse Vincent wrote:
A patch would be much appreciated.
Appended. Put together rather quickly, but I believe it to be sound.
Obviously should be reviewed carefully anyway.
Depends who you ask. A number of sites are using RT as an authentication
source for other services and rely on the fact that password storage is
or becomes MD5.
Hm, and they rely on this non-salted md5-format to be used? I that case
they’ll be out of luck if we update the code to use salted md5 as well,
since the format is different.
Having asked around, I’m told that changing this would break PAM
compatibility, which scares me more than a little.
-j
Jesse Vincent wrote:
Having asked around, I’m told that changing this would break PAM
compatibility, which scares me more than a little.
Surprising. Normal “md5-crypt” on every Unix I’m aware of is salted
md5, and this is handled by pam out of the box. Not knowing the details
of RT’s “PAM compatibility” it’s hard to comment either way, though.
Could we get some more details on this perceived breakage?
Arne.
Jesse Vincent wrote:
Having asked around, I’m told that changing this would break PAM
compatibility, which scares me more than a little.
Surprising. Normal “md5-crypt” on every Unix I’m aware of is salted
md5, and this is handled by pam out of the box. Not knowing the details
of RT’s “PAM compatibility” it’s hard to comment either way, though.
Could we get some more details on this perceived breakage?
I’ll poke harder at my informant and see if I can shake loose any sane
justification.
Thanks!
Jesse
I think we should allow admins(if it possible) to:
- choose between MD5 and MD5 salted
- script that changes stored hashes at once. Is it possible to add
salt to MD5 hash with password string?
It was big surprise to when I saw that we’d switched from md5 in
base64 to md5 in hex. And also it’s very hard to auth against RT’s DB
when some password hash strings are base64 encoded while other are
hex-encoded.On 9/4/06, Jesse Vincent jesse@bestpractical.com wrote:
On Mon, Sep 04, 2006 at 09:13:03PM +0200, Arne Georg Gleditsh wrote:
Jesse Vincent wrote:
Having asked around, I’m told that changing this would break PAM
compatibility, which scares me more than a little.
Surprising. Normal “md5-crypt” on every Unix I’m aware of is salted
md5, and this is handled by pam out of the box. Not knowing the details
of RT’s “PAM compatibility” it’s hard to comment either way, though.
Could we get some more details on this perceived breakage?
I’ll poke harder at my informant and see if I can shake loose any sane
justification.
Thanks!
Jesse
–
Arne.
Best regards, Ruslan.
Ruslan Zakirov wrote:
I think we should allow admins(if it possible) to:
- choose between MD5 and MD5 salted
- script that changes stored hashes at once. Is it possible to add
salt to MD5 hash with password string?
Not that I am aware of.
It was big surprise to when I saw that we’d switched from md5 in
base64 to md5 in hex. And also it’s very hard to auth against RT’s DB
when some password hash strings are base64 encoded while other are
hex-encoded.
This I can see. Instead of overwriting the original password in the db,
perhaps a new column should be added where updated passwords are written
when users log in. Existence of non-null entries in this column would
then dictate that this be used for authentication instead of the old
column. External entities authenticating towards RT’s database could
then switch to using the new format when the administrator decides
everyone has had sufficient time to log in to RT and so refresh their
accounts.
This also has issues, obviously. Do password updates change both old
and new columns? Given that the old-format password store is a security
liability, when do you remove it? How should the application logic
detect that it is removed and stop updating it?
Overall, I’m not sure this is not at least as painful as just migrating
the way it is done today, switching any external entities to use the new
format immediately and then telling the users to refresh their accounts
by logging in to RT before attempting to access services provided by the
mentioned external entities. Mileage will no doubt vary.
(The password scheme implemented by the proposed patch is general and
widely deployed, so the need for further migrations should not arise in
a good while. That is, until someone decides we need to move to SSHA1
or somesuch. 
Arne.