Roundup Robot devn...@psf.upfronthosting.co.za added the comment:
New changeset c82451eeb595 by Christian Heimes in branch 'default':
Issue #15061: Re-implemented hmac.compare_digest() in C
http://hg.python.org/cpython/rev/c82451eeb595
--
___
Python
Christian Heimes li...@cheimes.de added the comment:
Thanks to all for your input and assistance!
--
resolution: - fixed
stage: commit review - committed/rejected
status: open - closed
___
Python tracker rep...@bugs.python.org
Christian Heimes li...@cheimes.de added the comment:
Updated patch with volatile, better error report for non-ASCII strings and
updated comments
--
Added file: http://bugs.python.org/file26106/timingsafe_cmp-2.patch
___
Python tracker
Antoine Pitrou pit...@free.fr added the comment:
I'm not really happy with the addition of a separate extension module for a
single private function. You could just put it in the operator module, for
instance.
Also, the idea was not to expose timingsafe_cmp but to use it in
compare_digest().
Christian Heimes li...@cheimes.de added the comment:
Me neither but you didn't want it in the operator module in the first place
(msg162882). :) Please make a decision. I'm happy to follow it.
My idea is to drop the pure Python implementation of compare_digest() and just
use the C
Antoine Pitrou pit...@free.fr added the comment:
Me neither but you didn't want it in the operator module in the first
place (msg162882). :) Please make a decision. I'm happy to follow it.
Oh, sorry. I've changed my mind about it, but I think the operator module
should only export a private
Georg Brandl ge...@python.org added the comment:
Doesn't belong into operator IMO. We used to have a strop module where it
would have fitted...
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
Nick Coghlan ncogh...@gmail.com added the comment:
This is why I wanted to close the issue with the pure Python implementation,
and punt on the question of a C accelerator for the moment.
compare_digest is effectively the same as what all the Python web servers and
frameworks do now for
Antoine Pitrou pit...@free.fr added the comment:
Doesn't belong into operator IMO. We used to have a strop module
where it would have fitted...
Again, it can be a private function in the operator module that happens
to be wrapped or exposed in the hmac module. Practicality beats purity.
Georg Brandl ge...@python.org added the comment:
Yes.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
___
___
Python-bugs-list mailing list
Christian Heimes li...@cheimes.de added the comment:
Again, it can be a private function in the operator module that happens
to be wrapped or exposed in the hmac module. Practicality beats purity.
Yes, we just need a place for the function. The operator module is a
good place if we don't want
Hynek Schlawack h...@ox.cx added the comment:
For 3.4, I hope to see a discussion open up regarding the idea of something
like a securitytools module that aims to provide some basic primitives for
operations where Python's standard assumptions (such as flexibility and short
circuiting
Christian Heimes li...@cheimes.de added the comment:
New patch. The compare_digest method now lives in the operator module as
operator._compare_digest
--
Added file: http://bugs.python.org/file26112/compare_digest_c.patch
___
Python tracker
Martin v. Löwis mar...@v.loewis.de added the comment:
About code. Instead (PyBytes_CheckExact(a) PyBytes_CheckExact(b)) you
should use ((PyBytes_CheckExact(a) != 0) (PyBytes_CheckExact(b) !=
0)).
What's the difference? They are the same.
Laziness. If a (a secret key) is not bytes then
Changes by Gregory P. Smith g...@krypto.org:
--
nosy: +gregory.p.smith
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
___
___
Christian Heimes li...@cheimes.de added the comment:
New patch.
I've removed the special handling of PyBytes_CheckExact, support subclasses of
str, non compact ASCII str and updated the docs.
(Next time I'll create a sandbox and push my work to its own branch.)
--
Added file:
Serhiy Storchaka storch...@gmail.com added the comment:
We could handle all bytes-compatible objects, using the buffer API.
It is timing unsafe.
How so?
I checked myself, and I see that most likely I was wrong. At least for
bytes and bytearrays it is timing safe.
I don't think that's
Christian Heimes li...@cheimes.de added the comment:
The error will be if code works for developer from ASCII word, and then
on the other side of ocean it will no longer work with non-ASCII
strings. You are expected to be familiar with such issues. In any case,
the obvious (and simplest, and
Christian Heimes li...@cheimes.de added the comment:
In order to get the patch in before the beta release I'm willing to drop the
promise and document that the function may leak some information if the
arguments differ in length or the arguments' types are incompatible.
--
Antoine Pitrou pit...@free.fr added the comment:
In order to get the patch in before the beta release I'm willing to
drop the promise and document that the function may leak some
information if the arguments differ in length or the arguments' types
are incompatible.
That's not a problem to
Antoine Pitrou pit...@free.fr added the comment:
Shall explore option 2b) optionally create a C implementation as it's much
easier to check C code for timing issues
Definitely. I'm not sure whether that can go in 3.3 post-beta, though.
--
___
Maciej Fijalkowski fij...@gmail.com added the comment:
Hi.
This is what we did with Armin: http://bpaste.net/show/32123/
It seems there is still *some* information leaking via side-channels, although
it's a bit unclear what. Feel free to play with it (try swapping, having
different object
Christian Heimes li...@cheimes.de added the comment:
I've attached a header for that implements a single C function timingsafe_eq(a,
b). The file is targeted for Objects/stringlib/timingsafe.h. Please review the
file.
Comments
- I only handle exact byte or unicode types (no
Antoine Pitrou pit...@free.fr added the comment:
The file is targeted for Objects/stringlib/timingsafe.h.
stringlib is for type-generic functions, so I don't think it should be
put there.
- I only handle exact byte or unicode types (no subclasses) since a
user may have overwritten __eq__
Serhiy Storchaka storch...@gmail.com added the comment:
- I only handle exact byte or unicode types (no subclasses) since a
user may have overwritten __eq__ and I don't want to special case it.
We could handle all bytes-compatible objects, using the buffer API.
It is timing unsafe.
-
Martin v. Löwis mar...@v.loewis.de added the comment:
The user can just do timingsafe_eq(a.decode('ascii'),
b.decode('ascii')).
You mean .encode()?
I do not see a necessity in support of unicode
strings. Support ASCII strings will create the false impression that all
strings are
Serhiy Storchaka storch...@gmail.com added the comment:
You mean .encode()?
Yes, of cause. timingsafe_eq(a.encode('ascii'), b.encode('ascii')).
About code. Instead (PyBytes_CheckExact(a) PyBytes_CheckExact(b)) you
should use ((PyBytes_CheckExact(a) != 0) (PyBytes_CheckExact(b) !=
0)).
Antoine Pitrou pit...@free.fr added the comment:
- I only handle exact byte or unicode types (no subclasses) since a
user may have overwritten __eq__ and I don't want to special case it.
We could handle all bytes-compatible objects, using the buffer API.
It is timing unsafe.
How so?
Christian Heimes li...@cheimes.de added the comment:
I'm a bit rusty and I hope I got it right. The ASCII unicode case is a good
idea and IMO timing safe. The buffer path is also timing safe once I have both
views.
The function leaks some timing information when an error occurs. Since the
Christian Heimes li...@cheimes.de added the comment:
The patch has another flaw. The compiler may choose to fold and optimize code
in _tscmp(). I'm going to declare the length of the right side and both char*
as volatile. That should stop any compiler.
I could also add some pragmas:
MSVC:
Christian Heimes li...@cheimes.de added the comment:
I've increased the priority to release blocker.
Reason:
We should come to an agreement how to handle the issue. In particular we must
not pronounce something as secure that isn't secure.
Options:
1) Remove the function.
2) Rename the
Hynek Schlawack h...@ox.cx added the comment:
I thought this is settled as of f36af3766a20 (option 2)?
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
___
Christian Heimes li...@cheimes.de added the comment:
Oh, I totally missed Nick's checkin. Sorry for the noise.
Should we add the encode('unicode-internal') trick from #14955 as the next best
way to compare to unicode strings?
--
stage: needs patch - commit review
Antoine Pitrou pit...@free.fr added the comment:
Should we add the encode('unicode-internal') trick from #14955 as the next
best way to compare to unicode strings?
I don't think so.
--
___
Python tracker rep...@bugs.python.org
Changes by Alex Gaynor alex.gay...@gmail.com:
--
nosy: +alex
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
___
___
Python-bugs-list mailing
Serhiy Storchaka storch...@gmail.com added the comment:
Unicode string timing depends on the string implementation which depends on the
maximum character code in the string. Strings 'A'*+'$' 'A'*+'€' have
different timings for almost all operations (inluding
Serhiy Storchaka storch...@gmail.com added the comment:
Oh, I see, Antoine said the same thing (msg162771).
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
___
Christian Heimes li...@cheimes.de added the comment:
I'm well aware of the fact that they have different timings. That's why I
argued against including a unicode aware variant of the timing safe compare
function.
I've used Guido's time machine and seen requests for a unicode function in the
Antoine Pitrou pit...@free.fr added the comment:
I'm well aware of the fact that they have different timings. That's
why I argued against including a unicode aware variant of the timing
safe compare function.
I would not want to repeat myself, but the compare function can be made
safe if it
Christian Heimes li...@cheimes.de added the comment:
Alright, Antoine.
Shall explore option 2b) optionally create a C implementation as it's much
easier to check C code for timing issues as I suggested in
http://bugs.python.org/issue15061#msg162893 ?
--
Georg Brandl ge...@python.org added the comment:
So it's not a blocker anymore, right?
--
nosy: +georg.brandl
priority: release blocker - normal
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
Hynek Schlawack h...@ox.cx added the comment:
Secure vs not secure is not a binary state - it's about making attacks
progressively more difficult. Something that is secure against a casual
script kiddie scatter gunning attacks on various sites with an automated
script won't stand up to a
Martin v. Löwis mar...@v.loewis.de added the comment:
On 14.06.2012 14:26, Antoine Pitrou wrote:
Antoine Pitrou pit...@free.fr added the comment:
It's either secure or it's not.
I don't think that's true. By that reasoning, Python is not secure so
there's no point in fixing crashes or
Martin v. Löwis mar...@v.loewis.de added the comment:
Being able to tell people using hmac.total_compare will make you
less vulnerable to timing attacks than using ordinary short
circuiting comparisons is a *good thing*.
No, it's not. It's a *bad thing*. The two issues that have been
opened
Martin v. Löwis mar...@v.loewis.de added the comment:
Why not write a C function which can be more secure than Python code?
For Unicode strings, it's impossible to write a time-independent
comparison function even in C
I would argue that would be an general asset for the stdlib
I would
Hynek Schlawack h...@ox.cx added the comment:
Why not write a C function which can be more secure than Python code?
For Unicode strings, it's impossible to write a time-independent
comparison function even in C
Really? Some comments sounded different. That's too bad but also what I
Nick Coghlan ncogh...@gmail.com added the comment:
Can people please stop raising a false dichotomy and using that as an excuse
not to do anything?
The decision is not between leak some information and leak no information.
It is between leak more information and leak less information.
The
Martin v. Löwis mar...@v.loewis.de added the comment:
Well, one example:
https://github.com/mitsuhiko/python-pbkdf2/blob/master/pbkdf2.py
It says that it needs that, but I fail to understand why.
pbkdf2 is used to generate encryption keys from passwords, where
you don't need to compare
Nick Coghlan ncogh...@gmail.com added the comment:
To repeat, the specific feature being proposed for retention is:
* a function called hmac.total_compare() that is clearly documented as being
still vulnerable to timing analysis given a sufficiently sophisticated
attacker, while still being
Martin v. Löwis mar...@v.loewis.de added the comment:
The timing variations with standard comparison are relatively massive
and relatively easy to analyse (if the time taken goes up, you got
the previous digit correct).
If you have an application that is vulnerable to such an attack, you
Martin v. Löwis mar...@v.loewis.de added the comment:
To repeat, the specific feature being proposed for retention is:
To repeat, no use case has been demonstrated for that function. It
has been added because it was fun to write, not because it is useful.
--
Maciej Fijalkowski fij...@gmail.com added the comment:
On Fri, Jun 15, 2012 at 9:41 AM, Nick Coghlan rep...@bugs.python.orgwrote:
Nick Coghlan ncogh...@gmail.com added the comment:
To repeat, the specific feature being proposed for retention is:
* a function called hmac.total_compare()
Maciej Fijalkowski fij...@gmail.com added the comment:
On Fri, Jun 15, 2012 at 9:47 AM, Martin v. Löwis rep...@bugs.python.orgwrote:
Martin v. Löwis mar...@v.loewis.de added the comment:
To repeat, the specific feature being proposed for retention is:
To repeat, no use case has been
Hynek Schlawack h...@ox.cx added the comment:
and any other place that compares passwords, tokens, …
No no no. Any sensible place to compare passwords would use some
sort of one-way function (password hash) before the comparison,
so that someone breaking into the machine will not gain the
Nick Coghlan ncogh...@gmail.com added the comment:
I'm not really opposed to writing it in C - I just don't think rewriting it in
C should be a requirement for keeping it. Even in pure Python, it still leaks
less information than the standard comparison operator.
--
Maciej Fijalkowski fij...@gmail.com added the comment:
On Fri, Jun 15, 2012 at 9:55 AM, Hynek Schlawack rep...@bugs.python.orgwrote:
Hynek Schlawack h...@ox.cx added the comment:
and any other place that compares passwords, tokens, …
No no no. Any sensible place to compare passwords
Martin v. Löwis mar...@v.loewis.de added the comment:
Is comparing passwords against a secure one not useful?
I claim that this use case doesn't occur in practice. Everybody uses
hashed passwords. If they do compare against a plain-text password,
and they want to change something about it,
Nick Coghlan ncogh...@gmail.com added the comment:
This point was discussed in #14532 when the new API was added.
From http://bugs.python.org/issue14532#msg158045:
Given that this issue has affected a lot of security-sensitive third-party
code (keyczar, openid providers, almost every python
Martin v. Löwis mar...@v.loewis.de added the comment:
I see your point that adding such a function would leverage bad
security behavior and thus may be a bad thing. The usefulness of such
a function to some(?) people is IMHO not disputable though.
I think this entire issue is out of scale.
Martin v. Löwis mar...@v.loewis.de added the comment:
Note that this does not relief you from using a time-independent comparison
function. If you call some hash function (which time is known to the
attacker), then you compare it against a stored hashed version. If you use
a normal compare
Maciej Fijalkowski fij...@gmail.com added the comment:
On Fri, Jun 15, 2012 at 10:09 AM, Martin v. Löwis rep...@bugs.python.orgwrote:
Martin v. Löwis mar...@v.loewis.de added the comment:
Note that this does not relief you from using a time-independent
comparison
function. If you call
Martin v. Löwis mar...@v.loewis.de added the comment:
For password hashing, the attacker is unlikely to be able to provide
the digest directly, but for signature checking it's far more likely
to be the case.
Can you elaborate? What is the application, where is the digest
checking, and what
Martin v. Löwis mar...@v.loewis.de added the comment:
Martin, you fail to understand how this works. You don't do 2**32 tries to
leak the 4 charaters, you need 4 * 256, that's why this attack is so bad,
because the time needed for the next character is brute force, but then you
can move on
Nick Coghlan ncogh...@gmail.com added the comment:
That's why the vulnerable cases are far more likely to be related to
*signature* checking. In those you can generally provide both the hash input
(the message) and the hash target (the purported signature).
If the signature check uses a
Petri Lehtinen pe...@digip.org added the comment:
For example, Django uses time independent comparison to compare signatures of
signed cookies. A signed cookie consists of a plain-text value followed by a
signature.
An attacker wants to construct a cookie that has a malformed value and a
Nick Coghlan ncogh...@gmail.com added the comment:
FWIW, Petri's example also explains why leaking the expected length of the
string is considered an acceptable optimisation in most reimplementations of
this signature check comparison: the attacker is assumed to already know the
expected
Martin v. Löwis mar...@v.loewis.de added the comment:
That's why the vulnerable cases are far more likely to be related to
*signature* checking. In those you can generally provide both the
hash input (the message) and the hash target (the purported
signature).
I see. I wonder how feasible
Changes by Armin Rigo ar...@users.sourceforge.net:
--
nosy: -arigo
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
___
___
Python-bugs-list
Christian Heimes li...@cheimes.de added the comment:
Oh dead god, what have I done ... I threw a small stone and caused a major
landslide. :)
I'm all with Nick on this topic. A correctly named and documented function
provides a tool to users that greatly reduced the change of a side channel
Antoine Pitrou pit...@free.fr added the comment:
I could wrap up a quick C implementation if you like. The operator
module is a better place for a total_compare() function. Do you a
agree?
I think the function is fine in either hashlib or hmac. Putting it in
one of these modules is a hint
Nick Coghlan ncogh...@gmail.com added the comment:
As a first step, I'm going to make a change to:
1. Rename the function to compare_digest
2. Remove the support for comparing strings
3. Update the documentation to be much clearer about its limitations (including
why it's considered OK to leak
Roundup Robot devn...@psf.upfronthosting.co.za added the comment:
New changeset f36af3766a20 by Nick Coghlan in branch 'default':
Issue #15061: Don't oversell the capabilities of the new non-shortcircuiting
comparison function in hmac
http://hg.python.org/cpython/rev/f36af3766a20
--
Nick Coghlan ncogh...@gmail.com added the comment:
OK, the worst aspects (the misleading name and documentation) have been dealt
with, so that leaves the questions of:
1. Avoiding leaking the length information (seems unnecessary, since most
digests are part of protocols where they have a
Antoine Pitrou pit...@free.fr added the comment:
2. Providing a C implementation via the operator module (given the
restriction to bytes values, and the assumption of caching for all
relevant integers, would a C reimplementation really be buying us much
additional security?)
I like the fact
Christian Heimes li...@cheimes.de added the comment:
Am 15.06.2012 14:21, schrieb Antoine Pitrou:
I like the fact that a C implementation can be audited much more easily.
Who knows what kind of effects the Python implementation can trigger, if
some optimizations get added in the future.
Antoine Pitrou pit...@free.fr added the comment:
The point of supporting unicode would precisely be to avoid a
unicode-bytes conversion when unicode strings are received.
A byte-wise comparison of the memory representation would work IFF both
sides have the same type and unicode kind.
Nick Coghlan ncogh...@gmail.com added the comment:
(Ah, the dangers of using a real text editor for edit fields. This got rather
long, but I think it's all still relevant)
I'm persuaded that a C implementation is a good idea in the long run. However,
I *don't* think we should rush the design
Antoine Pitrou pit...@free.fr added the comment:
Secondly, it seems to me that the proposed lower level feature may
make more sense as a bytes method rather than as a function in the
operator module.
If it's a function, though, it can compare all kinds of buffer-like
objects (bytearrays,
Jon Oberheide j...@oberheide.org added the comment:
Wow, that escalated quickly. :-)
Nick, thanks for keeping things focused and on track.
To recap, the primary motivation here is two-fold. First, folks are using ==
pretty frequently in an unsafe manner when comparing digests, signatures, and
Jon Oberheide j...@oberheide.org added the comment:
On a side note, it may be useful to follow the conventions that already exist
in OpenBSD for their timingsafe_bcmp(3):
http://www.rootr.net/man/man/timingsafe_bcmp/3
timingsafe may be a more reasonable naming convention that is a bit less
Christian Heimes li...@cheimes.de added the comment:
I don't see how the function is going to leak this information when both this
patch and the patch in #14955 are applied. With
http://bugs.python.org/file25801/secure-compare-fix-v2.patch ord() is no longer
used and thus avoid the timing
Maciej Fijalkowski fij...@gmail.com added the comment:
Ah unicodes. is encode('unicode-internal') independent on the string
characters? I heavily doubt so. you leak at least some information through that
function alone.
--
___
Python tracker
Antoine Pitrou pit...@free.fr added the comment:
With PEP 393 unicode objects can have several representations, which makes it
unlikely that *really* constant-timing functions can be devised.
Speaking about this particular patch, I don't understand the point.
secure_compare() is obviously
Christian Heimes li...@cheimes.de added the comment:
IMHO it's not obvious to all users. Better safe than sorry. ;)
The invariant 'known and equal length' impresses an artificial limitation. Code
may need to compare outside data with internal data without exposing too many
details about the
Hynek Schlawack h...@ox.cx added the comment:
I don’t want to be the killjoy but I find it highly questionable to add a
function that is advertised as secure while we can't fully grok the
complexities at play. If we can't produce a provable secure one, we should
scrub the function for good;
Antoine Pitrou pit...@free.fr added the comment:
I don’t want to be the killjoy but I find it highly questionable to
add a function that is advertised as secure while we can't fully
grok the complexities at play. If we can't produce a provable secure
one, we should scrub the function for
Maciej Fijalkowski fij...@gmail.com added the comment:
Antoine, seriously? You want to explore a function that's called secure when
the only thing you know about it is probably secure? This is extremely tricky
business and I think it should be called secure only if you can prove it's
secure.
Maciej Fijalkowski fij...@gmail.com added the comment:
export not explore. Why can't I edit my own post?
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
___
Antoine Pitrou pit...@free.fr added the comment:
Antoine, seriously? You want to explore a function that's called
secure when the only thing you know about it is probably secure?
This is extremely tricky business and I think it should be called
secure only if you can prove it's secure.
Maciej Fijalkowski fij...@gmail.com added the comment:
For unicode at the very least it's not an improvement at all. With the patch
mentioned that does encode it's also not an improvement at all. Prove as in
reason about the function in C and make sure it does not do any conditionals
Martin v. Löwis mar...@v.loewis.de added the comment:
I recommend to revert the addition of the function, given that it can't be made
secure.
--
nosy: +loewis
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15061
Christian Heimes li...@cheimes.de added the comment:
I've two suggestions:
* rename the function to 'total_compare'. The name explains what the function
actually does in comparison to '=='. It takes the total input values into
account instead of using short circuit comparison.
* restrict the
Maciej Fijalkowski fij...@gmail.com added the comment:
Hi Christian. It's either secure or it's not. If it's not, there is no point in
introducing it at all as I don't think it's a good idea to have a
kind-of-secure-but-i-dont-know functions in stdlib.
If you restrict input to bytes it looks
Antoine Pitrou pit...@free.fr added the comment:
It's either secure or it's not.
I don't think that's true. By that reasoning, Python is not secure so there's
no point in fixing crashes or providing a hashlib module.
That said, I think renaming to total_compare isn't really helpful. The
Nick Coghlan ncogh...@gmail.com added the comment:
Maciej, please read http://mjg59.dreamwidth.org/13061.html
Secure vs not secure is not a binary state - it's about making attacks
progressively more difficult. Something that is secure against a casual script
kiddie scatter gunning attacks on
95 matches
Mail list logo
