bugfix (issue 25):

* bcrypt salts are now generated with padding bits set to 0
* bcrypt hashes with padding bits not set to 0 still same as before,
  but warning is issued
* bcrypt.normhash() can be used to correct existing bcrypt hashes.

TODO: documentation, verify against issue, look into verify_and_update() being able to take care of this.

3 files changed, 233 insertions, 31 deletions

diff --git a/passlib/handlers/bcrypt.py b/passlib/handlers/bcrypt.py
index 3b3d68f..28a4eea 100644
--- a/passlib/handlers/bcrypt.py
+++ b/passlib/handlers/bcrypt.py
@@ -25,7 +25,8 @@ try:
 except ImportError: #pragma: no cover - though should run whole suite w/o bcryptor installed
     bcryptor_engine = None
 #libs
-from passlib.utils import safe_os_crypt, classproperty, handlers as uh, h64, to_hash_str
+from passlib.utils import safe_os_crypt, classproperty, handlers as uh, \
+    h64, to_hash_str, rng, getrandstr
 
 #pkg
 #local
@@ -33,6 +34,14 @@ __all__ = [
     "bcrypt",
 ]
 
+# base64 character->value mapping used by bcrypt.
+# this is same as as H64_CHARS, but the positions are different.
+BCHARS = "./ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"
+
+# last bcrypt salt char should have 4 padding bits set to 0.
+# thus, only the following chars are allowed:
+BCLAST = ".Oeu"
+
 #=========================================================
 #handler
 #=========================================================
@@ -77,7 +86,7 @@ class bcrypt(uh.HasManyIdents, uh.HasRounds, uh.HasSalt, uh.HasManyBackends, uh.
     name = "bcrypt"
     setting_kwds = ("salt", "rounds", "ident")
     checksum_size = 31
-    checksum_chars = uh.H64_CHARS
+    checksum_chars = BCHARS
 
     #--HasManyIdents--
     default_ident = u"$2a$"
@@ -86,7 +95,8 @@ class bcrypt(uh.HasManyIdents, uh.HasRounds, uh.HasSalt, uh.HasManyBackends, uh.
 
     #--HasSalt--
     min_salt_size = max_salt_size = 22
-    salt_chars = uh.H64_CHARS
+    salt_chars = BCHARS
+    #NOTE: 22nd salt char must be in BCLAST, not full BCHARS
 
     #--HasRounds--
     default_rounds = 12 #current passlib default
@@ -127,6 +137,38 @@ class bcrypt(uh.HasManyIdents, uh.HasRounds, uh.HasSalt, uh.HasManyBackends, uh.
         return to_hash_str(hash) if native else hash
 
     #=========================================================
+    # specialized salt generation - fixes passlib issue 25
+    #=========================================================
+
+    @classmethod
+    def normhash(cls, hash):
+        "helper to normalize hash, correcting any bcrypt padding bits"
+        if cls.identify(hash):
+            return cls.from_string(hash).to_string()
+        else:
+            return hash
+
+    @classmethod
+    def generate_salt(cls, salt_size=None, strict=False):
+        assert cls.min_salt_size == cls.max_salt_size == cls.default_salt_size == 22
+        if salt_size is not None and salt_size != 22:
+            raise ValueError("bcrypt salts must be 22 characters in length")
+        return getrandstr(rng, BCHARS, 21) + getrandstr(rng, BCLAST, 1)
+
+    @classmethod
+    def norm_salt(cls, *args, **kwds):
+        salt = super(bcrypt, cls).norm_salt(*args, **kwds)
+        if salt and salt[-1] not in BCLAST:
+            salt = salt[:-1] + BCHARS[BCHARS.index(salt[-1]) & ~15]
+            assert salt[-1] in BCLAST
+            warn(
+                "encountered a bcrypt hash with incorrectly set padding bits; "
+                "you may want to run bcrypt.normhash() "
+                "to fix these hashes (see Passlib issue 25)."
+                )
+        return salt
+
+    #=========================================================
     #primary interface
     #=========================================================
     backends = ("pybcrypt", "bcryptor", "os_crypt")
diff --git a/passlib/tests/test_drivers.py b/passlib/tests/test_drivers.py
index 3a1d9eb..5dcfbb7 100644
--- a/passlib/tests/test_drivers.py
+++ b/passlib/tests/test_drivers.py
@@ -11,7 +11,7 @@ import warnings
 #pkg
 from passlib import hash
 from passlib.tests.utils import TestCase, HandlerCase, create_backend_case, \
-        enable_option, b
+        enable_option, b, catch_warnings
 #module
 
 
@@ -44,12 +44,10 @@ class AprMd5CryptTest(HandlerCase):
 #=========================================================
 #bcrypt
 #=========================================================
-from passlib.handlers.bcrypt import bcrypt
-
 class _BCryptTest(HandlerCase):
     "base for BCrypt test cases"
 
-    handler = bcrypt
+    handler = hash.bcrypt
     secret_chars = 72
 
     known_correct_hashes = [
@@ -57,8 +55,10 @@ class _BCryptTest(HandlerCase):
         ('', '$2a$06$DCq7YPn5Rq63x1Lad4cll.TV4S6ytwfsfvkgY8jIucDrjc8deX1s.'),
         ('a', '$2a$10$k87L/MF28Q673VKh8/cPi.SUl7MU/rWuSiIDDFayrKk/1tBsSQu4u'),
         ('abc', '$2a$10$WvvTPHKwdBJ3uk0Z37EMR.hLA2W6N9AEBhEgrAOljy2Ae5MtaSIUi'),
-        ('abcdefghijklmnopqrstuvwxyz', '$2a$10$fVH8e28OQRj9tqiDXs1e1uxpsjN0c7II7YPKXua2NAKYvM6iQk7dq'),
-        ('~!@#$%^&*()      ~!@#$%^&*()PNBFRD', '$2a$10$LgfYWkbzEvQ4JakH7rOvHe0y8pHKF9OaFgwUZ2q7W2FFZmZzJYlfS'),
+        ('abcdefghijklmnopqrstuvwxyz',
+                '$2a$10$fVH8e28OQRj9tqiDXs1e1uxpsjN0c7II7YPKXua2NAKYvM6iQk7dq'),
+        ('~!@#$%^&*()      ~!@#$%^&*()PNBFRD',
+                '$2a$10$LgfYWkbzEvQ4JakH7rOvHe0y8pHKF9OaFgwUZ2q7W2FFZmZzJYlfS'),
         ]
 
     known_unidentified_hashes = [
@@ -69,43 +69,164 @@ class _BCryptTest(HandlerCase):
     known_malformed_hashes = [
         #bad char in otherwise correct hash
         "$2a$12$EXRkfkdmXn!gzds2SSitu.MW9.gAVqa9eLS1//RYtYCmB1eLHg.9q",
+
         #rounds not zero-padded (pybcrypt rejects this, therefore so do we)
         '$2a$6$DCq7YPn5Rq63x1Lad4cll.TV4S6ytwfsfvkgY8jIucDrjc8deX1s.'
+
+        #NOTE: salts with padding bits set are technically malformed,
+        #      but that's one we can reliably correct & issue warning for.
         ]
 
-    def test_idents(self):
+    #===============================================================
+    # extra tests
+    #===============================================================
+    def iter_external_verifiers(self):
+        try:
+            from bcrypt import hashpw
+        except ImportError:
+            pass
+        else:
+            def check_pybcrypt(secret, hash):
+                self.assertEqual(hashpw(secret, hash), hash,
+                                 "pybcrypt: bcrypt.hashpw(%r,%r):" % (secret, hash))
+            yield check_pybcrypt
+
+        try:
+            from bcryptor.engine import Engine
+        except ImportError:
+            pass
+        else:
+            def check_bcryptor(secret, hash):
+                result = Engine(False).hash_key(secret, hash)
+                self.assertEqual(result, hash,
+                                 "bcryptor: hash_key(%r,%r):" % (secret, hash))
+            yield check_bcryptor
+
+    def test_90_idents(self):
+        "test identifier validation"
         handler = self.handler
 
-        kwds = dict(checksum='8CIhhFCj15KqqFvo/n.Jatx8dJ92f82', salt='VlsfIX9.apXuQBr6tego0M', rounds=12, ident="2a", strict=True)
+        kwds = dict(checksum='8CIhhFCj15KqqFvo/n.Jatx8dJ92f82',
+                    salt='VlsfIX9.apXuQBr6tego0.',
+                    rounds=12, ident="2a", strict=True)
+
         handler(**kwds)
 
         kwds['ident'] = None
         self.assertRaises(ValueError, handler, **kwds)
 
         del kwds['strict']
+
         kwds['ident'] = 'Q'
         self.assertRaises(ValueError, handler, **kwds)
 
-    #FIXME: this would probably still be useful,
-    #       but have to make it patch os_crypt in utils now,
-    #       so that it's called by safe_os_crypt...
-    #
-    ###this method is added in order to maximize test coverage on systems
-    ###where os_crypt is missing or doesn't support bcrypt
-    ##if enable_option("cover") and not bcrypt.has_backend("os_crypt") and bcrypt.has_backend("pybcrypt"):
-    ##    def test_backend(self):
-    ##        from passlib.handlers import bcrypt as bcrypt_mod
-    ##        orig = bcrypt_mod.os_crypt
-    ##        bcrypt_mod.sos_crypt = bcrypt_mod.pybcrypt_hashpw
-    ##        orig = bcrypt.get_backend()
-    ##        try:
-    ##            bcrypt.set_backend("os_crypt")
-    ##            bcrypt.encrypt(u"test", rounds=4)
-    ##        finally:
-    ##            bcrypt.set_backend(orig)
-    ##            bcrypt_mod.os_crypt = orig
-
-bcrypt._no_backends_msg() #call this for coverage purposes
+    #===============================================================
+    # see issue 25 - https://code.google.com/p/passlib/issues/detail?id=25
+    # bcrypt's salt ends with 4 padding bits.
+    # openbsd, pybcrypt, etc assume these bits are always 0.
+    # passlib <= 1.5.2 generated salts where this wasn't usually the case.
+    # as of 1.5.3, we want to always generate salts w/ 0 padding,
+    # and clear the padding of any incoming hashes
+    #===============================================================
+    def do_genconfig(self, **kwds):
+        # correct provided salts to handle ending correctly,
+        # so test_33_genconfig_saltchars doesn't throw warnings.
+        if 'salt' in kwds:
+            from passlib.handlers.bcrypt import BCHARS, BCLAST
+            salt = kwds['salt']
+            if salt and salt[-1] not in BCLAST:
+                salt = salt[:-1] + BCHARS[BCHARS.index(salt[-1])&~15]
+            kwds['salt'] = salt
+        return self.handler.genconfig(**kwds)
+
+    def test_91_bcrypt_padding(self):
+        "test passlib correctly handles bcrypt padding bits"
+        bcrypt = self.handler
+
+        def check_warning(wlog):
+            self.assertWarningMatches(wlog.pop(0),
+                message_re="^encountered a bcrypt hash with incorrectly set padding bits.*",
+            )
+            self.assertFalse(wlog)
+
+        def check_padding(hash):
+            "check bcrypt hash doesn't have salt padding bits set"
+            assert hash.startswith("$2a$") and len(hash) >= 28
+            self.assertTrue(hash[28] in BCLAST,
+                            "padding bits set in hash: %r" % (hash,))
+
+        #===============================================================
+        # test generated salts
+        #===============================================================
+        from passlib.handlers.bcrypt import BCHARS, BCLAST
+
+        # make sure genconfig & encrypt don't return bad hashes.
+        # bug had 15/16 chance of occurring every time salt generated.
+        # so we call it a few different way a number of times.
+        for i in xrange(6):
+            check_padding(bcrypt.genconfig())
+        for i in xrange(3):
+            check_padding(bcrypt.encrypt("bob", rounds=bcrypt.min_rounds))
+
+        # check passing salt to genconfig causes it to be normalized.
+        with catch_warnings(record=True) as wlog:
+            warnings.simplefilter("always")
+
+            hash = bcrypt.genconfig(salt="."*21 + "A.")
+            check_warning(wlog)
+            self.assertEqual(hash, "$2a$12$" + "." * 22)
+
+            hash = bcrypt.genconfig(salt="."*23)
+            self.assertFalse(wlog)
+            self.assertEqual(hash, "$2a$12$" + "." * 22)
+
+        #===============================================================
+        # test handling existing hashes
+        #===============================================================
+
+        PASS1 = "loppux"
+        BAD1  = "$2a$12$oaQbBqq8JnSM1NHRPQGXORm4GCUMqp7meTnkft4zgSnrbhoKdDV0C"
+        GOOD1 = "$2a$12$oaQbBqq8JnSM1NHRPQGXOOm4GCUMqp7meTnkft4zgSnrbhoKdDV0C"
+
+        PASS2 = "Passlib11"
+        BAD2  = "$2a$12$M8mKpW9a2vZ7PYhq/8eJVcUtKxpo6j0zAezu0G/HAMYgMkhPu4fLK"
+        GOOD2 = "$2a$12$M8mKpW9a2vZ7PYhq/8eJVOUtKxpo6j0zAezu0G/HAMYgMkhPu4fLK"
+
+        # make sure genhash() corrects input
+        with catch_warnings(record=True) as wlog:
+            warnings.simplefilter("always")
+
+            self.assertEqual(bcrypt.genhash(PASS1, BAD1), GOOD1)
+            check_warning(wlog)
+
+            self.assertEqual(bcrypt.genhash(PASS2, BAD2), GOOD2)
+            check_warning(wlog)
+
+            self.assertEqual(bcrypt.genhash(PASS2, GOOD2), GOOD2)
+            self.assertFalse(wlog)
+
+        # make sure verify works on both bad and good hashes
+        with catch_warnings(record=True) as wlog:
+            warnings.simplefilter("always")
+
+            self.assertTrue(bcrypt.verify(PASS1, BAD1))
+            check_warning(wlog)
+
+            self.assertTrue(bcrypt.verify(PASS1, GOOD1))
+            self.assertFalse(wlog)
+
+        #===============================================================
+        # test normhash cleans things up correctly
+        #===============================================================
+        with catch_warnings(record=True) as wlog:
+            warnings.simplefilter("always")
+            self.assertEqual(bcrypt.normhash(BAD1), GOOD1)
+            self.assertEqual(bcrypt.normhash(BAD2), GOOD2)
+            self.assertEqual(bcrypt.normhash(GOOD1), GOOD1)
+            self.assertEqual(bcrypt.normhash(GOOD2), GOOD2)
+            self.assertEqual(bcrypt.normhash("$md5$abc"), "$md5$abc")
+
+hash.bcrypt._no_backends_msg() #call this for coverage purposes
 
 #create test cases for specific backends
 Pybcrypt_BCryptTest = create_backend_case(_BCryptTest, "pybcrypt")
diff --git a/passlib/tests/utils.py b/passlib/tests/utils.py
index a2e6a98..20c95bf 100644
--- a/passlib/tests/utils.py
+++ b/passlib/tests/utils.py
@@ -946,6 +946,45 @@ class HandlerCase(TestCase):
         h2 = self.do_encrypt("stub")
         self.assertNotEqual(h1, h2)
 
+    # optional helper used by test_53_external_verifiers
+    iter_external_verifiers = None
+
+    def test_53_external_verifiers(self):
+        "test encrypt() output verifies against external libs"
+        # this makes sure our output can be verified by external libs,
+        # to avoid repeat of things like issue 25.
+
+        handler = self.handler
+        possible = False
+        if self.iter_external_verifiers:
+            helpers = list(self.iter_external_verifiers())
+            possible = True
+        else:
+            helpers = []
+
+        # provide default "os_crypt" helper
+        if hasattr(handler, "has_backend") and \
+                'os_crypt' in handler.backends and \
+                not hasattr(handler, "orig_prefix"):
+            possible = True
+            if handler.has_backend("os_crypt"):
+                def check_crypt(secret, hash):
+                    self.assertEqual(utils.os_crypt(secret, hash), hash,
+                                     "os_crypt(%r,%r):" % (secret, hash))
+                helpers.append(check_crypt)
+
+        if not helpers:
+            if possible:
+                raise self.skipTest("no external libs available")
+            else:
+                raise self.skipTest("not applicable")
+
+        # generate a single hash, and verify it using all helpers.
+        secret = 't\xc3\xa1\xd0\x91\xe2\x84\x93\xc9\x99'
+        hash = self.do_encrypt(secret)
+        for helper in helpers:
+            helper(secret, hash)
+
     #=========================================================
     #test max password size
     #=========================================================