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>
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>
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>
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
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>
* 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>
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>
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>
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>
[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>
- 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>
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>
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>
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>
The old code didn't produce very good random numbers. It had a bias.
And that was from performing some unnecessary floating-point
calculations that overcomplicate the problem.
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>
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>
