lib/pam_pass_non_interactive.c, src/{chfn,chsh,newgrp,passwd}.c: Don't log user-provided strings#1627
Conversation
…t log user-provided strings A few years ago, we started validating these strings as user or group names, which made them safe to log. However, these strings may be more than just a simple local user or group name. They may be fully qualified names, which don't pass the validation. This patch reverts the old one, and so we don't validate the strings anymore. Instead, let's not trust those strings, and thus don't log them. In some cases, we can log the UID or GID, which could be similarly useful. In others, we don't have that information, so just remove the string; that's less informative, but it's as much as we can do safely. Fixes: 326889c (2024-10-22; "Fix coverity unbound buffer issues") Closes: <shadow-maint#1626> Reported-by: Noor Eldeen Mansour <nooreldeen.mansour@canonical.com> Cc: Marcin Nowakowski <marcin.w.nowakowski@gmail.com> Cc: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "Failed to crypt password with previous salt of group '%s'", | ||
| groupname); | ||
| "Failed to crypt password with previous salt of group %ju", | ||
| (uintmax_t) grp->gr_gid); |
There was a problem hiding this comment.
/etc/group (which is what getgrnam(3) reads) is world-readable. How is this supposed to be sensitive?
|
I’ve only had a quick look at it, but I’ve got a question: wouldn’t it be better to allow UPNs as usernames? We could extend |
It could be; I'm not sure. Would that be okay everywhere? I mean, it seems that if we allow Maybe we should have separate is_valid_user_name() and is_valid_UPN(), and use them in each different case... If you (or anyone) research all the places where each function would be needed, I'm fine with that approach. :) |
This would make more sense. Let's see if I can find some minutes to investigate it and see if it can be done "easily" |
3 participants
A few years ago, we started validating these strings as user or group names, which made them safe to log. However, these strings may be more than just a simple local user or group name. They may be fully qualified names, which don't pass the validation.
This patch reverts the old one, and so we don't validate the strings anymore. Instead, let's not trust those strings, and thus don't log them. In some cases, we can log the UID or GID, which could be similarly useful. In others, we don't have that information, so just remove the string; that's less informative, but it's as much as we can do safely.
Fixes: 326889c (2024-10-22; "Fix coverity unbound buffer issues")
Closes: #1626
Reported-by: @nooreldeenmansour
Cc: @MarcinDigitic
Cc: @ikerexxe
Please be very skeptic when reviewing this patch. I'm less comfortable than usually. I believe the changes from username/groupname to uid/gid are good, but please revise. Also, I've removed much information, especially from the audit log. Please check that this is okay (maybe I can try keeping at least the UID/GID in the audit log too; let me know if you want that).
The regression affects all versions since (and including) 4.17.0.