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 function not hashing properly on Mac (uses a specific salt) #77394

Open
RonReiter mannequin opened this issue Apr 3, 2018 · 14 comments
Open

crypt function not hashing properly on Mac (uses a specific salt) #77394

RonReiter mannequin opened this issue Apr 3, 2018 · 14 comments
Labels
3.11 only security fixes extension-modules C modules in the Modules dir OS-mac type-security A security issue

Comments

@RonReiter
Copy link
Mannequin

RonReiter mannequin commented Apr 3, 2018

BPO 33213
Nosy @rhettinger, @ronaldoussoren, @tiran, @ned-deily, @serhiy-storchaka, @akulakov

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 = None
created_at = <Date 2018-04-03.08:26:58.700>
labels = ['type-security', 'extension-modules', 'OS-mac', '3.11']
title = 'crypt function not hashing properly on Mac (uses a specific salt)'
updated_at = <Date 2021-06-20.14:54:03.855>
user = 'https://bugs.python.org/RonReiter'

bugs.python.org fields:

activity = <Date 2021-06-20.14:54:03.855>
actor = 'andrei.avk'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules', 'macOS']
creation = <Date 2018-04-03.08:26:58.700>
creator = 'Ron Reiter'
dependencies = []
files = []
hgrepos = []
issue_num = 33213
keywords = []
message_count = 13.0
messages = ['314866', '314867', '314868', '314871', '314872', '314873', '314874', '314875', '314884', '314926', '314938', '396174', '396175']
nosy_count = 7.0
nosy_names = ['rhettinger', 'ronaldoussoren', 'christian.heimes', 'ned.deily', 'serhiy.storchaka', 'Ron Reiter', 'andrei.avk']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue33213'
versions = ['Python 3.11']

@RonReiter
Copy link
Mannequin Author

RonReiter mannequin commented Apr 3, 2018

import crypt
Expected result:
>>> crypt.crypt("test") == crypt.crypt("test")
False
>>> crypt.crypt("test", crypt.mksalt()) == crypt.crypt("test", crypt.mksalt())
False

Unexpected results:
>>> crypt.crypt("test", crypt.METHOD_SHA512) == crypt.crypt("test", crypt.METHOD_SHA512)
True
>>> crypt.crypt("test", crypt.mksalt(crypt.METHOD_SHA512)) == crypt.crypt("test", crypt.mksalt(crypt.METHOD_SHA512))
False

@RonReiter RonReiter mannequin added the extension-modules C modules in the Modules dir label Apr 3, 2018
@RonReiter
Copy link
Mannequin Author

RonReiter mannequin commented Apr 3, 2018

import crypt
Expected result:
>>> crypt.crypt("test") == crypt.crypt("test")
False
>>> crypt.crypt("test", crypt.mksalt()) == crypt.crypt("test", crypt.mksalt())
False

Unexpected results:
>>> crypt.crypt("test", crypt.METHOD_SHA512) == crypt.crypt("test", crypt.METHOD_SHA512)
True
>>> crypt.crypt("test", crypt.mksalt(crypt.METHOD_SHA512)) == crypt.crypt("test", crypt.mksalt(crypt.METHOD_SHA512))
True

@RonReiter
Copy link
Mannequin Author

RonReiter mannequin commented Apr 3, 2018

You guessed it, the salt is "$6"

@serhiy-storchaka
Copy link
Member

I can't reproduce this result. Does your os.urandom() broken and return a short repeated sequence?

@RonReiter
Copy link
Mannequin Author

RonReiter mannequin commented Apr 3, 2018

Apparently it's a Mac issue. My crypt.methods only contains [<crypt.METHOD_CRYPT>] which is probably why this fails. It's a silent failure of some sort that is causing this.

@tiran
Copy link
Member

tiran commented Apr 3, 2018

The crypt module is a thin wrapper around the OS' crypt(3) function. The function should return NULL for unsupported salt types. The module turns NULL into None.

What's the return value of crypt.crypt("test", crypt.METHOD_SHA512) ?

@RonReiter
Copy link
Mannequin Author

RonReiter mannequin commented Apr 3, 2018

Python 3.6.4 (default, Mar 22 2018, 23:35:12)
[GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import crypt
>>> crypt.crypt("test", crypt.METHOD_SHA512)
'$6asQOJRqB1i2'
>>> crypt.crypt("test", crypt.METHOD_SHA512)
'$6asQOJRqB1i2'

@RonReiter
Copy link
Mannequin Author

RonReiter mannequin commented Apr 3, 2018

Also:
>>> crypt.crypt("test", "$5")
'$5yVOkTkyRzn.'
>>> crypt.crypt("test", "$6")
'$6asQOJRqB1i2'
>>> crypt.crypt("test", "$7")
'$7tSOkvDyiL6U'

So the salt is "$6"

@RonReiter RonReiter mannequin changed the title crypt function not hashing properly crypt function not hashing properly on Mac (uses a specific salt) Apr 3, 2018
@rhettinger
Copy link
Contributor

What's the return value of crypt.crypt("test", crypt.METHOD_SHA512)

This is from my Mac (10.13.3):

$ python3.6
Python 3.6.4 (v3.6.4:d48ecebad5, Dec 18 2017, 21:07:28)
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import crypt
>>> crypt.crypt("test", crypt.METHOD_SHA512)
'$6asQOJRqB1i2'
>>> crypt.crypt("test", crypt.METHOD_SHA512)
'$6asQOJRqB1i2'
>>> crypt.crypt("test", "$5")
'$5yVOkTkyRzn.'
>>> crypt.crypt("test", "$6")
'$6asQOJRqB1i2'
>>> crypt.crypt("test", "$7")
'$7tSOkvDyiL6U'
>>> crypt.crypt("test") == crypt.crypt("test")
False
>>> crypt.crypt("test", crypt.mksalt()) == crypt.crypt("test", crypt.mksalt())
False

@ned-deily
Copy link
Member

$ ./bin/python3
Python 3.8.0a0 (heads/master:55966f3a0d, Apr  2 2018, 18:16:13)
[Clang 9.1.0 (clang-902.0.39.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import crypt
>>> crypt.methods
[<crypt.METHOD_CRYPT>]

@ned-deily ned-deily added 3.7 (EOL) end of life 3.8 only security fixes labels Apr 4, 2018
@ronaldoussoren
Copy link
Contributor

As far as I know macOS does not support different salt types at all. The manpage does mention an "extended crypt", but according to the documentation that just controls the number of DES rounds used.

In particular:

The salt is a 9-character array consisting of an underscore, followed by 
4 bytes of iteration count and 4 bytes of salt.  These are encoded as
printable characters, 6 bits per character, least significant character
first.  The values 0 to 63 are encoded as ``./0-9A-Za-z''.  This allows 
24 bits for both count and salt.

If anything needs to change it would have to be a macOS specific patch to the _crypt extension that rejects any attempt of using algorithm selection (but that's technically a backward incompatible change as)

@RonReiter RonReiter mannequin added the type-security A security issue label Apr 7, 2018
@ned-deily ned-deily added OS-mac 3.11 only security fixes and removed 3.7 (EOL) end of life 3.8 only security fixes labels May 22, 2021
@akulakov
Copy link
Contributor

How about adding a check to crypt.mksalt():

if method and method not in methods:
    raise ValueError(f'method {method} is not supported')

If a method is supplied to crypt.crypt(), mksalt() is called with it as an arg, so adding this check will take care of both paths:
crypt(val, method)
crypt(val, mksalt(method))

the only remaining issue is if an (improperly generated) salt is loaded
from somewhere and used to call crypt(), but the check above fixes most of the issue.

I can put up a PR if this sounds good.

@akulakov
Copy link
Contributor

Actually it should be:

  if method is not None and method not in methods:
    ...

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@AA-Turner
Copy link
Member

@tiran should this issue remain open now that crypt has been deprecated?

A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes extension-modules C modules in the Modules dir OS-mac type-security A security issue
Projects
None yet
Development

No branches or pull requests

7 participants