Don't reveal string length to attacker. #28
Merged
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
| for c1, c2 in zip(s1, s2): | ||
| for c1, c2 in izip_longest(s1, s2): | ||
| if c1 is None or c2 is None: | ||
| pass |
There was a problem hiding this comment.
This line looks odd - pass will fallthrough to the next line, and fail when xoring None. e.g:
>>> None ^ 1
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for ^: 'NoneType' and 'int'There was a problem hiding this comment.
Noted. Will change pass to continue.
|
Nice - created a Gist to test the code with |
|
There is also a version which makes it do almost as much "work" when the smaller string runs out of characters: def strings_equal(s1, s2):
differences = 0
for c1, c2 in izip_longest(s1, s2):
if c1 is None or c2 is None:
differences |= ord('a') ^ ord('b')
else:
differences |= ord(c1) ^ ord(c2)
return differences == 0But I'm not sure if it's just over-engineering. |
|
Yes, my guess is that's unnecessary. Nathan's benchmark shows the improvement clearly, LGTM |
|
For reference, this is the stdlib implementation: http://bugs.python.org/issue19259. |
|
@kislyuk The standard library version and the one proposed is largely equivalent IMHO. |
Closed
psivesely
pushed a commit
to freedomofpress/securedrop
that referenced
this pull request
Aug 31, 2016
Normally, we're hesistant to issue an update for dependencies when we've already
entered the release candidate(s) stage of the release process. In this case, the
changes I'm adding are all minor bug fixes that I've reviewed. Two of the fixes
were labeled as security issues, however, they don't really affect us as
explained below.
* Werkzeug
* A bug that allowed XSS attacks on the debug page has been fixed (we
don't run Flask in debug mode in production) -
pallets/werkzeug#1001
* Invalid Content-Type makes for parsing throw ValueError exception (the fix
returns an invalid request 400 Bad Request page instead of an internal
server error when the content-type field of a HTTP request is bad--such as
' ' or ',') - pallets/werkzeug#995
* Raise BadRequestKeyError instead of IndexError in MultiDict when calling
__getitem__ on a key with an empty associated list of values (Flask returns
forms and query strings as MultiDicts. This is just better error-handling,
no real bug being fixed here.) -
pallets/werkzeug#979
* pytop
* The string comparison function now no longer leaks string length (shouldn't
affect SD because the length of our TOTP codes are already known) -
pyauth/pyotp#28
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.
Instead of terminating at the shortest string, terminate at the longest string.