Commit Graph

3049 Commits

Author SHA1 Message Date
pepe 1f0ac29e17 :( 2023-05-28 03:50:23 +00:00
Samanta Navarro dcc90658fd xgetXXbyYY: Avoid duplicated error handling block
The error handling is performed after the loop. By just calling break it
is possible to reuse the error handling if status is not ERANGE.

Signed-off-by: Samanta Navarro <>
2023-05-26 16:03:27 -05:00
Samanta Navarro e73a2194b3 xgetXXbyYY: Handle DUP_FUNCTION failure
A failure of DUP_FUNCTION is already handled for non-reentrant
function wrapper. Perform the check for reentrant version as well.

Signed-off-by: Samanta Navarro <>
2023-05-26 16:03:27 -05:00
Serge Hallyn a80b792afc sub_[ug]id_{add,remove}: fix return values
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 <>
2023-05-26 15:16:29 -05:00
Martin Kletzander 8665fe1957 usermod: Small optimization using memmove for password unlock
Signed-off-by: Martin Kletzander <>
2023-05-26 15:14:02 -05:00
Alejandro Colomar e3b7058110 Reorder logic to improve comprehensibility
-  Don't else after return or fail_exit().
-  Prefer == over != (negated logic is more complex to think about it).
-  Reduce nesting when reasonable.

Signed-off-by: Alejandro Colomar <>
2023-05-25 21:22:08 -05:00
Alejandro Colomar 5b117d5526 newusers: Fail early
There's no reason to report all errors.  Bail out at the first one,
which is simpler.

Suggested-by: Serge Hallyn <>
Signed-off-by: Alejandro Colomar <>
2023-05-25 21:22:08 -05:00
Alejandro Colomar 1957c8c881 newusers: Add missing error handling
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').

Cc: Serge Hallyn <>
Signed-off-by: Alejandro Colomar <>
2023-05-25 21:22:08 -05:00
Samanta Navarro 6491fef1e0 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.

This is a purely defensive measure.

Signed-off-by: Samanta Navarro <>
2023-05-25 08:25:42 -05:00
Samanta Navarro a116e20c76 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.

Co-developed-by: Serge Hallyn <>
Signed-off-by: Samanta Navarro <>
2023-05-25 08:25:42 -05:00
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

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

Signed-off-by: Tobias Stoeckmann <>
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 <>
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 <>
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 <>
Signed-off-by: Iker Pedrosa <>
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 <>
2023-05-15 09:21:16 +02:00
Markus Hiereth a022d39d2a replace inadequate German translation of login error message
Signed-off-by: Serge Hallyn <>
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

Signed-off-by: Serge Hallyn <>
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 <>
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 <>
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

to enable the tags which are in use by docbook.

Signed-off-by: Serge Hallyn <>
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

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 <>
2023-05-05 16:15:46 +02:00
Samanta Navarro e899e3d745 commonio_lock_nowait: Remove deprecated code
Signed-off-by: Samanta Navarro <>
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 <>
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 <>
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 <>
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 <>
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

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: <>
Link: <>
Cc: Mike Frysinger <>
Signed-off-by: Alejandro Colomar <>
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 <>
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 <>
Cc: Serge Hallyn <>
Signed-off-by: Alejandro Colomar <>
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 <>
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 <>
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
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 <>
Reviewed-by: Alejandro Colomar <>
Reviewed-by: Serge Hallyn <>
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 <>
Signed-off-by: Alejandro Colomar <>
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 <>
2023-04-13 13:05:32 +02:00
Iker Pedrosa e047a3acc6 README: add reference to contribution guidelines
Signed-off-by: Iker Pedrosa <>
2023-04-03 10:42:22 -05:00
Iker Pedrosa 981bb8f9d1 doc: add contributions introduction
Signed-off-by: Iker Pedrosa <>
2023-04-03 10:42:22 -05:00
Iker Pedrosa f6f14a2a83 doc: add license
Signed-off-by: Iker Pedrosa <>
2023-04-03 10:42:22 -05:00
Iker Pedrosa 29df94eb4a doc: add releases
Signed-off-by: Iker Pedrosa <>
2023-04-03 10:42:22 -05:00
Iker Pedrosa 6ceddd9205 doc: add Continuous Integration
Signed-off-by: Iker Pedrosa <>
2023-04-03 10:42:22 -05:00
Iker Pedrosa 77c6cc9857 doc: add tests
Signed-off-by: Iker Pedrosa <>
2023-04-03 10:42:22 -05:00
Iker Pedrosa d40c58dc3b doc: add coding style
Signed-off-by: Iker Pedrosa <>
2023-04-03 10:42:22 -05:00
Iker Pedrosa 1f15ea5955 doc: add build & install
Signed-off-by: Iker Pedrosa <>
2023-04-03 10:42:22 -05:00