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>
This means we set the pointees on error, which we didn't do before, but
since we return -1 on error and ignore (don't use) the pointees at call
site, that's fine.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
All 3 non-error paths in the second part resulted in *has_min = true.
Set in once before the switch(), to simplify.
This means we set this variable on error, which we didn't do before,
but since we return -1 on error and ignore (don't use) the pointees at
call site, that's fine.
Also, move a couple of *has_max = true statements to before a comment,
in preparation for future commits.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Set *has_{min,max} = false at the begining, so we only need to set them
to true later.
This means we set these variables on error, which we didn't do before,
but since we return -1 on error and ignore (don't use) the pointees at
call site, that's fine.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
libpam is enabled to provide `passwd` binary from this package, as there
are several password quality checks that are enabled through a PAM
module. Same reason to disable account-tools-setuid.
sssd is disabled because `files provider` has been removed in sssd, and
the underlying functionality in shadow isn't needed anymore.
libcrack dependency was disabled some time ago, but the upstream repo
wasn't updated. Doing it now.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
The manpages for newuidmap and newgidmap had a typo "[pid[" instead
of "[pid]". They were also unclear about what the /proc/pid fd should
be. Fix both.
Closes#977
Reported-by: igo95862@yandex.ru
Signed-off-by: Serge Hallyn <serge@hallyn.com>
If not enough memory is available for more environment variables, treat
it exactly like not enough memory for new environment variable content.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.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>
This macro makes sure that the first argument is an array, and
calculates its size.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Keep pot creation date out of our po files when we compare them.
Otherwise, we always think they need to be updated.
We prepend a line '# To re-generate, ....' to the shadow-man-pages.pot
file. Do that before we compare the new candidate, because right
now our comparison to see if we've made changes always thinks we have.
Put some of the tempfiles in a mktemp -d'd directory, which we remove when
all's done. This keeps the working tree cleaner.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
def_find can return NULL for unset, not just unknown, config options. So
move the decision of whether to log an error message about an unknown config
option back into def_find, which knows the difference. Only putdef_str()
will pass a char* srcfile to def_find, so only calls from putdef_str will
cause the message, which was the original intent of fa68441bc4.
closes#967
fixes: fa68441bc4 ("Improve the login.defs unknown item error message")
Signed-off-by: Serge Hallyn <serge@hallyn.com>
There are no guarantees that fstatat() does not clobber the stat
buffer on errors.
Use a temporary buffer so that the following code sees correct
attributes of the source entry.
Issue #973
Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
The combination of bzero and free could be optimized away.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
This change executes `i++` one more time before breaking, so we need to
update the `i+1` after the loop to just `i`.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
cp can only be an empty string literal in that conditional. Use a
string literal to be more explicit.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Performance tests made in 2007 are obsolete. We should assume libc is
reasonably fast today (otherwise, report a bug to libc).
$ git blame -- lib/sgetgrent.c | grep strchr
45c6603cc (nekral-guest 2007-10-07 11:44:02 +0000 30) * WARNING: I profiled this once with and without strchr() calls
6f88bcf58 (nekral-guest 2008-05-26 08:31:14 +0000 97) cp = strchr (cp, ':');
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>
It was always being called with 'day * DAY', so do that internally and
simplify. This grabs some code from print_day_as_date().
Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
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>
Previously, we were performing the following two checks:
- if (ranges != ((argc + 2) / 3)) {
- if ((ranges * 3) > argc) {
Let's draw a table of the possible input that would pass the first check:
argc: 0 1 2 3 4 5 6 7 8 9
rng: 0 1 1 1 2 2 2 3 3 3
a+2/3*3:0 3 3 3 6 6 6 9 9 9 <-- this is roundup(argc, 3);
a+2/3: 0 1 1 1 2 2 2 3 3 3 <-- this is roundup(argc, 3) / 3;
rng*3: 0 3 3 3 6 6 6 9 9 9
From those, let's extract those that would also pass the second check:
argc: 0 3 6 9
rng: 0 1 2 3
rng*3: 0 3 6 9
We can see that there's a simple check for this input:
+ if (ranges * 3 != argc) {
As a sanity check, let's draw a table of the acceptable input with that
check:
rng: 0 1 2 3
rng*3: 0 3 6 9
argc: 0 3 6 9
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Serge Hallyn <serge@hallyn.com>
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>
The function should never be used; it's always used via its wrapper
macro. To simplify, and reduce chances of confusion: remove the
function, and implement the macro directly in terms of
stpcpy(mempcpy(strnlen())).
Update the documentation, and improve the example, which was rather
confusing.
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-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>
This macro is useful to get the size of a member of a structure
without having a variable of that type.
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>
If (maxsize == -1), then ((size_t)maxsize == SIZE_MAX). And no size can
ever be >= SIZE_MAX, so it will never return false if sysconf(3) reports
an unlimited user-name size via returning -1. Well, to be pedantic,
that disallows a user-name siz of precisely SIZE_MAX bytes when
sysconf(3) returns -1. However, that's probably a good thing; such a
long user name might trigger Undefined Behavior somewhere else, so be
cautious and disallow it. I hope nobody will be using the entire
address space for a user name.
The commit that introduced that check missed that this code had always
supported unlimited user-name sizes since it was introduced by Iker in
3b7cc05387 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), and
6be85b0baf ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare
warning") even clarified this in the commit message.
So, while the code in 6a1f45d932 ("lib/chkname.c: Support unlimited
user name lengths") wasn't bad per se, the commit message was incorrect.
What that patch did was adding code for handling EINVAL (or any other
errors that a future kernel might add).
To be more pedantically correct, that commit also allowed (under certain
circumstances, user names of SIZE_MAX bytes, but those were originally
allowed (by accident), and only became disallowed in 403a2e3771
("lib/chkname.c: Take NUL byte into account"). But again, let's
disallow those, just to be cautious.
Link: <https://github.com/shadow-maint/shadow/pull/935>
Link: <https://github.com/shadow-maint/shadow/pull/935#discussion_r1477429492>
See-also: 6be85b0baf ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning")
Fixes: 6a1f45d932 ("lib/chkname.c: Support unlimited user name lengths")
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>
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>
Days officially roll over at 00:00 UTC, not at 12:00 UTC. I see no
reason to add that half day.
Also, remove the comment. It's likely to get stale.
So, get_date() gets the number of seconds since the Epoch. I wonder how
that thing works, but I'll assume it's something similar to getdate(3)
+ mktime(3). After that, we need to convert seconds since Epoch to days
since Epoch. That should be a simple division, AFAICS, since Epoch is
"1970‐01‐01 00:00:00 +0000 (UTC)". See mktime(3).
Fixes: 45c6603cc8 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Link: <https://github.com/shadow-maint/shadow/issues/939>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Tested-by: Gus Kenion <https://github.com/kenion>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Very large values in /etc/shadow could lead to overflows. Make sure
that these calculations are saturated at LONG_MAX. Since entries are
based on days and not seconds since epoch, saturating won't hurt anyone.
Co-developed-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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>
If the system does not have a user name length limit, support it
accordingly. If the system has no _SC_LOGIN_NAME_MAX, use
LOGIN_NAME_MAX constant instead.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The _SC_LOGIN_NAME_MAX value includes space for the NUL byte. The length
of name must smaller than this value to be valid.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The password returned by agetpass can be used directly without copying
it into a char array first.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Clarify how this endless while(true) loop can be stopped by using a
boolean variable as condition and turn it into a do-while loop.
Suggested-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
New option --stdin/-t is available for root user. It is useful
for automation/setup and it makes shadow utils passwd more versatile.
Signed-off-by: Tomas Halman <tomas@halman.net>
There is an inconsistent use of the MAYBE_UNUSED macro. Sometimes the
`int unused(x)` form is used form and others the `unused int x`. We'd
like to use the second form always.
Related-To: https://github.com/shadow-maint/shadow/issues/918
Suggested-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Pablo Saavedra <psaavedra@igalia.com>
This fixes build with glibc-2.33 (newer glibc merged libdl and libpthread
into libc):
```
libtool: link: x86_64-pc-linux-gnu-gcc -isystem /usr/include/bsd -DLIBBSD_OVERLAY -O2 -pipe -Wl,-O1 -o login login.o login_nopam.o -Wl,--as-needed ../lib/.libs/libshadow.a -lcrypt -lsystemd -lpam -lpam_misc -lbsd
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: ../lib/.libs/libshadow.a(libshadow_la-nss.o): undefined reference to symbol 'dlclose@@GLIBC_2.2.5'
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: /lib64/libdl.so.2: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
```
In Debian, the needed macro from libtool seems to be in libltdl-dev.
Signed-off-by: Sam James <sam@gentoo.org>
argv is passed to execve(3), which for historic reasons is non-const,
but doesn't modify the strings.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Uses of this macro indicate a code smell, but in some cases, libc
functions require breaking const correctness. Use this macro to wrap
casts in such cases, so that we limit the danger of the cast.
It only permits discarding const. Discarding any other qualifiers, or
doing other type changes should result in a compile-time error.
Link: <https://software.codidact.com/posts/286575/287345#answer-287345>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strtou[l]l(3) silently converts negative numbers into positive. This
behavior is wrong: a negative value should be parsed as a negative
value, which would underflow unsigned (long) long, and so would return
the smallest possible value, 0, and set errno to ERANGE to report an
error.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions reject negative numbers, instead of silently converting
them into unsigned, which strtou[l]l(3) do.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The sulogin program calls pw_entry in a loop while incorrect root
passwords are entered.
Free the previously allocated memory to avoid memory exhaustion.
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
We don't need 'static', because it's in main(), which is only called
once. However, we will need initialization as if it were 'static', so
use ={} to initialize it. This will allow freeing the pointers before
they have been allocated.
Cc: Samanta Navarro <ferivoz@riseup.net>
Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Those variables are only used in main(). Restrict their scope.
Keep them static (.bss), as changing that may be dangerous.
Suggested-by: Samanta Navarro <ferivoz@riseup.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Add a variadic macro addsl() that accepts an arbitrary number of
addends, instead of having specific versions like addsl2() or addsl3().
It is internally implemented by the addslN() function, which itself
calls addsl2(). addsl3() is now obsolete and thus removed.
Code should just call addsl().
Link: <https://github.com/shadow-maint/shadow/pull/882#discussion_r1437155212>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This is for consistency with addsl3(), and in preparation for the
following commit, which will unify the interface into a single addsl()
macro.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
ISO C guarantees that #undef is a no-op if there is no such macro.
C11::6.10.3.5p2:
> A preprocessing directive of the form
>
> # undef identifier new-line
>
> causes the specified identifier no longer to be defined as a macro
> name. It is ignored if the specified identifier is not currently
> defined as a macro name.
Link: <http://port70.net/~nsz/c/c11/n1570.html#6.10.3.5p2>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions (e.g., gr_free()), explicitly dereference the pointer
and read the pointee.
The /@out@/ comment, which is (almost) analogous to the
[[gnu::access(write_only, ...)]] attribute, means that the pointee can
be uninitialized, since it won't read it. There's a difference between
/@out@/ and the GCC attribute: the attribute doesn't require that the
call writes to the pointee, while /@out@/ requires that the pointee be
fully initialized after the call, so it _must_ write to it.
A guess of why it was used is that these functions are similar to
free(3), which does not read the memory it frees, and so one would
assume that if it doesn't read, write_only (or equivalents) are good.
That's wrong in several ways:
- free(3) does not read _nor_ write to the memory, so it would
be slightly inappropriate to use write_only with it. It wouldn't be
"wrong", but [[gnu::access(none, ...)]] would be more appropriate.
- Because /@out@/ requires that the call writes to the pointee, it
would be wrong to use it in free(3), which doesn't write to the
pointee.
- Our functions are similar to free(3) conceptually, but they don't
behave like free(3), since they do read the memory (pointee) (and
also write to it), and thus they're actually read_write.
Link: <https://splint.org/manual/manual.html#undefined>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If reallocation fails in function list, then reset the size to 0 again.
Without the reset, the next call assumes that `members` points to
a memory location with reserved space.
Also use size_t instead of int for size to prevent signed integer
overflows. The length of group lines is not limited.
Fixes 45c0003e53 (4.14 release series)
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
The size of time_t varies across systems, but since data type long is
more than enough to calculate with days (precision of shadow file),
use it instead.
Just in case a shadow file contains huge values, check for a possible
signed integer overflow.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Link: <https://github.com/shadow-maint/shadow/pull/876>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Contrary to the comment in ENV_HZ.xml, ENV_HZ is not even used in
sulogin (anymore) if PAM support is enabled.
Skip paragraphs of sulogin if PAM support is enabled, since they would
be empty now.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Special care has to be taken for 32 bit systems with a 64 bit time_t,
since their long data type is still 32 bit.
Since this macro expresses a number of seconds, and seconds are in units
of 'time_t' in C, the appropriate type for the multiplication is
'time_t'.
Reported-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The values are retrieved from login.defs files, which normally do not
contain negative values. In fact, negative value -1 is used in many
code places as "feature disabled", which is normally achieved by
simply commenting out the key from the file.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The variable declarations for the buffers have been aligned in this
commit, so that they appear in the diff, making it easier to review.
Some important but somewhat tangent changes included in this commit:
- lib/nss.c: The size was being defined as 65, but then used as 64.
That was a bug, although not an important one; we were just wasting
one byte. Fix that while we replace snprintf() by SNPRINTF(), which
will get the size from sizeof(), and thus will use the real size.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions are like [v]snprintf(3), but return -1 on truncation,
which makes it easier to test. In fact, the API of swprintf(3), which
was invented later than snprintf(3), and is the wide-character version
of it, is identical to this snprintf_().
snprintf(3) is iseful in two cases:
- We don't care if the output is truncated. snprintf(3) is fine for
those, and the return value can be ignored. But snprintf_() is also
fine for those.
- Truncation is bad. In that case, it's as bad as a hard error (-1)
from snprintf, so merging both problems into the same error code
makes it easier to handle errors. Return the length if no truncation
so that we can use it if necessary.
Not returning the whole length before truncation makes a better API,
which need not read the entire input, so it's less vulnerable to DoS
attacks when a malicious user controls the input.
Use these functions to implement SNPRINTF().
Cc: Samanta Navarro <ferivoz@riseup.net>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It wraps snprintf(3) so that it performs some steps that one might
forget, or might be prone to accidents:
- It calculates the size of the destination buffer, and makes sure it's
an array (otherwise, using sizeof(s) would be very bad).
- It calculates if there's truncation or an error, returning -1 if so.
BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then the
static_assert(3) within NITEMS() makes sure VLAs are not allowed).
This macro is very similar to STRTCPY(), defined in
<lib/string/strtcpy.h>.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
commonio.c: In function 'commonio_unlock':
commonio.c:487:49: warning: '.lock' directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
487 | snprintf (lock, sizeof lock, "%s.lock", db->filename);
| ^~~~~
commonio.c:487:17: note: 'snprintf' output between 6 and 1029 bytes into a destination of size 1024
487 | snprintf (lock, sizeof lock, "%s.lock", db->filename);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Check for close(2) failure at more places closing a file descriptor
written to.
Also ignore failures with errno set to EINTR (see man:close(2) for
details).
sprintf(3) does not take the destination buffer into account. Although
the destination in these case is large enough, sprintf(3) indicates a
code smell.
Use the xasprintf() wrapper.
Group them at the end of the list of variable definitions, and use
'#if defined()' instead of '#if[n]def'. Also indent nested ones.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
ITI_AGING is not set through any build environment. If it would be set,
then timings in /etc/shadow would not fit anymore.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
su.c:678:26: warning: format ‘%s’ expects argument of type ‘char *’, but argument 4 has type ‘const void *’ [-Wformat=]
su.c:681:44: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘const void *’ [-Wformat=]
su.c:683:46: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘const void *’ [-Wformat=]
Reported-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We don't need to terminate them manually after the call. Remove all
that paranoid code, which in some cases was even wrong. While at it,
let's do a few more things:
- Use sizeof(buf) for the size of the buffer. I found that a few cases
were passing one less byte (probably because the last one was
manually zeroed later). This caused a double NUL. snprintf(3) wants
the size of the entire buffer to properly terminate it. Passing the
exact value hardcoded is brittle, so use sizeof().
- Align and improve style of variable declarations. This makes them
appear in this diff, which will help review the patch.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions consume a source string. Document that. There's no way
to mark that they also produce a string in dst, though. That will be up
to the static analyzer to guess.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It signals that a function parameter is a string _before_ the call.
Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Set errno = 0 before the call. Otherwise, it may contain anything.
- ERANGE is not the only possible errno value of these functions. They
can also set it to EINVAL.
- Any errno value after these calls is bad; just compare against 0.
- Don't check for the return value; just errno. This function is
guaranteed to not modify errno on success (POSIX).
- Check endptr == str, which may or may not set EINVAL.
Suggested-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Since failure() is [[noreturn]], we can invert the conditional so that
we don't need an else. This silences a -Wunused-parameter warning.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This simplifies the code a little bit, and prepares for the next
commits, which will clean up further.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The multiplication was already invoking UB. The test was flawed.
Use __builtin_mul_overflow() instead.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strtol(3) doesn't specify a return value if (value == endptr).
It is always an error, if (value==endptr).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
I used size_t because:
sysconf(3) can return -1 if the value is not supported, but then it can
only mean that there's no limit. Having no limit is the same as having
a limit of SIZE_MAX (to which -1 is converted).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
By writing the terminating null byte via stpcpy(3), we take advantage of
_FORTIFY_SOURCE for the last byte, which was unprotected before this
commit.
Reported-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We were using strncpy(3), which is designed to copy from a string into a
(null-padded) fixed-size character array. However, we were doing the
opposite: copying from a known-size array (which was a prefix of a
string), into a string. That's why we had to manually zero the buffer
afterwards.
Use instead mempcpy(3) to copy the non-null bytes, and then terminate
with a null byte with stpcpy(..., "").
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Upcoming `gcc-14` enabled a few warnings into errors, like
`-Wimplicit-function-declaration`. This caused `shadow` build to fail
as:
pwunconv.c: In function 'main':
pwunconv.c:132:13: error: implicit declaration of function 'getdef_bool' [-Wimplicit-function-declaration]
132 | if (getdef_bool("USE_TCB")) {
| ^~~~~~~~~~~
The change adds missing include headers.
If an entry in /etc/shells is not an absolute path (comments or
partial reads due to fgets), the line should not be considered as
a valid login shell.
In general all systems should have getusershells, but let's better
be safe than sorry.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
For arrays of char, both NITEMS() and SIZEOF_ARRAY() return the same
value. However, NITEMS() is more appropriate. Think of wide-character
equivalents of the same code; with NITEMS(), they would continue to be
valid, while with SIZEOF_ARRAY(), they would be wrong.
In the implementation of ZUSTR2STP(), we want SIZEOF_ARRAY() within the
static assert, because we're just comparing the sizes of the source and
destination buffers, and we don't care if we compare sizes or numbers of
elements, and using sizes is just simpler. But we want NITEMS() in the
zustr2stp() call, where we want to copy a specific number of characters.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We've recently fixed several bugs in the calculation of the size in this
function call. Use this wrapper to prevent similar mistakes in the
future.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This wrapper calculates the destination buffer's size, to avoid errors
in the size calculation.
A curious fact: this macro did exist in Version 7 Unix (with a slightly
different name). I found it by chance, investigating the origins of
strncpy(3) and strncat(3) in V7, after Branden suggested me to do so,
related to recent discussions about string_copying(7).
alx@debian:~/src/unix/unix/Research-V7$ grepc SCPYN .
./usr/src/cmd/login.c:#define SCPYN(a, b) strncpy(a, b, sizeof(a))
Our implementation is slightly better, because using nitems() we're
protected against passing a pointer instead of an array, and it's also
conceptually more appropriate: for wide characters, it would be
#define WCSNCPY(dst, src) wcsncpy(dst, src, NITEMS(dst))
Cc: "G. Branden Robinson" <branden@debian.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We're not even zeroing the last byte after this call. This was a
completely gratuitous truncation of one byte, and the resulting
character array still wasn't guaranteed to be null terminated, because
strncpy(3) can't do that.
Just to clarify, none of these structures needed zeroing, as they are
treated as null-padded fixed-size character arrays. Calling strncpy(3)
was actually the correct call, and the only problem was unnecessarily
truncating strings by one byte more than necessary.
Cc: Matthew House <mattlloydhouse@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This call was too clever. It relied on the last byte of ll_line
being 0 due to a previous memzero() and not writing to it later.
Write an explicit terminating null byte, by using STRTCPY().
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This call was way too clever. It relied on the last byte of fail_line
being 0 due to it being in a static structure and never writing to it.
Write an explicit terminating null byte, by using STRTCPY().
Cc: Matthew House <mattlloydhouse@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We were copying from a (zero-padded) fixed-width character array to a
string, but strncpy(3) is meant to do the opposite thing. ZUSTR2STP()
is designed to be used in this case (like strncat(3)).
Fixes: f40bdfa66a ("libmisc: implement `get_session_host()`")
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions don't seem to exist anymore. I can't find them in
Debian, nor in a web search. They probably were functions from an
ancient implementation of cracklib that doesn't exist anymore.
$ git remote -v
origin git@github.com:cracklib/cracklib.git (fetch)
origin git@github.com:cracklib/cracklib.git (push)
$ grep -rni fascisthistory
$ git log --grep FascistHistory
$ git log -S FascistHistory
Closes: <https://codesearch.debian.net/search?q=FascistHistory&literal=1>
Cc: Mike Frysinger <vapier@gentoo.org>
Acked-by: Michael Vetter <jubalh@iodoru.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strftime(3) makes no guarantees about the contents of the buffer if the
formatted string wouldn't fit in the buffer. It simply returns 0, and
it's the programmer's responsibility to do the right thing after that.
Let's write the string "future" if there's an error, similar to what we
do with gmtime(3)'s errors.
Also, `buf[size - 1] = '\0';` didn't make sense. If the copy fits,
strftime(3) guarantees to terminate with NUL. If it doesn't, the entire
contents of buf are undefined, so adding a NUL at the end of the buffer
would be dangerous: the string could contain anything, such as
"gimme root access now". Remove that, now that we set the string to
"future", as with gmtime(3) errors. This setting to '\0' comes from the
times when we used strncpy(3) in the implementation, and should have
been removed when I changed it to use strlcpy(3); however, I didn't
check we didn't need it anymore.
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
ut_line doesn't hold a string. It is a null-padded fixed-width array.
Luckily, I don't think there has ever existed a ut_line ("/dev/tty*")
that was 32 bytes long. That would have resulted in a buffer overrun.
Anyway, do the right thing, which is copying into a temporary string.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There's been a very long and interesting discussion in linux-man@ and
libc-alpha@, where we've discussed all the string-copying functions,
their pros and cons, when should each be used and avoided, etc.
Paul Eggert pointed out an important problem of strlcpy(3): it is
vulnerable to DoS attacks if an attacker controls the length of the
source string. And even if it doesn't control it, the function is dead
slow (because its API forces it to calculate strlen(src)).
We've agreed that the general solution for a truncating string-copying
function is to write a wrapper over strnlen(3)+memcpy(3), which is
limited to strnlen(src, sizeof(dst)). This is not vulnerable to DoS,
and is very fast for all buffer sizes. string_copying(7) has been
updated to reflect this, and provides a reference implementation for
this wrapper function.
This strtcpy(3) (t for truncation) wrapper happens to have the same API
that our strlcpy_() function had, so replace it with the better
implementation. We don't need to update callers nor tests, since the
API is the same.
A future commit will rename STRLCPY() to STRTCPY(), and replace
remaining calls to strlcpy(3) by calls to this strtcpy(3).
Link: <https://lore.kernel.org/linux-man/ZU4SDh-Se5gjPny5@debian/T/#mfb5a3fdeb35487dec6f8d9e3d8548bd0d92c4975/>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This test fails now, due to a bug: the return type of strlcpy_() is
size_t, but it should be ssize_t. The next commit will pass the test,
by fixing the bug.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
libbsd is unwanted in Fedora and RHEL, and the recently released Fedora
39 doesn't contain this dependency in the base image.
shadow removed libbsd from its dependencies for Fedora 39, so let's
build without it to avoid compilation errors.
Resolves: https://github.com/shadow-maint/shadow/issues/839
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
free(NULL) is valid; there's no need to check for NULL. Simplify.
Fixes: 5178f8c5af ("utmp: call prepare_utmp() even if utent is NULL")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
copydir.c:429:4: warning: Value stored to 'err' is never read [deadcode.DeadStores]
Also reduce indentation by bailing out early.
(cherry picked from commit d89f2fb06d1b81b56299f9d0bfe7a927a2282f19)
Closes#746
Only print the 'unknown item' message to syslog if we are
actually parsing a login.defs. Prefix it with "shadow:" to make
it clear in syslog where it came from.
Also add the source filename to the console message. I'm not
quite clear on the econf API, so not sure whether in that path we
will end up actually having the path, or printing ''.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
This is not just a style issue. This should be a hard error, and never
compile. ISO C89 already had this feature as deprecated. ISO C99
removed this deprecated feature, for good reasons. If we compile
ignoring this warning, shadow is not going to behave well.
Cc: Sam James <sam@gentoo.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
asprintf(3) is non-standard, but is provided by GNU, the BSDs, and musl.
That makes it portable enough for us to use.
This function is much simpler than the burdensome code for allocating
the right size. Being simpler, it's thus safer.
I took the opportunity to fix the style to my preferred one in the
definitions of variables used in these calls, and also in the calls to
free(3) with these pointers. That isn't gratuituous, but has a reason:
it makes those appear in the diff for this patch, which helps review it.
Oh, well, I had an excuse :)
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
As other x...() wrappers around functions that allocate, these wrappers
are like [v]asprintf(3), but exit on failure.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Badnames still accepted, note that previously usage already stated
singular form, whilst manpage and real one was plural only.
Fixes: 45d6746219 ("src: correct "badname" option")
Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
The file isn't up to date with the latest development, the last change
was made 15 years ago, so I'm removing it.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Another file that I remove with sadness. We were unable to complete the
first item but we are working hard on it.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
I remove this file with sadness, as it contains data from old times.
Unfortunately, this data is no longer relevant. The source code
management tool will keep it in memory.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
I guess we are keeping this for historical purposes more than anything
else. If so, anybody can check the git history to recover the
specification.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Having source code in a compressed file doesn't seem like a good idea. I
checked several distributions and they don't distribute this binary, so
let's remove it.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
The patch is never applied upstream. If I were to take a gamble, I would
even say that it throws an error when trying to patch.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Not sure what this file is exactly, but there's already a groupmems.c
that should generate the binary responsible for managing the members of
a user's primary group.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
AFAIK, it isn't included in any distribution and it isn't used
internally in the project, so let's remove it.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
And do not set 'clear' to point to the empty string. After this commit,
'clear' only stores the result of getpass(3). This will be useful to
change the code to use agetpass().
$ grep '\<clear\>' lib/pwauth.c;
char *clear = NULL;
clear = getpass (prompt);
input = (clear == NULL) ? "" : clear;
clear = getpass (prompt);
input = (clear == NULL) ? "" : clear;
if (NULL != clear) {
strzero (clear);
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There are no users of 'clear_pass' and 'wipe_clear_pass'.
$ grep -rn '\<clear_pass\>'
lib/pwauth.c:35:/*@null@*/char *clear_pass = NULL;
lib/pwauth.c:199: * not wipe it (the caller should wipe clear_pass when it is
lib/pwauth.c:203: clear_pass = clear;
$ grep -rn wipe_clear_pass
lib/pwauth.c:34:bool wipe_clear_pass = true;
lib/pwauth.c:198: * if the external variable wipe_clear_pass is zero, we will
lib/pwauth.c:204: if (wipe_clear_pass && (NULL != clear) && ('\0' != *clear)) {
ChangeLog:3813: * lib/pwauth.c: Use a boolean for wipe_clear_pass and use_skey.
Remove them.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If the destination buffer is an array, we can check our assumptions.
This adds a readable way to explain that dsize must be strictly > ssize.
The reason is that the destination string is the source + '\0'.
If the destination is not an array, it's up to _FORTIFY_SOURCE or
-fanalyzer to catch newly introduced errors. There's nothing we can do;
at least not portably.
Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This function is like strlcpy(3), but returns -1 on truncation, which
makes it much easier to test. strlcpy(3) is useful in two cases:
- We don't care if the output is truncated. strlcpy(3) is fine for
those, and the return value can be ignored.
- Truncation is bad. In that case, we just want to signal truncation,
and the length of the original string is quite useless. Return the
length iff no truncation so that we can use it if necessary.
This simplifies the definition of the STRLCPY() macro.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It was blessed by POSIX.1-2001, and GCC says that it won't go away,
possibly ever.
memset(3) is dangerous, as the 2nd and 3rd arguments can be accidentally
swapped --who remembers what's the order of the 2nd and 3rd parameters
to memset(3) without checking the manual page or some code that uses
it?--. Some recent compilers may be able to catch that via some
warnings, but those are not infalible. And even if compiler warnings
could always catch that, the time lost in fixing or checking the docs is
lost for no clear gain. Having a sane API that is unambiguous is the
Right Thing (tm); and that API is bzero(3).
If someone doesn't believe memset(3) is error-prone, please read the
book "Unix Network Programming", Volume 1, 3rd Edition by Stevens, et
al., Section 1.2. See a stackoverflow reference in the link below[1].
bzero(3) had a bad fame in the bad old days, because some ancient
systems (I'm talking of many decades ago) shipped a broken version of
bzero(3). We can assume that all systems in which current shadow utils
can be built, have a working version of bzero(3) --if not, please fix
your broken system; don't blame the programmer--.
One reason that some use today to avoid bzero(3) in favor of memset(3)
is that memset(3) is more often used; but that's a circular reasoning.
Even if bzero(3) wasn't supported by the system, it would need to be
invented. It's the right API.
Another reason that some argue is that POSIX.1-2008 removed the
specification of bzero(3). That's not a problem, because GCC will
probably support it forever, and even if it didn't, we can redefine it
like we do with memzero(). bzero(3) is just a one-liner wrapper around
memset(3).
Link: [1] <https://stackoverflow.com/a/17097978>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This makes it harder to make mistakes while editing the code. Since the
sizeof's can be autocalculated, let the machine do that. It also
reduces the cognitive load while reading the code.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It's not being used anymore. We got rid of it in favor of better APIs.
Well, it's still being used in one place: a contrib/ patch, but I
explicitly want to break it, so that someone reviews it. I don't want
to modify it, since it's not being tested, so it would be very risky for
me to touch it. Instead, let it bitrot, and if someone cares, they'll
update it correctly.
BTW, the comment that said /* danger -side effects */ was wrong:
sizeof() doesn't evaluate the argument (unless it's a VLA), so there
wasn't really a double-evaluation issue.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It wraps strlcpy(3bsd) so that it performs some steps that one might
forget, or might be prone to accidents:
- It calculates the size of the destination buffer, and makes sure it's
an array (otherwise, using sizeof(dst) would be very bad).
- It calculates if there's truncation, returning an easy-to-use value.
BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then
the static_assert(3) within SIZEOF_ARRAY() makes sure VLAs are not
allowed).
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It's a wrapper around zustr2stp() that calls SIZEOF_ARRAY() internally.
The function call is usually --in our code base, always-- called with an
array as the second argument. For such an argument, one should call
SIZEOF_ARRAY(). To avoid mistakes, and simplify usage, let's add this
macro that does it internally.
BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then
the static_assert(3) within SIZEOF_ARRAY() makes sure VLAs are not
allowed).
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These calls were intending to copy from a NUL-padded (possibly
non-NUL-terminated) character sequences contained in fixed-width arrays,
into a string, where extra padding is superfluous. Use the appropriate
call, which removes the superfluous work. That reduces the chance of
confusing maintainers about the intention of the code.
While at it, use the appropriate third parameter, which is the size of
the source buffer, and not the one of the destination buffer. As a side
effect, this reduces the use of '-1', which itself reduces the chance of
off-by-one bugs.
Also, since using sizeof() on an array is dangerous, use SIZEOF_ARRAY().
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There's no standard function that copies from a null-padded character
sequence into a string.
A few standard functions can be workarounded to do that:
- strncat(3): This function is designed to catenate from a null-padded
character sequence into a string. The catch is that there's no
*cpy() equivalent of it --strncpy(3) is not at all related to
strncat(3); don't be fooled by the confusing name--, so one would
need to zero the first byte before the call to strncat(3). It also
has the inconvenient that it returns a useless value.
- strncpy(3): This function is designed to copy from a string to a
null-padded character sequence; the opposite of what we want to do.
If one passes the size of src instead of the size of dst, and then
manually zeroes the last byte of the dst buffer, something similar
to what we want happens. However, this does more than what we want:
it also padds with NUL the remaining bytes after the terminating NUL.
That extra work can confuse maintainers to believe that it's
necessary. That is exactly what happens in logout.c.
src/logoutd.c-46- /*
src/logoutd.c-47- * ut_user may not have the terminating NUL.
src/logoutd.c-48- */
src/logoutd.c:49: strncpy (user, ut->ut_user, sizeof (ut->ut_user));
src/logoutd.c-50- user[sizeof (ut->ut_user)] = '\0';
In that logout.c case --and in most invocations of strncpy(3), which
is usually a wrong tool-- the extra work is not wanted, so it's
preferrable to use the right tool, a function that does exactly
what's needed and nothing more than that. That tool is zustr2stp().
Read string_copying(7) for a more complete comparison of string copying
functions.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This makes it safe to call sizeof() on an array. Calling sizeof()
directly on an array is dangerous, because if the array changes to be a
pointer, the behavior will unexpectedly change. It's the same problem
as with NITEMS().
Link: <https://stackoverflow.com/a/57537491>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There's no need to have these as macros, so use functions, which are a
lot safer: there's no need to worry about multiple evaluation of args,
and there's also more type safety. Compiler warnings are also simpler,
as they don't dump all the nested macros.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These comments were wrong. Remove them instead of fixing them, since
now that we have this small header file, it's much easier to follow the
preprocessor conditionals.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
sprintf(3) does not take the destination buffer into account. Although
the destination in these case is large enough, sprintf(3) indicates a
code smell.
Use snprintf(3).
commonio.c:522:15: warning: Although the value stored to 'cp' is used in the enclosing expression, the value is never actually read from 'cp' [deadcode.DeadStores]
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Documentation:
- Correct the comment documenting the function:
write_full() doesn't write "up to" count bytes (which is write(2)'s
behavior, and exactly what this function is designed to avoid), but
rather exactly count bytes (on success).
- While fixing the documentation, take the time to add a man-page-like
comment as in other APIs. Especially, since we'll have to document
a few other changes from this patch, such as the modified return
values.
- Partial writes are still possible on error. It's the caller's
responsibility to handle that possibility.
API:
- In write(2), it's useful to know how many bytes were transferred,
since it can have short writes. In this API, since it either writes
it all or fails, that value is useless, and callers only want to know
if it succeeded or not. Thus, just return 0 or -1.
Implementation:
- Use `== -1` instead of `< 0` to check for write(2) syscall errors.
This is wisdom from Michael Kerrisk. This convention is useful
because it more explicitly tells maintainers that the only value
which can lead to that path is -1. Otherwise, a maintainer of the
code might be confused to think that other negative values are
possible. Keep it simple.
- The path under `if (res == 0)` was unreachable, since the loop
condition `while (count > 0)` precludes that possibility. Remove the
dead code.
- Use a temporary variable of type `const char *` to avoid a cast.
- Rename `res`, which just holds the result from write(2), to `w`,
which more clearly shows that it's just a very-short-lived variable
(by it's one-letter name), and also relates itself more to write(2).
I find it more readable.
- Move the definition of `w` to the top of the function. Now that the
function is significantly shorter, the lifetime of the variable is
clearer, and I find it more readable this way.
Use:
- Also use `== -1` to check errors.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If shadow is built without libbsd support, then readpassphrase() needs
to be provided from the project.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
With the recent changes both login and su compilation fail because there
are some missing dependencies from SELINUX library. Thus, add LIBSELINUX
to su and login for those cases where the library is used.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Remove `utmp` structure as an argument and include its logic inside the
function. This will help remove any reference to utmp from login.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
The functionality from this function is related to utmp. Restrict access
to `setutmp()` to the same file.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
In some embedded systems, users only use the ps
provided by the busybox. But the ps provided by
the busybox does not support the -eo option by
default. As a result, an error is reported when
the userdel is used. So add a judgment on ps.
If there is no ps -eo, traverse the process directly.
The error information is as follows:
# userdel xsl
ps: invalid option -- 'e'
Signed-off-by: xiongshenglan <xiongshenglan@huawei.com>
Print a warning even for the root user if the provided shell isn't
listed in /etc/shells, but continue to execute the action.
In case of non root user exit.
See https://github.com/shadow-maint/shadow/issues/535
Add a comment at the top of that file explaining how to
regenerate it.
We should add a README, but I don't have time to draft one
right now.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Since newgrp is setuid-root, any write() system calls it does in order
to print error messages will be done as the root user.
Unprivileged users can get newgrp to print essentially arbitrary strings
to any open file in this way by passing those strings as argv[0] when
calling execve(). For example:
$ setpid() { (exec -a $1$'\n:' newgrp '' 2>/proc/sys/kernel/ns_last_pid & wait) >/dev/null; }
$ setpid 31000
$ readlink /proc/self
31001
This is not a vulnerability in newgrp; it is a bug in the Linux kernel.
However, this type of bug is not new [1] and it makes sense to try to
mitigate these types of bugs in userspace where possible.
[1]: https://lwn.net/Articles/476947/
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
b1282224 (Add maximum padding to fit IPv6-Addresses, 2020-05-24) pads
the From field header using `maxIPv6Addrlen - 3`. This leaves the
Latest field header misaligned. Subtract 4 (the length of "From").
Fixes build error:
newusers.c: In function 'update_passwd':
newusers.c:433:21: error: 'sflg' undeclared (first use in this function); did you mean 'rflg'?
introduced by
5cd04d03f9
which forgot to define sflg for these configure options:
--without-sha-crypt --without-bcrypt --with-yescrypt
Using the --sha-rounds option without first giving a crypt method via the --crypt-method option results in comparisons with a NULL pointer and thus make chgpasswd segfault:
$ chgpasswd -s 1
zsh: segmentation fault chgpasswd -s 1
Current patch add a sanity check before these comparisons to ensure there is a defined encryption method.
How to trigger this password leak?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
When gpasswd(1) asks for the new password, it asks twice (as is usual
for confirming the new password). Each of those 2 password prompts
uses agetpass() to get the password. If the second agetpass() fails,
the first password, which has been copied into the 'static' buffer
'pass' via STRFCPY(), wasn't being zeroed.
agetpass() is defined in <./libmisc/agetpass.c> (around line 91), and
can fail for any of the following reasons:
- malloc(3) or readpassphrase(3) failure.
These are going to be difficult to trigger. Maybe getting the system
to the limits of memory utilization at that exact point, so that the
next malloc(3) gets ENOMEM, and possibly even the OOM is triggered.
About readpassphrase(3), ENFILE and EINTR seem the only plausible
ones, and EINTR probably requires privilege or being the same user;
but I wouldn't discard ENFILE so easily, if a process starts opening
files.
- The password is longer than PASS_MAX.
The is plausible with physical access. However, at that point, a
keylogger will be a much simpler attack.
And, the attacker must be able to know when the second password is being
introduced, which is not going to be easy.
How to read the password after the leak?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Provoking the leak yourself at the right point by entering a very long
password is easy, and inspecting the process stack at that point should
be doable. Try to find some consistent patterns.
Then, search for those patterns in free memory, right after the victim
leaks their password.
Once you get the leak, a program should read all the free memory
searching for patterns that gpasswd(1) leaves nearby the leaked
password.
On 6/10/23 03:14, Seth Arnold wrote:
> An attacker process wouldn't be able to use malloc(3) for this task.
> There's a handful of tools available for userspace to allocate memory:
>
> - brk / sbrk
> - mmap MAP_ANONYMOUS
> - mmap /dev/zero
> - mmap some other file
> - shm_open
> - shmget
>
> Most of these return only pages of zeros to a process. Using mmap of an
> existing file, you can get some of the contents of the file demand-loaded
> into the memory space on the first use.
>
> The MAP_UNINITIALIZED flag only works if the kernel was compiled with
> CONFIG_MMAP_ALLOW_UNINITIALIZED. This is rare.
>
> malloc(3) doesn't zero memory, to our collective frustration, but all the
> garbage in the allocations is from previous allocations in the current
> process. It isn't leftover from other processes.
>
> The avenues available for reading the memory:
> - /dev/mem and /dev/kmem (requires root, not available with Secure Boot)
> - /proc/pid/mem (requires ptrace privileges, mediated by YAMA)
> - ptrace (requires ptrace privileges, mediated by YAMA)
> - causing memory to be swapped to disk, and then inspecting the swap
>
> These all require a certain amount of privileges.
How to fix it?
~~~~~~~~~~~~~~
memzero(), which internally calls explicit_bzero(3), or whatever
alternative the system provides with a slightly different name, will
make sure that the buffer is zeroed in memory, and optimizations are not
allowed to impede this zeroing.
This is not really 100% effective, since compilers may place copies of
the string somewhere hidden in the stack. Those copies won't get zeroed
by explicit_bzero(3). However, that's arguably a compiler bug, since
compilers should make everything possible to avoid optimizing strings
that are later passed to explicit_bzero(3). But we all know that
sometimes it's impossible to have perfect knowledge in the compiler, so
this is plausible. Nevertheless, there's nothing we can do against such
issues, except minimizing the time such passwords are stored in plain
text.
Security concerns
~~~~~~~~~~~~~~~~~
We believe this isn't easy to exploit. Nevertheless, and since the fix
is trivial, this fix should probably be applied soon, and backported to
all supported distributions, to prevent someone else having more
imagination than us to find a way.
Affected versions
~~~~~~~~~~~~~~~~~
All. Bug introduced in shadow 19990709. That's the second commit in
the git history.
Fixes: 45c6603cc8 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Reported-by: Alejandro Colomar <alx@kernel.org>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Seth Arnold <seth.arnold@canonical.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: Balint Reczey <rbalint@debian.org>
Cc: Sam James <sam@gentoo.org>
Cc: David Runge <dvzrv@archlinux.org>
Cc: Andreas Jaeger <aj@suse.de>
Cc: <~hallyn/shadow@lists.sr.ht>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Invert conditional to reduce indentation.
- Reduce use of whitespace and newlines while unindenting.
- Reorder variable declarations.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Invert conditional to reduce indentation.
- Rewrite while loop calling strtok(3) as a for loop. This allows
doing more simplification inside the loop (see next commit).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Fix indentation. It was very broken.
- Move variable declaration to the top of the block in which it's used.
- Reduce use of whitespace and newlines.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Invert conditional, to reduce indentation.
- Reduce use of whitespace and newlines while unindenting.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Merge nested conditionals into a single if, to reduce indentation.
- Indent (1 SP) nested preprocessor conditionals.
- Reduce use of whitespace and newlines while unindenting.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Use the temporary variable more, as it helps readability: it removes
a derefecence, which itself allows removing some parentheses.
- Use a shorter name, which is more common with temporaries, and so
there's less to read.
- Assign to *ranges at the end of the function. It's the same, but
with the other changes, I think this makes it slightly clearer.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If we consider simple objects as arrays of size 1, we can considerably
simplify these APIs, merging the *ARRAY and the non-array variants.
That will produce more readable code, since lines will be shorter (by
not having ARRAY in the macro names, as all macros will consistently
handle arrays), and the allocated size will be also more explicit.
The syntax will now be of the form:
p = MALLOC(42, foo_t); // allocate 42 elements of type foo_t.
p = MALLOC(1, bar_t); // allocate 1 element of type foo_t.
The _array() allocation functions should _never_ be called directly, and
instead these macros should be used.
The non-array functions (e.g., malloc(3)) still have their place, but
are limited to allocating structures with flexible array members. For
any other uses, the macros should be used.
Thus, we don't use any array or ARRAY variants in any code any more, and
they are only used as implementation details of these macros.
Link: <https://software.codidact.com/posts/285898/288023#answer-288023>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alloca(3) fails silently if not enough memory can be allocated on the
stack. Use checked dynamic allocation instead.
Also drop unnecessary manual NUL assignment, ensured by snprintf(3).
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Allocate enough memory for the strings, two slashes and the NUL
terminator.
Reported-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Those comments were written when this function used 64 bits (and
temporary variables of 128 bits). Now it uses 32 bits, with temporaries
of 64 bits, so some values have changed.
Fixes: 2a61122b5e ("Unoptimize the higher part of the domain of csrand_uniform()")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This makes the function fit in less screens. This is to avoid consuming
more natural resources than we have available, and everyone knows the
supply of new-lines on a screen is not a renewable source[1].
Some transformations have been done thanks to free(NULL) being an alias
for loopity_loop(), as defined three comits ago. The real definition of
free(3) that everyone has been hiding is this:
void
free(void *p)
{
if (p == NULL)
loopity_loop();
else
real_free(p);
}
Link: [1] <https://www.kernel.org/doc/html/v6.3/process/coding-style.html#placing-braces-and-spaces>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This was to a loop, as "1234" is to computer security.
No really; a loop that ends in a (forward) goto, and has no continue in it.
Still want a loop? Take two:
#define loopity_loop() do { for (;;) { break; } continue; } while (0-0)
Closes: <https://github.com/shadow-maint/shadow/issues/736>
Easter-egg: 8492dee663 ("subids: support nsswitch")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The permission also need to be checked before process_root_flag() since
that can chroot into non-selinux environment (unavailable selinux mount
point for example).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
- Use SIZE_MAX rather than (size_t)-1, to improve readability.
- Move the only branch that breaks to the first place, so that we
remove an else. This reduces nesting while parsing the code.
- Now that we only have a 2-branch conditional where both branches
assign to the same variable, rewrite it as a ternary, to shorten.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The error handling is performed after the loop. By just calling break it
is possible to reuse the error handling if status is not ERANGE.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
A failure of DUP_FUNCTION is already handled for non-reentrant
function wrapper. Perform the check for reentrant version as well.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
On failure, these are meant to return 0 with errno set. But if
an nss module is loaded, they were returning -ERRNO instead.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
- Don't else after return or fail_exit().
- Prefer == over != (negated logic is more complex to think about it).
- Reduce nesting when reasonable.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There's no reason to report all errors. Bail out at the first one,
which is simpler.
Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Some errors were being reported in stderr, but then they weren't really
being treated as errors.
If mkdir(2) for EEXIST, it's possible that the sysadmin pre-created the
user dir; don't fail. However, let's keep a log line, for having some
notice that it happened.
Also, run chmod(2) if mkdir(2) failed for EEXIST (so transform the
'else if' into an 'if').
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
OpenSSH and coreutils' chroot call chroot first and then chdir. Doing it
this way is a bit safer because otherwise something could happen between
chdir and chroot to the specified path (like exchange of links) so the
working directory would not end up within the chroot environment.
This is a purely defensive measure.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
This is no real world security fix.
The overflow could occur if too many layered subsystems are encountered
because the function check_perms calls itself recursively.
It would already take a misconfigured system for this to achieve it.
Use an iterative approach by calling the do_check_perms in a loop
instead of calling itself recursively.
As a side note: At least GCC 13 optimized this code and already uses
a jmp in its assembler code. I could only see the stack overflow by
activating address sanitizer which prevented the optimization.
Co-developed-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
If a user has home directory "/" and login shell "*" then login and su
enter an endless loop by constantly switching to the next subsystem.
This could also be achieved with a layered approach so just checking
for "/" as home directory is not enough to protect against such a
misconfiguration.
Just break the loop if it progressed too far. I doubt that this has
negative impact on any real setup.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
If econf_getStringValue() fails, it will return an error and
set value to NULL. Look for the error and avoid dereferencing
value in that case.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
The function is completely different based on USE_CONF. Either copy
will be easier to read if we just keep them completely separate.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
You can see the memory leaks with address sanitizer if shadow is
compiled with `--enable-vendordir=/usr/etc`.
How to reproduce:
1. Prepare a custom shell file as root
```
mkdir -p /etc/shells.d
echo /bin/myshell > /etc/shells.d/custom
```
2. Run chsh as regular user
```
chsh
```
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The getusershell implementation of musl returns every line within the
/etc/shells file, which even includes comments. Only consider absolute
paths for login shells.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Using --prefix in a setuid binary is quite dangerous. An unprivileged
user could prepare a custom shadow file in home directory. During a data
race the user could exchange directories with links which could lead to
exchange of shadow file in system's /etc directory.
This could be used for local privilege escalation.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Please find attached the french updated translation of shadow-man-page,
proofread by the debian-l10n-french mailing list contributors.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Neither a pid_t below 1 nor a negative fd could be valid in this context.
Proof of Concept:
$ newuidmap -1 1 1 1
newuidmap: Could not open proc directory for target 4294967295
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Some strings are first written into static char arrays before passed to
functions which expect a const char pointer anyway.
It is easier to pass these strings directly as arguments.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
The fcntl call to set FD_CLOEXEC can be performed directly with the
previously performed open call by using the O_CLOEXEC flag.
O_CLOEXEC is required by POSIX.1-2008.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
The only user of login_prompt is the login tool. This implies that the
first argument is always the same.
It is much easier to verify printf's format string and its argument if
both are next to each other.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Parsing optional environment variables after a login name is a feature
which is neither documented nor available in util-linux or busybox
login which are other wide spread login utilities used in Linux
distributions as reference.
Removing this feature resolves two issues:
- A memory leak exists if variables without an equal sign are used,
because set_env creates copies on its own. This could lead to OOM
situations in privileged part of login or may lead to heap spraying.
- Environment variables are not reset between login attempts. This
could lead to additional environment variables set for a user who
never intended to do so.
Proof of Concept on a system with shadow login without PAM and
util-linux agetty:
1. Provoke an invalid login, e.g. user `noone` and password `invalid`.
This starts shadow login and subsequent inputs are passed through
the function login_prompt.
2. Provoke an invalid login with environment variables, e.g.
user `noone HISTFILE=/tmp/owo` and password `invalid`.
3. Log in correctly with user `root`.
Now you can see with `echo $HISTFILE` that `/tmp/owo` has been set for
the root user.
This requires a malicious failed login attempt and a successful login
within the configured login timeout (default 60 seconds).
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
The getline function does not return a pointer but the amount of read
characters. The error return value to check for is -1.
Set buf to NULL to avoid dereference of an uninitialized stack value.
The getline function returns -1 if size argument is NULL. Always use
a valid pointer even if size is unimportant.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Add the relevant XKCD to the passwd(1) manual page. It already explains
most of the rationale behind this patch.
Add also reference to makepasswd(1), which is a good way to generate
strong passwords. Personally, I commonly run `makepasswd --chars 64` to
create my passwords, or 32 for passwords I need to type interactively
often.
The strength of a password is an exponential formula, where the base is
the size of the character set, and the exponent is the length of the
password. That already shows why long passwords of just lowercase
letters are better than short Pa$sw0rdZ3. But an even more important
point is that humans, when forced to use symbols in a password, are more
likely to do trivial substitutions on simple passwords, which doesn't
increase strength, and can instead give a false sense of strength, which
is dangerous.
Closes: <https://github.com/shadow-maint/shadow/issues/688>
Link: <https://xkcd.com/936/>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Destroying the handle does not actually disconnect, see [1].
Also free the key on user removal.
[1]: e9072e7d45/libsemanage/src/direct_api.c (L330)
Example adduser leak:
Direct leak of 1008 byte(s) in 14 object(s) allocated from:
#0 0x5638f2e782ae in __interceptor_malloc (./src/useradd+0xee2ae)
#1 0x7fb5cfffad09 in dbase_file_init src/database_file.c:170:45
Direct leak of 392 byte(s) in 7 object(s) allocated from:
#0 0x5638f2e782ae in __interceptor_malloc (./src/useradd+0xee2ae)
#1 0x7fb5cfffc929 in dbase_policydb_init src/database_policydb.c:187:27
Direct leak of 144 byte(s) in 2 object(s) allocated from:
#0 0x5638f2e782ae in __interceptor_malloc (./src/useradd+0xee2ae)
#1 0x7fb5cfffb519 in dbase_join_init src/database_join.c:249:28
[...]
Free the actual struct of the removed entry.
Example userdel report:
Direct leak of 40 byte(s) in 1 object(s) allocated from:
#0 0x55b230efe857 in reallocarray (./src/userdel+0xda857)
#1 0x55b230f6041f in mallocarray ./lib/./alloc.h:97:9
#2 0x55b230f6041f in commonio_open ./lib/commonio.c:563:7
#3 0x55b230f39098 in open_files ./src/userdel.c:555:6
#4 0x55b230f39098 in main ./src/userdel.c:1189:2
#5 0x7f9b48c64189 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
run_parts currently exists in useradd and userdel, this commit mirrors
the functionality with groupadd and groupdel
Hook for group{add,del} to include killing processes that have group
membership that would no longer exist to avoid membership ID reuse.
getline(3) is much more readable than manually looping. It has some
overhead due to the allocation of a buffer, but that shouldn't be a
problem here. If that was a problem, we could reuse the buffer (thus
making the function non-reentrant), but I don't think that's worth the
extra complexity.
Using rpmatch(3) instead of a simple y/n test provides i18n to the
response checking. We have a fall-back minimalistic implementation for
systems that lack this function (e.g., musl libc).
While we're at it, apply some other minor improvements to this file:
- Remove comment saying which files use this function. That's likely
to get outdated. And anyway, it's just a grep(1) away, so it doesn't
really add any value.
- Remove unnecessary casts to (void) that were used to verbosely ignore
errors from stdio calls. They add clutter without really adding much
value to the code (or I don't see it).
- Remove comments from the function body. They make the function less
readable. Instead, centralize the description of the function into a
man-page-like comment before the function definition. This keeps the
function body short and sweet.
- Add '#include <stdbool.h>', which was missing.
- Minor whitespace style changes (it doesn't hurt the diff at this
point, since most of the affected lines were already touched by other
changes, so I applied my preferred style :).
Acked-by: Samanta Navarro <ferivoz@riseup.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The tools newgrp and useradd expect waitpid to behave as described in
its manual page. But the notes indicate that if SIGCHLD is ignored,
waitpid behaves differently.
A user could set SIGCHLD to ignore before starting newgrp through exec.
Children of newgrp would not become zombies and their PIDs could be
reassigned before newgrp could call kill with the child pid and SIGCONT.
The useradd tool is not installed setuid, but I have added the default
there as well (copied from vipw).
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Do not stop after 79 characters. Read the complete line to avoid
arbitrary limitations.
Proof of Concept:
```
cat > passwd-poc << EOF
root:x:0:0:root:/root:/bin/bash
root:x:0:0:root:/root:/bin/bash
root:x:0:0:root:/root:/bin/bash
EOF
python -c "print(80*'y')" | pwck passwd-poc
```
Two lines should still be within the file because we agreed only once
to remove a duplicated line.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
If make fails in a multi-process invocation, the log is pretty much
unreadable. To make it readable, build as much as can be built without
failing. Then run a single-process make again. If we succeeded
previously, this should be a no-op. If not, this run will stop at the
first error, which should be more readable, and will only print the few
lines we're interested in.
This has some side effects: Now we build as much as we can, instead of
failing as early as possible; this may make CI a bit slower. However,
it also has the benefit that you see _all_ the error messages that could
be given, instead of needing to fix the first error to see the next and
so on.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
libbsd-devel libeconf-devel have already been added to the spec file and
they should be installed by the `dnf builddep` command.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
e5905c4b ("Added control character check") introduced checking for
control characters but had the logic inverted, so it rejects all
characters that are not control ones.
Cast the character to `unsigned char` before passing to the character
checking functions to avoid UB.
Use strpbrk(3) for the illegal character test and return early.
Both semanage and libsemanage actually set the user's mls range to the
default of the seuser, which makes more sense and removes a bit of code
for usermod and useradd. More fine-grained details must always be set
with some other tool
(semanage) anyway.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
The --gid option accepts a group name or id. When a name is provided, it
is resolved to an id by looking up the name in the group database
(/etc/group).
The --prefix option overides the location of the passwd and group
databases. I suspect the --gid option was overlooked when wiring up the
--prefix option.
useradd --gid already respects --prefix; this change makes usermod
behave the same way.
Fixes: b6b2c756c9
Signed-off-by: Mike Gilbert <floppym@gentoo.org>
This commit will serve to document why we shouldn't worry about the
truncation in the call to strlcpy(3). Since we have one more byte in
tmptty than in full_tty, truncation will produce a string that is at
least one byte longer than full_tty. Such a string could never compare
equal, so we're actually handling the truncation in a clever way. Maybe
too clever, but that's why I'm documenting it here.
Now, about the simplification itself:
Since we made sure that both full_tty and tmptty are null-terminated, we
can call strcmp(3) instead of strncmp(3). We can also simplify the
return logic avoiding one branch.
Cc: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
* libmisc/utmp.c (is_my_tty): Declare the parameter as a char array,
not char *, as it is not necessarily null-terminated.
Avoid a read overrun when reading 'tty', which comes from
'ut_utname'.
Reported-by: Paul Eggert <eggert@cs.ucla.edu>
Co-developed-by: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
* libmisc/date_to_str.c (date_to_str): Do not crash if gmtime(3)
returns NULL because the timestamp is far in the future.
Reported-by: Paul Eggert <eggert@cs.ucla.edu>
Co-developed-by: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
* lib/gshadow.c (sgetsgent): Use strcpy(3) not strlcpy(3),
since the string is known to fit.
Signed-off-by: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
* lib/fields.c (change_field): Don't point
before array start; that has undefined behavior.
Signed-off-by: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
* lib/fields.c (change_field): Since we know the string fits,
use strcpy(3) rather than strlcpy(3).
Signed-off-by: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
gettime.c:25:30: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
/*@observer@*/time_t gettime ()
^
void
shellcheck warns against using echo with flags, as posix sh won't
support it. It suggests using printf, so let's do that.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
bc github...
For some reason, the first test - ONLY on github - seems to not
give the '$ ' prompt expected when you spawn 'su testsuite'.
So just run the first test twice, and ignore the first failure.
It messes with the expected results.
We can do better than this in the expect scripts, but let's
get things running for now.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
write_mapping() will do the following:
openat(proc_dir_fd, map_file, O_WRONLY);
An attacker could create a directory containing a symlink named
"uid_map" pointing to any file owned by root, and thus allow him to
overwrite any root-owned file.
Closes#635
newuidmap and newgidmap currently take an integner pid as
the first argument, determining the process id on which to
act. Accept also "fd:N", where N must be an open file
descriptor to the /proc/pid directory for the process to
act upon. This way, if you
exec 10</proc/99
newuidmap fd:10 100000 0 65536
and pid 99 dies and a new process happens to take pid 99 before
newuidmap happens to do its work, then since newuidmap will use
openat() using fd 10, it won't change the mapping for the new
process.
Example:
// terminal 1:
serge@jerom ~/src/nsexec$ ./nsexec -W -s 0 -S 0 -U
about to unshare with 10000000
Press any key to exec (I am 129176)
// terminal 2:
serge@jerom ~/src/shadow$ exec 10</proc/129176
serge@jerom ~/src/shadow$ sudo chown root src/newuidmap src/newgidmap
serge@jerom ~/src/shadow$ sudo chmod u+s src/newuidmap
serge@jerom ~/src/shadow$ sudo chmod u+s src/newgidmap
serge@jerom ~/src/shadow$ ./src/newuidmap fd:10 0 100000 10
serge@jerom ~/src/shadow$ ./src/newgidmap fd:10 0 100000 10
// Terminal 1:
uid=0(root) gid=0(root) groups=0(root)
Signed-off-by: Serge Hallyn <serge@hallyn.com>
We can't use a pointer that was input to realloc(3), nor any pointers
that point to reallocated memory, without making sure that the memory
wasn't moved. If we do, the Behavior is Undefined.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Use of these macros, apart from the benefits mentioned in the commit
that adds the macros, has some other good side effects:
- Consistency in getting the size of the object from sizeof(type),
instead of a mix of sizeof(type) sometimes and sizeof(*p) other
times.
- More readable code: no casts, and no sizeof(), so also shorter lines
that we don't need to cut.
- Consistency in using array allocation calls for allocations of arrays
of objects, even when the object size is 1.
Cc: Valentin V. Bartenev <vbartenev@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This macros have several benefits over the standard functions:
- The type of the allocated object (not the pointer) is specified as an
argument, which improves readability:
- It is directly obvious what is the type of the object just by
reading the macro call.
- It allows grepping for all allocations of a given type.
This is admittedly similar to using sizeof() to get the size of the
object, but we'll see why this is better.
- In the case of reallocation macros, an extra check is performed to
make sure that the previous pointer was compatible with the allocated
type, which can avoid some mistakes.
- The cast is performed automatically, with a pointer type derived from
the type of the object. This is the best point of this macro, since
it does an automatic cast, where there's no chance of typos.
Usually, programmers have to decide whether to cast or not the result
of malloc(3). Casts usually hide warnings, so are to be avoided.
However, these functions already return a void *, so a cast doesn't
really add much danger. Moreover, a cast can even add warnings in
this exceptional case, if the type of the cast is different than the
type of the assigned pointer. Performing a manual cast is still not
perfect, since there are chances that a mistake will be done, and
even ignoring accidents, they clutter code, hurting readability.
And now we have a cast that is synced with sizeof.
- Whenever the type of the object changes, since we perform an explicit
cast to the old type, there will be a warning due to type mismatch in
the assignment, so we'll be able to see all lines that are affected
by such a change. This is especially important, since changing the
type of a variable and missing to update an allocation call far away
from the declaration is easy, and the consequences can be quite bad.
Cc: Valentin V. Bartenev <vbartenev@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We'll expand the contents in a following commit, so let's move the file
to a more generic name, have a dedicated header, and update includes.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Use the new header for xstrdup()
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This is guaranteed by ISO C. Now that we require ISO C (and even POSIX)
to compile, we can simplify this code.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
On 2/19/23 18:09, David Mudrich wrote:
> I am working on a RAM based Linux OS from source, and try to use
> latest versions of all software. I found shadow needs libbsd's
> readpassphrase(3) as superior alternative to getpass(3). While
> considering if I a) include libbsd, or include libbsd's code of
> readpassphrase(3) into shadow, found, that libbsd's readpassphrase(3)
> never returns \n or \r
> <https://cgit.freedesktop.org/libbsd/tree/src/readpassphrase.c>
> line 122, while agetpass() uses a check for \n in agetpass.c line 108.
> I assume it always fails.
Indeed, it always failed. I made a mistake when writing agetpass(),
assuming that readpassphrase(3) would keep newlines.
>
> I propose a check of len == PASS_MAX - 1, with false positive error for
> exactly PASS_MAX - 1 long passwords.
Instead, I added an extra byte to the allocation to allow a maximum
password length of PASS_MAX (which is the maximum for getpass(3), which
we're replacing.
While doing that, I notice that my previous implementation also had
another bug (minor): The maximum password length was PASS_MAX - 1
instead of PASS_MAX. That's also fixed in this commit.
Reported-by: David Mudrich <dmudrich@gmx.de>
Fixes: 155c9421b9 ("libmisc: agetpass(), erase_pass(): Add functions for getting passwords safely")
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Some programs don't support `(uint16_t) -1` or `(uint32_t) -1` as user
or group IDs. This is because `-1` is used as an error code or as an
unspecified ID, e.g. in `chown(2)` parameters, and in the past, `gid_t`
and `uid_t` have changed width. For legacy reasons, those values have
been kept reserved in programs today (for example systemd does this; see
the documentation in the link below).
This should not be confused with catching overflow in the ID values,
since that is already caught by our ERANGE checks. This is about not
using reserved values that have been reserved for legacy reasons.
Link: <https://systemd.io/UIDS-GIDS/>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
These comments should indicate which functions they really wrap.
An alternative would be to remove the line completely to avoid
future copy&paste mistakes.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
This function simplifies the calculation of the bounds of the buffer for
catenating strings. It would also reduce error checking, but we don't
care about truncation in this specific code. :)
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strncat(3), strlcpy(3), and many other functions are often misused for
catenating strings, when they should never be used for that. strlcat(3)
is good. However, there's no equivalent to strlcat(3) similar to
snprintf(3). Let's add stpecpy(), which is similar to strlcat(3), but
it is also the only function compatible with stpeprintf(), which makes
it more useful than strlcat(3).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
All the string-copying functions called above do terminate the strings
they create with a NUL byte. Writing it again at the end of the buffer
is unnecessary paranoid code. Let's remove it.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This function allows reducing error checking (since errors are
propagated across chained calls), and also simplifies the calculation of
the start and end of the buffer where the string should be written.
Moreover, the new code is more optimized, since many calls to strlen(3)
have been removed.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
[v]stpeprintf() are similar to [v]snprintf(3), but they allow chaining.
[v]snprintf(3) are very dangerous for catenating strings, since the
obvious ways to do it invoke Undefined Behavior, and the ways that avoid
UB are very error-prone.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
When trying to build shadow in a different directory I stumbled upon few
issues, this commit aims to fix all of them:
- The `subid.h` file is generated and hence in the build directory and
not in the source directory, so use `$(builddir)` instead of
`$(srcdir)`.
- Using `$<` instead of filenames utilises autotools to locate the files
in either the source or build directory automatically.
- `xsltproc` needs to access the files in login.defs.d in either the
source directory or the symlink in a language subdirectory, but it
does not interpret the `--path` as prefix of the entity path, but
rather a path under which to locate the basename of the entity
from the XML file. So specify the whole path to login.defs.d.
- The above point could be used to make the symlinks of login.defs.d
and entity path specifications in the XMLs obsolete, but I trying
not to propose possibly disrupting patches, so for the sake of
simplicity just specify `$(srcdir)` when creating the symlink.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
The intention of the code is just to not report an error message when
'typefile' doesn't exist. If we call access(2) and then fopen(2),
there's a race. It's not a huge problem, and the worst thing that can
happen is reporting an error when the file has been removed after
access(2). It's not a problem, but we can fix the race and at the same
time clarify the intention of not warning about ENOENT and also remove
one syscall. Seems like a win-win.
Suggested-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Every non-const pointer converts automatically to void *.
- Every pointer converts automatically to void *.
- void * converts to any other pointer.
- const void * converts to any other const pointer.
- Integer variables convert to each other.
I changed the declaration of a few variables in order to allow removing
a cast.
However, I didn't attempt to edit casts inside comparisons, since they
are very delicate. I also kept casts in variadic functions, since they
are necessary, and in allocation functions, because I have other plans
for them.
I also changed a few casts to int that are better as ptrdiff_t.
This change has triggered some warnings about const correctness issues,
which have also been fixed in this patch (see for example src/login.c).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We could use the standard (C11) _Noreturn qualifier, but it will be
deprecated in C23, and replaced by C++'s [[noreturn]], which is
compatible with the GCC attribute, so let's directly use the attribute,
and in the future we'll be able to switch to [[]].
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Recently, we removed support for 'struct utmpx'. We did it because utmp
and utmpx are identical, and while POSIX specifies utmpx (and not utmp),
GNU/Linux documentation seems to favor utmp. Also, this project
defaulted to utmp, so changing to utmpx would be more dangerous than
keeping old defaults, even if it's supposed to be the same.
Now, I just found more code that didn't make much sense: lib/utent.c
provides definitions for getutent(3) and friends in case the system
doesn't provide them, but we don't provide prototypes for those
definitions, so code using the functions would have never compiled.
Let's just remove these definitions as dead code.
Fixes: 3be7b9d75a ("Remove traces of utmpx")
Fixes: 170b76cdd1 ("Disable utmpx permanently")
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Closes#457
The existing prose was confusing, or simply wrong. Make it clear
that only the group ownership of the tty is affected, and how.
Also move the paragraph about defaults after the discussion of
acceptable TTYGROUPs, as this seems more natural.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
In variadic functions we still do the cast. In POSIX, it's not
necessary, since NULL is required to be of type 'void *', and 'void *'
is guaranteed to have the same alignment and representation as 'char *'.
However, since ISO C still doesn't mandate that, and moreover they're
doing dubious stuff by adding nullptr, let's be on the cautious side.
Also, C++ requires that NULL is _not_ 'void *', but either plain 0 or
some magic stuff.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If lines start with '\0' then it is possible to trigger out of
boundary accesses.
Check if indices are valid before accessing them.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
If the file referenced by ENV_TZ has a zero length string, then an out
of boundary write occurs. Also the result can be wrong because it is
assumed that the file will always end with a newline.
Only override a newline character with '\0' to avoid these cases.
This cannot be considered to be security relevant because login.defs
and its contained references to system files should be trusted to begin
with.
Proof of Concept:
1. Compile shadow's su with address sanitizer and --without-libpam
2. Setup your /etc/login.defs to contain ENV_TZ=/etc/tzname
3. Prepare /etc/tzname to contain a '\0' byte at the beginning
`python -c "print('\x00')" > /etc/tzname`
4. Use su
`su -l`
You can see the following output:
`tz.c:45:8: runtime error: index 18446744073709551615 out of bounds for type 'char [8192]'`
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
We do need the unoptimized version of csrand_uniform() for high values
of `n`, since the optimized version depends on having __int128, and it's
not available on several platforms, including ARMv7, IA32, and MK68k.
This reverts commit 848f53c1d3c1362c86d3baab6906e1e4419d2634; however,
I applied some tweaks to the reverted commit.
Reported-by: Adam Sampson <ats@offog.org>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Now that we optimized csrand_uniform(), we don't need these functions.
This reverts commit 7c8fe291b1260e127c10562bfd7616961013730f.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Use a different algorithm to minimize rejection. This is essentially
the same algorithm implemented in the Linux kernel for
__get_random_u32_below(), but written in a more readable way, and
avoiding microopimizations that make it less readable.
Which (the Linux kernel implementation) is itself based on Daniel
Lemire's algorithm from "Fast Random Integer Generation in an Interval",
linked below. However, I couldn't really understand that paper very
much, so I had to reconstruct the proofs from scratch, just from what I
could understand from the Linux kernel implementation source code.
I constructed some graphical explanation of how it works, and why it
is optimal, because I needed to visualize it to understand it. It is
published in the GitHub pull request linked below.
Here goes a wordy explanation of why this algorithm based on
multiplication is better optimized than my original implementation based
on masking.
masking:
It discards the extra bits of entropy that are not necessary for
this operation. This works as if dividing the entire space of
possible csrand() values into smaller spaces of a size that is
a smaller power of 2. Each of those smaller spaces has a
rejection band, so we get as many rejection bands as spaces
there are. For smaller values of 'n', the size of each
rejection band is smaller, but having more rejection bands
compensates for this, and results in the same inefficiency as
for large values of 'n'.
multiplication:
It divides the entire space of possible random numbers in
chunks of size exactly 'n', so that there is only one rejection
band that is the remainder of `2^64 % n`. The worst case is
still similar to the masking algorithm, a rejection band that is
almost half the entire space (n = 2^63 + 1), but for lower
values of 'n', by only having one small rejection band, it is
much faster than the masking algorithm.
This algorithm, however, has one caveat: the implementation
is harder to read, since it relies on several bitwise tricky
operations to perform operations like `2^64 % n`, `mult % 2^64`,
and `mult / 2^64`. And those operations are different depending
on the number of bits of the maximum possible random number
generated by the function. This means that while this algorithm
could also be applied to get uniform random numbers in the range
[0, n-1] quickly from a function like rand(3), which only
produces 31 bits of (non-CS) random numbers, it would need to be
implemented differently. However, that's not a concern for us,
it's just a note so that nobody picks this code and expects it
to just work with rand(3) (which BTW I tried for testing it, and
got a bit confused until I realized this).
Finally, here's some light testing of this implementation, just to know
that I didn't goof it. I pasted this function into a standalone
program, and run it many times to find if it has any bias (I tested also
to see how many iterations it performs, and it's also almost always 1,
but that test is big enough to not paste it here).
int main(int argc, char *argv[])
{
printf("%lu\n", csrand_uniform(atoi(argv[1])));
}
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 1 | wc -l
341
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 1 | wc -l
339
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 1 | wc -l
338
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 2 | wc -l
336
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 2 | wc -l
328
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 2 | wc -l
335
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 0 | wc -l
332
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 0 | wc -l
331
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 0 | wc -l
327
This isn't a complete test for a cryptographically-secure random number
generator, of course, but I leave that for interested parties.
Link: <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e9a688bcb19348862afe30d7c85bc37c4c293471>
Link: <https://github.com/shadow-maint/shadow/pull/624#discussion_r1059574358>
Link: <https://arxiv.org/abs/1805.10941>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Björn Esser <besser82@fedoraproject.org>
Cc: Yann Droneaud <ydroneaud@opteya.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Sam James <sam@gentoo.org>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
[Daniel Lemire: Added link to research paper in source code]
Cc: Daniel Lemire <daniel@lemire.me>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It is common to use the expression 'sizeof(x) * CHAR_BIT' to mean the
width in bits of a type or object. Now that there are _WIDTH macros for
some types, indicating the number of bits that they use, it makes sense
to wrap this calculation in a macro of a similar name.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This API is similar to arc4random_uniform(3). However, for an input of
0, this function is equivalent to csrand(), while arc4random_uniform(0)
returns 0.
This function will be used to reimplement csrand_interval() as a wrapper
around this one.
The current implementation of csrand_interval() doesn't produce very good
random numbers. It has a bias. And that comes from performing some
unnecessary floating-point calculations that overcomplicate the problem.
Looping until the random number hits within bounds is unbiased, and
truncating unwanted bits makes the overhead of the loop very small.
We could reduce loop overhead even more, by keeping unused bits of the
random number, if the width of the mask is not greater than
ULONG_WIDTH/2, however, that complicates the code considerably, and I
prefer to be a bit slower but have simple code.
BTW, Björn really deserves the copyright for csrand() (previously known
as read_random_bytes()), since he rewrote it almost from scratch last
year, and I kept most of its contents. Since he didn't put himself in
the copyright back then, and BSD-3-Clause doesn't allow me to attribute
derived works, I won't add his name, but if he asks, he should be put in
the copyright too.
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Björn Esser <besser82@fedoraproject.org>
Cc: Yann Droneaud <ydroneaud@opteya.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Sam James <sam@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions implement bit manipulation APIs, which will be added to
C23, so that in the far future, we will be able to replace our functions
by the standard ones, just by adding the stdc_ prefix, and including
<stdbit.h>.
However, we need to avoid UB for an input of 0, so slightly deviate from
C23, and use a different name (with _wrap) for distunguishing our API
from the standard one.
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Yann Droneaud <ydroneaud@opteya.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
arc4random(3) returns a number.
arc4random_buf(3) fills a buffer.
arc4random_uniform(3) returns a number less than a bound.
and I'd add a hypothetical one which we use:
*_interval() should return a number within the interval [min, max].
In reality, the function being called csrand() in this patch is not
really cryptographically secure, since it had a bias, but a subsequent
patch will fix that.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We were always casting the result to u_long. Better just use that type
in the function. Since we're returning u_long, it makes sense to also
specify the input as u_long. In fact, that'll help for doing bitwise
operations inside this function.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
I have plans to split this function in smaller functions that implement
bits of this functionallity, to simplify the implementation. So, let's
use names that distinguish them.
This one produces a number within an interval, so make that clear. Also
make clear that the function produces cryptographically-secure numbers.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strlcpy(3) might not be visible since it is declared in <bsd/string.h>.
This can lead to warnings, like:
fields.c: In function 'change_field':
fields.c:103:17: warning: implicit declaration of function 'strlcpy'; did you mean 'strncpy'? [-Wimplicit-function-declaration]
103 | strlcpy (buf, cp, maxsize);
| ^~~~~~~
| strncpy
../lib/fields.c:103:17: warning: type of 'strlcpy' does not match original declaration [-Wlto-type-mismatch]
103 | strlcpy (buf, cp, maxsize);
| ^
/usr/include/bsd/string.h:44:8: note: return value type mismatch
44 | size_t strlcpy(char *dst, const char *src, size_t siz);
| ^
/usr/include/bsd/string.h:44:8: note: type 'size_t' should match type 'int'
/usr/include/bsd/string.h:44:8: note: 'strlcpy' was previously declared here
/usr/include/bsd/string.h:44:8: note: code may be misoptimized unless '-fno-strict-aliasing' is used
Comparisons if different signedness can result in unexpected results.
Add casts to ensure operants are of the same type.
gettime.c: In function 'gettime':
gettime.c:58:26: warning: comparison of integer expressions of different signedness: 'long long unsigned int' and 'time_t' {aka 'long int'} [-Wsign-compare]
58 | } else if (epoch > fallback) {
| ^
Cast to time_t, since epoch is less than ULONG_MAX at this point.
idmapping.c: In function 'write_mapping':
idmapping.c:202:48: warning: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Wsign-compare]
202 | if ((written <= 0) || (written >= (bufsize - (pos - buf)))) {
| ^~
newgidmap.c: In function ‘main’:
newgidmap.c:178:40: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
178 | if ((written <= 0) || (written >= sizeof(proc_dir_name))) {
| ^~
newuidmap.c: In function ‘main’:
newuidmap.c:107:40: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
107 | if ((written <= 0) || (written >= sizeof(proc_dir_name))) {
| ^~
Instead of using volatile pointers to prevent the compiler from
optimizing the call away, use a memory barrier.
This requires support for embedded assembly, which should be fine after
the recent requirement bumps.
memset_s() has a different signature than memset(3) or explicit_bzero(),
thus the current code would not compile. Also memset_s()
implementations are quite rare.
Use the C23 standardized version memset_explicit(3).
Fixes: 7a799ebb ("Ensure memory cleaning")
Login timed out message prints only first few bytes when write is immediately followed by exit.
Calling exit from new handler provides enough time to display full message.
Commit 90424e7c ("Don't warn when failed to open /etc/nsswitch.conf")
removed the logging for failing to read /etc/nsswitch.conf to reduce the
noise in the case the file does not exists (e.g. musl based systems).
Reintroduce a warning if /etc/nsswitch.conf exists but we failed to read
it (e.g. permission denied).
Improves: 90424e7c ("Don't warn when failed to open /etc/nsswitch.conf")
gethostbyname(3) was removed in POSIX.1-2008. It has been obsoleted,
and replaced by getaddrinfo(3), which is superior in several ways:
- gethostbyname(3) is not reentrant. There's a GNU extension,
gethostbyname_r(3) which is reentrant, but it's not likely to be
standardized for the following reason. And we don't care too much
about this point either.
- gethostbyname(3) only supports IPv4, but getaddrinfo(3) supports both
IPv4 and IPv6 (and may support other address families in the future).
We don't care about reentrancy, so for keeping the code simple (i.e.,
not touch call site to add code to free(3) an allocated buffer), I added
a static buffer for inet_ntop(3). We could address that in the future,
but I don't think it's worth it.
BTW, we also replace inet_ntoa(3) by inet_ntop(3), as a consequence of
using getaddrinfo(3). inet_ntoa(3) is also marked as deprecated, but
that deprecation seems to have been documented only in the manual page,
and POSIX doesn't mark it as deprecated. The deprecation notice goes
back to when the inet_ntop(3) manual page was added by Sam Varshavchik
to the Linux man-pages in version 1.30 (year 2000).
So, this, apart from updating the code to POSIX.1-2008, is also adding
support for IPv6 :) Although, probably many other parts of the code are
written for IPv4 only, so I wouldn't yet claim support for it.
A few notes:
- I didn't check the return value of inet_ntop(3), since it can't fail
for the given input:
- EAFNOSUPPORT: We only call it with AF_INET and AF_INET6.
- ENOSPC: We calculate the size of the buffer to be wide enough:
MAX(INET_ADDRSTRLEN, INET6_ADDRSTRLEN) so it always fits.
Cc: Dave Hagewood <admin@arrowweb.com>
Cc: Sam Varshavchik
Cc: Jakub Jelinek <jakub@redhat.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Systems can suffer power interruptions whilst .lock files are in /etc,
preventing scripts and other automation tools from updating shadow's
files which persist across boots.
This commit replaces that mechanism with file locking to avoid problems
of power interruption/crashing.
Minor tweak to groupmems man page, requested by 'xx' on IRC.
Signed-off-by: ed neville <ed@s5h.net>
When the caller may not change the room number, work phone, or
home number, then rather than prompting for the new one it will
print the existing one. But due to a typo it printed the full name
in place of each of those.
Fix the fields being printed.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
- Since strncpy(3) is not designed to write strings, but rather
(null-padded) character sequences (a.k.a. unterminated strings), we
had to manually append a '\0'. strlcpy(3) creates strings, so they
are always terminated. This removes dependencies between lines, and
also removes chances of accidents.
- Repurposing strncpy(3) to create strings requires calculating the
location of the terminating null byte, which involves a '-1'
calculation. This is a source of off-by-one bugs. The new code has
no '-1' calculations, so there's almost-zero chance of these bugs.
- strlcpy(3) doesn't padd with null bytes. Padding is relevant when
writing fixed-width buffers to binary files, when interfacing certain
APIs (I believe utmpx requires null padding at lease in some
systems), or when sending them to other processes or through the
network. This is not the case, so padding is effectively ignored.
- strlcpy(3) requires that the input string is really a string;
otherwise it crashes (SIGSEGV). Let's check if the input strings are
really strings:
- lib/fields.c:
- 'cp' was assigned from 'newft', and 'newft' comes from fgets(3).
- lib/gshadow.c:
- strlen(string) is calculated a few lines above.
- libmisc/console.c:
- 'cons' comes from getdef_str, which is a bit cryptic, but seems
to generate strings, I guess.1
- libmisc/date_to_str.c:
- It receives a string literal. :)
- libmisc/utmp.c:
- 'tname' comes from ttyname(3), which returns a string.
- src/su.c:
- 'tmp_name' has been passed to strcmp(3) a few lines above.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Since the project is supposed to be POSIX.1-2001 compliant it doesn't
make sense to have that added conditionally.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
It is Undefined Behavior to declare errno (see NOTES in its manual page).
Instead of using the errno dummy declaration, use one that doesn't need
a comment.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Previous commits, to keep readability of the diffs, left the code that
was previously wrapped by preprocessor coditionals untouched. Apply
some minor cosmetic changes to merge it in the surrounding code.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- USER_NAME_MAX_LENGTH was being calculated in terms of utmpx. Do it
in terms of utmp.
- Remove utmpx support from the whishlist.
- Remove unused tests about utmpx members.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
On Linux, utmpx and utmp are identical. However, documentation (manual
pages) covers utmp, and just says about utmpx that it's identical to
utmp. It seems that it's preferred to use utmp, at least by reading the
manual pages.
Moreover, we were defaulting to utmp (utmpx had to be explicitly enabled
at configuration time). So, it seems safer to just make it permanent,
which should not affect default builds.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We already made that assumption in commit b47aa1e9aa. While the
header is not required by POSIX (it is an XSI extension), it is defined
in systems that are of interest to this project (GNU/Linux).
Fixes: b47aa1e9aa ("Assume <utmpx.h> exists")
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
When useradd sends its ADD_USER event, it is filling in the id field. This is not yet written to disk. When auditd sees the event and the log format is enriched, auditd tries to lookup the user name but it does not exist. This causes the event to never be resolvable since ausearch relies on the lookup information attached by auditd.
The fix is to not send the id information for any event until after close_files() is called. Just the acct field is all that is
Patch by Steve Grubb (afaik).
Reported at https://bugzilla.redhat.com/show_bug.cgi?id=1713432
The OSes that are referred to by these comments, are extinct, but
their comments survived, fossilized in amber.
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
In a previous commit, we made USE_TERMIOS unconditionally defined.
Let's just remove it, and remove the condition everywhere.
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
All of the macros we're using are required by POSIX.1-2001.
Cc: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The function is obsolete. It is recommended to use getrlimit(2) instead
(see the manual page for ulimit(3) or the POSIX manual for it). Since
getrlimit(2) is required by POSIX.1-2001, we can rely on it.
Cc: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Clang doesn't implement this attribute and reports an error. Work
around it by hiding it in a macro that will be empty in clang.
Reported-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There are several issues with getpass(3).
Many implementations of it share the same issues that the infamous
gets(3). In glibc it's not so terrible, since it's a wrapper
around getline(3). But it still has an important bug:
If the password is long enough, getline(3) will realloc(3) memory,
and prefixes of the password will be laying around in some
deallocated memory.
See the getpass(3) manual page for more details, and especially
the commit that marked it as deprecated, which links to a long
discussion in the linux-man@ mailing list.
So, readpassphrase(3bsd) is preferrable, which is provided by
libbsd on GNU systems. However, using readpassphrase(3) directly
is a bit verbose, so we can write our own wrapper with a simpler
interface similar to that of getpass(3).
One of the benefits of writing our own interface around
readpassphrase(3) is that we can hide there any checks that should
be done always and which would be error-prone to repeat every
time. For example, check that there was no truncation in the
password.
Also, use malloc(3) to get the buffer, instead of using a global
buffer. We're not using a multithreaded program (and it wouldn't
make sense to do so), but it's nice to know that the visibility of
our passwords is as limited as possible.
erase_pass() is a clean-up function that handles all clean-up
correctly, including zeroing the entire buffer, and then
free(3)ing the memory. By using [[gnu::malloc(erase_pass)]], we
make sure that we don't leak the buffers in any case, since the
compiler will be able to enforce clean up.
Link: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit?id=7ca189099d73bde954eed2d7fc21732bcc8ddc6b>
Reported-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The missing #include <gshadow.h> causes the configure check to fail
spuriously, resulting in HAVE_SHADOWGRP not being defined even
on systems that actually have sgetsgent (such as current glibc).
Allow supplementary groups to be set via the /etc/default/useradd config
file. Allowing an administrator to set additonal groups via the GROUPS
configurable and control the default behaviour of useradd.
This program has 10 calls to gets(3) according to grep(1). That
makes it a very unsafe program which should not be used at all.
Let's kill the program already.
See what gets(3) has to say:
SYNOPSIS
#include <stdio.h>
[[deprecated]] char *gets(char *s);
DESCRIPTION
Never use this function.
...
BUGS
Never use gets(). Because it is impossible to tell with‐
out knowing the data in advance how many characters
gets() will read, and because gets() will continue to
store characters past the end of the buffer, it is ex‐
tremely dangerous to use. It has been used to break com‐
puter security. Use fgets() instead.
For more information, see CWE‐242 (aka "Use of Inherently
Dangerous Function") at http://cwe.mitre.org/data/defini‐
tions/242.html
Acked-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The minimum id allocation for system accounts shouldn't be 0 as this is
reserved for root.
Signed-off-by: Tomáš Mráz <tm@t8m.info>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Report error if usermod asked for moving homedir and it does not exist.
Signed-off-by: Tomáš Mráz <tm@t8m.info>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Introduced by c6c8130db4
After removing snprintf, the format string should get unescaped once.
Fixes#564
Reporter and patch author: DerMouse (github.com/DerMouse)
glibc, musl, FreeBSD, and OpenBSD define the MAX() and MIN()
macros in <sys/param.h> with the same definition that we use.
Let's not redefine it here and use the system one, as it's
effectively the same as we define (modulo whitespace).
See:
shadow (previously):
alx@asus5775:~/src/shadow/shadow$ grepc -ktm MAX
./lib/defines.h:318:#define MAX(x,y) (((x) > (y)) ? (x) : (y))
glibc:
alx@asus5775:~/src/gnu/glibc$ grepc -ktm -x 'sys/param.h$' MAX
./misc/sys/param.h:103:#define MAX(a,b) (((a)>(b))?(a):(b))
musl:
alx@asus5775:~/src/musl/musl$ grepc -ktm -x 'sys/param.h$' MAX
./include/sys/param.h:19:#define MAX(a,b) (((a)>(b))?(a):(b))
OpenBSD:
alx@asus5775:~/src/bsd/openbsd/src$ grepc -ktm -x 'sys/param.h$' MAX
./sys/sys/param.h:193:#define MAX(a,b) (((a)>(b))?(a):(b))
FreeBSD:
alx@asus5775:~/src/bsd/freebsd/freebsd-src$ grepc -ktm -x 'sys/param.h$' MAX
./sys/sys/param.h:333:#define MAX(a,b) (((a)>(b))?(a):(b))
Signed-off-by: Alejandro Colomar <alx@kernel.org>
free(3) accepts NULL, since the oldest ISO C. I guess the
paranoid code was taking care of prehistoric implementations of
free(3). I've never known of an implementation that doesn't
conform to this, so let's simplify this.
Remove xfree(3), which was effectively an equivalent of free(3).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This tweaks the database locking logic so that failures in the
link-checking paths are more detailed.
The rationale for this is that I've experienced a non-deterministic
bug which seems to be coming from this logic, and I'd like to get
more details about the actual failing condition.
The setuid, setgid, and sticky bits are not copied during copy_tree.
Also start with very restrictive permissions before setting ownerships.
This prevents situations in which users in a group with less permissions
than others could win a race in opening the file before permissions are
removed again.
Proof of concept:
$ echo $HOME
/home/uwu
$ install -o uwu -g fandom -m 604 /dev/null /home/uwu/owo
$ ls -l /home/uwu/owo
-rw----r-- 1 uwu fandom 0 Sep 4 00:00 /home/uwu/owo
If /tmp is on another filesystem, then "usermod -md /tmp/uwu uwu" leads
to this temporary situation:
$ ls -l /tmp/uwu/owo
-rw----r-- 1 root root 0 Sep 4 00:00 /tmp/uwu/owo
This means that between openat and chownat_if_needed a user of group
fandom could open /tmp/uwu/owo and read the content when it is finally
written into the file.
Fixes regression introduced in faeab50e71.
If a directory contains fifos, then openat blocks until the other side
of the fifo is connected as well.
This means that users can prevent "usermod -m" from completing if their
home directories contain at least one fifo.
The groupadd from shadow does not allow upper case group names, the
same is true for the upstream shadow. But distributions like
Debian/Ubuntu/CentOS has their own way to cope with this problem,
this patch is picked up from Fedora [1] to relax the usernames
restrictions to allow the upper case group names, and the relaxation is
POSIX compliant because POSIX indicate that usernames are composed of
characters from the portable filename character set [A-Za-z0-9._-].
[1] https://src.fedoraproject.org/rpms/shadow-utils/blob/rawhide/f/shadow-4.8-goodname.patch
Signed-off-by: Alexander Kanavin <alex@linutronix.de>
useradd does not create the files if they don't exist, but if they exist
it will reset user data even if the data did not exist before creating
a hole and an explicitly zero'd data point resulting (especially for
high UIDs) in a lot of zeros ending up in containers and tarballs.
As rbalint points out, this was an exported fn. It also is
the only way for a libsubid user to do what it does, so let's
not drop it.
This reverts commit 477c8e6f42.
/* $OpenBSD: malloc.c,v 1.267 2020/11/23 15:42:11 otto Exp $ */
/*
* Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek <otto@drijf.net>
*
* Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
* copyright notice and this permission notice appear in all copies.
*
* THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
* WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
* MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
* ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
* WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
* ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
#include<stdlib.h>
#include<string.h>
void
freezero(void*ptr,size_tsz)
{
/* This is legal. */
if(ptr==NULL)
return;
explicit_bzero(ptr,sz);
free(ptr);
}
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
