Commit Graph

3039 Commits

Author SHA1 Message Date
Samanta Navarro
4ad359ccc6 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.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-25 08:25:42 -05:00
Serge Hallyn
419cf1f1c4 def_load: avoid NULL deref
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>
2023-05-22 10:23:12 +02:00
Serge Hallyn
9e854f525d def_load: split the econf from non-econf definition
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>
2023-05-22 10:23:12 +02:00
Tobias Stoeckmann
8175b1532e Plug econf memory leaks
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>
2023-05-19 08:02:24 -05:00
Samanta Navarro
7321ceaf69 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.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-18 16:03:41 +02:00
Samanta Navarro
812f934e77 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.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-18 15:36:59 +02:00
bubu
1132b89236 Update French translations
Please find attached the french updated translation of shadow-man-page,
proofread by the debian-l10n-french mailing list contributors.

Signed-off-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-05-15 08:42:40 -05:00
Samanta Navarro
4ef4477535 get_pid.c: Use tighter validation checks
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>
2023-05-15 09:21:16 +02:00
Markus Hiereth
a022d39d2a replace inadequate German translation of login error message
Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-05-11 17:01:43 -05:00
Markus Hiereth
bd6db6f226 Update German translations
find the attached German message catalogue proofread by the German
language team.

Best regards
Markus

Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-05-11 11:05:59 -05:00
Samanta Navarro
666468cc36 Remove some static char arrays
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>
2023-05-11 11:05:29 -05:00
Samanta Navarro
72290ede0e commonio: Use do_lock_file again
This avoids regressions introduced with do_fcntl_lock.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-11 10:59:21 -05:00
Serge Hallyn
4abcbb056e Fix broken docbook translations
its by default does not support xml tags inside translatable
units.  Use custom its rules from

https://www.w3.org/TR/xml-i18n-bp/#relating-docbook-plus-its

to enable the tags which are in use by docbook.

Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-05-10 09:15:53 +02:00
ed neville
0bce9c9808 open with O_CREAT when lock path does not exist
Reported in #686, by wyj611 when trying to lock a file that is not
present

Lock method should be F_SETLKW rather than open file descriptor
2023-05-08 08:16:11 -05:00
Samanta Navarro
627631bf9a commonio_open: Remove fcntl call
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>
2023-05-05 16:15:46 +02:00
Samanta Navarro
e899e3d745 commonio_lock_nowait: Remove deprecated code
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-05 16:15:46 +02:00
Samanta Navarro
7109b7c066 login_prompt: Simplify login_prompt API
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>
2023-05-03 07:54:28 -05:00
Samanta Navarro
3010ec11ae login_prompt: Use _exit in signal handler
Calling exit is not signal safe.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-03 07:54:28 -05:00
Samanta Navarro
8fc8de382a login_prompt: Do not parse environment variables
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>
2023-05-03 07:54:28 -05:00
Samanta Navarro
c0fc4d2122 libmisc/yesno.c: Fix regression
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>
2023-04-28 11:22:48 -05:00
Alejandro Colomar
0c4fa6ee0a 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.

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>
2023-04-27 09:16:08 +02:00
Christian Göttsche
7078ed1e0b semanage: disconnect to free libsemanage internals
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

    [...]
2023-04-26 17:52:54 -05:00
Christian Göttsche
a8dd8ce6c9 commonio: free removed database entries
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
2023-04-26 17:52:54 -05:00
ed neville
4e1f674c41 run_parts for groupadd and groupdel
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.
2023-04-26 17:38:24 -05:00
lilinjie
15a64f9e7f fix typos
Signed-off-by: lilinjie <lilinjie@uniontech.com>
2023-04-26 17:35:58 -05:00
Alejandro Colomar
e28deeb8e9 libmisc/yesno.c: Use getline(3) and rpmatch(3)
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>
2023-04-26 17:32:47 -05:00
Samanta Navarro
7b686d8bd6 newgrp/useradd: always set SIGCHLD to default
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>
2023-04-26 08:27:45 -05:00
Serge Hallyn
7ed1df2e80 Update AUTHORS to add Marek Michałkiewicz
Closes #708

Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-04-24 09:01:22 +02:00
Samanta Navarro
0c83b98105 Read whole line in yes_or_no
Do not stop after 79 characters. Read the complete line to avoid
arbitrary limitations.

Proof of Concept:

```
cat > passwd-poc << EOF
root0:0:root:/root:/bin/bash
root0:0:root:/root:/bin/bash
root0: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>
2023-04-21 18:12:56 -05:00
Christian Göttsche
c80788a3ac useradd/usermod: add --selinux-range argument
Add a command line argument to useradd(8) and usermod(8) to specify the
MLS range for a SELinux user mapping.

Improves: #676
2023-04-19 09:19:19 +02:00
Alejandro Colomar
97f79e3b27 CI: Make build logs more readable
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>
2023-04-18 09:21:09 +02:00
Iker Pedrosa
075ed522be ci: remove explicit fedora dependencies
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>
2023-04-13 13:05:32 +02:00
Iker Pedrosa
e047a3acc6 README: add reference to contribution guidelines
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-04-03 10:42:22 -05:00
Iker Pedrosa
981bb8f9d1 doc: add contributions introduction
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-04-03 10:42:22 -05:00
Iker Pedrosa
f6f14a2a83 doc: add license
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-04-03 10:42:22 -05:00
Iker Pedrosa
29df94eb4a doc: add releases
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-04-03 10:42:22 -05:00
Iker Pedrosa
6ceddd9205 doc: add Continuous Integration
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-04-03 10:42:22 -05:00
Iker Pedrosa
77c6cc9857 doc: add tests
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-04-03 10:42:22 -05:00
Iker Pedrosa
d40c58dc3b doc: add coding style
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-04-03 10:42:22 -05:00
Iker Pedrosa
1f15ea5955 doc: add build & install
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-04-03 10:42:22 -05:00
Serge Hallyn
30f3ea4bd3 trivial: vipw.8: fix grammar
Signed-off-by: Serge Hallyn <shallyn@cisco.com>
2023-04-03 13:06:14 +02:00
Christian Göttsche
2eee4c67f5 sssd: skip flushing if executable does not exist
Avoid unnecessary syslog output, like:

    Apr 01 13:35:09 dlaptop userdel[45872]: userdel: sss_cache exited with status 1
    Apr 01 13:35:09 dlaptop userdel[45872]: userdel: Failed to flush the sssd cache.
2023-04-03 13:05:30 +02:00
Christian Göttsche
2eaea70111 Overhaul valid_field()
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.
2023-03-31 09:53:40 -05:00
Martin Kletzander
a5f9ef8b7f semanage: Do not set default SELinux range
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>
2023-03-31 16:13:02 +02:00
Michael Vetter
b44d7f78e0 Fix typo in groupadd usage 2023-03-31 16:04:42 +02:00
Christian Göttsche
189a01f7a7 ci: update Differential ShellCheck
Run on pushes and drop unnecessary write access.

Should avoid pull-requests comments like
https://github.com/shadow-maint/shadow/pull/695#issuecomment-1491876950
2023-03-31 15:54:39 +02:00
tomspiderlabs
e5905c4b84 Added control character check
Added control character check, returning -1 (to "err") if control characters are present.
2023-03-30 19:23:00 -05:00
Mike Gilbert
bd2d0079c9 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: b6b2c756c9
Signed-off-by: Mike Gilbert <floppym@gentoo.org>
2023-03-29 09:05:23 +02:00
Alejandro Colomar
4c210a29bc Fix su(1) silent truncation
*  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>
2023-03-28 13:00:38 +02:00
Alejandro Colomar
1d7d94ed7d Simplify is_my_tty()
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>
2023-03-28 13:00:38 +02:00