Repository: pyca/bcrypt

Issue:
Possible regression of "BSD wraparound" bug?
[passlib implements a specific check](https://bitbucket.org/ecollins/passlib/src/934db60eea9cd9fe16fed88dcac114435256af47/passlib/handlers/bcrypt.py#bcrypt.py-318) against the ["BSD wraparound" bug](http://www.openwall.com/lists/oss-security/2012/01/02/4) that now appears to be failing with bcrypt 3.0

Although the included source from libcrypt appears to have been updated since the bug was fixed ([OpenBSD 5.5, according to passlib](http://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#bsd-wraparound-bug)) I'm not familiar enough with the code to be able to tell if this is a genuine regression or not


Discussion:
reaperhulk: This fix is a bit weird to me since arguably OpenBSD's "If you're on the old $2a$ hash you may want this behavior because you're checking existing hashes that were made with that behavior" is more correct, but I guess preserving compatibility with ourselves is more important.
dstufft: @reaperhulk Yea-- I think it's especially important to break those particular hashes because they're trivial to brute force (since they hash the same as ``b"0" * 72``) which is one of the first things that something like JackTheRipper will try. However, given that we had the old behavior previously, if we didn't restore it we'd also be breaking anyone who happened to use this kind of password (although the edge case this hits it's quite rare).

Ultimately though, I think the edge case is sufficiently narrow so as not to matter *that* much, but the fact it generates a warning in passlib motivates me to ensure this is fixed :)

Patch:
diff --git a/README.rst b/README.rst
index 62f189f..0883286 100644
--- a/README.rst
+++ b/README.rst
@@ -37,6 +37,10 @@ For Fedora and RHEL-derivatives, the following command will ensure that the requ
 Changelog
 =========
 
+3.1.0
+-----
+* Added support for ``checkpw`` as another method of verifying a password.
+
 3.0.0
 -----
 * Switched the C backend to code obtained from the OpenBSD project rather than
@@ -51,8 +55,8 @@ Changelog
 Usage
 -----
 
-Hashing
-~~~~~~~
+Password Hashing
+~~~~~~~~~~~~~~~~
 
 Hashing and then later checking that a password matches the previous hashed
 password is very simple:
@@ -63,9 +67,9 @@ password is very simple:
     >>> password = b"super secret password"
     >>> # Hash a password for the first time, with a randomly-generated salt
     >>> hashed = bcrypt.hashpw(password, bcrypt.gensalt())
-    >>> # Check that a unhashed password matches one that has previously been
-    >>> #   hashed
-    >>> if bcrypt.hashpw(password, hashed) == hashed:
+    >>> # Check that an unhashed password matches one that has previously been
+    >>> # hashed
+    >>> if bcrypt.checkpw(password, hashed):
     ...     print("It Matches!")
     ... else:
     ...     print("It Does not Match :(")
diff --git a/src/bcrypt/__init__.py b/src/bcrypt/__init__.py
index ad44e93..d6acb84 100644
--- a/src/bcrypt/__init__.py
+++ b/src/bcrypt/__init__.py
@@ -67,6 +67,14 @@ def hashpw(password, salt):
     if b"\x00" in password:
         raise ValueError("password may not contain NUL bytes")
 
+    # bcrypt originally suffered from a wraparound bug:
+    # http://www.openwall.com/lists/oss-security/2012/01/02/4
+    # This bug was corrected in the OpenBSD source by truncating inputs to 72
+    # bytes on the updated prefix $2b$, but leaving $2a$ unchanged for
+    # compatibility. However, pyca/bcrypt 2.0.0 *did* correctly truncate inputs
+    # on $2a$, so we do it here to preserve compatibility with 2.0.0
+    password = password[:72]
+
     salt = _normalize_prefix(salt)
 
     hashed = _bcrypt.ffi.new("unsigned char[]", 128)
@@ -78,6 +86,24 @@ def hashpw(password, salt):
     return _bcrypt.ffi.string(hashed)
 
 
+def checkpw(password, hashed_password):
+    if (isinstance(password, six.text_type) or
+            isinstance(hashed_password, six.text_type)):
+        raise TypeError("Unicode-objects must be encoded before checking")
+
+    if b"\x00" in password or b"\x00" in hashed_password:
+        raise ValueError(
+            "password and hashed_password may not contain NUL bytes"
+        )
+
+    # If the user supplies a $2y$ prefix we normalize to $2b$
+    hashed_password = _normalize_prefix(hashed_password)
+
+    ret = hashpw(password, hashed_password)
+
+    return _bcrypt.lib.timingsafe_bcmp(ret, hashed_password, len(ret)) == 0
+
+
 def kdf(password, salt, desired_key_bytes, rounds):
     if isinstance(password, six.text_type) or isinstance(salt, six.text_type):
         raise TypeError("Unicode-objects must be encoded before hashing")
diff --git a/src/build_bcrypt.py b/src/build_bcrypt.py
index e7aca4c..3eec35c 100644
--- a/src/build_bcrypt.py
+++ b/src/build_bcrypt.py
@@ -25,6 +25,7 @@ int bcrypt_hashpass(const char *, const char *, char *, size_t);
 int encode_base64(char *, const uint8_t *, size_t);
 int bcrypt_pbkdf(const char *, size_t, const uint8_t *, size_t,
                  uint8_t *, size_t, unsigned int);
+int timingsafe_bcmp(const void *, const void *, size_t);
 """)
 
 ffi.set_source(
diff --git a/tasks.py b/tasks.py
new file mode 100644
index 0000000..06b1492
--- /dev/null
+++ b/tasks.py
@@ -0,0 +1,120 @@
+from __future__ import absolute_import, division, print_function
+
+import getpass
+import io
+import os
+import time
+
+from clint.textui.progress import Bar as ProgressBar
+
+import invoke
+
+import requests
+
+
+JENKINS_URL = "https://jenkins.cryptography.io/job/bcrypt-wheel-builder"
+
+
+def wait_for_build_completed(session):
+    # Wait 20 seconds before actually checking if the build is complete, to
+    # ensure that it had time to really start.
+    time.sleep(20)
+    while True:
+        response = session.get(
+            "{0}/lastBuild/api/json/".format(JENKINS_URL),
+            headers={
+                "Accept": "application/json",
+            }
+        )
+        response.raise_for_status()
+        if not response.json()["building"]:
+            assert response.json()["result"] == "SUCCESS"
+            break
+        time.sleep(0.1)
+
+
+def download_artifacts(session):
+    response = session.get(
+        "{0}/lastBuild/api/json/".format(JENKINS_URL),
+        headers={
+            "Accept": "application/json"
+        }
+    )
+    response.raise_for_status()
+    assert not response.json()["building"]
+    assert response.json()["result"] == "SUCCESS"
+
+    paths = []
+
+    last_build_number = response.json()["number"]
+    for run in response.json()["runs"]:
+        if run["number"] != last_build_number:
+            print(
+                "Skipping {0} as it is not from the latest build ({1})".format(
+                    run["url"], last_build_number
+                )
+            )
+            continue
+
+        response = session.get(
+            run["url"] + "api/json/",
+            headers={
+                "Accept": "application/json",
+            }
+        )
+        response.raise_for_status()
+        for artifact in response.json()["artifacts"]:
+            response = session.get(
+                "{0}artifact/{1}".format(run["url"], artifact["relativePath"]),
+                stream=True
+            )
+            assert response.headers["content-length"]
+            print("Downloading {0}".format(artifact["fileName"]))
+            bar = ProgressBar(
+                expected_size=int(response.headers["content-length"]),
+                filled_char="="
+            )
+            content = io.BytesIO()
+            for data in response.iter_content(chunk_size=8192):
+                content.write(data)
+                bar.show(content.tell())
+            assert bar.expected_size == content.tell()
+            bar.done()
+            out_path = os.path.join(
+                os.path.dirname(__file__),
+                "dist",
+                artifact["fileName"],
+            )
+            with open(out_path, "wb") as f:
+                f.write(content.getvalue())
+            paths.append(out_path)
+    return paths
+
+
+@invoke.task
+def release(version):
+    """
+    ``version`` should be a string like '0.4' or '1.0'.
+    """
+    invoke.run("git tag -s {0} -m '{0} release'".format(version))
+    invoke.run("git push --tags")
+
+    invoke.run("python setup.py sdist")
+
+    invoke.run(
+        "twine upload -s dist/bcrypt-{0}*".format(version)
+    )
+
+    session = requests.Session()
+
+    token = getpass.getpass("Input the Jenkins token: ")
+    response = session.post(
+        "{0}/build?token={1}".format(JENKINS_URL, token),
+        params={
+            "cause": "Building wheels for {0}".format(version)
+        }
+    )
+    response.raise_for_status()
+    wait_for_build_completed(session)
+    paths = download_artifacts(session)
+    invoke.run("twine upload {0}".format(" ".join(paths)))


Test patch:
diff --git a/tests/test_bcrypt.py b/tests/test_bcrypt.py
index b506a7a..47f315a 100644
--- a/tests/test_bcrypt.py
+++ b/tests/test_bcrypt.py
@@ -216,6 +216,11 @@ def test_hashpw_new(password, salt, hashed):
     assert bcrypt.hashpw(password, salt) == hashed
 
 
+@pytest.mark.parametrize(("password", "salt", "hashed"), _test_vectors)
+def test_checkpw(password, salt, hashed):
+    assert bcrypt.checkpw(password, hashed) is True
+
+
 @pytest.mark.parametrize(("password", "salt", "hashed"), _test_vectors)
 def test_hashpw_existing(password, salt, hashed):
     assert bcrypt.hashpw(password, hashed) == hashed
@@ -226,11 +231,47 @@ def test_hashpw_2y_prefix(password, hashed, expected):
     assert bcrypt.hashpw(password, hashed) == expected
 
 
+@pytest.mark.parametrize(("password", "hashed", "expected"), _2y_test_vectors)
+def test_checkpw_2y_prefix(password, hashed, expected):
+    assert bcrypt.checkpw(password, hashed) is True
+
+
 def test_hashpw_invalid():
     with pytest.raises(ValueError):
         bcrypt.hashpw(b"password", b"$2z$04$cVWp4XaNU8a4v1uMRum2SO")
 
 
+def test_checkpw_wrong_password():
+    assert bcrypt.checkpw(
+        b"badpass",
+        b"$2b$04$2Siw3Nv3Q/gTOIPetAyPr.GNj3aO0lb1E5E9UumYGKjP9BYqlNWJe"
+    ) is False
+
+
+def test_checkpw_bad_salt():
+    with pytest.raises(ValueError):
+        bcrypt.checkpw(
+            b"badpass",
+            b"$2b$04$?Siw3Nv3Q/gTOIPetAyPr.GNj3aO0lb1E5E9UumYGKjP9BYqlNWJe"
+        )
+
+
+def test_checkpw_str_password():
+    with pytest.raises(TypeError):
+        bcrypt.checkpw(
+            six.text_type("password"),
+            b"$2b$04$cVWp4XaNU8a4v1uMRum2SO",
+        )
+
+
+def test_checkpw_str_salt():
+    with pytest.raises(TypeError):
+        bcrypt.checkpw(
+            b"password",
+            six.text_type("$2b$04$cVWp4XaNU8a4v1uMRum2SO"),
+        )
+
+
 def test_hashpw_str_password():
     with pytest.raises(TypeError):
         bcrypt.hashpw(
@@ -247,6 +288,20 @@ def test_hashpw_str_salt():
         )
 
 
+def test_checkpw_nul_byte():
+    with pytest.raises(ValueError):
+        bcrypt.checkpw(
+            b"abc\0def",
+            b"$2b$04$2Siw3Nv3Q/gTOIPetAyPr.GNj3aO0lb1E5E9UumYGKjP9BYqlNWJe"
+        )
+
+    with pytest.raises(ValueError):
+        bcrypt.checkpw(
+            b"abcdef",
+            b"$2b$04$2S\0w3Nv3Q/gTOIPetAyPr.GNj3aO0lb1E5E9UumYGKjP9BYqlNWJe"
+        )
+
+
 def test_hashpw_nul_byte():
     salt = bcrypt.gensalt(4)
     with pytest.raises(ValueError):
@@ -375,3 +430,9 @@ def test_invalid_params(password, salt, desired_key_bytes, rounds, error):
 def test_bcrypt_assert():
     with pytest.raises(SystemError):
         bcrypt._bcrypt_assert(False)
+
+
+def test_2a_wraparound_bug():
+    assert bcrypt.hashpw(
+        (b"0123456789" * 26)[:255], b"$2a$04$R1lJ2gkNaoPGdafE.H.16."
+    ) == b"$2a$04$R1lJ2gkNaoPGdafE.H.16.1MKHPvmKwryeulRe225LKProWYwt9Oi"