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
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>
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>
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>
- 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>
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>
