Skip to content

Commit

Permalink
Fix coverity unbound buffer issues
Browse files Browse the repository at this point in the history 
During coverity scan, there are reported four issues
with unbounded source buffer for each usage of input arg
directly with syslog function.

Sample coverity test report for chsh.c file:

 1. string_size_argv: argv contains strings with unknown size.
 int main (int argc, char **argv)
[...]
 4. var_assign_var: Assigning: user = argv[optind]. Both are now tainted.
 user = argv[optind];
[...]
CID 5771784: (#1 of 1): Unbounded source buffer (STRING_SIZE)
15. string_size: Passing string user of unknown size to syslog.
 SYSLOG ((LOG_INFO, "changed user '%s' shell to '%s'", user, loginsh));

Similar issue is reported three times more:
File: chfn.c, function: main, variable: user
File: passwd.c, function: main, variable: name
File: newgrp.c, function: main, variable: group

The proposed commit is a try to fix the reported issues.
  • Loading branch information
@MarcinDigitic
MarcinDigitic committed Jun 25, 2024
1 parent 47edcd3 commit 1351181
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 10 deletions.
13 changes: 11 additions & 2 deletions src/chfn.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
#include "shadowlog.h"
#include "string/sprintf.h"
#include "string/strtcpy.h"

#include "chkname.h"

/*
* Global variables.
Expand Down Expand Up @@ -648,11 +648,17 @@ int main (int argc, char **argv)
* name, or the name getlogin() returns.
*/
if (optind < argc) {
user = argv[optind];
if (!is_valid_user_name (argv[optind])) {
fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog);
fail_exit (E_NOPERM);
}
user = xstrdup (argv[optind]);

pw = xgetpwnam (user);
if (NULL == pw) {
fprintf (stderr, _("%s: user '%s' does not exist\n"), Prog,
user);
free (user);
fail_exit (E_NOPERM);
}
} else {
Expand All @@ -665,6 +671,7 @@ int main (int argc, char **argv)
(unsigned long) getuid ()));
fail_exit (E_NOPERM);
}

user = xstrdup (pw->pw_name);
}

Expand Down Expand Up @@ -708,6 +715,8 @@ int main (int argc, char **argv)

SYSLOG ((LOG_INFO, "changed user '%s' information", user));

free (user);

nscd_flush_cache ("passwd");
sssd_flush_cache (SSSD_DB_PASSWD);

Expand Down
12 changes: 11 additions & 1 deletion src/chsh.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "exitcodes.h"
#include "shadowlog.h"
#include "string/strtcpy.h"
#include "chkname.h"

#ifndef SHELLS_FILE
#define SHELLS_FILE "/etc/shells"
Expand Down Expand Up @@ -498,11 +499,17 @@ int main (int argc, char **argv)
* name, or the name getlogin() returns.
*/
if (optind < argc) {
user = argv[optind];
if (!is_valid_user_name (argv[optind])) {
fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog);
fail_exit (E_NOPERM);
}
user = xstrdup (argv[optind]);

pw = xgetpwnam (user);
if (NULL == pw) {
fprintf (stderr,
_("%s: user '%s' does not exist\n"), Prog, user);
free (user);
fail_exit (1);
}
} else {
Expand All @@ -515,6 +522,7 @@ int main (int argc, char **argv)
(unsigned long) getuid ()));
fail_exit (1);
}

user = xstrdup (pw->pw_name);
}

Expand Down Expand Up @@ -569,6 +577,8 @@ int main (int argc, char **argv)

SYSLOG ((LOG_INFO, "changed user '%s' shell to '%s'", user, loginsh));

free (user);

nscd_flush_cache ("passwd");
sssd_flush_cache (SSSD_DB_PASSWD);

Expand Down
26 changes: 22 additions & 4 deletions src/newgrp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
#include "exitcodes.h"
#include "shadowlog.h"
#include "string/sprintf.h"

#include "chkname.h"

/*
* Global variables
Expand Down Expand Up @@ -482,7 +482,13 @@ int main (int argc, char **argv)
* not "newgrp".
*/
if ((argc > 0) && (argv[0][0] != '-')) {
group = argv[0];
if (!is_valid_group_name (argv[optind])) {
fprintf (
stderr, _("%s: provided group is not a valid group name\n"),
Prog);
goto failure;
}
group = xstrdup (argv[optind]);
argc--;
argv++;
} else {
Expand Down Expand Up @@ -513,7 +519,13 @@ int main (int argc, char **argv)
usage ();
goto failure;
} else if (argv[0] != NULL) {
group = argv[0];
if (!is_valid_group_name (argv[optind])) {
fprintf (
stderr, _("%s: provided group is not a valid group name\n"),
Prog);
goto failure;
}
group = xstrdup (argv[optind]);
} else {
/*
* get the group file entry for her login group id.
Expand All @@ -531,7 +543,7 @@ int main (int argc, char **argv)
(unsigned long) pwd->pw_gid));
goto failure;
} else {
group = grp->gr_name;
group = xstrdup (grp->gr_name);
}
}
}
Expand Down Expand Up @@ -567,6 +579,7 @@ int main (int argc, char **argv)
"changing", NULL, getuid (), 0);
}
#endif
free (group);
exit (EXIT_FAILURE);
}
#endif /* HAVE_SETGROUPS */
Expand Down Expand Up @@ -722,6 +735,7 @@ int main (int argc, char **argv)
audit_logger (AUDIT_CHGRP_ID, Prog,
audit_buf, NULL, getuid (), 0);
#endif
free (group);
exit (EXIT_FAILURE);
}

Expand All @@ -732,6 +746,7 @@ int main (int argc, char **argv)
audit_logger (AUDIT_CHGRP_ID, Prog,
audit_buf, NULL, getuid (), 0);
#endif
free (group);
exit (EXIT_FAILURE);
}

Expand All @@ -748,6 +763,7 @@ int main (int argc, char **argv)
audit_buf, NULL, getuid (), 0);
#endif
perror (SHELL);
free (group);
exit ((errno == ENOENT) ? E_CMD_NOTFOUND : E_CMD_NOEXEC);
}

Expand Down Expand Up @@ -818,6 +834,7 @@ int main (int argc, char **argv)
* the previous environment which should be the user's login shell.
*/
err = shell (prog, initflag ? NULL : progbase, newenvp);
free (group);
exit ((err == ENOENT) ? E_CMD_NOTFOUND : E_CMD_NOEXEC);
/*@notreached@*/
failure:
Expand All @@ -843,6 +860,7 @@ int main (int argc, char **argv)
"changing", NULL, getuid (), 0);
}
#endif
free (group);
exit (EXIT_FAILURE);
}

23 changes: 20 additions & 3 deletions src/passwd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@
#include "shadowlog.h"
#include "string/strtcpy.h"
#include "time/day_to_str.h"


#include "chkname.h"

/*
* exit status values
Expand Down Expand Up @@ -907,7 +906,11 @@ int main (int argc, char **argv)
}
myname = xstrdup (pw->pw_name);
if (optind < argc) {
name = argv[optind];
if (!is_valid_user_name (argv[optind])) {
fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog);
fail_exit (E_NOPERM);
}
name = xstrdup (argv[optind]);
} else {
name = myname;
}
Expand All @@ -931,13 +934,15 @@ int main (int argc, char **argv)
(void) fprintf (stderr,
_("%s: Permission denied.\n"),
Prog);
free (name);
exit (E_NOPERM);
}
prefix_setpwent ();
while ( (pw = prefix_getpwent ()) != NULL ) {
print_status (pw);
}
prefix_endpwent ();
free (name);
exit (E_SUCCESS);
}
#if 0
Expand Down Expand Up @@ -971,6 +976,7 @@ int main (int argc, char **argv)

if (anyflag && !amroot) {
(void) fprintf (stderr, _("%s: Permission denied.\n"), Prog);
free (name);
exit (E_NOPERM);
}

Expand All @@ -979,6 +985,7 @@ int main (int argc, char **argv)
(void) fprintf (stderr,
_("%s: user '%s' does not exist\n"),
Prog, name);
free (name);
exit (E_NOPERM);
}
#ifdef WITH_SELINUX
Expand All @@ -991,6 +998,7 @@ int main (int argc, char **argv)
(void) fprintf(stderr,
_("%s: root is not authorized by SELinux to change the password of %s\n"),
Prog, name);
free (name);
exit (E_NOPERM);
}
#endif /* WITH_SELINUX */
Expand All @@ -1007,11 +1015,13 @@ int main (int argc, char **argv)
"can't view or modify password information for %s",
name));
closelog ();
free (name);
exit (E_NOPERM);
}

if (Sflg) {
print_status (pw);
free (name);
exit (E_SUCCESS);
}
if (!use_pam)
Expand All @@ -1025,6 +1035,7 @@ int main (int argc, char **argv)
(void) fprintf (stderr,
_("%s: Permission denied.\n"),
Prog);
free (name);
exit (E_NOPERM);
}
sp = pwd_to_spwd (pw);
Expand Down Expand Up @@ -1056,6 +1067,7 @@ int main (int argc, char **argv)
_("The password for %s is unchanged.\n"),
name);
closelog ();
free (name);
exit (E_NOPERM);
}
do_update_pwd = true;
Expand All @@ -1079,20 +1091,23 @@ int main (int argc, char **argv)
if (sflg) {
cp = agetpass_stdin ();
if (cp == NULL) {
free (name);
exit (E_FAILURE);
}
do_pam_passwd_non_interactive ("passwd", name, cp);
erase_pass (cp);
} else {
do_pam_passwd (name, qflg, kflg);
}
free (name);
exit (E_SUCCESS);
}
#endif /* USE_PAM */
if (setuid (0) != 0) {
(void) fputs (_("Cannot change ID to root.\n"), stderr);
SYSLOG ((LOG_ERR, "can't setuid(0)"));
closelog ();
free (name);
exit (E_NOPERM);
}
if (spw_file_present ()) {
Expand All @@ -1106,6 +1121,8 @@ int main (int argc, char **argv)
sssd_flush_cache (SSSD_DB_PASSWD | SSSD_DB_GROUP);

SYSLOG ((LOG_INFO, "password for '%s' changed by '%s'", name, myname));

free (name);
closelog ();
if (!qflg) {
if (!anyflag) {
Expand Down

0 comments on commit 1351181

Please sign in to comment.