From 4c16416ebc5f0958d58a1ea1e7890eafd9f8bb75 Mon Sep 17 00:00:00 2001 From: Iker Pedrosa 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 Reviewed-by: Alejandro Colomar --- 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 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 Signed-off-by: Alejandro Colomar --- 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 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 Signed-off-by: Alejandro Colomar --- 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 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 Signed-off-by: Alejandro Colomar --- 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 Date: Fri, 17 May 2024 13:14:31 +0200 Subject: [PATCH 05/16] src/useradd.c: De-duplicate code Reviewed-by: Iker Pedrosa Signed-off-by: Alejandro Colomar --- 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 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 Signed-off-by: Alejandro Colomar --- 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 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: Reported-by: Iker Pedrosa Reviewed-by: Iker Pedrosa Signed-off-by: Alejandro Colomar --- 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 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 --- 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 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 --- 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 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 --- 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 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 Signed-off-by: Alejandro Colomar --- 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 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 Signed-off-by: Alejandro Colomar --- 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 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 '\' -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 --- 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 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 '\' -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 --- 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 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 Signed-off-by: Alejandro Colomar --- 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 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 Signed-off-by: Alejandro Colomar --- 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