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