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

Jump to bottom

Use OpenSSL's HMAC API #84825

Closed
tiran opened this issue May 16, 2020 · 14 comments
Closed

Use OpenSSL's HMAC API #84825

tiran opened this issue May 16, 2020 · 14 comments
Assignees
@tiran
Labels
3.9 only security fixes 3.10 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@tiran
Copy link
Member

tiran commented May 16, 2020

BPO 40645
Nosy @gpshead, @vstinner, @tiran, @encukou, @pablogsal, @miss-islington, @erlend-aasland
PRs
  • bpo-40645: Implement HMAC in C (GH-20129) #20129
  • bpo-40645: Deprecated internal details of hmac.HMAC (GH-20132) #20132
  • bpo-40645: restrict HMAC key len to INT_MAX #20238
  • [3.9] bpo-40645: restrict HMAC key len to INT_MAX (GH-20238) #20245
  • bpo-40645: use C implementation of HMAC #24920
  • bpo-40645: Fix reference leak in the _hashopenssl extension #25063
  • bpo-40645: Fix reference leak in the _hashopenssl extension #26072
  • bpo-40645: Fix ref leaks in _hashopenssl #26079
  • [3.10] bpo-40645: Fix ref leaks in _hashopenssl (GH-26079) #26082
    		•

    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 = 'https://github.com/tiran'
closed_at = <Date 2021-10-20.14:44:16.885>
created_at = <Date 2020-05-16.15:01:31.309>
labels = ['extension-modules', 'type-feature', '3.9', '3.10']
title = "Use OpenSSL's HMAC API"
updated_at = <Date 2021-10-20.14:44:16.884>
user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2021-10-20.14:44:16.884>
actor = 'christian.heimes'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2021-10-20.14:44:16.885>
closer = 'christian.heimes'
components = ['Extension Modules']
creation = <Date 2020-05-16.15:01:31.309>
creator = 'christian.heimes'
dependencies = []
files = []
hgrepos = []
issue_num = 40645
keywords = ['patch']
message_count = 14.0
messages = ['369050', '369079', '369104', '369114', '369375', '369388', '369403', '369404', '389027', '389601', '389682', '389684', '389701', '393542']
nosy_count = 7.0
nosy_names = ['gregory.p.smith', 'vstinner', 'christian.heimes', 'petr.viktorin', 'pablogsal', 'miss-islington', 'erlendaasland']
pr_nums = ['20129', '20132', '20238', '20245', '24920', '25063', '26072', '26079', '26082']
priority = 'critical'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue40645'
versions = ['Python 3.9', 'Python 3.10']
    @tiran
    Copy link
    Member Author

    tiran commented May 16, 2020

    Python's hmac module provides a pure Python based implementation on top of the hashlib module. OpenSSL offers a dedicated HMAC implementation that has a couple of benefits over pure Python implementation:

    • OpenSSL HMAC is slightly faster and requires slightly less memory and allocations.
    • Python's HMAC only works for RFC 2104 HMACs with digests like MD5, SHA1, SHA2, and SHA3. Other digests types like Blake2 use a completely different style of HMAC. OpenSSL's HMAC API works for all sorts of digests. OpenSSL 3.0.0 also supports Blake2 MAC with its standard API.
    • OpenSSL HMAC is standard and compliance conform. Certain compliance restrictions require that MAC and keyed hashing is implemented in a certain way. Our HMAC code is considered a custom implementation of a crypto primitive and in violation of compliance rules.

    For 3.9 I plan to deprecate hmac.HMAC.digest_con, hmac.HMAC.inner, and hmac.HMAC.outer attributes. They are implementation specific details any way. I'm also going to provide a _hashlib.hmac_new() function so we can test the new code.

    For 3.10 I'll be switching over to _haslib.hmac_new() when the digestmod is a string or a callable that returns _hashlib.HASH code.

    @tiran tiran added the 3.9 only security fixes label May 16, 2020
    @tiran tiran self-assigned this May 16, 2020
    @tiran tiran added 3.9 only security fixes extension-modules C modules in the Modules dir labels May 16, 2020
    @tiran tiran self-assigned this May 16, 2020
    @tiran tiran added type-feature A feature request or enhancement extension-modules C modules in the Modules dir labels May 16, 2020
    @tiran
    Copy link
    Member Author

    tiran commented May 16, 2020

    New changeset 837f9e4 by Christian Heimes in branch 'master':
    bpo-40645: Deprecated internal details of hmac.HMAC (GH-20132)
    837f9e4

    @tiran
    Copy link
    Member Author

    tiran commented May 17, 2020 

    def new(key, msg=None, digestmod=''):
    # use fast HMAC if OpenSSL bindings are available and digestmod is
    # either a string or a callable that returns an OpenSSL HASH object.
    if _hashopenssl is not None:
        if isinstance(digestmod, str):
            return _hashopenssl.hmac_new(key, msg, digestmod)
        if callable(digestmod):
            digest = digestmod(b'')
            if isinstance(digest, _hashopenssl.HASH):
                return _hashopenssl.hmac_new(key, msg, digest.name)
    return HMAC(key, msg, digestmod)

    @tiran
    Copy link
    Member Author

    tiran commented May 17, 2020

    New changeset 54f2898 by Christian Heimes in branch 'master':
    bpo-40645: Implement HMAC in C (GH-20129)
    54f2898

    @vstinner
    Copy link
    Member

    Compiler warning on 64-bit Windows:

    c:\vstinner\python\3.9\modules\_hashopenssl.c(1427): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of data

    @tiran
    Copy link
    Member Author

    tiran commented May 19, 2020

    Thanks, Victor!

    @miss-islington
    Copy link
    Contributor

    New changeset aca4670 by Christian Heimes in branch 'master':
    bpo-40645: restrict HMAC key len to INT_MAX (GH-20238)
    aca4670

    @miss-islington
    Copy link
    Contributor

    New changeset 6ed3743 by Miss Islington (bot) in branch '3.9':
    bpo-40645: restrict HMAC key len to INT_MAX (GH-20238)
    6ed3743

    @tiran
    Copy link
    Member Author

    tiran commented Mar 18, 2021

    memo to me: switch to new C implementation of HMAC.

    @tiran tiran added 3.10 only security fixes labels Mar 18, 2021
    @miss-islington
    Copy link
    Contributor

    New changeset 933dfd7 by Christian Heimes in branch 'master':
    bpo-40645: use C implementation of HMAC (GH-24920)
    933dfd7

    @pablogsal
    Copy link
    Member

    Seems that commit 933dfd7 introduced some reference leaks:

    commit 933dfd7
    Author: Christian Heimes <christian@python.org>
    Date: Sat Mar 27 14:55:03 2021 +0100

    bpo-40645: use C implementation of HMAC (GH-24920)

    𓋹 ./python.exe -m test test_hashlib -R 3:3
    0:00:00 load avg: 5.67 Run tests sequentially
    0:00:00 load avg: 5.67 [1/1] test_hashlib
    beginning 6 repetitions
    123456
    ......
    test_hashlib leaked [1, 1, 1] references, sum=3
    test_hashlib failed

    == Tests result: FAILURE ==

    1 test failed:
    test_hashlib

    Total duration: 8.7 sec
    Tests result: FAILURE

    - [x] fix tests
- [ ] add test scenarios for old/new code.

Signed-off-by: Christian Heimes <christian@python.org>

    Lib/hashlib.py | 1 +
    Lib/hmac.py | 86 +++++++-----
    Lib/test/test_hmac.py | 114 +++++++++-------
    .../2021-03-19-10-22-17.bpo-40645.5pXhb-.rst | 2 +
    Modules/_hashopenssl.c | 150 +++++++++++++++++++--
    Modules/clinic/_hashopenssl.c.h | 38 +-----
    6 files changed, 267 insertions(+), 124 deletions(-)
    create mode 100644 Misc/NEWS.d/next/Library/2021-03-19-10-22-17.bpo-40645.5pXhb-.rst

    @pablogsal
    Copy link
    Member

    See for instance: https://buildbot.python.org/all/#/builders/75/builds/224

    @miss-islington
    Copy link
    Contributor

    New changeset 70cdf18 by Pablo Galindo in branch 'master':
    bpo-40645: Fix reference leak in the _hashopenssl extension (GH-25063)
    70cdf18

    @miss-islington
    Copy link
    Contributor

    New changeset 1ee58f2 by Miss Islington (bot) in branch '3.10':
    bpo-40645: Fix ref leaks in _hashopenssl (GH-26079)
    1ee58f2

    @tiran tiran closed this as completed Oct 20, 2021
    @tiran tiran closed this as completed Oct 20, 2021
    @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

    @tiran tiran

    Labels
    3.9 only security fixes 3.10 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @tiran @pablogsal @miss-islington