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>
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).
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
The getline function does not return a pointer but the amount of read
characters. The error return value to check for is -1.
Set buf to NULL to avoid dereference of an uninitialized stack value.
The getline function returns -1 if size argument is NULL. Always use
a valid pointer even if size is unimportant.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
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.
Closes: <https://github.com/shadow-maint/shadow/issues/688>
Link: <https://xkcd.com/936/>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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
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.
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 :).
Acked-by: Samanta Navarro <ferivoz@riseup.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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>
Do not stop after 79 characters. Read the complete line to avoid
arbitrary limitations.
Proof of Concept:
```
cat > passwd-poc << EOF
root❌0:0:root:/root:/bin/bash
root❌0:0:root:/root:/bin/bash
root❌0:0:root:/root:/bin/bash
EOF
python -c "print(80*'y')" | pwck passwd-poc
```
Two lines should still be within the file because we agreed only once
to remove a duplicated line.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
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.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
libbsd-devel libeconf-devel have already been added to the spec file and
they should be installed by the `dnf builddep` command.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
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>
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>
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.
Cc: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
* 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>
* libmisc/date_to_str.c (date_to_str): Do not crash if gmtime(3)
returns NULL because the timestamp is far in the future.
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>
* 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>
