- 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>
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>
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>
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 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>
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.
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>
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>
* src/su.c (check_perms): Do not silently truncate user name.
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>
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>
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>
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 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>
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>
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>
- 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>
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>
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))) {
| ^~
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.
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>
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>
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>
This is shorter to write than 'unsigned long int', so we can collapse
some lines. It is guaranteed by C99.
Link: <https://github.com/shadow-maint/shadow/pull/607>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The buffers have a size of 512 (see xmalloc() above), which is what
snprintf(3) expects.
Link: <https://github.com/shadow-maint/shadow/pull/607>
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>
