From 83d4c5b5237129e246536b7d686e7d79c4bc0fd2 Mon Sep 17 00:00:00 2001 From: Eli Collins Date: Sun, 29 Jan 2017 16:27:43 -0500 Subject: Per issue 83, all "harden_verify" code is now deprecated & a noop. will be removed completely in 1.8. Rationale: Aside from the arguments in issue 83, performed a timing analysis, and decided harden_verify framework wasn't going to be easily workable to prevent a timing attack anyways (see attached admin/plot_verify_timing.py script). Changes: * dummy_verify() has been kept around, but now uses .verify() against a dummy hash, which is guaranteed to have correct timing (though wastes cpu cycles). * Removed most harden_verify code, treating it as NOOP just like min_verify_time. Similarly, removed most documentation references to. --- admin/plot_verify_timing.py | 113 ++++++++++++++++ docs/history/1.7.rst | 35 +++-- docs/lib/passlib.context.rst | 28 +--- docs/narr/context-tutorial.rst | 4 - passlib/context.py | 223 +++++++++----------------------- passlib/tests/test_context.py | 108 +++------------- passlib/tests/test_ext_django_source.py | 5 +- 7 files changed, 220 insertions(+), 296 deletions(-) create mode 100644 admin/plot_verify_timing.py diff --git a/admin/plot_verify_timing.py b/admin/plot_verify_timing.py new file mode 100644 index 0000000..3f6255f --- /dev/null +++ b/admin/plot_verify_timing.py @@ -0,0 +1,113 @@ +#!/usr/bin/env python3 +""" +small helper script used to compare timing of verify() & dummy_verify() +""" +# core +from __future__ import absolute_import, division, print_function, unicode_literals +from argparse import ArgumentParser +import sys +from timeit import default_timer as tick +# site +import matplotlib.pyplot as plt +from matplotlib.backends.backend_pdf import PdfPages +# pkg +from passlib.context import CryptContext +# local +__all__ = [ + "main" +] + +def main(*args): + # + # parse args + # + parser = ArgumentParser("""plot hash timing""") + parser.add_argument("-n", "--number", help="number of iterations", + type=int, default=300) + parser.add_argument("-o", "--output", help="pdf file", + default="plot_hash.pdf") + + opts = parser.parse_args(args) + + # + # init vars + # + ctx = CryptContext(schemes=["pbkdf2_sha256"]) + + secret = "a9w3857naw958ioa" + wrong_secret = "q0389wairuowieru" + hash = ctx.hash(secret) + ctx.dummy_verify() + + correct = [] + wrong = [] + missing = [] + + loops = opts.number + + # + # run timing loop + # + for _ in range(loops): + # correct pwd + start = tick() + ctx.verify(secret, hash) + delta = tick() - start + correct.append(delta) + + # wrong pwd + start = tick() + ctx.verify(wrong_secret, hash) + delta = tick() - start + wrong.append(delta) + + # wrong user / dummy verify + start = tick() + ctx.dummy_verify() + delta = tick() - start + missing.append(delta) + + # + # calc quartiles for verify() samples + # + samples = sorted(correct + wrong) + count = len(samples) + q1 = samples[count // 4] + q3 = samples[count * 3 // 4] + iqr = q3 - q1 + ub = q3 + 1.5 * iqr + lb = q1 - 1.5 * iqr + + # + # add in dummy samples, calc bounds + # + samples.extend(missing) + ymin = min(min(samples), lb - 0.5 * iqr) + ymax = max(max(samples), ub) + + # + # generate graph + # + with PdfPages(opts.output) as pdf: + plt.plot(range(loops), correct, 'go', label="verify success") + plt.plot(range(loops), wrong, 'yo', label="verify failed") + plt.plot(range(loops), missing, 'ro', label="dummy_verify()") + + plt.axis([0, loops, ymin, ymax]) + + plt.axhline(y=q1, xmax=count, color="r") + plt.axhline(y=q3, xmax=count, color="r") + plt.axhline(y=lb, xmax=count, color="orange") + plt.axhline(y=ub, xmax=count, color="orange") + + plt.ylabel('elapsed time') + plt.xlabel('loop count') + plt.legend(shadow=True, title="Legend", fancybox=True) + + plt.title('%s verify timing' % ctx.handler().name) + pdf.savefig() + plt.close() + +if __name__ == "__main__": + sys.exit(main(*sys.argv[1:])) + diff --git a/docs/history/1.7.rst b/docs/history/1.7.rst index 2be35a2..f2a2cd1 100644 --- a/docs/history/1.7.rst +++ b/docs/history/1.7.rst @@ -7,16 +7,33 @@ Passlib 1.7 .. py:currentmodule:: passlib.ifc -* bugfix: :meth:`PasswordHash.hash` will now warn if passed any settings +This release rolls up assorted bug & compatibility fixes since 1.7.0. + +Bugfixes +-------- + +* :meth:`PasswordHash.hash` will now warn if passed any settings keywords. This usage was deprecated in 1.7.0, but warning wasn't properly enabled. See :ref:`hash-configuring` for the preferred way to pass settings. -* bugfix: setup.py: prevent erroneous version strings when run from an sdist. +* setup.py: Prevent erroneous version strings when run from an sdist. + This should fix some reproducible-build issues observed downstream. + +* TOTP tests: Test suite now traps additional errors that :func:`datetime.utcfromtimestamp` + may throw under python 3, and works around python 3.6 bug `29100 `_. + This should fix some test failures under python 3.6 and certain bit-size architectures. + +Deprecations +------------ + +* The :class:`!CryptContext` ``harden_verify`` flag has been deprecated, + turned into a NOOP, and will be removed in passlib 1.8 along with already-deprecated + ``min_verify_time`` (:issue:`83`). -* bugfix: TOTP tests: test setup now traps additional errors utcfromtimestamp() - may throw under python 3. +Other Changes +------------- -* various documentation updates +* Various documentation updates & corrections. .. _whats-new: @@ -94,12 +111,6 @@ New Features methods for dealing with hashes representing :ref:`disabled accounts ` (:issue:`45`). - * The :class:`~passlib.context.CryptContext` object now supports - a :ref:`harden_verify ` option, - allowing applications to introduce a delay in verification - to help prevent attackers discovering weak or missing hashes - through timing attacks. - * All hashers which truncate passwords (e.g. :class:`~passlib.hash.bcrypt` and :class:`~passlib.hash.des_crypt`) can now be configured to raise a :exc:`~passlib.exc.PasswordTruncateError` when a overly-large password is provided. @@ -306,8 +317,6 @@ Scheduled removal of features: * **[minor]** :mod:`passlib.context`: The :ref:`min_verify_time ` keyword that was deprecated in release 1.6, is now completely ignored. Support will be removed entirely in release 1.8. - See the new :ref:`harden_verify ` keyword - that replaces it. * **[trivial]** :mod:`passlib.hash`: The internal :meth:`!PasswordHash.parse_rounds` method, deprecated in 1.6, has been removed. diff --git a/docs/lib/passlib.context.rst b/docs/lib/passlib.context.rst index b2f7248..5ed7413 100644 --- a/docs/lib/passlib.context.rst +++ b/docs/lib/passlib.context.rst @@ -180,37 +180,21 @@ Options which directly affect the behavior of the CryptContext instance: and will be removed in version 1.8. .. versionchanged:: 1.7 - Per deprecation roadmap above, this option is now ignored. - See ``harden_verify`` below for a replacement. + Per deprecation roadmap above, this option is now ignored. .. _context-harden-verify-option: ``harden_verify`` - If set to ``true``, CryptContext will pause the first time :meth:`verify` - is called, in order to calculate the "average" time it would take - to verify a hash created using the default settings. - - Subsequent :meth:`verify` calls using will have their time padded - to this minimum time, in order to make it harder for an attacker - to guess which accounts have weak hashes. - - Applications may also wish to call :meth:`~CryptContext.dummy_verify` for login - attempts where the user does not exist, in order to mask which - users accounts have valid hashes. + Companion to ``min_verify_time``, currently ignored. - This option can be set to ``True`` or ``False`` (the default). - - The default (may) be changed in a later 2.x release. - - .. warning:: + .. versionadded:: 1.7 - This feature is new, and adjustments may need to be made - to when (and how) the code calculates what the "minimum verification time" - is supposed to be. + .. deprecated:: 1.7.1 - .. versionadded:: 1.7 + This option is ignored by 1.7.1, and will be removed in 1.8 + along with ``min_verify_time``. .. _context-algorithm-options: diff --git a/docs/narr/context-tutorial.rst b/docs/narr/context-tutorial.rst index 8e4cb7a..af093fa 100644 --- a/docs/narr/context-tutorial.rst +++ b/docs/narr/context-tutorial.rst @@ -371,7 +371,6 @@ If an existing hash below the minimum is tested, it will show up as needing reha Undocumented Features ===================== -.. todo:: Document usage of the :ref:`harden_verify ` option. .. todo:: Document usage of the :ref:`context-disabled-hashes` options. .. rst-class:: html-toggle @@ -420,9 +419,6 @@ All of the documented :ref:`context-options` are allowed. admin__pbkdf2_sha1__min_rounds = 18000 admin__pbkdf2_sha1__default_rounds = 20000 - ; delay failed verify() calls to mask hashes with weak rounds - harden_verify = true - Initializing the CryptContext ----------------------------- Applications which choose to use a policy file will typically want diff --git a/passlib/context.py b/passlib/context.py index 433a9d0..fa700f7 100644 --- a/passlib/context.py +++ b/passlib/context.py @@ -428,11 +428,7 @@ class CryptPolicy(object): warn("get_min_verify_time() and min_verify_time option is deprecated and ignored, " "and will be removed in Passlib 1.8", DeprecationWarning, stacklevel=2) - context = self._context - if context.harden_verify: - return context.min_verify_time - else: - return 0 + return 0 def get_options(self, name, category=None): """return dictionary of options specific to a given handler. @@ -793,12 +789,12 @@ class _CryptConfig(object): "in policy: %r" % (scheme,)) elif key == "min_verify_time": warn("'min_verify_time' was deprecated in Passlib 1.6, is " - "ignored in 1.7, and will be removed in 1.8; use 'harden_verify' instead", + "ignored in 1.7, and will be removed in 1.8", DeprecationWarning) elif key == "harden_verify": - if cat: - raise ValueError("'harden_verify' cannot be specified per category") - value = as_bool(value, param="harden_verify") + warn("'harden_verify' is deprecated & ignored as of Passlib 1.7.1, " + " and will be removed in 1.8", + DeprecationWarning) elif key != "schemes": raise KeyError("unknown CryptContext keyword: %r" % (key,)) return key, value @@ -1595,7 +1591,7 @@ class CryptContext(object): #----------------------------------------------------------- config = _CryptConfig(source) self._config = config - self.reset_min_verify_time() + self._reset_dummy_verify() self._get_record = config.get_record self._identify_record = config.identify_record if config.context_kwds: @@ -1987,149 +1983,20 @@ class CryptContext(object): #=================================================================== # verify() hardening + # NOTE: this entire feature has been disabled. + # all contents of this section are NOOPs as of 1.7.1, + # and will be removed in 1.8. #=================================================================== - # NOTE: the estimation is currently algorithm is a little rough, so - # the control values are exposed here to make "poking" at them easier. - - #: maximum samples to take when estimating min_verify_time - mvt_estimate_max_samples = 10 - - #: minimum samples to take when estimating min_verify_time - mvt_estimate_min_samples = 4 - - #: maximum time to spend estimating min_verify_time - #: (this value is overridden by min_samples) - mvt_estimate_max_time = 1.2 - - #: minimum measurement resolution required by estimate - mvt_estimate_resolution = 0.05 - - # XXX: make writable (once CryptPolicy is removed)? - @memoized_property - def harden_verify(self): - return self._config.get_context_option_with_flag(None, "harden_verify")[0] - - #: global lock used to prevent multiple copies of _calc_min_verify_time() - #: from running at the same time (whether as part of same context or not); - #: as this would cause inaccurate measurements - _mvt_lock = threading.Lock() - - # XXX: how to handle multiple categories? admin cateogry would stand out. - # but dont' want multiple levels of min_verify_time, *right*? - # maybe want to have CryptContext switch into a "nested" mode - # if categories are put in place, and have it act like multiple contexts. - - @memoized_property - def min_verify_time(self): - """ - minimum time verify() should take, to mask presence of weak hashes. - when first accessed, an estimate is performed based on - how long default hash takes. - can be overridden by manually writing to this attribute. - - will default to 0 (no estimate performed) unless 'harden_verify = true' - passed in CryptContext config. - """ - with self._mvt_lock: - # check if value was set in another thread - value = type(self).min_verify_time.peek_cache(self) - if value is None: - # value wasn't set, use calc function - value = self._calc_min_verify_time() - return value - - def _calc_min_verify_time(self): - """ - calculate min_verify_time based on system performance. - - .. warning:: - this assumes caller has acquired :attr:`_mvt_lock` - - :return: - estimated min_verify_time value - """ - # load config - log.debug("estimating min_verify_time") - min_samples = self.mvt_estimate_min_samples - max_samples = self.mvt_estimate_max_samples - record = self._get_record(None, None) - repeat = 1 - - # generate random secret to test against, - # and generate sample hash - secret = getrandstr(rng, BASE64_CHARS, 16) - start = timer() - hash = record.hash(secret) - samples = [timer() - start] - - # gather samples until condition met - loop_end = start + self.mvt_estimate_max_time - while True: - # gather sample - start = timer() - for _ in irange(repeat): - # XXX: using record.verify() instead of self.verify() - # since that would cause recursion errors back to here - # (and we can't temporarily set .min_verify_time=0 - # without temporarily letting other threads through - # w/o any delay). presuming there's not a noticeable - # overhead for the CryptContext.verify() wrapper. - record.verify(secret, hash) - end = timer() - elapsed = end - start - - # make sure we're far enough above timer's minimum resolution - if elapsed < self.mvt_estimate_resolution: - repeat *= 2 - continue - samples.append(elapsed / repeat) - - # stop as soon as possible if we have plenty of samples - if len(samples) > max_samples: - break - - # otherwise stop after max time, as long as we have bare minimum. - if end > loop_end and len(samples) > min_samples: - break - - # calc median, to cheaply exclude outliers - samples.sort() - result = round(samples[(len(samples)+1)//2], 3) - log.debug("setting min_verify_time=%f (median of %d samples)", - result, len(samples)) - return result + mvt_estimate_max_samples = 20 + mvt_estimate_min_samples = 10 + mvt_estimate_max_time = 2 + mvt_estimate_resolution = 0.01 + harden_verify = None + min_verify_time = 0 def reset_min_verify_time(self): - """ - clear min_verify_time estimate, - will be recalculated next time :attr:`min_verify_time` is accessed - (i.e. when :meth:`verify` is called). - """ - type(self).harden_verify.clear_cache(self) - type(self).min_verify_time.clear_cache(self) - - def dummy_verify(self, elapsed=0): - """ - Helper that applications can call when user wasn't found, - in order to simulate time it would take to hash a password. - - Invokes :func:`time.sleep` for amount of time needed to make - total elapsed time >= :attr:`min_verify_time`. - - :param elapsed: - optional amount of time spent hashing so far - (mainly used internally by :meth:`verify` and :meth:`verify_and_update`). - - See :ref:`harden_verify ` for details - about this feature. - - .. versionadded:: 1.7 - """ - assert elapsed >= 0 - remaining = self.min_verify_time - elapsed - if remaining > 0: - time.sleep(remaining) + self._reset_dummy_verify() #=================================================================== # password hash api @@ -2465,19 +2332,14 @@ class CryptContext(object): DeprecationWarning) if hash is None: # convenience feature -- let apps pass in hash=None when user - # isn't found / has no hash; mainly to get dummy_verify() benefit. - if self.harden_verify: - self.dummy_verify() + # isn't found / has no hash; useful because it invokes dummy_verify() + self.dummy_verify() return False record = self._get_or_identify_record(hash, scheme, category) strip_unused = self._strip_unused_context_kwds if strip_unused: strip_unused(kwds, record) - start = timer() - ok = record.verify(secret, hash, **kwds) - if not ok and self.harden_verify: - self.dummy_verify(timer() - start) - return ok + return record.verify(secret, hash, **kwds) def verify_and_update(self, secret, hash, scheme=None, category=None, **kwds): """verify password and re-hash the password if needed, all in a single call. @@ -2549,9 +2411,8 @@ class CryptContext(object): DeprecationWarning) if hash is None: # convenience feature -- let apps pass in hash=None when user - # isn't found / has no hash; mainly to get dummy_verify() benefit. - if self.harden_verify: - self.dummy_verify() + # isn't found / has no hash; useful because it invokes dummy_verify() + self.dummy_verify() return False, None record = self._get_or_identify_record(hash, scheme, category) strip_unused = self._strip_unused_context_kwds @@ -2564,10 +2425,7 @@ class CryptContext(object): # api to combine verify & needs_update to single call, # potentially saving some round-trip parsing. # but might make these codepaths more complex... - start = timer() if not record.verify(secret, hash, **clean_kwds): - if self.harden_verify: - self.dummy_verify(timer() - start) return False, None elif record.deprecated or record.needs_update(hash, secret=secret): # NOTE: we re-hash with default scheme, not current one. @@ -2575,6 +2433,45 @@ class CryptContext(object): else: return True, None + #=================================================================== + # missing-user helper + #=================================================================== + + #: secret used for dummy_verify() + _dummy_secret = "too many secrets" + + @memoized_property + def _dummy_hash(self): + """ + precalculated hash for dummy_verify() to use + """ + return self.hash(self._dummy_secret) + + def _reset_dummy_verify(self): + """ + flush memoized values used by dummy_verify() + """ + type(self)._dummy_hash.clear_cache(self) + + def dummy_verify(self, elapsed=0): + """ + Helper that applications can call when user wasn't found, + in order to simulate time it would take to hash a password. + + Runs verify() against a dummy hash, to simulate verification + of a real account password. + + :param elapsed: + + .. deprecated:: 1.7.1 + + this option is ignored, and will be removed in passlib 1.8. + + .. versionadded:: 1.7 + """ + self.verify(self._dummy_secret, self._dummy_hash) + return False + #=================================================================== # disabled hash support #=================================================================== diff --git a/passlib/tests/test_context.py b/passlib/tests/test_context.py index 7ad26cd..89fb9fe 100644 --- a/passlib/tests/test_context.py +++ b/passlib/tests/test_context.py @@ -1580,106 +1580,32 @@ sha512_crypt__min_rounds = 45000 #=================================================================== def test_harden_verify_parsing(self): """harden_verify -- parsing""" + warnings.filterwarnings("ignore", ".*harden_verify.*", + category=DeprecationWarning) # valid values ctx = CryptContext(schemes=["sha256_crypt"]) self.assertEqual(ctx.harden_verify, None) self.assertEqual(ctx.using(harden_verify="").harden_verify, None) - self.assertEqual(ctx.using(harden_verify="true").harden_verify, True) - self.assertEqual(ctx.using(harden_verify="false").harden_verify, False) - - # invalid value - self.assertRaises(ValueError, ctx.using, harden_verify="foo") - - # forbid w/ category - self.assertRaises(ValueError, ctx.using, admin__context__harden_verify="true") - - def test_harden_verify_estimate(self): - """harden_verify -- min_verify_time estimation""" - # setup - handler = DelayHash.using() - ctx = CryptContext(schemes=[handler]) - maxtime = 0.25 - scale = 0.01 - accuracy = scale * 0.1 - secret = "secret" - hash = handler.hash(secret) - - # establish baseline w/ 0 delay - elapsed, _ = time_call(partial(ctx.verify, secret, hash), maxtime=maxtime) - self.assertAlmostEqual(elapsed, 0, delta=accuracy) - - # run min_verify_time calc, see what it gets - ctx.reset_min_verify_time() - self.assertAlmostEqual(ctx.min_verify_time, 0, delta=accuracy) - - # add delay, make sure verify sees it - handler.delay = scale - elapsed, _ = time_call(partial(ctx.verify, secret, hash), maxtime=maxtime) - self.assertAlmostEqual(elapsed, scale, delta=accuracy) - - # re-run min_verify_time calc, see what it gets - ctx.reset_min_verify_time() - self.assertAlmostEqual(ctx.min_verify_time, scale, delta=accuracy) - - # TODO: * check that min_verify_time honors thread lock - # * check that min_verify_time correctly uses median - # * check that mvt_* config settings are honored + self.assertEqual(ctx.using(harden_verify="true").harden_verify, None) + self.assertEqual(ctx.using(harden_verify="false").harden_verify, None) def test_dummy_verify(self): - """harden_verify -- min_verify_time honored by dummy_verify() methods""" - # setup + """ + dummy_verify() method + """ + # check dummy_verify() takes expected time + expected = 0.05 + accuracy = 0.2 handler = DelayHash.using() - maxtime = 0.25 - scale = 0.01 - accuracy = scale * 0.1 - - ctx = CryptContext(schemes=[handler], harden_verify=False) - ctx.min_verify_time = scale - - # check dummy_verify() honors mvt even if hardening is off - elapsed, _ = time_call(partial(ctx.dummy_verify), maxtime=maxtime) - self.assertAlmostEqual(elapsed, scale, delta=accuracy) - - # check dummy_verify() honors elapsed kwd - elapsed, _ = time_call(partial(ctx.dummy_verify, elapsed=scale/2), maxtime=maxtime) - self.assertAlmostEqual(elapsed, scale/2, delta=accuracy) + handler.delay = expected + ctx = CryptContext(schemes=[handler]) + ctx.dummy_verify() # prime the memoized helpers + elapsed, _ = time_call(ctx.dummy_verify) + self.assertAlmostEqual(elapsed, expected, delta=expected * accuracy) - def test_harden_verify_w_verify(self, verify_and_update=False): - """harden_verify -- min_verify_time honored by verify()""" - # setup - handler = DelayHash.using() - maxtime = 0.25 - scale = 0.01 - accuracy = scale * 0.1 - secret = "secret" - other = "other" - hash = handler.hash(secret) - - def time_verify(pwd): - if verify_and_update: - func = partial(ctx.verify_and_update, pwd, hash) - else: - func = partial(ctx.verify, pwd, hash) - elapsed, _ = time_call(func, maxtime=maxtime) - return elapsed - - # w/o harden_verify, verify() should ignore min_verify_time even if failed - ctx = CryptContext(schemes=[handler], harden_verify=False) - ctx.min_verify_time = scale - self.assertAlmostEqual(time_verify(other), 0, delta=accuracy) - - # w/ harden_verify, verify() should ignore min_verify_time if successful - ctx.update(harden_verify=True) - ctx.min_verify_time = scale - self.assertAlmostEqual(time_verify(secret), 0, delta=accuracy) - - # w/ harden_verify, verify() should honor min_verify_time if failed - self.assertAlmostEqual(time_verify(other), scale, delta=accuracy) - - def test_harden_verify_w_verify_and_update(self): - """harden_verify -- min_verify_time honored by verify_and_update()""" - self.test_harden_verify_w_verify(verify_and_update=True) + # TODO: test dummy_verify() invoked by .verify() when hash is None, + # and same for .verify_and_update() #=================================================================== # feature tests diff --git a/passlib/tests/test_ext_django_source.py b/passlib/tests/test_ext_django_source.py index b86e9ee..4b42e59 100644 --- a/passlib/tests/test_ext_django_source.py +++ b/passlib/tests/test_ext_django_source.py @@ -226,9 +226,8 @@ if test_hashers_mod: # that anyways? get_hashers_by_algorithm() should throw KeyError, right? test_pbkdf2_upgrade_new_hasher = _OMIT - # TODO: need to support wrapping django's harden-runtime feature w/ our own. - # still won't be able to pass their tests, because what they test for - # isn't how CryptContext.harden_verify works. + # TODO: support wrapping django's harden-runtime feature? + # would help pass their tests. test_check_password_calls_harden_runtime = _OMIT test_bcrypt_harden_runtime = _OMIT test_pbkdf2_harden_runtime = _OMIT -- cgit v1.2.1