Timing and token reuse #1669
Closed
Timing and token reuse #1669
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
April 25, 2017 15:59
Removes timing information from two functions that are executed during the JI login process. In `valid_password()` there was a timing-variable, byte-by-byte comparison of the salted scrypt hash of submitted password with the salted scrypt hash of their actual password. Since the salt is kept secret from the Journalist, this makes it impossible to know what the final value of the submitted password hash is. Thus an attacker may learn how many prefix bytes in common the hash of their submitted password has with actual password hash, but not what those bytes are. Due to the avalanche effect of cryptographic hash functions, this makes this information totally useless. Therefore, *this timing information is not exploitable*, but we are removing it anyway because we have gotten several complaints about it, and would rather not go through the explanation of why it's safe here again. In `verify_token()` there was a timing-variable, byte-by-byte comparison of `self.last_token` (a Journalist field corresponding to the last token a Journalist attempted to login with) with the submitted token. If `self.last_token` was a valid login token, and assuming best possible circumstances (client time was skewed ahead of server and attacker immediately starts guessing after it's entered, giving a >60s window of validity, timing information resolution is perfect, etc.) it can be exploited to increase the probability of an attacker guessing a correct token by ~2.2 times during that period: ``` >>> p = (10 / float(10 ** 6)) >>> (sum([9 / (float(10 ** (6 - n)) * float(10 ** n)) for n in range(5)]) + (2 * p)) / (3 * p) 2.1666666666666665 ``` Still, when you look at the actual value, it's pretty insignificant: ``` >>> sum([9 / (float(10 ** (6 - n)) * float(10 ** n)) for n in range(5)]) + (2 * p) 6.500000000000001e-05 ``` Although the information gain is almost nothing here even under the best-case scenario, we're again going to patch this one with a constant-time comparison just to avoid bug reports in the future, and play it extra safe.
The intention of the orignal implementation of the `last_token` attribute was to prevent a 2FA token from being re-used as it might be in some MitM or shoulder-surfing attacks. The problem with it was that it always saved the last token entered instead of the last valid token. Thus if attacker A watches Journalist J use token T, they could attempt to login using T', a random token, and then successfully login on their next attempt using T as `last_token` will have been set to T'. This is resolved by updating `last_token` only for valid tokens.
psivesely
force-pushed
the
timing-and-token-reuse
branch
from
eaec341
to
6e1e652
Compare
|
Hmm, builds seem to be reliably failing on Travis on this - you didn't see any test failures locally? |
|
Also seeing 22 failures locally, same as in Travis. |
|
|
|
We cannot trivially upgrade the version of Python running on SecureDrop instances, so we'll need to consider another path. Worth nothing that #1530 considers upgrading the Trusty hosts to Xenial, which would get us newer versions. |
:meth:`hmac.compare_digest()` is not available until Python 2.7.7, which has not landed in Ubuntu Trusty yet (still on 2.7.6). This function will hold us over until then, and will automatically switch us over to `compare_digest()` when it becomes available.
|
Re-posting from Slack:
Also, re-opening because I just replaced |
added 2 commits
April 26, 2017 15:25
Just like my implementation this is replacing, if :meth:`hmac.compare_digest()` is available in the running Python version, it will be used. Thus, we need not worry about "upgrading" this function in the future should our distribution include a new version of Python.
|
Closing for now because I know this won't get considered for merge for a really long time and will be out of date by then. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
Ready for review.
Description of Changes
Eliminates some timing leaks and prevents the same valid token from being used to authenticate twice in a row.
Checklist
If you made changes to the app code:
If you made changes to the system configuration: