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.

Sign up for GitHub

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

Jump to bottom

hmac.secure_compare() is not time-independent for unicode strings #59160

Closed
JonOberheide mannequin opened this issue May 29, 2012 · 9 comments
Closed

hmac.secure_compare() is not time-independent for unicode strings #59160

JonOberheide mannequin opened this issue May 29, 2012 · 9 comments
Labels
stdlib Python modules in the Lib dir type-security A security issue

Comments

@JonOberheide
Copy link
Mannequin

JonOberheide mannequin commented May 29, 2012

BPO 14955
Nosy @ncoghlan, @pitrou, @vstinner, @tiran, @hynek
Dependencies
  • bpo-15061: hmac.secure_compare() leaks information about length of strings
    		•
    Files
  • secure-compare-fix-v1.patch
  • secure-compare-fix-v2.patch
    		•

    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 2012-10-06.23:34:12.885>
created_at = <Date 2012-05-29.16:36:51.415>
labels = ['type-security', 'library']
title = 'hmac.secure_compare() is not time-independent for unicode strings'
updated_at = <Date 2012-10-06.23:34:12.884>
user = 'https://bugs.python.org/JonOberheide'

    bugs.python.org fields:

    activity = <Date 2012-10-06.23:34:12.884>
actor = 'christian.heimes'
assignee = 'none'
closed = True
closed_date = <Date 2012-10-06.23:34:12.885>
closer = 'christian.heimes'
components = ['Library (Lib)']
creation = <Date 2012-05-29.16:36:51.415>
creator = 'Jon.Oberheide'
dependencies = ['15061']
files = ['25756', '25801']
hgrepos = []
issue_num = 14955
keywords = ['patch']
message_count = 9.0
messages = ['161898', '161966', '162159', '162737', '162771', '162889', '162915', '163169', '172264']
nosy_count = 6.0
nosy_names = ['ncoghlan', 'pitrou', 'vstinner', 'christian.heimes', 'hynek', 'Jon.Oberheide']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue14955'
versions = ['Python 3.3']
    @JonOberheide
    Copy link
    Mannequin Author

    JonOberheide mannequin commented May 29, 2012

    Hi all,

    I was informed that the hmac.secure_compare() function added in 14532 is not time-independent when processing unicode values:

    "The function as given is probably not timing independent if the attacker can provide unicode values. This is because (in CPython at least) all integer values in the range [-5, 256] inclusive are made singletons to avoid the performance hit of integer object creation, meaning that as long as (x ^ y) < 257, no integer object is created and the function appears constant time. When that assumption is violated, you get a timing delta that is actually fairly large compared to the delta for a single character compare."

    One way to work around this issue is to perform strict 8-bit byte comparisons by converting any string type parameters to bytes via a utf-8 encode. The attached patch does this.

    Regards,
    Jon Oberheide

    @JonOberheide JonOberheide mannequin added stdlib Python modules in the Lib dir type-security A security issue labels May 29, 2012
    @vstinner
    Copy link
    Member

    I'm not sure that encoding to UTF-8 is time indenpendant. You may try UTF-32-LE or unicode-internal?

    @JonOberheide
    Copy link
    Mannequin Author

    JonOberheide mannequin commented Jun 2, 2012

    Thanks for the feedback, haypo. I've updated the patch to use unicode-internal. As long as the encode() of the expected non-attacker-controlled digest is not dependent on the actual contents of the digest, we should be good.

    @tiran
    Copy link
    Member

    tiran commented Jun 13, 2012

    The second patch looks fine.

    @pitrou
    Copy link
    Member

    pitrou commented Jun 14, 2012

    With PEP-393 unicode objects can have several representations, which makes it unlikely that *really* constant-timing functions can be devised.

    However, a C version could provide some guarantees, by raising an error if the passed unicode strings use a different representation from each other.

    @ncoghlan
    Copy link
    Contributor

    As a result of the discussions on bpo-15061, I removed unicode comparison support altogether in f36af3766a20 (updating the warning on the hexdigest() method accordingly).

    Are folks happy to close this issue on that basis? (I'll raise the question of a separate C implementation on the other issue)

    @JonOberheide
    Copy link
    Mannequin Author

    JonOberheide mannequin commented Jun 15, 2012

    Sounds good to me, Nick.

    @tiran
    Copy link
    Member

    tiran commented Jun 19, 2012

    Nick has pushed a patch in rf36af3766a20 that disables the comparison of unicode strings. See bpo-15061

    @tiran
    Copy link
    Member

    tiran commented Oct 6, 2012

    Python 3.3 contains a secure and working implementation for bytes. unicode isn't supported unless both sides contains ASCII text only.

    @tiran tiran closed this as completed Oct 6, 2012
    @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
    Assignees
    No one assigned
    Labels
    stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @tiran @ncoghlan @pitrou