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

[Patch] '_crypt' module: fix implicit declaration of crypt(), use crypt_r() where available #72689

Closed
EdSchouten mannequin opened this issue Oct 22, 2016 · 10 comments
Closed

[Patch] '_crypt' module: fix implicit declaration of crypt(), use crypt_r() where available #72689

EdSchouten mannequin opened this issue Oct 22, 2016 · 10 comments
Assignees
@gpshead
Labels
3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir

Comments

@EdSchouten
Copy link
Mannequin

EdSchouten mannequin commented Oct 22, 2016

BPO 28503
Nosy @gpshead, @pitrou, @benjaminp, @xdegaye, @serhiy-storchaka, @EdSchouten
PRs
  • bpo-28503: _crypt: fix implicit declaration of crypt(), use crypt_r() if available. #4691
  • bpo-28503: Use crypt_r() when available instead of crypt() #11373
  • bpo-28503: Use crypt_r() when available instead of crypt() #11373
  • bpo-28503: Use crypt_r() when available instead of crypt() #11373
  • [3.7] bpo-28503: Use crypt_r() when available instead of crypt() (GH-11373) #11376
  • [3.7] bpo-28503: Use crypt_r() when available instead of crypt() (GH-11373) #11376
  • [3.7] bpo-28503: Use crypt_r() when available instead of crypt() (GH-11373) #11376
    		•
    Files
  • crypt.diff: Fix implicit declaration of crypt(), use crypt_r() where available
    		•

    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/gpshead'
closed_at = <Date 2018-12-31.02:01:37.801>
created_at = <Date 2016-10-22.07:52:55.782>
labels = ['extension-modules', '3.7', '3.8']
title = "[Patch] '_crypt' module: fix implicit declaration of crypt(), use crypt_r() where available"
updated_at = <Date 2018-12-31.02:01:37.801>
user = 'https://github.com/EdSchouten'

    bugs.python.org fields:

    activity = <Date 2018-12-31.02:01:37.801>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2018-12-31.02:01:37.801>
closer = 'gregory.p.smith'
components = ['Extension Modules']
creation = <Date 2016-10-22.07:52:55.782>
creator = 'EdSchouten'
dependencies = []
files = ['45185']
hgrepos = []
issue_num = 28503
keywords = ['patch']
message_count = 10.0
messages = ['279186', '304943', '304965', '304966', '305026', '307525', '332774', '332775', '332785', '332786']
nosy_count = 6.0
nosy_names = ['gregory.p.smith', 'pitrou', 'benjamin.peterson', 'xdegaye', 'serhiy.storchaka', 'EdSchouten']
pr_nums = ['4691', '11373', '11373', '11373', '11376', '11376', '11376']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue28503'
versions = ['Python 3.7', 'Python 3.8']
    @EdSchouten
    Copy link
    Mannequin Author

    EdSchouten mannequin commented Oct 22, 2016

    The '_crypt' module provides a binding to the C crypt(3) function. It is used by the crypt.crypt() function.

    Looking at the C code, there are a couple of things we can improve:

    • Because crypt() only depends on primitive C types, we currently get away with calling it without it being declared. Ensure that we include <unistd.h>, which is the POSIX header file declaring this.

    • The disadvantage of crypt() is that it's thread-unsafe. Systems like Linux and recent versions of FreeBSD nowadays provide crypt_r() as a replacement. This function allows you to pass in a 'crypt_data' object that will hold the resulting string for you. Extend the code to use this function when available.

    This patch is actually needed to make this module build on CloudABI (https://mail.python.org/pipermail/python-dev/2016-July/145708.html). CloudABI's C library doesn't provide any thread-unsafe functions, meaning that crypt_r() is the only way you can crypt passwords.

    @EdSchouten EdSchouten mannequin added the extension-modules C modules in the Modules dir label Oct 22, 2016
    @serhiy-storchaka
    Copy link
    Member

    What is the performance of crypt_r() in comparison with crypt()?

    @pitrou
    Copy link
    Member

    pitrou commented Oct 25, 2017

    I'm not sure performance matters. Modern crypt() algorithms should actually be slow enough (using a large number of rounds) to make brute-force attacks impractical...

    @pitrou pitrou added the 3.7 (EOL) end of life label Oct 25, 2017
    @EdSchouten
    Copy link
    Mannequin Author

    EdSchouten mannequin commented Oct 25, 2017

    Having looked at various implementations of crypt() and crypt_r(), I can't think of a reason why there would be any significant difference in performance. On systems like FreeBSD, crypt() is just a simple wrapper around crypt_r():

    https://svnweb.freebsd.org/base/head/lib/libcrypt/crypt.c?view=markup#l134

    If there would be any difference in performance, it would also be astronomically small compared to the computation time spent by the cryptographic hash functions themselves.

    @benjaminp
    Copy link
    Contributor

    This sgtm. Can you send a PR?

    @EdSchouten
    Copy link
    Mannequin Author

    EdSchouten mannequin commented Dec 3, 2017

    Ah, you folks switched to Git + Github in the mean time. Nice!

    I've just sent this pull request: #4691

    @gpshead gpshead self-assigned this Dec 30, 2018
    @gpshead gpshead added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Dec 30, 2018
    @gpshead
    Copy link
    Member

    gpshead commented Dec 30, 2018

    New changeset 387512c by Gregory P. Smith in branch 'master':
    bpo-28503: Use crypt_r() when available instead of crypt() (GH-11373)
    387512c

    @gpshead
    Copy link
    Member

    gpshead commented Dec 30, 2018

    As this is internal only rather than a feature, i'll bring this into 3.7 as well.

    @gpshead gpshead added the 3.7 (EOL) end of life label Dec 30, 2018
    @gpshead
    Copy link
    Member

    gpshead commented Dec 31, 2018

    New changeset a144fee by Gregory P. Smith (Miss Islington (bot)) in branch '3.7':
    bpo-28503: Use crypt_r() when available instead of crypt() (GH-11373) (GH-11376)
    a144fee

    @gpshead
    Copy link
    Member

    gpshead commented Dec 31, 2018

    I'm going to close this as 3.7 and 3.8 are good now, but if someone wants to see the same thing done in 2.7 it should be possible for them make a PR. This is primarily just a configure.ac change.

    @gpshead gpshead closed this as completed Dec 31, 2018
    @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

    @gpshead gpshead

    Labels
    3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @gpshead @benjaminp @pitrou @serhiy-storchaka