Skip to content

bpo-42981: Clarify error messages raised by _hashlib_scrypt_impl() #24274

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

Closed
wants to merge 1 commit into from

Conversation

illia-v
Copy link
Contributor

@illia-v illia-v commented Jan 20, 2021

The changes clarify that INT_MAX is still a valid value for maxmem and dklen, and that 1 is not a valid value for n despite it is a power of 2.

https://bugs.python.org/issue42981

@tiran
Copy link
Member

tiran commented Jan 21, 2021

In my opinion it does not make any sense to make the error messages more explicit. hashlib.scrypt() is a low-level interface. I expect that users are somewhat familiar with the algorithm. The additional checks are primarily to avoid overflows in C.

n=1 is nonsensical. Any value remotely close to INT_MAX is nonsensical. scrypt with n=2, r=1, p=INT_MAX would require 500 GB (!) of virtual memory.

@illia-v
Copy link
Contributor Author

illia-v commented Jan 21, 2021

I see your point.

I expect that users are somewhat familiar with the algorithm.

Your expectations are reasonable, but people may start exploring the algorithm from calling the Python function. I did so and was confused when it failed with "n must be a power of 2" after I set n to 1.

scrypt with n=2, r=1, p=INT_MAX would require 500 GB (!) of virtual memory.

With INT_MAX equal to 32767, it fits the default 32 MiB limit. If it is 2147483647 or 9223372036854775807, the parameters look invalid.
RFC 7914 says that "p is a positive integer less than or equal to ((2^32-1) * 32) / (128 * r)." That means that 1073741823 is the larges possible p and the number is twice smaller than 2147483647, is not it?
Anyway, I didn't change any checks related to p.

@tiran
Copy link
Member

tiran commented Jan 22, 2021

The value of INT_MAX depends on OS and platform. It's much larger on 64bit machines:

>>> import _testcapi
>>> _testcapi.INT_MAX
2147483647

@illia-v
Copy link
Contributor Author

illia-v commented Jan 27, 2021

The value of INT_MAX depends on OS and platform.

I looked at OpenSSL's code to find whether max values of maxmem and dklen in their implementation depend on a platform too. It looks like they do, but are SIZE_MAX that is much bigger.
Password and salt length seems to be limited to SIZE_MAX too.

https://github.com/openssl/openssl/blob/90cebd1b216e0a160fcfd8e0eddca47dad47c183/crypto/evp/pbe_scrypt.c#L158-L161
https://github.com/openssl/openssl/blob/90cebd1b216e0a160fcfd8e0eddca47dad47c183/crypto/evp/pbe_scrypt.c#L227-L229

@illia-v illia-v closed this Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants