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

crypt.mksalt() result has unnecessarily low entropy #62605

Closed
tiran opened this issue Jul 8, 2013 · 4 comments
Closed

crypt.mksalt() result has unnecessarily low entropy #62605

tiran opened this issue Jul 8, 2013 · 4 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-security A security issue

Comments

@tiran
Copy link
Member

tiran commented Jul 8, 2013

BPO 18405
Nosy @gpshead, @vstinner, @tiran
Files
  • crypt_salt_choice.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 = 'https://github.com/tiran'
    closed_at = <Date 2013-08-13.23:41:10.166>
    created_at = <Date 2013-07-08.17:57:20.192>
    labels = ['type-security', 'library']
    title = 'crypt.mksalt() result has unnecessarily low entropy'
    updated_at = <Date 2013-08-13.23:45:11.215>
    user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2013-08-13.23:45:11.215>
    actor = 'vstinner'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2013-08-13.23:41:10.166>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2013-07-08.17:57:20.192>
    creator = 'christian.heimes'
    dependencies = []
    files = ['31018']
    hgrepos = []
    issue_num = 18405
    keywords = ['patch']
    message_count = 4.0
    messages = ['192683', '193561', '195105', '195106']
    nosy_count = 4.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'christian.heimes', 'python-dev']
    pr_nums = []
    priority = 'critical'
    resolution = 'fixed'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue18405'
    versions = ['Python 3.3', 'Python 3.4']

    @tiran
    Copy link
    Member Author

    tiran commented Jul 8, 2013

    crypt.mksalt() creates a salt with a lower entropy than possible. It uses random.SystemRandom().sample() to generate a salt string from the set of 64 chars (string.ascii_letters + string.digits + './'). SystemRandom() uses a CPRNG (good) but sample() returns n UNIQUE members of the set (very bad). sample() reduces the set possible chars by one for each salt char.

    Suggested fix:

    salt = base64.b64encode(os.urandom(salt_chars * 3 // 4), b"./").decode("ascii")

    @tiran tiran self-assigned this Jul 8, 2013
    @tiran tiran added stdlib Python modules in the Lib dir type-security A security issue labels Jul 8, 2013
    @vstinner
    Copy link
    Member

    I prefer to avoid conversion to/from base64, and use random.choice() instead: see attached patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 13, 2013

    New changeset e8a314fe248b by Victor Stinner in branch '3.3':
    Issue bpo-18405: Improve the entropy of crypt.mksalt().
    http://hg.python.org/cpython/rev/e8a314fe248b

    New changeset 122e074c56f7 by Victor Stinner in branch 'default':
    (Merge 3.3) Issue bpo-18405: Improve the entropy of crypt.mksalt().
    http://hg.python.org/cpython/rev/122e074c56f7

    @vstinner
    Copy link
    Member

    With my change, any character can appear more than once. Example:

    >>> crypt.mksalt()
    '$6$idm7/asaywTgRf9V'
    >>> sorted(_[3:])
    ['/', '7', '9', 'R', 'T', 'V', 'a', 'a', 'd', 'f', 'g', 'i', 'm', 's', 'w', 'y']

    In this case, the 'a' letter occurs twice.

    @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
    stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants