keystone password creation and verification mismatch

Bug #1279849 reported by Abu Shohel Ahmed
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Fix Released
High
David Stanek

Bug Description

In keystone stable/Havana release, password for a user can be set during user creation process. Now if a user initially do a sha512 hash of his password and sent it to the keystone server over the wire, then hash_password method of keystone/common/utils.py

def hash_password(password):
   ~ | """Hash a password. Hard."""
   ~ | password_utf8 = trunc_password(password).encode('utf-8')
   ~ | if passlib.hash.sha512_crypt.identify(password_utf8):
   ~ | return password_utf8
   ~ | h = passlib.hash.sha512_crypt.encrypt(password_utf8,
   ~ | rounds=CONF.crypt_strength)
   ~ | return h

will not do any hashing, or it directly store the password in DB.

However, during authentication, the user needs to provide the clear text password for authentication because during authentication it always does sha512 over the password field (it does not check the password is already hashed)

def check_password(password, hashed):
  ""Check that a plaintext password matches hashed.
    hashpw returns the salt value concatenated with the actual hash value.
   It extracts the actual salt if this value is then passed as the salt.

   """
         if password is None or hashed is None:
                  return False
         password_utf8 = trunc_password(password).encode('utf-8')
         return passlib.hash.sha512_crypt.verify(password_utf8, hashed)

Now in this case,
1) user chooses a password which is similar to a sha512 output, now keystone thinks it is already hashed, so it will store it as it is. User provides the sha512 during authentication but he cannot login this time cause now the password is hashed before matching.

There should be consistency for both password creation and verification process.

Tags: security
Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote :

As this is not a vulnerability but a bug with some security implication, there is no need to keep this private.

information type: Private Security → Public Security
Thierry Carrez (ttx)
information type: Public Security → Public
tags: added: security
Revision history for this message
Dolph Mathews (dolph) wrote :

Well, it does present a vulnerability in that keystone is "tricked" into storing passwords in plaintext, however users have to take special action to expose themselves (odds are, I'm guessing, knowingly).

Changed in keystone:
importance: Undecided → High
milestone: none → icehouse-3
status: New → Triaged
David Stanek (dstanek)
Changed in keystone:
assignee: nobody → David Stanek (dstanek)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (master)

Fix proposed to branch: master
Review: https://review.openstack.org/74801

Changed in keystone:
status: Triaged → In Progress
Revision history for this message
David Stanek (dstanek) wrote :

In my proposed fix I took the stance that we should always be treating user input as untrusted. Just hash it and move on.

The code that checked if the password was already hash seemed to have been written so that a migration process could use the same API code to move users around.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (master)

Reviewed: https://review.openstack.org/74801
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=e492bbc68ef41b276a0a18c6dbeda242d46b66f4
Submitter: Jenkins
Branch: master

commit e492bbc68ef41b276a0a18c6dbeda242d46b66f4
Author: David Stanek <email address hidden>
Date: Wed Feb 19 18:26:58 2014 +0000

    Always hash passwords on their way into the DB

    Way back in ed793ad5 a feature was added to detect if a password was
    already hashed. If it looked like it was already hashed the password
    was put into the database as is. It appears that this was added to
    support a migration that was using the identity API to store users
    instead of direct database inserts.

    Change-Id: I37b575e4db2fa7090c5171b8bc32cf2c57ad6e03
    Fixes-bug: #1279849

Changed in keystone:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in keystone:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in keystone:
milestone: icehouse-3 → 2014.1
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.