The "quick hack" finally disappeared. Probably nobody noticed. ;)
(See the changes in <configure.ac> for the context of this pun.)
Probably everybody uses SSH these days for remote login. Let's remove
this insecure method.
Closes: <https://github.com/shadow-maint/shadow/issues/992>
Reviewed-by: dkwo <nicolopiazzalunga@gmail.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Michael Vetter <jubalh@iodoru.org>
Cc: Sam James <sam@gentoo.org>
Cc: Benedikt Brinkmann <datacobra@thinkbot.de>
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>
Per https://tdg.docbook.org/tdg/4.5/term, term is a word being
defined in a varlistentry. The 'high uid' description is not a
varlistentry, so <term> and </term> show up in the processed
manpage. See debian Bug#1072297.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
`PKG_CONFIG` variable needs to be set for `PKG_CHECK_MODULES` to
succeed, but this wasn't happening in Fedora because the first
appearance of `PKG_CHECK_MODULES` was conditionally skipped because this
distribution is compiled without `libbsd` support. Thus, moving the
cmocka library detection before libbsd fixes the problem.
Suggested-by: Lukas Slebodnik <lslebodn@fedoraproject.org>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
All call sites have been replaced by functions from "atoi/a2i.h" and
"atoi/str2i.h" recently.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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>
time_t isn't necessarily unsigned (in fact, it's likely to be signed.
Therefore, parse the number as the right type, via a2i(time_t, ...).
Still, reject negative numbers, just to be cautious. It was done
before (strtoull_noneg()), so it shouldn't be a problem. (However,
strtoull_noneg() was only introduced recently, and before that we called
strtoull(3), which silently accepted negative values.)
Remove the limitation of ULONG_MAX, which seems arbitrary. It probably
was written in times where 'time_t' had the same length of 'long', and
this was thus a test that the value didn't overflow 'time_t'. Such a
test is implicit in the a2i() call, so forget about it.
Unify the error messages into a single one that provides all the info
(except the value of 'fallback').
Link: <cb610d54b4 (r136407772)>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Chris Lamb <lamby@debian.org>
Cc: Serge Hallyn <serge@hallyn.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>
Instead of GNU builtins and extensions, these macros can be implemented
with C11's _Generic(3), and the result is much simpler (and safer, since
it's now an error, not just a warning).
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>
It's doesn't make much sense to break from a switch() just to return.
Let's return early, to simplify.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
