From db098f2c90aa5e8a37b8de65335a5e348341633a Mon Sep 17 00:00:00 2001 From: Alejandro Colomar Date: Tue, 19 May 2026 13:16:45 +0200 Subject: [PATCH] lib/pam_pass_non_interactive.c, src/{chfn,chsh,newgrp,passwd}.c: Don'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: 326889ca (2024-10-22; "Fix coverity unbound buffer issues") Closes: Reported-by: Noor Eldeen Mansour Cc: Marcin Nowakowski Cc: Iker Pedrosa Signed-off-by: Alejandro Colomar --- lib/pam_pass_non_interactive.c | 8 ++-- src/chfn.c | 17 +++---- src/chsh.c | 15 ++----- src/newgrp.c | 82 ++++++++++------------------------ src/passwd.c | 42 +++++++---------- 5 files changed, 53 insertions(+), 111 deletions(-) diff --git a/lib/pam_pass_non_interactive.c b/lib/pam_pass_non_interactive.c index 6a8dcd78cf..92f8141f4b 100644 --- a/lib/pam_pass_non_interactive.c +++ b/lib/pam_pass_non_interactive.c @@ -126,8 +126,8 @@ int do_pam_passwd_non_interactive (const char *pam_service, ret = pam_start (pam_service, username, &non_interactive_pam_conv, &pamh); if (ret != PAM_SUCCESS) { fprintf (log_get_logfd(), - _("%s: (user %s) pam_start failure %d\n"), - log_get_progname(), username, ret); + _("%s: pam_start failure %d\n"), + log_get_progname(), ret); return 1; } @@ -135,9 +135,9 @@ int do_pam_passwd_non_interactive (const char *pam_service, ret = pam_chauthtok (pamh, 0); if (ret != PAM_SUCCESS) { fprintf (log_get_logfd(), - _("%s: (user %s) pam_chauthtok() failed, error:\n" + _("%s: pam_chauthtok() failed, error:\n" "%s\n"), - log_get_progname(), username, pam_strerror (pamh, ret)); + log_get_progname(), pam_strerror (pamh, ret)); } (void) pam_end (pamh, PAM_SUCCESS); diff --git a/src/chfn.c b/src/chfn.c index 71875253e9..5f3fdd4534 100644 --- a/src/chfn.c +++ b/src/chfn.c @@ -19,7 +19,6 @@ #include #include -#include "chkname.h" #include "defines.h" /*@-exitarg@*/ #include "exitcodes.h" @@ -434,8 +433,7 @@ static void update_gecos(const char *user, char *gecos, const struct option_flag pw = pw_locate (user); if (NULL == pw) { fprintf (stderr, - _("%s: user '%s' does not exist in %s\n"), - Prog, user, pw_dbname ()); + _("%s: user does not exist in %s\n"), Prog, pw_dbname()); fail_exit (E_NOPERM, process_selinux); } @@ -604,15 +602,10 @@ int main (int argc, char **argv) * name, or the name getlogin() returns. */ if (optind < argc) { - if (!is_valid_user_name (argv[optind])) { - fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog); - fail_exit (E_NOPERM, process_selinux); - } user = argv[optind]; pw = xgetpwnam (user); if (NULL == pw) { - fprintf (stderr, _("%s: user '%s' does not exist\n"), Prog, - user); + fprintf(stderr, _("%s: user does not exist\n"), Prog); fail_exit (E_NOPERM, process_selinux); } } else { @@ -641,7 +634,7 @@ int main (int argc, char **argv) * user interactively change them. */ if (!fflg && !rflg && !wflg && !hflg && !oflg) { - printf (_("Changing the user information for %s\n"), user); + printf (_("Changing information for user %ju\n"), (uintmax_t) pw->pw_uid); new_fields (); } @@ -661,14 +654,14 @@ int main (int argc, char **argv) p = seprintf(p, e, ",%s", slop); if (p == e || p == NULL) { - fprintf (stderr, _("%s: fields too long\n"), Prog); + fprintf(stderr, _("%s: fields too long\n"), Prog); fail_exit (E_NOPERM, process_selinux); } /* Rewrite the user's gecos in the passwd file */ update_gecos (user, new_gecos, &flags); - SYSLOG(LOG_INFO, "changed user '%s' information", user); + SYSLOG(LOG_INFO, "changed user %ju information", (uintmax_t) pw->pw_uid); nscd_flush_cache ("passwd"); sssd_flush_cache (SSSD_DB_PASSWD); diff --git a/src/chsh.c b/src/chsh.c index fafa9759d6..d334c9f007 100644 --- a/src/chsh.c +++ b/src/chsh.c @@ -17,7 +17,6 @@ #include #include -#include "chkname.h" #include "defines.h" /*@-exitarg@*/ #include "exitcodes.h" @@ -413,8 +412,7 @@ static void update_shell (const char *user, char *newshell, const struct option_ pw = pw_locate (user); if (NULL == pw) { fprintf (stderr, - _("%s: user '%s' does not exist in %s\n"), - Prog, user, pw_dbname ()); + _("%s: user does not exist in %s\n"), Prog, pw_dbname()); fail_exit (1, process_selinux); } @@ -493,15 +491,10 @@ int main (int argc, char **argv) * name, or the name getlogin() returns. */ if (optind < argc) { - if (!is_valid_user_name (argv[optind])) { - fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog); - fail_exit (1, process_selinux); - } user = argv[optind]; pw = xgetpwnam (user); if (NULL == pw) { - fprintf (stderr, - _("%s: user '%s' does not exist\n"), Prog, user); + fprintf(stderr, _("%s: user does not exist\n"), Prog); fail_exit (1, process_selinux); } } else { @@ -532,7 +525,7 @@ int main (int argc, char **argv) * interactively change it. */ if (!sflg) { - printf (_("Changing the login shell for %s\n"), user); + printf(_("Changing the login shell for %ju\n"), (uintmax_t) pw->pw_uid); new_fields (); } @@ -571,7 +564,7 @@ int main (int argc, char **argv) update_shell (user, loginsh, &flags); - SYSLOG(LOG_INFO, "changed user '%s' shell to '%s'", user, loginsh); + SYSLOG(LOG_INFO, "changed user %ju shell to '%s'", (uintmax_t) pw->pw_uid, loginsh); nscd_flush_cache ("passwd"); sssd_flush_cache (SSSD_DB_PASSWD); diff --git a/src/newgrp.c b/src/newgrp.c index afe4d85185..d05975063b 100644 --- a/src/newgrp.c +++ b/src/newgrp.c @@ -19,7 +19,6 @@ #include "agetpass.h" #include "alloc/malloc.h" #include "alloc/realloc.h" -#include "chkname.h" #include "defines.h" /*@-exitarg@*/ #include "exitcodes.h" @@ -60,10 +59,8 @@ static char audit_buf[80]; /* local function prototypes */ static void usage (void); -static void check_perms (const struct group *grp, - struct passwd *pwd, - const char *groupname); -static void syslog_sg (const char *name, const char *group); +static void check_perms(const struct group *grp, struct passwd *pwd); +static void syslog_sg(const char *name, gid_t group); /* * usage - print command usage message @@ -125,9 +122,8 @@ static /*@null@*/struct group *find_matching_group (const char *name, struct gro * * It will not return if the user could not be authenticated. */ -static void check_perms (const struct group *grp, - struct passwd *pwd, - const char *groupname) +static void +check_perms(const struct group *grp, struct passwd *pwd) { bool needspasswd = false; struct spwd *spwd; @@ -192,8 +188,8 @@ static void check_perms (const struct group *grp, _("%s: failed to crypt password with previous salt: %s\n"), Prog, strerrno()); SYSLOG(LOG_INFO, - "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); goto failure; } @@ -206,8 +202,8 @@ static void check_perms (const struct group *grp, audit_buf, NULL, getuid (), SHADOW_AUDIT_FAILURE); #endif SYSLOG(LOG_INFO, - "Invalid password for group '%s' from '%s'", - groupname, pwd->pw_name); + "Invalid password for group %ju from '%s'", + (uintmax_t) grp->gr_gid, pwd->pw_name); (void) sleep (1); (void) fputs (_("Invalid password.\n"), stderr); goto failure; @@ -236,7 +232,8 @@ static void check_perms (const struct group *grp, * The logout will also be logged when the user will quit the * sg/newgrp session. */ -static void syslog_sg (const char *name, const char *group) +static void +syslog_sg(const char *name, gid_t group) { const char *loginname = getlogin (); const char *tty = ttyname (0); @@ -259,8 +256,8 @@ static void syslog_sg (const char *name, const char *group) } tty = strprefix(tty, "/dev/") ?: tty; - SYSLOG(LOG_INFO, "user '%s' (login '%s' on %s) switched to group '%s'", - name, loginname, tty, group); + SYSLOG(LOG_INFO, "user '%s' (login '%s' on %s) switched to group %ju", + name, loginname, tty, (uintmax_t) group); #ifdef USE_PAM /* * We want to fork and exec the new shell in the child, leaving the @@ -298,15 +295,9 @@ static void syslog_sg (const char *name, const char *group) fprintf (stderr, _("%s: failure forking: %s\n"), is_newgrp ? "newgrp" : "sg", strerrno()); #ifdef WITH_AUDIT - if (group) { - audit_logger_with_group(AUDIT_CHGRP_ID, "changing", NULL, - getuid(), "new_group", group, - SHADOW_AUDIT_FAILURE); - } else { - audit_logger (AUDIT_CHGRP_ID, - "changing", NULL, getuid(), - SHADOW_AUDIT_FAILURE); - } + audit_logger(AUDIT_CHGRP_ID, + "changing", NULL, getuid(), + SHADOW_AUDIT_FAILURE); #endif exit (EXIT_FAILURE); } else if (child != 0) { @@ -480,12 +471,6 @@ int main (int argc, char **argv) * not "newgrp". */ if ((argc > 0) && (argv[0][0] != '-')) { - if (!is_valid_group_name (argv[0])) { - fprintf ( - stderr, _("%s: provided group is not a valid group name\n"), - Prog); - goto failure; - } group = argv[0]; argc--; argv++; @@ -516,12 +501,6 @@ int main (int argc, char **argv) usage (); goto failure; } else if (argv[0] != NULL) { - if (!is_valid_group_name (argv[0])) { - fprintf ( - stderr, _("%s: provided group is not a valid group name\n"), - Prog); - goto failure; - } group = argv[0]; } else { /* @@ -555,13 +534,8 @@ int main (int argc, char **argv) if (gids == NULL) { perror("agetgroups"); #ifdef WITH_AUDIT - if (group) { - audit_logger_with_group(AUDIT_CHGRP_ID, "changing", NULL, getuid(), - "new_group", group, SHADOW_AUDIT_FAILURE); - } else { - audit_logger(AUDIT_CHGRP_ID, - "changing", NULL, getuid(), SHADOW_AUDIT_FAILURE); - } + audit_logger(AUDIT_CHGRP_ID, + "changing", NULL, getuid(), SHADOW_AUDIT_FAILURE); #endif exit(EXIT_FAILURE); } @@ -608,7 +582,7 @@ int main (int argc, char **argv) */ grp = getgrnam (group); /* local, no need for xgetgrnam */ if (NULL == grp) { - fprintf (stderr, _("%s: group '%s' does not exist\n"), Prog, group); + fprintf(stderr, _("%s: group does not exist\n"), Prog); goto failure; } @@ -645,19 +619,16 @@ int main (int argc, char **argv) } #endif - /* - * Check if the user is allowed to access this group. - */ - if (!is_member) { - check_perms (grp, pwd, group); - } + // Check if the user is allowed to access this group. + if (!is_member) + check_perms(grp, pwd); /* * all successful validations pass through this point. The group id * will be set, and the group added to the concurrent groupset. */ if (getdef_bool ("SYSLOG_SG_ENAB")) { - syslog_sg (name, group); + syslog_sg (name, grp->gr_gid); } gid = grp->gr_gid; @@ -814,14 +785,7 @@ int main (int argc, char **argv) */ closelog (); #ifdef WITH_AUDIT - if (NULL != group) { - audit_logger_with_group(AUDIT_CHGRP_ID, "changing", NULL, - getuid(), "new_group", group, - SHADOW_AUDIT_FAILURE); - } else { - audit_logger (AUDIT_CHGRP_ID, - "changing", NULL, getuid (), 0); - } + audit_logger(AUDIT_CHGRP_ID, "changing", NULL, getuid(), 0); #endif exit (EXIT_FAILURE); } diff --git a/src/passwd.c b/src/passwd.c index b2cac80b00..b61ebd45e6 100644 --- a/src/passwd.c +++ b/src/passwd.c @@ -22,7 +22,6 @@ #include "agetpass.h" #include "atoi/a2i.h" -#include "chkname.h" #include "defines.h" #include "getdef.h" #include "nscd.h" @@ -993,10 +992,6 @@ main(int argc, char **argv) } myname = xstrdup (pw->pw_name); if (optind < argc) { - if (!is_valid_user_name (argv[optind])) { - fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog); - fail_exit (E_NOPERM, process_selinux); - } name = argv[optind]; } else { name = myname; @@ -1072,9 +1067,7 @@ main(int argc, char **argv) pw = xprefix_getpwnam (name); if (NULL == pw) { - (void) fprintf (stderr, - _("%s: user '%s' does not exist\n"), - Prog, name); + fprintf(stderr, _("%s: user does not exist\n"), Prog); exit (E_NOPERM); } #ifdef WITH_SELINUX @@ -1088,11 +1081,11 @@ main(int argc, char **argv) SHADOW_AUDIT_FAILURE); #endif /* WITH_AUDIT */ SYSLOG(LOG_ALERT, - "root is not authorized by SELinux to change the password of %s", - name); - (void) fprintf(stderr, - _("%s: root is not authorized by SELinux to change the password of %s\n"), - Prog, name); + "root is not authorized by SELinux to change the password of %ju", + (uintmax_t) pw->pw_uid); + fprintf(stderr, + _("%s: root is not authorized by SELinux to change the password of %ju\n"), + Prog, (uintmax_t) pw->pw_uid); exit (E_NOPERM); } #endif /* WITH_SELINUX */ @@ -1108,12 +1101,12 @@ main(int argc, char **argv) NULL, pw->pw_uid, SHADOW_AUDIT_FAILURE); #endif /* WITH_AUDIT */ - (void) fprintf (stderr, - _("%s: You may not view or modify password information for %s.\n"), - Prog, name); + fprintf(stderr, + _("%s: You may not view or modify password information for %ju.\n"), + Prog, (uintmax_t) pw->pw_uid); SYSLOG(LOG_WARN, - "can't view or modify password information for %s", - name); + "can't view or modify password information for %ju", + (uintmax_t) pw->pw_uid); closelog (); exit (E_NOPERM); } @@ -1155,14 +1148,13 @@ main(int argc, char **argv) /* * Let the user know whose password is being changed. */ - if (!qflg) { - (void) printf (_("Changing password for %s\n"), name); - } + if (!qflg) + printf(_("Changing password for %ju\n"), (uintmax_t) pw->pw_uid); if (new_password (pw) != 0) { - (void) fprintf (stderr, - _("The password for %s is unchanged.\n"), - name); + fprintf(stderr, + _("The password for %ju is unchanged.\n"), + (uintmax_t) pw->pw_uid); closelog (); exit (E_NOPERM); } @@ -1211,7 +1203,7 @@ main(int argc, char **argv) nscd_flush_cache ("group"); sssd_flush_cache (SSSD_DB_PASSWD | SSSD_DB_GROUP); - SYSLOG(LOG_INFO, "password for '%s' changed by '%s'", name, myname); + SYSLOG(LOG_INFO, "password for %ju changed by '%s'", (uintmax_t) pw->pw_uid, myname); closelog (); if (!qflg) { if (!anyflag) {