classification
Title: PEP 466: backport hmac.compare_digest
Type: enhancement Stage: resolved
Components: Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: alex, benjamin.peterson, christian.heimes, dstufft, giampaolo.rodola, janssen, ncoghlan, pitrou, python-dev, smurfix
Priority: normal Keywords: needs review, patch

Created on 2014-04-19 00:53 by ncoghlan, last changed 2014-05-31 15:14 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
compare_digest.diff alex, 2014-04-22 22:06 review
compare_digest.diff alex, 2014-04-30 20:47 review
Messages (14)
msg216826 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-04-19 00:53
Tracker issue for the hmac.compare_digest backport to 2.7 described in PEP 466.
msg217026 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-04-22 21:27
Design question here: compare_digest on Python 3 supports comparing str (text) objects, if they're both ascii-only. This feature is provided, primarily, so you can compare hexdigests or similar.

Should the Python 2 version support comparing unicodes? Arguments in favor: some amount of consistency. Against: it's not necessary because hexdigest is still a str (binary), further it's not actually posisble to replicate the ascii only semantic.
msg217027 - (view) Author: Donald Stufft (dstufft) * (Python committer) Date: 2014-04-22 21:30
try:
    data = data.encode("ascii")
except UnicodeEncodeError:
    raise TypeError("comparing unicode with non-ASCII characters is not supported")

?
msg217028 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-04-22 21:30
8-bit str only makes more sense to me. The wishy-washiness of some APIs in
Py3 is mostly to work around porting issues where stuff that should have
become bytes was left as str.
msg217029 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-04-22 21:31
encode("ascii") has data dependent branches, so it's to be avoided.
msg217030 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-04-22 21:31
Thanks Nick. I'll get a patch up for str (bytes) only this afternoon.
msg217031 - (view) Author: Donald Stufft (dstufft) * (Python committer) Date: 2014-04-22 21:40
I'm not sure that the timing leakage in an encode is actually something to be worried about. I'm not sure what secret information would be getting leaked in a way that you could determine it by examining the timing.

However I think the bigger thing is if I'm an app developer and I attempt to pass a unicode to hmac.compare_digest() and it tells me it only accepts bytes, the first thing I'm going to do is is .encode() it myself before I pass it in.

IOW hmac.compare_digest could avoid the encode, but it's just pushing that back up to the user of hmac.compare_digest, who might possibly have a byte string laying around that they won't have to do the encode/decode dance on (although if they have a unicode they have already done it at least once), or they only have a unicode available to them then they'll be forced to do the encode themselves.
msg217035 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-04-22 22:06
Attached patch implements compare_digest. Code is mostly a 1-1 from 3.x, except the Unicode paths are changed, and the tests are a tiny bit different.

* Still needs to backport the docs.
* Compares all unicode objects, not just ascii ones.

If the patch looks good to folks I'll add the docs as well.
msg217649 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-04-30 20:47
Attached patch now includes documentation and should be complete.
msg218241 - (view) Author: Donald Stufft (dstufft) * (Python committer) Date: 2014-05-10 23:37
The attached patch looks good to me.
msg218304 - (view) Author: Roundup Robot (python-dev) Date: 2014-05-11 23:11
New changeset b40f1a00b134 by Benjamin Peterson in branch '2.7':
backport hmac.compare_digest to partially implement PEP 466 (closes #21306)
http://hg.python.org/cpython/rev/b40f1a00b134
msg219428 - (view) Author: Matthias Urlichs (smurfix) * Date: 2014-05-30 21:38
Currently (Debian's 2.7.7-rc1 package) hmac.compare_digest accepts two bytestring arguments, or two Unicode stings, but not one bytestring and one unicode.

I don't think that's a good idea.
msg219435 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-05-31 01:31
That restriction is deliberate (and documented). As a 3.x backport, this
utility inherits some of Python 3's pedantry about requiring explicit
conversions between binary and text data and being consistent as to which
domain you're operating in.
msg219451 - (view) Author: Donald Stufft (dstufft) * (Python committer) Date: 2014-05-31 15:14
That's also a security sensitive thing, you don't want to compare two different encoding and have it accidentally fail. Strictly speaking you can only do a constant time comparison on bytes, the fact it accepts unicode at all (even on Python 3.x) is a convenience feature.
History
Date User Action Args
2014-05-31 15:14:57dstufftsetmessages: + msg219451
2014-05-31 01:31:13ncoghlansetmessages: + msg219435
2014-05-30 21:38:24smurfixsetnosy: + smurfix
messages: + msg219428
2014-05-11 23:11:51python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg218304

resolution: fixed
stage: needs patch -> resolved
2014-05-10 23:37:49dstufftsetmessages: + msg218241
2014-04-30 20:47:41alexsetkeywords: + needs review
files: + compare_digest.diff
messages: + msg217649
2014-04-22 22:06:14alexsetfiles: + compare_digest.diff
keywords: + patch
messages: + msg217035
2014-04-22 21:40:54dstufftsetmessages: + msg217031
2014-04-22 21:31:44alexsetmessages: + msg217030
2014-04-22 21:31:20alexsetmessages: + msg217029
2014-04-22 21:30:41ncoghlansetmessages: + msg217028
2014-04-22 21:30:06dstufftsetmessages: + msg217027
2014-04-22 21:27:08alexsetmessages: + msg217026
2014-04-19 00:53:07ncoghlancreate