Skip to content
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

PEP 466: backport hmac.compare_digest #65505

Closed
ncoghlan opened this issue Apr 19, 2014 · 14 comments
Closed

PEP 466: backport hmac.compare_digest #65505

ncoghlan opened this issue Apr 19, 2014 · 14 comments
Labels
type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 21306
Nosy @ncoghlan, @pitrou, @giampaolo, @tiran, @benjaminp, @smurfix, @alex, @dstufft
Files
  • compare_digest.diff
  • compare_digest.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2014-05-11.23:11:51.295>
    created_at = <Date 2014-04-19.00:53:07.439>
    labels = ['type-feature']
    title = 'PEP 466: backport hmac.compare_digest'
    updated_at = <Date 2014-05-31.15:14:57.492>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2014-05-31.15:14:57.492>
    actor = 'dstufft'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-05-11.23:11:51.295>
    closer = 'python-dev'
    components = []
    creation = <Date 2014-04-19.00:53:07.439>
    creator = 'ncoghlan'
    dependencies = []
    files = ['35005', '35122']
    hgrepos = []
    issue_num = 21306
    keywords = ['patch', 'needs review']
    message_count = 14.0
    messages = ['216826', '217026', '217027', '217028', '217029', '217030', '217031', '217035', '217649', '218241', '218304', '219428', '219435', '219451']
    nosy_count = 10.0
    nosy_names = ['ncoghlan', 'janssen', 'pitrou', 'giampaolo.rodola', 'christian.heimes', 'benjamin.peterson', 'smurfix', 'alex', 'python-dev', 'dstufft']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue21306'
    versions = ['Python 2.7']

    @ncoghlan
    Copy link
    Contributor Author

    Tracker issue for the hmac.compare_digest backport to 2.7 described in PEP-466.

    @ncoghlan ncoghlan added the type-feature A feature request or enhancement label Apr 19, 2014
    @alex
    Copy link
    Member

    alex commented Apr 22, 2014

    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.

    @dstufft
    Copy link
    Member

    dstufft commented Apr 22, 2014

    try:
    data = data.encode("ascii")
    except UnicodeEncodeError:
    raise TypeError("comparing unicode with non-ASCII characters is not supported")

    ?

    @ncoghlan
    Copy link
    Contributor Author

    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.

    @alex
    Copy link
    Member

    alex commented Apr 22, 2014

    encode("ascii") has data dependent branches, so it's to be avoided.

    @alex
    Copy link
    Member

    alex commented Apr 22, 2014

    Thanks Nick. I'll get a patch up for str (bytes) only this afternoon.

    @dstufft
    Copy link
    Member

    dstufft commented Apr 22, 2014

    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.

    @alex
    Copy link
    Member

    alex commented Apr 22, 2014

    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.

    @alex
    Copy link
    Member

    alex commented Apr 30, 2014

    Attached patch now includes documentation and should be complete.

    @dstufft
    Copy link
    Member

    dstufft commented May 10, 2014

    The attached patch looks good to me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 11, 2014

    New changeset b40f1a00b134 by Benjamin Peterson in branch '2.7':
    backport hmac.compare_digest to partially implement PEP-466 (closes bpo-21306)
    http://hg.python.org/cpython/rev/b40f1a00b134

    @python-dev python-dev mannequin closed this as completed May 11, 2014
    @smurfix
    Copy link
    Mannequin

    smurfix mannequin commented May 30, 2014

    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.

    @ncoghlan
    Copy link
    Contributor Author

    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.

    @dstufft
    Copy link
    Member

    dstufft commented May 31, 2014

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants