Martin v. Löwis mar...@v.loewis.de added the comment:
I recommend to revert the addition of this function. It's not possible to
implement a time-independent comparison function, as demonstrated in issues
14955 and 15061
--
nosy: +loewis
___
Python
Antoine Pitrou pit...@free.fr added the comment:
How is it not possible? The implementation may be buggy, but it's possible to
write a C version that does the right thing.
--
___
Python tracker rep...@bugs.python.org
Changes by Hynek Schlawack h...@ox.cx:
--
nosy: +hynek
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue14532
___
___
Python-bugs-list mailing list
Nick Coghlan ncogh...@gmail.com added the comment:
A comment above the length check referring back to this issue and the
deliberate decision to allow a timing attack to determine the length of the
expected digest would be handy.
I was just looking at hmac.secure_compare and my thought when
Charles-François Natali neolo...@free.fr added the comment:
Committed.
Jon, thanks for your patch and your patience!
--
resolution: - fixed
stage: commit review - committed/rejected
status: open - closed
___
Python tracker rep...@bugs.python.org
Roundup Robot devn...@psf.upfronthosting.co.za added the comment:
New changeset ddcc8ee680d7 by Charles-François Natali in branch 'default':
Issue #14532: Add a secure_compare() helper to the hmac module, to mitigate
http://hg.python.org/cpython/rev/ddcc8ee680d7
--
nosy: +python-dev
STINNER Victor victor.stin...@gmail.com added the comment:
However, this generally is not a security risk.
You should explain what you already said: it is not a risk because the
length of a HMAC is fixed.
--
___
Python tracker
Jon Oberheide j...@oberheide.org added the comment:
You should explain what you already said: it is not a risk because the
length of a HMAC is fixed.
Well, that's not entirely accurate. Exposing the length of the HMAC can expose
what underlying hash is being used (eg. HMAC-SHA1 has different
Jon Oberheide j...@oberheide.org added the comment:
Ok, patch v4 uploaded. Only change is the rename to secure_compare.
--
Added file: http://bugs.python.org/file25414/hmac-time-independent-v4.patch
___
Python tracker rep...@bugs.python.org
Changes by Charles-François Natali neolo...@free.fr:
--
stage: - commit review
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue14532
___
___
Charles-François Natali neolo...@free.fr added the comment:
I have used the name secure_compare in the past for such a function. That
said, I don't have strong feelings either way about the naming, so I'll
yield to the others.
I prefer this name too.
Wait one day or two (to let others chime
Charles-François Natali neolo...@free.fr added the comment:
v3 patch, based on feedback from the review here:
http://bugs.python.org/review/14532/show
Looks good to me.
One last thing (sorry for not bringing this up earlier): I don't like
bikeshedding, but at least to me,
Jon Oberheide j...@oberheide.org added the comment:
I have used the name secure_compare in the past for such a function. That
said, I don't have strong feelings either way about the naming, so I'll yield
to the others.
--
___
Python tracker
Jon Oberheide j...@oberheide.org added the comment:
v3 patch, based on feedback from the review here:
http://bugs.python.org/review/14532/show
--
Added file: http://bugs.python.org/file25262/hmac-time-independent-v3.patch
___
Python tracker
STINNER Victor victor.stin...@gmail.com added the comment:
+def time_independent_equals(a, b):
+if len(a) != len(b):
+return False
This is not time independent. Is it an issue?
+if type(a[0]) is int:
It's better to write isinstance(a, bytes). You should raise a
TypeError if a
Jon Oberheide j...@oberheide.org added the comment:
This is not time independent. Is it an issue?
You're correct, the length check does leak the length of the expected digest as
a performance enhancement (otherwise, your comparison runtime is bounded by the
length of the attackers input).
Antoine Pitrou pit...@free.fr added the comment:
You could rewrite:
result |= x ^ y
as:
result |= (x != y)
Of course, this assumes that the != operator is constant-time for 1-element
strings.
--
nosy: +pitrou
___
Python tracker
Jon Oberheide j...@oberheide.org added the comment:
You could rewrite:
result |= x ^ y
as:
result |= (x != y)
You could, but it's best not to introduce any conditional branching based if at
all possible. For reference, see:
sbt shibt...@gmail.com added the comment:
Why not just
def time_independent_equals(a, b):
return len(a) == len(b) and sum(x != y for x, y in zip(a, b)) == 0
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue14532
Jon Oberheide j...@oberheide.org added the comment:
Here's a v2 patch. Changes include checking the input types via isinstance,
test cases to exercise the type checking, and a note documenting the leak of
the input length.
--
Added file:
Changes by Charles-François Natali neolo...@free.fr:
--
nosy: +haypo
status: open -
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue14532
___
___
Changes by Charles-François Natali neolo...@free.fr:
--
status: - open
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue14532
___
___
STINNER Victor victor.stin...@gmail.com added the comment:
if response == digest:
can be replaced by:
if sum(x^y for x, y in itertools.zip_longest(response, digest,
fillvalue=256)) == 0:
I hope that zip_longest() does not depend too much on response and digest.
--
Charles-François Natali neolo...@free.fr added the comment:
if response == digest:
can be replaced by:
if sum(x^y for x, y in itertools.zip_longest(response, digest,
fillvalue=256)) == 0:
Yeah, sure, but is it useful at all?
The digest changes at every connection attempt, so this should
Jon Oberheide j...@oberheide.org added the comment:
In fact, it'd probably be useful to have a time_independenct_comparison()
helper function somewhere in general.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue14532
Jon Oberheide j...@oberheide.org added the comment:
Ah yeah, I suppose it's not be exploitable in this case due to the challenge
nonce.
However, it might still be a good thing to fix for to set an example for other
hmac module users (internal or external) that might not have the same
Charles-François Natali neolo...@free.fr added the comment:
I don't see the point of obfuscating the code to avoid a vulnerability
to which the code is not even vulnerable, just so that it can be used
as example...
There are *thousands* of ways to introduce security flaws, and the
Python code
Jon Oberheide j...@oberheide.org added the comment:
You call it obfuscating, I call it security correctness and developer
education. Tomayto, tomahto. ;-)
Anywho, your call of course, feel free to close.
--
___
Python tracker rep...@bugs.python.org
Charles-François Natali neolo...@free.fr added the comment:
You call it obfuscating, I call it security correctness and developer
education. Tomayto, tomahto. ;-)
Well, I'd be prompt to changing to a more robust digest check
algorithm if the current one had a flaw, but AFAICT, it's not the
sbt shibt...@gmail.com added the comment:
I think it would be reasonable to add a safe comparison function to hmac.
Its documentation could explain briefly when it would be preferable to ==.
--
___
Python tracker rep...@bugs.python.org
R. David Murray rdmur...@bitdance.com added the comment:
It would also be reasonable to add a comment to the code mentioning why this
particular (security) comparison is *not* vulnerable to a timing attack, which
would serve the education purpose if someone does look at the code.
--
Jon Oberheide j...@oberheide.org added the comment:
One thing that could definitely be interesting is to look through the
code base and example to see if a similar - but vulnerable - pattern
is used, and fix such occurrences.
Based on some quick greps, I didn't see many internal users of
Charles-François Natali neolo...@free.fr added the comment:
Given that this issue has affected a lot of security-sensitive third-party
code (keyczar, openid providers, almost every python web service that
implements secure cookies [1] or other HMAC-based REST API signatures), I
do like
Jon Oberheide j...@oberheide.org added the comment:
Will do!
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue14532
___
___
Python-bugs-list
Jon Oberheide j...@oberheide.org added the comment:
Here's a v1. Works with both str and bytes types for Python 3.x.
Not sure I'm completely happy with the docs, but I'd appreciate any feedback on
them!
--
keywords: +patch
Added file:
Charles-François Natali neolo...@free.fr added the comment:
Therefore the expected digest changes each time.
Exactly.
Timing attacks (which aren't really new :-) depend on a constant digest to
successively determine the characters composing the digest. Here, that won't
work, because the
sbt shibt...@gmail.com added the comment:
I only looked quickly at the web pages, so I may have misunderstood.
But it sounds like this applies when the attacker gets multiple chances to
guess the digest for a *fixed* message (which was presumably chosen by the
attacker).
That is not the case
New submission from Jon Oberheide j...@oberheide.org:
The multiprocessing module performs a time-dependent comparison of the HMAC
digest used for authentication:
def deliver_challenge(connection, authkey):
import hmac
assert isinstance(authkey, bytes)
message =
Changes by Antoine Pitrou pit...@free.fr:
--
nosy: +sbt
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue14532
___
___
Python-bugs-list mailing list
39 matches
Mail list logo
