Clear plaintext passwords in more error cases #1190
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
For all three commits: Thanks! |
If crypt fails, pw_encrypt calls exit. This has the consequence that the plaintext password is not cleared. A valid password can fail if the underlying library does not support it. One such example is SHA512, for which the password must not be longer than 256 characters on musl. A password longer than this with glibc works, so it is actually possible that a user, running passwd, tries to enter the old password but the musl-based passwd binary simply exits. Let passwd clear the password before exiting. Reviewed-by: Alejandro Colomar <[email protected]> Signed-off-by: Tobias Stoeckmann <[email protected]>
If encryption of password fails, clear the memory before exiting. Reviewed-by: Alejandro Colomar <[email protected]> Signed-off-by: Tobias Stoeckmann <[email protected]>
Use PASS_MAX + 1 instead of BUFSIZ to clarify where this size comes from. Technically, PASS_MAX is BUFSIZ - 1 so this is a no-op change. Just make sure that the size of pass stays in sync with agetpass. Reviewed-by: Alejandro Colomar <[email protected]> Signed-off-by: Tobias Stoeckmann <[email protected]>
stoeckmann
force-pushed
the
memzero_plain
branch
from
ca16003 to
d2314fb
Compare
alejandro-colomar
approved these changes
Jan 19, 2025
|
Since nobody commented, and it's quite simple, I'll merge already. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most code paths make sure that a plaintext password, if not read from stdin or a file, is properly cleared.
This is not true for all
pw_encrypterror cases. If the salt itself is considered "unknown", thenexitis called. Letpw_encryptreturn an error code so the program logic can clear passwords properly before handling the error.One such example is musl. If passwd compiled for musl tries to encrypt a password with SHA512 which is longer than 256 characters, it fails. But this can be easily the valid password on the system, because glibc has no such problem. So, just wipe it.
Example output with this patch (passwd linked with musl):
While at it, fixed missing clearing in gpasswd and adjusted a preprocessor definition usage.