Fix coverity unbound buffer issues

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: (shadow-maint#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.
MarcinDigitic · Oct 14, 2024 · dfa4448 · dfa4448
1 parent 4a15739
commit dfa4448
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 9 deletions.
diff --git a/src/chfn.c b/src/chfn.c
@@ -31,10 +31,11 @@
 /*@-exitarg@*/
 #include "exitcodes.h"
 #include "shadowlog.h"
+
 #include "string/sprintf/snprintf.h"
 #include "string/strcpy/strtcpy.h"
 #include "string/strdup/xstrdup.h"
-
+#include "chkname.h"
 
 /*
  * Global variables.
@@ -643,7 +644,12 @@ 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,
@@ -660,6 +666,7 @@ int main (int argc, char **argv)
 			         (unsigned long) getuid ()));
 			fail_exit (E_NOPERM);
 		}
+
 		user = xstrdup (pw->pw_name);
 	}
 

diff --git a/src/chsh.c b/src/chsh.c
@@ -32,6 +32,7 @@
 #include "shadowlog.h"
 #include "string/strcpy/strtcpy.h"
 #include "string/strdup/xstrdup.h"
+#include "chkname.h"
 
 #ifndef SHELLS_FILE
 #define SHELLS_FILE "/etc/shells"
@@ -499,7 +500,12 @@ 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,
@@ -516,6 +522,7 @@ int main (int argc, char **argv)
 			         (unsigned long) getuid ()));
 			fail_exit (1);
 		}
+
 		user = xstrdup (pw->pw_name);
 	}
 

diff --git a/src/newgrp.c b/src/newgrp.c
@@ -27,7 +27,7 @@
 #include "shadowlog.h"
 #include "string/sprintf/snprintf.h"
 #include "string/strdup/xstrdup.h"
-
+#include "chkname.h"
 
 /*
  * Global variables
@@ -483,7 +483,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 {
@@ -514,7 +520,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.
@@ -532,7 +544,7 @@ int main (int argc, char **argv)
 				        (unsigned long) pwd->pw_gid));
 				goto failure;
 			} else {
-				group = grp->gr_name;
+				group = xstrdup (grp->gr_name);
 			}
 		}
 	}
@@ -844,6 +856,7 @@ int main (int argc, char **argv)
 		              "changing", NULL, getuid (), 0);
 	}
 #endif
+
 	exit (EXIT_FAILURE);
 }
 
diff --git a/src/passwd.c b/src/passwd.c
@@ -36,7 +36,7 @@
 #include "string/strcpy/strtcpy.h"
 #include "string/strdup/xstrdup.h"
 #include "time/day_to_str.h"
-
+#include "chkname.h"
 
 /*
  * exit status values
@@ -910,7 +910,11 @@ 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;
 	}
@@ -1109,6 +1113,7 @@ 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));
+
 	closelog ();
 	if (!qflg) {
 		if (!anyflag) {