From 94a2053cd05cabee2e4233ef33e1f116201d9368 Mon Sep 17 00:00:00 2001 From: Li Ma Date: Fri, 28 Feb 2014 18:54:35 -0800 Subject: [PATCH] Password trunction makes password insecure The trunc_password function attempts to correct and truncate password. It is not recommended to 'fix' invalid input and continue on processing and logging it. Instead, strict check is introduced to validate password. If a password exceeds the maximum length, an HTTP 403 Forbidden error is thrown. In order to keep compatibility, an option 'strict_password_check' is also introduced to let operator decide which method to use. DocImpact Change-Id: I560daa843b94a05412af59a059de5a98bad2925e Closes-Bug: #1175904 --- etc/keystone.conf.sample | 8 +++++++ keystone/common/config.py | 8 ++++++- keystone/common/utils.py | 20 +++++++++++------ keystone/exception.py | 8 +++++++ keystone/tests/test_utils.py | 42 ++++++++++++++++++++++++++++++++---- 5 files changed, 74 insertions(+), 12 deletions(-) diff --git a/etc/keystone.conf.sample b/etc/keystone.conf.sample index f4807357ec..bd83507c89 100644 --- a/etc/keystone.conf.sample +++ b/etc/keystone.conf.sample @@ -109,6 +109,14 @@ # policy.v3cloudsample as an example). (boolean value) #domain_id_immutable=true +# If set to true, strict password length checking is performed +# for password manipulation. +# If a password exceeds the maximum length, the operation will +# fail with 403 Forbidden Error. +# If set to false, passwords are automatically truncated to +# the maximum length. +# strict_password_check=false + # # Options defined in oslo.messaging diff --git a/keystone/common/config.py b/keystone/common/config.py index e936973f70..fde26c91b7 100644 --- a/keystone/common/config.py +++ b/keystone/common/config.py @@ -119,7 +119,13 @@ FILE_OPTIONS = { 'recommended if the scope of a domain admin is being ' 'restricted by use of an appropriate policy file ' '(see policy.v3cloudsample as an example).'), - ], + cfg.BoolOpt('strict_password_check', default=False, + help='If set to true, strict password length checking is ' + 'performed for password manipulation.' + 'If a password exceeds the maximum length, ' + 'the operation will fail with 403 Forbidden Error.' + 'If set to false, passwords are automatically ' + 'truncated to the maximum length.')], 'identity': [ cfg.StrOpt('default_domain_id', default='default', help='This references the domain to use for all ' diff --git a/keystone/common/utils.py b/keystone/common/utils.py index e5d10e88e7..7b0333fa51 100644 --- a/keystone/common/utils.py +++ b/keystone/common/utils.py @@ -86,14 +86,20 @@ class SmarterEncoder(json.JSONEncoder): return super(SmarterEncoder, self).default(obj) -def trunc_password(password): - """Truncate passwords to the max_length.""" +def verify_length_and_trunc_password(password): + """Verify and truncate the provided password to the max_password_length.""" max_length = CONF.identity.max_password_length try: if len(password) > max_length: - LOG.warning( - _('Truncating user password to %s characters.'), max_length) - return password[:max_length] + if CONF.strict_password_check: + raise exception.PasswordVerificationError(size=max_length) + else: + LOG.warning( + _('Truncating user password to ' + '%d characters.'), max_length) + return password[:max_length] + else: + return password except TypeError: raise exception.ValidationError(attribute='string', target='password') @@ -115,7 +121,7 @@ def hash_user_password(user): def hash_password(password): """Hash a password. Hard.""" - password_utf8 = trunc_password(password).encode('utf-8') + password_utf8 = verify_length_and_trunc_password(password).encode('utf-8') return passlib.hash.sha512_crypt.encrypt( password_utf8, rounds=CONF.crypt_strength) @@ -129,7 +135,7 @@ def check_password(password, hashed): """ if password is None or hashed is None: return False - password_utf8 = trunc_password(password).encode('utf-8') + password_utf8 = verify_length_and_trunc_password(password).encode('utf-8') return passlib.hash.sha512_crypt.verify(password_utf8, hashed) diff --git a/keystone/exception.py b/keystone/exception.py index 96aa96dd16..2a4ec51f77 100644 --- a/keystone/exception.py +++ b/keystone/exception.py @@ -109,6 +109,14 @@ class ValidationSizeError(Error): title = 'Bad Request' +class PasswordVerificationError(Error): + message_format = _("The password length must be less than or equal " + "to %(size)i. The server could not comply with the " + "request because the password is invalid.") + code = 403 + title = 'Forbidden' + + class PKITokenExpected(Error): message_format = _('The certificates you requested are not available. ' 'It is likely that this server does not use PKI tokens ' diff --git a/keystone/tests/test_utils.py b/keystone/tests/test_utils.py index c9867eb352..7267131ad8 100644 --- a/keystone/tests/test_utils.py +++ b/keystone/tests/test_utils.py @@ -31,11 +31,15 @@ import datetime import functools import os import time +import uuid from keystone.common import utils +from keystone import config +from keystone import exception from keystone import service from keystone import tests +CONF = config.CONF TZ = None @@ -70,10 +74,40 @@ class UtilsTestCase(tests.TestCase): self.assertTrue(utils.check_password(password, hashed)) self.assertFalse(utils.check_password(wrong, hashed)) - def test_hash_long_password(self): - bigboy = '0' * 9999999 - hashed = utils.hash_password(bigboy) - self.assertTrue(utils.check_password(bigboy, hashed)) + def test_verify_normal_password_strict(self): + self.config_fixture.config(strict_password_check=False) + normal_password = uuid.uuid4().hex + verified = utils.verify_length_and_trunc_password(normal_password) + self.assertEqual(normal_password, verified) + + def test_verify_long_password_strict(self): + self.config_fixture.config(strict_password_check=False) + self.config_fixture.config(group='identity', max_password_length=5) + max_length = CONF.identity.max_password_length + invalid_password = 'passw0rd' + truncated = utils.verify_length_and_trunc_password(invalid_password) + self.assertEqual(invalid_password[:max_length], truncated) + + def test_verify_long_password_strict_raises_exception(self): + self.config_fixture.config(strict_password_check=True) + self.config_fixture.config(group='identity', max_password_length=5) + invalid_password = 'passw0rd' + self.assertRaises(exception.PasswordVerificationError, + utils.verify_length_and_trunc_password, + invalid_password) + + def test_hash_long_password_truncation(self): + self.config_fixture.config(strict_password_check=False) + invalid_length_password = '0' * 9999999 + hashed = utils.hash_password(invalid_length_password) + self.assertTrue(utils.check_password(invalid_length_password, hashed)) + + def test_hash_long_password_strict(self): + self.config_fixture.config(strict_password_check=True) + invalid_length_password = '0' * 9999999 + self.assertRaises(exception.PasswordVerificationError, + utils.hash_password, + invalid_length_password) def _create_test_user(self, password=OPTIONAL): user = {"name": "hthtest"}