-
-
Notifications
You must be signed in to change notification settings - Fork 324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't reveal string length to attacker. #28
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 == 0 But 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. |
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
Instead of terminating at the shortest string, terminate at the longest string.