shadow-utils/shadow-4.15.0-sast-fixes.patch

From 4c16416ebc5f0958d58a1ea1e7890eafd9f8bb75 Mon Sep 17 00:00:00 2001
From: Iker Pedrosa <ipedrosa@redhat.com>
Date: Wed, 15 May 2024 12:25:51 +0200
Subject: [PATCH 01/16] port: fix OVERRUN (CWE-119)

```
shadow-4.15.0/lib/port.c:154:2: alias: Assigning: "port.pt_names" = "ttys". "port.pt_names" now points to element 0 of "ttys" (which consists of 65 8-byte elements).
shadow-4.15.0/lib/port.c:155:2: cond_const: Checking "j < 64" implies that "j" is 64 on the false branch.
shadow-4.15.0/lib/port.c:175:2: overrun-local: Overrunning array of 65 8-byte elements at element index 65 (byte offset 527) by dereferencing pointer "port.pt_names + (j + 1)".
173|           *cp = '\0';
174|           cp++;
175|->         port.pt_names[j + 1] = NULL;
176|
177|           /*
```

Resolves: https://issues.redhat.com/browse/RHEL-35383

Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
---
 lib/port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/port.c b/lib/port.c
index 05b95651..60ff8989 100644
--- a/lib/port.c
+++ b/lib/port.c
@@ -168,7 +168,7 @@ again:
 	}
 	*cp = '\0';
 	cp++;
-	port.pt_names[j + 1] = NULL;
+	port.pt_names[j] = NULL;
 
 	/*
 	 * Get the list of user names.  It is the second colon
-- 
2.45.1


From f8fc6371f69930bbd5801284256e182ba35ced2a Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 14:05:31 +0200
Subject: [PATCH 02/16] src/useradd.c: set_defaults(): Fix order of clean-ups

Resources should be freed in the inverse order of the allocation.
This refactor prepares for the following commits, which fix some leaks.

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/useradd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/useradd.c b/src/useradd.c
index 88d8ab7f..56a74559 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -745,10 +745,9 @@ static int set_defaults (void)
 	         def_create_mail_spool, def_log_init));
 	ret = 0;
     setdef_err:
-	free(new_file);
-	if (prefix[0]) {
+	if (prefix[0])
 		free(default_file);
-	}
+	free(new_file);
 
 	return ret;
 }
-- 
2.45.1


From 37ae8827a0869ee4a723954c3c9e7c48165d9b50 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 14:28:50 +0200
Subject: [PATCH 03/16] src/useradd.c: set_defaults(): Rename goto label

This will help add other labels in the following commits.

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/useradd.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/src/useradd.c b/src/useradd.c
index 56a74559..bc72e6bc 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -558,7 +558,7 @@ static int set_defaults (void)
 			fprintf(stderr,
 			        _("%s: cannot create new defaults file: %s\n"),
 			        Prog, strerror(errno));
-			goto setdef_err;
+			goto err_free_def;
 		}
 	}
 
@@ -567,7 +567,7 @@ static int set_defaults (void)
 		fprintf (stderr,
 			_("%s: cannot create directory for defaults file\n"),
 			Prog);
-		goto setdef_err;
+		goto err_free_def;
 	}
 
 	ret = mkdir(dirname(new_file_dup), 0755);
@@ -576,7 +576,7 @@ static int set_defaults (void)
 			_("%s: cannot create directory for defaults file\n"),
 			Prog);
 		free(new_file_dup);
-		goto setdef_err;
+		goto err_free_def;
 	}
 	free(new_file_dup);
 
@@ -588,7 +588,7 @@ static int set_defaults (void)
 		fprintf (stderr,
 		         _("%s: cannot create new defaults file\n"),
 		         Prog);
-		goto setdef_err;
+		goto err_free_def;
 	}
 
 	ofp = fdopen (ofd, "w");
@@ -596,7 +596,7 @@ static int set_defaults (void)
 		fprintf (stderr,
 		         _("%s: cannot open new defaults file\n"),
 		         Prog);
-		goto setdef_err;
+		goto err_free_def;
 	}
 
 	/*
@@ -623,7 +623,7 @@ static int set_defaults (void)
 				         _("%s: line too long in %s: %s..."),
 				         Prog, default_file, buf);
 				(void) fclose (ifp);
-				goto setdef_err;
+				goto err_free_def;
 			}
 		}
 
@@ -702,9 +702,10 @@ static int set_defaults (void)
 	(void) fflush (ofp);
 	if (   (ferror (ofp) != 0)
 	    || (fsync (fileno (ofp)) != 0)
-	    || (fclose (ofp) != 0)) {
+	    || (fclose (ofp) != 0))
+	{
 		unlink (new_file);
-		goto setdef_err;
+		goto err_free_def;
 	}
 
 	/*
@@ -718,7 +719,7 @@ static int set_defaults (void)
 		         _("%s: Cannot create backup file (%s): %s\n"),
 		         Prog, buf, strerror (err));
 		unlink (new_file);
-		goto setdef_err;
+		goto err_free_def;
 	}
 
 	/*
@@ -729,7 +730,7 @@ static int set_defaults (void)
 		fprintf (stderr,
 		         _("%s: rename: %s: %s\n"),
 		         Prog, new_file, strerror (err));
-		goto setdef_err;
+		goto err_free_def;
 	}
 #ifdef WITH_AUDIT
 	audit_logger (AUDIT_USYS_CONFIG, Prog,
@@ -744,7 +745,8 @@ static int set_defaults (void)
 	         def_inactive, def_expire, def_template,
 	         def_create_mail_spool, def_log_init));
 	ret = 0;
-    setdef_err:
+
+err_free_def:
 	if (prefix[0])
 		free(default_file);
 	free(new_file);
-- 
2.45.1


From 701fe4cf1aeac9e66fa949369c91d135dbf375d2 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 13:10:46 +0200
Subject: [PATCH 04/16] src/useradd.c: set_defaults(): Do not free(3) the
 result of asprintf(3) if it failed
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

See asprintf(3):

RETURN VALUE
     When successful,  these  functions  return  the  number  of  bytes
     printed, just like sprintf(3).  If memory allocation wasn’t possi‐
     ble,  or  some other error occurs, these functions will return -1,
     and the contents of strp are undefined.

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/useradd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/useradd.c b/src/useradd.c
index bc72e6bc..6a3edfe3 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -558,7 +558,7 @@ static int set_defaults (void)
 			fprintf(stderr,
 			        _("%s: cannot create new defaults file: %s\n"),
 			        Prog, strerror(errno));
-			goto err_free_def;
+			goto err_free_new;
 		}
 	}
 
@@ -749,6 +749,7 @@ static int set_defaults (void)
 err_free_def:
 	if (prefix[0])
 		free(default_file);
+err_free_new:
 	free(new_file);
 
 	return ret;
-- 
2.45.1


From a74c4b6ae124a55cd272e574e0d056102f331e17 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 13:14:31 +0200
Subject: [PATCH 05/16] src/useradd.c: De-duplicate code

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/useradd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/useradd.c b/src/useradd.c
index 6a3edfe3..ad2676c1 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -571,14 +571,13 @@ static int set_defaults (void)
 	}
 
 	ret = mkdir(dirname(new_file_dup), 0755);
+	free(new_file_dup);
 	if (-1 == ret && EEXIST != errno) {
 		fprintf (stderr,
 			_("%s: cannot create directory for defaults file\n"),
 			Prog);
-		free(new_file_dup);
 		goto err_free_def;
 	}
-	free(new_file_dup);
 
 	/*
 	 * Create a temporary file to copy the new output to.
-- 
2.45.1


From e7d1508e076bbf4053faacc0370c6fe43d9c8f04 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 13:40:58 +0200
Subject: [PATCH 06/16] src/useradd.c: Add fmkstemp() to fix file-descriptor
 leak

This function creates a temporary file, and returns a FILE pointer to
it.  This avoids dealing with both a file descriptor and a FILE pointer,
and correctly deallocating the resources on error.

The code before this patch was leaking the file descriptor if fdopen(3)
failed.

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/useradd.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/src/useradd.c b/src/useradd.c
index ad2676c1..e0238457 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -238,6 +238,9 @@ static void create_home (void);
 static void create_mail (void);
 static void check_uid_range(int rflg, uid_t user_id);
 
+static FILE *fmkstemp(char *template);
+
+
 /*
  * fail_exit - undo as much as possible
  */
@@ -524,7 +527,6 @@ static void show_defaults (void)
  */
 static int set_defaults (void)
 {
-	int   ofd;
 	int   ret = -1;
 	bool  out_group = false;
 	bool  out_groups = false;
@@ -582,15 +584,7 @@ static int set_defaults (void)
 	/*
 	 * Create a temporary file to copy the new output to.
 	 */
-	ofd = mkstemp (new_file);
-	if (-1 == ofd) {
-		fprintf (stderr,
-		         _("%s: cannot create new defaults file\n"),
-		         Prog);
-		goto err_free_def;
-	}
-
-	ofp = fdopen (ofd, "w");
+	ofp = fmkstemp(new_file);
 	if (NULL == ofp) {
 		fprintf (stderr,
 		         _("%s: cannot open new defaults file\n"),
@@ -2752,3 +2746,23 @@ int main (int argc, char **argv)
 	return E_SUCCESS;
 }
 
+
+static FILE *
+fmkstemp(char *template)
+{
+	int   fd;
+	FILE  *fp;
+
+	fd = mkstemp(template);
+	if (fd == -1)
+		return NULL;
+
+	fp = fdopen(fd, "w");
+	if (fp == NULL) {
+		close(fd);
+		unlink(template);
+		return NULL;
+	}
+
+	return fp;
+}
-- 
2.45.1


From 1ee066ae1e5b39ac42120ad0f6f8af0f102db952 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 13:52:07 +0200
Subject: [PATCH 07/16] src/useradd.c: set_defaults(): Fix FILE* leak

Report:
> shadow-4.15.0/src/useradd.c:575:2: alloc_fn: Storage is returned from allocation function "fdopen".
> shadow-4.15.0/src/useradd.c:575:2: var_assign: Assigning: "ofp" = storage returned from "fdopen(ofd, "w")".
> shadow-4.15.0/src/useradd.c:734:2: leaked_storage: Variable "ofp" going out of scope leaks the storage it points to.
> 732|           }
> 733|
> 734|->         return ret;
> 735|   }
> 736|

Link: <https://issues.redhat.com/browse/RHEL-35383>
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/useradd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/useradd.c b/src/useradd.c
index e0238457..347334a6 100644
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -615,7 +615,8 @@ static int set_defaults (void)
 				fprintf (stderr,
 				         _("%s: line too long in %s: %s..."),
 				         Prog, default_file, buf);
-				(void) fclose (ifp);
+				fclose(ifp);
+				fclose(ofp);
 				goto err_free_def;
 			}
 		}
-- 
2.45.1


From 151f14ad69de8100d25c1974947d53ae40d1448a Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 13:52:15 +0200
Subject: [PATCH 08/16] src/usermod.c: Reduce scope of local variables

Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/usermod.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/usermod.c b/src/usermod.c
index 0fcf0325..57b58f5b 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -687,11 +687,8 @@ fail_exit (int code)
 
 static void update_group (void)
 {
-	bool is_member;
-	bool was_member;
-	bool changed;
-	const struct group *grp;
-	struct group *ngrp;
+	bool                changed;
+	const struct group  *grp;
 
 	changed = false;
 
@@ -700,6 +697,9 @@ static void update_group (void)
 	 * the user is a member of.
 	 */
 	while ((grp = gr_next ()) != NULL) {
+		bool          is_member;
+		bool          was_member;
+		struct group  *ngrp;
 		/*
 		 * See if the user specified this group as one of their
 		 * concurrent groups.
@@ -799,12 +799,8 @@ static void update_group (void)
 #ifdef SHADOWGRP
 static void update_gshadow (void)
 {
-	bool is_member;
-	bool was_member;
-	bool was_admin;
-	bool changed;
-	const struct sgrp *sgrp;
-	struct sgrp *nsgrp;
+	bool               changed;
+	const struct sgrp  *sgrp;
 
 	changed = false;
 
@@ -813,6 +809,10 @@ static void update_gshadow (void)
 	 * that the user is a member of.
 	 */
 	while ((sgrp = sgr_next ()) != NULL) {
+		bool         is_member;
+		bool         was_member;
+		bool         was_admin;
+		struct sgrp  *nsgrp;
 
 		/*
 		 * See if the user was a member of this group
-- 
2.45.1


From b089a63ab38f69c32d099320fe8181802f7f4092 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 13:49:34 +0200
Subject: [PATCH 09/16] src/usermod.c: Rename update_group() =>
 update_group_file()

Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/usermod.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/usermod.c b/src/usermod.c
index 57b58f5b..aaa83d7d 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -178,7 +178,7 @@ NORETURN static void usage (int status);
 static void new_pwent (struct passwd *);
 static void new_spent (struct spwd *);
 NORETURN static void fail_exit (int);
-static void update_group (void);
+static void update_group_file(void);
 
 #ifdef SHADOWGRP
 static void update_gshadow (void);
@@ -685,7 +685,8 @@ fail_exit (int code)
 }
 
 
-static void update_group (void)
+static void
+update_group_file(void)
 {
 	bool                changed;
 	const struct group  *grp;
@@ -950,7 +951,7 @@ static void update_gshadow (void)
  */
 static void grp_update (void)
 {
-	update_group ();
+	update_group_file();
 #ifdef SHADOWGRP
 	if (is_shadow_grp) {
 		update_gshadow ();
-- 
2.45.1


From 81bc78ec5cdd59790bc7c591c9d1f66bd4d7b78e Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 02:11:22 +0200
Subject: [PATCH 10/16] src/usermod.c: Rename update_gshadow() =>
 update_gshadow_file()

Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/usermod.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/usermod.c b/src/usermod.c
index aaa83d7d..3048f801 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -181,7 +181,7 @@ NORETURN static void fail_exit (int);
 static void update_group_file(void);
 
 #ifdef SHADOWGRP
-static void update_gshadow (void);
+static void update_gshadow_file(void);
 #endif
 static void grp_update (void);
 
@@ -798,7 +798,8 @@ update_group_file(void)
 }
 
 #ifdef SHADOWGRP
-static void update_gshadow (void)
+static void
+update_gshadow_file(void)
 {
 	bool               changed;
 	const struct sgrp  *sgrp;
@@ -954,7 +955,7 @@ static void grp_update (void)
 	update_group_file();
 #ifdef SHADOWGRP
 	if (is_shadow_grp) {
-		update_gshadow ();
+		update_gshadow_file();
 	}
 #endif
 }
-- 
2.45.1


From 61964aa06b9e6e0643a6519f64290f18ac04867f Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 13:54:06 +0200
Subject: [PATCH 11/16] src/usermod.c: update_group_file(): Fix RESOURCE_LEAK
 (CWE-772)

Report:
> shadow-4.15.0/src/usermod.c:734:3: alloc_fn: Storage is returned from allocation function "__gr_dup".
> shadow-4.15.0/src/usermod.c:734:3: var_assign: Assigning: "ngrp" = storage returned from "__gr_dup(grp)".
> shadow-4.15.0/src/usermod.c:815:1: leaked_storage: Variable "ngrp" going out of scope leaks the storage it points to.
> 813|                   gr_free(ngrp);
> 814|           }
> 815|-> }
> 816|
> 817|   #ifdef SHADOWGRP

Link: https://issues.redhat.com/browse/RHEL-35383
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/usermod.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/usermod.c b/src/usermod.c
index 3048f801..e0cfdd83 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -780,9 +780,8 @@ update_group_file(void)
 			SYSLOG ((LOG_INFO, "add '%s' to group '%s'",
 			         user_newname, ngrp->gr_name));
 		}
-		if (!changed) {
-			continue;
-		}
+		if (!changed)
+			goto free_ngrp;
 
 		changed = false;
 		if (gr_update (ngrp) == 0) {
@@ -793,6 +792,7 @@ update_group_file(void)
 			fail_exit (E_GRP_UPDATE);
 		}
 
+free_ngrp:
 		gr_free(ngrp);
 	}
 }
-- 
2.45.1


From 71a3238b7996285fc3c8dec841244ba95d663fa5 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 02:15:15 +0200
Subject: [PATCH 12/16] src/usermod.c: update_gshadow_file(): Fix RESOURCE_LEAK
 (CWE-772)

Report:
> shadow-4.15.0/src/usermod.c:864:3: alloc_fn: Storage is returned from allocation function "__sgr_dup".
> shadow-4.15.0/src/usermod.c:864:3: var_assign: Assigning: "nsgrp" = storage returned from "__sgr_dup(sgrp)".
> shadow-4.15.0/src/usermod.c:964:1: leaked_storage: Variable "nsgrp" going out of scope leaks the storage it points to.
> 962|                   free (nsgrp);
> 963|           }
> 964|-> }
> 965|   #endif                                /* SHADOWGRP */
> 966|

Link: https://issues.redhat.com/browse/RHEL-35383
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/usermod.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/usermod.c b/src/usermod.c
index e0cfdd83..bb5d3535 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -921,9 +921,8 @@ update_gshadow_file(void)
 			SYSLOG ((LOG_INFO, "add '%s' to shadow group '%s'",
 			         user_newname, nsgrp->sg_name));
 		}
-		if (!changed) {
-			continue;
-		}
+		if (!changed)
+			goto free_nsgrp;
 
 		changed = false;
 
@@ -939,6 +938,7 @@ update_gshadow_file(void)
 			fail_exit (E_GRP_UPDATE);
 		}
 
+free_nsgrp:
 		free (nsgrp);
 	}
 }
-- 
2.45.1


From 68d42a8fbe42b89cf13d3f672ad8502dbaf05835 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 14:02:54 +0200
Subject: [PATCH 13/16] src/usermod.c: update_group_file(): Reduce scope of
 local variable

After _every_ iteration, 'changed' is always 'false'.  We don't need to
have it outside of the loop.

See:

$ grepc update_group_file . \
| grep -e changed -e goto -e continue -e break -e free_ngrp -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
	bool                changed;
	changed = false;
	while ((grp = gr_next ()) != NULL) {
		if (!was_member && !is_member) {
			continue;
		}
		if (was_member) {
			if ((!Gflg) || is_member) {
				if (lflg) {
					changed = true;
				}
			} else {
				changed = true;
			}
		} else if (is_member) {
			changed = true;
		}
		if (!changed)
			goto free_ngrp;
		changed = false;
free_ngrp:
	}
}

This was already true in the commit that introduced the code:

$ git show 45c6603cc:src/usermod.c \
| grepc update_group \
| grep -e changed -e goto -e break -e continue -e '\<if\>' -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
	int changed;
	changed = 0;
	while ((grp = gr_next())) {
		 * See if the user specified this group as one of their
		if (!was_member && !is_member)
			continue;
		if (was_member && (!Gflg || is_member)) {
			if (lflg) {
				changed = 1;
			}
		} else if (was_member && Gflg && !is_member) {
			changed = 1;
		} else if (!was_member && Gflg && is_member) {
			changed = 1;
		}
		if (!changed)
			continue;
		changed = 0;
	}
}

Fixes: 45c6603cc86c ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/usermod.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/usermod.c b/src/usermod.c
index bb5d3535..30f47b8a 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -688,19 +688,20 @@ fail_exit (int code)
 static void
 update_group_file(void)
 {
-	bool                changed;
 	const struct group  *grp;
 
-	changed = false;
-
 	/*
 	 * Scan through the entire group file looking for the groups that
 	 * the user is a member of.
 	 */
 	while ((grp = gr_next ()) != NULL) {
+		bool          changed;
 		bool          is_member;
 		bool          was_member;
 		struct group  *ngrp;
+
+		changed = false;
+
 		/*
 		 * See if the user specified this group as one of their
 		 * concurrent groups.
@@ -783,7 +784,6 @@ update_group_file(void)
 		if (!changed)
 			goto free_ngrp;
 
-		changed = false;
 		if (gr_update (ngrp) == 0) {
 			fprintf (stderr,
 			         _("%s: failed to prepare the new %s entry '%s'\n"),
-- 
2.45.1


From da77a82ecbc90e89808f143e7fa2abb7650f50d7 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 02:19:46 +0200
Subject: [PATCH 14/16] src/usermod.c: update_gshadow_file(): Reduce scope of
 local variable

After _every_ iteration, 'changed' is always 'false'.  We don't need to
have it outside of the loop.

See:

$ grepc update_gshadow_file . \
| grep -e changed -e goto -e continue -e break -e free_ngrp -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
	bool               changed;
	changed = false;
	while ((sgrp = sgr_next ()) != NULL) {
		if (!was_member && !was_admin && !is_member) {
			continue;
		}
		if (was_admin && lflg) {
			changed = true;
		}
		if (was_member) {
			if ((!Gflg) || is_member) {
				if (lflg) {
					changed = true;
				}
			} else {
				changed = true;
			}
		} else if (is_member) {
			changed = true;
		}
		if (!changed)
			goto free_nsgrp;
		changed = false;
	}
}

This was already true in the commit that introduced the code:

$ git show 45c6603cc:src/usermod.c \
| grepc update_gshadow \
| grep -e changed -e goto -e break -e continue -e '\<if\>' -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
	int changed;
	changed = 0;
	while ((sgrp = sgr_next())) {
		 * See if the user was a member of this group
		 * See if the user was an administrator of this group
		 * See if the user specified this group as one of their
		if (!was_member && !was_admin && !is_member)
			continue;
		if (was_admin && lflg) {
			changed = 1;
		}
		if (was_member && (!Gflg || is_member)) {
			if (lflg) {
				changed = 1;
			}
		} else if (was_member && Gflg && !is_member) {
			changed = 1;
		} else if (!was_member && Gflg && is_member) {
			changed = 1;
		}
		if (!changed)
			continue;
		changed = 0;
	}
}

Fixes: 45c6603cc86c ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/usermod.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/usermod.c b/src/usermod.c
index 30f47b8a..7b1e0581 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -801,21 +801,21 @@ free_ngrp:
 static void
 update_gshadow_file(void)
 {
-	bool               changed;
 	const struct sgrp  *sgrp;
 
-	changed = false;
-
 	/*
 	 * Scan through the entire shadow group file looking for the groups
 	 * that the user is a member of.
 	 */
 	while ((sgrp = sgr_next ()) != NULL) {
+		bool         changed;
 		bool         is_member;
 		bool         was_member;
 		bool         was_admin;
 		struct sgrp  *nsgrp;
 
+		changed = false;
+
 		/*
 		 * See if the user was a member of this group
 		 */
@@ -924,8 +924,6 @@ update_gshadow_file(void)
 		if (!changed)
 			goto free_nsgrp;
 
-		changed = false;
-
 		/*
 		 * Update the group entry to reflect the changes.
 		 */
-- 
2.45.1


From adf37cccd0fa4ce7d05644514b0af57fe71905c3 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 16 May 2024 14:12:09 +0200
Subject: [PATCH 15/16] src/usermod.c: update_group(): Add helper function

Keep the while loop in the outer function, and move the iteration code
to this new helper.  This makes it a bit more readable.

Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/usermod.c | 167 ++++++++++++++++++++++++++------------------------
 1 file changed, 87 insertions(+), 80 deletions(-)

diff --git a/src/usermod.c b/src/usermod.c
index 7b1e0581..4ea11376 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -179,6 +179,7 @@ static void new_pwent (struct passwd *);
 static void new_spent (struct spwd *);
 NORETURN static void fail_exit (int);
 static void update_group_file(void);
+static void update_group(const struct group *grp);
 
 #ifdef SHADOWGRP
 static void update_gshadow_file(void);
@@ -694,109 +695,115 @@ update_group_file(void)
 	 * Scan through the entire group file looking for the groups that
 	 * the user is a member of.
 	 */
-	while ((grp = gr_next ()) != NULL) {
-		bool          changed;
-		bool          is_member;
-		bool          was_member;
-		struct group  *ngrp;
+	while ((grp = gr_next()) != NULL)
+		update_group(grp);
+}
 
-		changed = false;
 
-		/*
-		 * See if the user specified this group as one of their
-		 * concurrent groups.
-		 */
-		was_member = is_on_list (grp->gr_mem, user_name);
-		is_member = Gflg && (   (was_member && aflg)
-		                     || is_on_list (user_groups, grp->gr_name));
+static void
+update_group(const struct group *grp)
+{
+	bool          changed;
+	bool          is_member;
+	bool          was_member;
+	struct group  *ngrp;
 
-		if (!was_member && !is_member) {
-			continue;
-		}
+	changed = false;
 
-		/*
-		* If rflg+Gflg  is passed in AKA -rG invert is_member flag, which removes
-		* mentioned groups while leaving the others.
-		*/
-		if (Gflg && rflg) {
-			is_member = !is_member;
-		}
+	/*
+	 * See if the user specified this group as one of their
+	 * concurrent groups.
+	 */
+	was_member = is_on_list (grp->gr_mem, user_name);
+	is_member = Gflg && (   (was_member && aflg)
+			     || is_on_list (user_groups, grp->gr_name));
 
-		ngrp = __gr_dup (grp);
-		if (NULL == ngrp) {
-			fprintf (stderr,
-			         _("%s: Out of memory. Cannot update %s.\n"),
-			         Prog, gr_dbname ());
-			fail_exit (E_GRP_UPDATE);
-		}
+	if (!was_member && !is_member)
+		return;
 
-		if (was_member) {
-			if ((!Gflg) || is_member) {
-				/* User was a member and is still a member
-				 * of this group.
-				 * But the user might have been renamed.
-				 */
-				if (lflg) {
-					ngrp->gr_mem = del_list (ngrp->gr_mem,
-					                         user_name);
-					ngrp->gr_mem = add_list (ngrp->gr_mem,
-					                         user_newname);
-					changed = true;
-#ifdef WITH_AUDIT
-					audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
-					              "changing group member",
-					              user_newname, AUDIT_NO_ID, 1);
-#endif
-					SYSLOG ((LOG_INFO,
-					         "change '%s' to '%s' in group '%s'",
-					         user_name, user_newname,
-					         ngrp->gr_name));
-				}
-			} else {
-				/* User was a member but is no more a
-				 * member of this group.
-				 */
-				ngrp->gr_mem = del_list (ngrp->gr_mem, user_name);
+	/*
+	* If rflg+Gflg  is passed in AKA -rG invert is_member flag, which removes
+	* mentioned groups while leaving the others.
+	*/
+	if (Gflg && rflg) {
+		is_member = !is_member;
+	}
+
+	ngrp = __gr_dup (grp);
+	if (NULL == ngrp) {
+		fprintf (stderr,
+			 _("%s: Out of memory. Cannot update %s.\n"),
+			 Prog, gr_dbname ());
+		fail_exit (E_GRP_UPDATE);
+	}
+
+	if (was_member) {
+		if ((!Gflg) || is_member) {
+			/* User was a member and is still a member
+			 * of this group.
+			 * But the user might have been renamed.
+			 */
+			if (lflg) {
+				ngrp->gr_mem = del_list (ngrp->gr_mem,
+							 user_name);
+				ngrp->gr_mem = add_list (ngrp->gr_mem,
+							 user_newname);
 				changed = true;
 #ifdef WITH_AUDIT
 				audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
-				              "removing group member",
-				              user_name, AUDIT_NO_ID, 1);
+					      "changing group member",
+					      user_newname, AUDIT_NO_ID, 1);
 #endif
 				SYSLOG ((LOG_INFO,
-				         "delete '%s' from group '%s'",
-				         user_name, ngrp->gr_name));
+					 "change '%s' to '%s' in group '%s'",
+					 user_name, user_newname,
+					 ngrp->gr_name));
 			}
-		} else if (is_member) {
-			/* User was not a member but is now a member this
-			 * group.
+		} else {
+			/* User was a member but is no more a
+			 * member of this group.
 			 */
-			ngrp->gr_mem = add_list (ngrp->gr_mem, user_newname);
+			ngrp->gr_mem = del_list (ngrp->gr_mem, user_name);
 			changed = true;
 #ifdef WITH_AUDIT
 			audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
-			              "adding user to group",
-			              user_name, AUDIT_NO_ID, 1);
+				      "removing group member",
+				      user_name, AUDIT_NO_ID, 1);
 #endif
-			SYSLOG ((LOG_INFO, "add '%s' to group '%s'",
-			         user_newname, ngrp->gr_name));
+			SYSLOG ((LOG_INFO,
+				 "delete '%s' from group '%s'",
+				 user_name, ngrp->gr_name));
 		}
-		if (!changed)
-			goto free_ngrp;
+	} else if (is_member) {
+		/* User was not a member but is now a member this
+		 * group.
+		 */
+		ngrp->gr_mem = add_list (ngrp->gr_mem, user_newname);
+		changed = true;
+#ifdef WITH_AUDIT
+		audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
+			      "adding user to group",
+			      user_name, AUDIT_NO_ID, 1);
+#endif
+		SYSLOG ((LOG_INFO, "add '%s' to group '%s'",
+			 user_newname, ngrp->gr_name));
+	}
+	if (!changed)
+		goto free_ngrp;
 
-		if (gr_update (ngrp) == 0) {
-			fprintf (stderr,
-			         _("%s: failed to prepare the new %s entry '%s'\n"),
-			         Prog, gr_dbname (), ngrp->gr_name);
-			SYSLOG ((LOG_WARN, "failed to prepare the new %s entry '%s'", gr_dbname (), ngrp->gr_name));
-			fail_exit (E_GRP_UPDATE);
-		}
+	if (gr_update (ngrp) == 0) {
+		fprintf (stderr,
+			 _("%s: failed to prepare the new %s entry '%s'\n"),
+			 Prog, gr_dbname (), ngrp->gr_name);
+		SYSLOG ((LOG_WARN, "failed to prepare the new %s entry '%s'", gr_dbname (), ngrp->gr_name));
+		fail_exit (E_GRP_UPDATE);
+	}
 
 free_ngrp:
-		gr_free(ngrp);
-	}
+	gr_free(ngrp);
 }
 
+
 #ifdef SHADOWGRP
 static void
 update_gshadow_file(void)
-- 
2.45.1


From d8e6a8b99b4d844328d875287babf6e13860d464 Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <alx@kernel.org>
Date: Fri, 17 May 2024 02:29:46 +0200
Subject: [PATCH 16/16] src/usermod.c: update_gshadow(): Add helper function

Keep the while loop in the outer function, and move the iteration code
to this new helper.  This makes it a bit more readable.

Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 src/usermod.c | 223 ++++++++++++++++++++++++++------------------------
 1 file changed, 116 insertions(+), 107 deletions(-)

diff --git a/src/usermod.c b/src/usermod.c
index 4ea11376..f8896984 100644
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -183,6 +183,7 @@ static void update_group(const struct group *grp);
 
 #ifdef SHADOWGRP
 static void update_gshadow_file(void);
+static void update_gshadow(const struct sgrp *sgrp);
 #endif
 static void grp_update (void);
 
@@ -814,141 +815,149 @@ update_gshadow_file(void)
 	 * Scan through the entire shadow group file looking for the groups
 	 * that the user is a member of.
 	 */
-	while ((sgrp = sgr_next ()) != NULL) {
-		bool         changed;
-		bool         is_member;
-		bool         was_member;
-		bool         was_admin;
-		struct sgrp  *nsgrp;
+	while ((sgrp = sgr_next()) != NULL)
+		update_gshadow(sgrp);
+}
+#endif				/* SHADOWGRP */
 
-		changed = false;
 
-		/*
-		 * See if the user was a member of this group
-		 */
-		was_member = is_on_list (sgrp->sg_mem, user_name);
+#ifdef SHADOWGRP
+static void
+update_gshadow(const struct sgrp *sgrp)
+{
+	bool         changed;
+	bool         is_member;
+	bool         was_member;
+	bool         was_admin;
+	struct sgrp  *nsgrp;
 
-		/*
-		 * See if the user was an administrator of this group
-		 */
-		was_admin = is_on_list (sgrp->sg_adm, user_name);
+	changed = false;
 
-		/*
-		 * See if the user specified this group as one of their
-		 * concurrent groups.
-		 */
-		is_member = Gflg && (   (was_member && aflg)
-		                     || is_on_list (user_groups, sgrp->sg_name));
+	/*
+	 * See if the user was a member of this group
+	 */
+	was_member = is_on_list (sgrp->sg_mem, user_name);
 
-		if (!was_member && !was_admin && !is_member) {
-			continue;
-		}
+	/*
+	 * See if the user was an administrator of this group
+	 */
+	was_admin = is_on_list (sgrp->sg_adm, user_name);
 
-		/*
-		* If rflg+Gflg  is passed in AKA -rG invert is_member, to remove targeted
-		* groups while leaving the user apart of groups not mentioned
-		*/
-		if (Gflg && rflg) {
-			is_member = !is_member;
-		}
+	/*
+	 * See if the user specified this group as one of their
+	 * concurrent groups.
+	 */
+	is_member = Gflg && (   (was_member && aflg)
+			     || is_on_list (user_groups, sgrp->sg_name));
 
-		nsgrp = __sgr_dup (sgrp);
-		if (NULL == nsgrp) {
-			fprintf (stderr,
-			         _("%s: Out of memory. Cannot update %s.\n"),
-			         Prog, sgr_dbname ());
-			fail_exit (E_GRP_UPDATE);
-		}
+	if (!was_member && !was_admin && !is_member)
+		return;
 
-		if (was_admin && lflg) {
-			/* User was an admin of this group but the user
-			 * has been renamed.
-			 */
-			nsgrp->sg_adm = del_list (nsgrp->sg_adm, user_name);
-			nsgrp->sg_adm = add_list (nsgrp->sg_adm, user_newname);
-			changed = true;
-#ifdef WITH_AUDIT
-			audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
-			              "changing admin name in shadow group",
-			              user_name, AUDIT_NO_ID, 1);
-#endif
-			SYSLOG ((LOG_INFO,
-			         "change admin '%s' to '%s' in shadow group '%s'",
-			         user_name, user_newname, nsgrp->sg_name));
-		}
-
-		if (was_member) {
-			if ((!Gflg) || is_member) {
-				/* User was a member and is still a member
-				 * of this group.
-				 * But the user might have been renamed.
-				 */
-				if (lflg) {
-					nsgrp->sg_mem = del_list (nsgrp->sg_mem,
-					                          user_name);
-					nsgrp->sg_mem = add_list (nsgrp->sg_mem,
-					                          user_newname);
-					changed = true;
+	/*
+	* If rflg+Gflg  is passed in AKA -rG invert is_member, to remove targeted
+	* groups while leaving the user apart of groups not mentioned
+	*/
+	if (Gflg && rflg) {
+		is_member = !is_member;
+	}
+
+	nsgrp = __sgr_dup (sgrp);
+	if (NULL == nsgrp) {
+		fprintf (stderr,
+			 _("%s: Out of memory. Cannot update %s.\n"),
+			 Prog, sgr_dbname ());
+		fail_exit (E_GRP_UPDATE);
+	}
+
+	if (was_admin && lflg) {
+		/* User was an admin of this group but the user
+		 * has been renamed.
+		 */
+		nsgrp->sg_adm = del_list (nsgrp->sg_adm, user_name);
+		nsgrp->sg_adm = add_list (nsgrp->sg_adm, user_newname);
+		changed = true;
 #ifdef WITH_AUDIT
-					audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
-					              "changing member in shadow group",
-					              user_name, AUDIT_NO_ID, 1);
+		audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
+			      "changing admin name in shadow group",
+			      user_name, AUDIT_NO_ID, 1);
 #endif
-					SYSLOG ((LOG_INFO,
-					         "change '%s' to '%s' in shadow group '%s'",
-					         user_name, user_newname,
-					         nsgrp->sg_name));
-				}
-			} else {
-				/* User was a member but is no more a
-				 * member of this group.
-				 */
-				nsgrp->sg_mem = del_list (nsgrp->sg_mem, user_name);
+		SYSLOG ((LOG_INFO,
+			 "change admin '%s' to '%s' in shadow group '%s'",
+			 user_name, user_newname, nsgrp->sg_name));
+	}
+
+	if (was_member) {
+		if ((!Gflg) || is_member) {
+			/* User was a member and is still a member
+			 * of this group.
+			 * But the user might have been renamed.
+			 */
+			if (lflg) {
+				nsgrp->sg_mem = del_list (nsgrp->sg_mem,
+							  user_name);
+				nsgrp->sg_mem = add_list (nsgrp->sg_mem,
+							  user_newname);
 				changed = true;
 #ifdef WITH_AUDIT
 				audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
-				              "removing user from shadow group",
-				              user_name, AUDIT_NO_ID, 1);
+					      "changing member in shadow group",
+					      user_name, AUDIT_NO_ID, 1);
 #endif
 				SYSLOG ((LOG_INFO,
-				         "delete '%s' from shadow group '%s'",
-				         user_name, nsgrp->sg_name));
+					 "change '%s' to '%s' in shadow group '%s'",
+					 user_name, user_newname,
+					 nsgrp->sg_name));
 			}
-		} else if (is_member) {
-			/* User was not a member but is now a member this
-			 * group.
+		} else {
+			/* User was a member but is no more a
+			 * member of this group.
 			 */
-			nsgrp->sg_mem = add_list (nsgrp->sg_mem, user_newname);
+			nsgrp->sg_mem = del_list (nsgrp->sg_mem, user_name);
 			changed = true;
 #ifdef WITH_AUDIT
 			audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
-			              "adding user to shadow group",
-			              user_newname, AUDIT_NO_ID, 1);
+				      "removing user from shadow group",
+				      user_name, AUDIT_NO_ID, 1);
 #endif
-			SYSLOG ((LOG_INFO, "add '%s' to shadow group '%s'",
-			         user_newname, nsgrp->sg_name));
+			SYSLOG ((LOG_INFO,
+				 "delete '%s' from shadow group '%s'",
+				 user_name, nsgrp->sg_name));
 		}
-		if (!changed)
-			goto free_nsgrp;
-
-		/*
-		 * Update the group entry to reflect the changes.
+	} else if (is_member) {
+		/* User was not a member but is now a member this
+		 * group.
 		 */
-		if (sgr_update (nsgrp) == 0) {
-			fprintf (stderr,
-			         _("%s: failed to prepare the new %s entry '%s'\n"),
-			         Prog, sgr_dbname (), nsgrp->sg_name);
-			SYSLOG ((LOG_WARN, "failed to prepare the new %s entry '%s'",
-			         sgr_dbname (), nsgrp->sg_name));
-			fail_exit (E_GRP_UPDATE);
-		}
+		nsgrp->sg_mem = add_list (nsgrp->sg_mem, user_newname);
+		changed = true;
+#ifdef WITH_AUDIT
+		audit_logger (AUDIT_USER_CHAUTHTOK, Prog,
+			      "adding user to shadow group",
+			      user_newname, AUDIT_NO_ID, 1);
+#endif
+		SYSLOG ((LOG_INFO, "add '%s' to shadow group '%s'",
+			 user_newname, nsgrp->sg_name));
+	}
+	if (!changed)
+		goto free_nsgrp;
 
-free_nsgrp:
-		free (nsgrp);
+	/*
+	 * Update the group entry to reflect the changes.
+	 */
+	if (sgr_update (nsgrp) == 0) {
+		fprintf (stderr,
+			 _("%s: failed to prepare the new %s entry '%s'\n"),
+			 Prog, sgr_dbname (), nsgrp->sg_name);
+		SYSLOG ((LOG_WARN, "failed to prepare the new %s entry '%s'",
+			 sgr_dbname (), nsgrp->sg_name));
+		fail_exit (E_GRP_UPDATE);
 	}
+
+free_nsgrp:
+	free (nsgrp);
 }
 #endif				/* SHADOWGRP */
 
+
 /*
  * grp_update - add user to secondary group set
  *
-- 
2.45.1