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.
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: 2a61122b5e8f ("Unoptimize the higher part of the domain of csrand_uniform()") Signed-off-by: Alejandro Colomar <alx@kernel.org>
getline(3) might have succeeded in a previous iteration, in which case
p points to an offset that is not valid. Make p NULL at the end of the
loop, to make sure it doesn't hold old stuff.
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:
ch(g)passwd: Check selinux permissions upon startup
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>
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').
Samanta Navarro [Tue, 23 May 2023 11:57:50 +0000 (11:57 +0000)]
libmisc: Use safer chroot/chdir sequence
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.
Samanta Navarro [Tue, 23 May 2023 11:55:09 +0000 (11:55 +0000)]
su: Prevent stack overflow in check_perms
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.
Samanta Navarro [Tue, 23 May 2023 11:53:53 +0000 (11:53 +0000)]
subsystem: Prevent endless loop
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.
Samanta Navarro [Thu, 18 May 2023 11:58:19 +0000 (11:58 +0000)]
chsh: Verify that login shell path is absolute
The getusershell implementation of musl returns every line within the
/etc/shells file, which even includes comments. Only consider absolute
paths for login shells.
Samanta Navarro [Thu, 18 May 2023 11:56:17 +0000 (11:56 +0000)]
process_prefix_flag: Drop privileges
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.
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).
libmisc, man: Drop old check and advice for complex character sets in passwords
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.
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
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
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 :).
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).
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.
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>
Mike Gilbert [Sun, 26 Mar 2023 01:16:55 +0000 (21:16 -0400)]
usermod: respect --prefix for --gid option
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: b6b2c756c91806b1c3e150ea0ee4721c6cdaf9d0 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.
* 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>
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
projects / thirdparty / shadow.git / log
