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

Mismatch between SHA_ROUNDS_MAX in shadow and musl #79

Open
Strahinja opened this issue Jul 7, 2022 · 4 comments
Open

Mismatch between SHA_ROUNDS_MAX in shadow and musl #79

Strahinja opened this issue Jul 7, 2022 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@Strahinja
Copy link

Strahinja commented Jul 7, 2022

Regarding the

# Fix a simple programming error ...fixed in 4.11.1?
there is another hidden potential mismatch between shadow and musl.

Currently, the SHA_ROUNDS_MAX in shadow and musl differ. The one in musl 1.2.3 is defined as
https://git.musl-libc.org/cgit/musl/tree/src/crypt/crypt_sha512.c?h=v1.2.3#n196

/* key limit is not part of the original design, added for DoS protection.
 * rounds limit has been lowered (versus the reference/spec), also for DoS
 * protection. runtime is O(klen^2 + klen*rounds) */
#define KEY_MAX 256
#define SALT_MAX 16
#define ROUNDS_DEFAULT 5000
#define ROUNDS_MIN 1000
#define ROUNDS_MAX 9999999

(7 digits), whereas the one expected by shadow 4.11.1 is
https://github.com/shadow-maint/shadow/blob/1bf5868e3378aef0f36ba3490852709d79729419/libmisc/salt.c#L63

#define SHA_ROUNDS_MAX 999999999

(9 digits), which means that shadow can potentially request the larger number of rounds than supported by musl.

@dslm4515
Copy link
Owner

Per the LFS 11.0 for shadow 4.9:

Fix a simple programming error by modifying a file with following command:
sed -e "224s/rounds/min_rounds/" -i libmisc/salt.c

in LFS 11.1, shadow is updated to 4.11.1 and the suggested fix above was omitted. closed issue

But LFS, is based on Glibc. I haven’t checked if Glibc limits the max as well or if it matches shadow. As reference, I check Alpine Linux (and sometimes Void Linux) for any suggested patches or modifications. So far I have not seen any patches. If there is, I probably didn’t see it by mistake.

So far, I haven’t seen this cause any issues on my daily driver system.

@Strahinja
Copy link
Author

I haven’t checked if Glibc limits the max as well or if it matches shadow. As reference, I check Alpine Linux (and sometimes Void Linux) for any suggested patches or modifications. So far I have not seen any patches. If there is, I probably didn’t see it by mistake.

glibc matches shadow with 9 digits:

https://github.com/bminor/glibc/blob/b92a49359f33a461db080a33940d73f47c756126/crypt/sha512-crypt.c#L91

So far, I haven’t seen this cause any issues on my daily driver system.

Nevertheless, the potential is there. What would happen if the number of rounds was passed greater than 9999999 (7 digits) is that musl would silently fallback to returning a DES-encrypted string, making shadow print out error about SHA512 "not being supported?", when it actually is. I believe this is either not being considered by musl developers, or left to the user's configuration of userland programs such as those from shadow.

On my system, I used sed to apply musl's limit to shadow as well, because it seems saner to me.

@dslm4515
Copy link
Owner

Oh okay. I thank you for the explanation as… I wasn’t sure where to start researching.

then yes, I agree on applying the same limit in Musl Libc on shadow.

@dslm4515 dslm4515 self-assigned this Jul 10, 2022
@dslm4515 dslm4515 added the enhancement New feature or request label Jul 10, 2022
@Strahinja
Copy link
Author

As reference, this is the exact command I used:

sed -i -e 's/^\(#define SHA_ROUNDS_MAX 9999999\)99/\1/' libmisc/salt.c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants