These functions were open-coding get_gid(). Use the actual function.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Implement it as an inline function, and add restrict and ATTR_STRING()
and ATTR_ACCESS() as appropriate.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
atoi(3) easily triggers Undefined Behavior. Replace it by str2[u]l(),
which are safe from that, and add type safety too.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Groupmod -U may cause crashes because of double free. If without -a, the first free of (*ogrp).gr_mem is in gr_free_members(&grp), and then in gr_update without -n or gr_remove with -n.
Considering the minimal impact of modifications on existing code, delete gr_free_members(&grp) to avoid double free.Although this may seem reckless, the second free in two different positions will definitely be triggered, and the following two test cases can be used to illustrate the situation :
[root@localhost src]# ./useradd u1
[root@localhost src]# ./useradd u2
[root@localhost src]# ./useradd u3
[root@localhost src]# ./groupadd -U u1,u2,u3 g1
[root@localhost src]# ./groupmod -n g2 -U u1,u2 g1
Segmentation fault
This case would free (*ogrp).gr_mem in gr_free_members(&grp) due to assignment statements grp = *ogrp, then in if (nflg && (gr_remove (group_name) == 0)), which finally calls gr_free_members(grent) to free (*ogrp).gr_mem again.
[root@localhost src]# ./useradd u1
[root@localhost src]# ./useradd u2
[root@localhost src]# ./useradd u3
[root@localhost src]# ./groupadd -U u1,u2,u3 g1
[root@localhost src]# ./groupmod -U u1,u2 g1
Segmentation fault
The other case would free (*ogrp).gr_mem in gr_free_members(&grp) too, then in if (gr_update (&grp) == 0), which finally calls gr_free_members(grent) too to free (*ogrp).gr_mem again.
So the first free is unnecessary, maybe we can drop it.
Fixes: 342c934a35 ("add -U option to groupadd and groupmod")
Closes: <https://github.com/shadow-maint/shadow/issues/1013>
Link: <https://github.com/shadow-maint/shadow/pull/1007>
Link: <https://github.com/shadow-maint/shadow/pull/271>
Link: <https://github.com/shadow-maint/shadow/issues/265>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: lixinyun <li.xinyun@h3c.com>
It is a simpler call, with more type safety.
A consequence of this change is that the program now accepts numbers in
bases 8 and 16. That's not a problem here, I think.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Instead of raw sysconf(_SC_LOGIN_NAME_MAX) calls, which was being used
without error handling.
Fixes: 3b7cc05387 ("lib: replace `USER_NAME_MAX_LENGTH` macro")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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>
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>
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: 45c6603cc8 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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>
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>
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>
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>
'endptr' is appropriate internally in strtol(3) because it's a pointer
to 'end', and 'end' itself is a pointer to one-after-the-last character
of the numeric string. In other words,
endptr == &end
However, naming the pointer whose address we pass to strtol(3)'s
'endptr' feels wrong, and causes me trouble while parsing the code; I
need to double check the number of dereferences, because something feels
wrong in my head.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This silences a CodeQL warning. We don't care about reentrancy, but
after this patch we don't need to break a long line, so that's a win.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It's trivial to do the change, and it removes a CodeQL warning.
We don't need to be reentrant, but it doesn't hurt either.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Amazing that this triggered no warnings at all.
Fixes: 355ad6a9e0 ("Have a single definition of date_to_str()")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Also, it was checking for >=0 for success, but since that code is for
opening a different tty as stdin, that was bogus. But since it's
guaranteed to be either 0 or -1, this commit doesn't add any code to
make sure it's 0 (i.e., we could say !=0 instead of ==-1). That's more
appropriate for a different commit.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Remove /*ARGSUSED*/ comments. Instead, use appropriate declarators for
main(). ISO C allows using int main(void) if the parameters are going
to be unused.
Also, do some cosmetic changes in the uses of argc and argv, to show
where they are used.
And use *argv[], instead of **argv. Array notation is friendlier, IMO.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
OPENLOG() already sets the program name as the prefix.
This resulted in entries like:
$ journalctl 2>/dev/null | grep passwd
Mar 03 01:09:47 debian passwd[140744]: passwd: can't view or modify password information for root
Fixes: 8e167d28af ("[svn-upgrade] Integrating new upstream version, shadow (4.0.8)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Adding function check_fds to new file fd.c. The function check_fds
should be called in every setuid/setgid program.
Co-developed-by: Alejandro Colomar <alx@kernel.org>
A difference between 'struct utmp' and 'struct utmpx' is that
the former uses UT_LINESIZE for the size of its array members,
while the latter doesn't have a standard variable to get its
size. Therefore, we need to get the number of elements in
the array with NITEMS().
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
utmpx is specified by POSIX as an XSI extension. That's more portable
than utmp, which is unavailable for example in musl libc. The manual
page specifies that in Linux (but it probably means in glibc), utmp and
utmpx (and the functions that use them) are identical, so this commit
shouldn't affect glibc systems.
Assume utmpx is always present.
Also, if utmpx is present, POSIX guarantees that some members exist:
- ut_user
- ut_id
- ut_line
- ut_pid
- ut_type
- ut_tv
So, rely on them unconditionally.
Fixes: 170b76cdd1 ("Disable utmpx permanently")
Closes: <https://github.com/shadow-maint/shadow/issues/945>
Reported-by: Firas Khalil Khana <firasuke@gmail.com>
Reported-by: "A. Wilfox" <https://github.com/awilfox>
Tested-by: Firas Khalil Khana <firasuke@gmail.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The passwd silently truncated the password length to PASS_MAX.
This patch introduces check that prints an error message
and exits the call.
Signed-off-by: Tomas Halman <tomas@halman.net>
The passwd utility had hardcoded limit for password lenght set
to 200 characters. In the agetpass.c is used PASS_MAX for
this purpose.
This patch moves the PASS_MAX definition to common place
and uses it in both places.
Signed-off-by: Tomas Halman <tomas@halman.net>
Before 3b7cc05387 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), this
code did use a length. It used a utmp(5) fixed-width buffer, so the
length matches the buffer size (there was no terminating NUL byte).
However, sysconf(_SC_LOGIN_NAME_MAX) returns a buffer size that accounts
for the terminating null byte; see sysconf(3). Thus, the commit that
introduced the call to sysconf(3), should have taken that detail into
account.
403a2e3771 ("lib/chkname.c: Take NUL byte into account"), by Tobias,
caught that bug in <lib/chkname.c>, but missed that the same commit that
introduced that bug, introduced the same bug in two other places.
This fixes all remaining calls to sysconf(_SC_LOGIN_NAME_MAX).
I still observe some suspicious code after this fix:
if (do_rlogin(hostname, username, max_size - 1, term, sizeof(term)))
...
login_prompt(username, max_size - 1);
We're passing size-1 to functions that want a size. But since the fix
to those will be different, let's do that in the following commits.
Link: <https://github.com/shadow-maint/shadow/pull/935>
Link: <https://github.com/shadow-maint/shadow/issues/920#issuecomment-1926002209>
Link: <https://github.com/shadow-maint/shadow/pull/757>
Link: <https://github.com/shadow-maint/shadow/issues/674>
See-also: 403a2e3771 ("lib/chkname.c: Take NUL byte into account")
Fixes: 3b7cc05387 ("lib: replace `USER_NAME_MAX_LENGTH` macro")
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It is slightly confusing to allow adding these only to later refuse them.
Here is a (lightly tested :) patch to also refuse them when adding.
Signed-off-by: Tycho Andersen <tycho@tycho.pizza>
The conversion from day to seconds can be done in print_date
(renamed to print_day_as_date for clarification). This has the nice
benefit that DAY multiplication and long to time_t conversion are done
at just one place.
Co-developed-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
