lib/chkname.c, src/: Strictly disallow really bad names
Some names are bad, and some names are really bad. '--badname' should
only allow the mildly bad ones, which we can handle. Some names are too
bad, and it's not possible to deal with them. Reject them
unconditionally.
- A leading '-' is too dangerous. It breaks things like execve(2), and
almost every command.
- Spaces are used for delimiting lists of users and groups.
- '"' is special in many languages, including the shell. Having it in
user names would be unnecessarily dangerous.
- '#' is used for delimiting comments in several of our config files.
Having it in usernames could result in incorrect configuration files.
- "'" is special in many languages, including the shell. Having it in
user names would be unnecessarily dangerous.
- ',' is used for delimiting lists of users and groups.
- '/' is used for delimiting files, and thus could result in incorrect
handling of users and groups.
- ':' is the main delimiter in /etc/shadow and /etc/passwd.
- ';' is special in many languages, including the shell. Having it in
user names would be unnecessarily dangerous.
There are other characters that we should disallow, but they need more
research to make sure we don't introduce regressions. This set should
be less problematic.
Acked-by: Tobias Stoeckmann <tobias@stoeckmann.org> Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Chris Hofstaedtler <zeha@debian.org> Cc: Marc 'Zugschlus' Haber <mh+githubvisible@zugschlus.de> Cc: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
Duplicating name and hash is not needed here, because duplication
occurs in spw_update. You can detect the small memory leak with
tools like valgrind.
More importantly though, if xstrdup fails, it calls exit. The
update_age function is in the "criticial section" between
open_files and close_files, though. Correct error handling would
require fail_exit to release the held locks.
Iker Pedrosa [Fri, 19 Dec 2025 15:27:54 +0000 (16:27 +0100)]
src/gpasswd.c: fix segfault in clean up callbacks
The gpasswd utility was segfaulting when cleanup functions were called
because these functions expect a pointer to `process_selinux` but was
being passed NULL. This caused a NULL pointer dereference.
This commits adds the pointer to `process_selinux` to clean up
functions making `gpasswd` consistent with other group utilities.
Reproduction steps:
$ useradd tuser
$ groupadd tuser
$ gpasswd -a tuser tgroup
Adding user tuser to group tgroup
Segmentation fault (core dumped)
Unify the retrieval of PASS_MIN_LEN and PASS_MAX_LEN for output
in passwd and actual checks.
Fixes wrong output for minimum password lengths if no such
restriction is configured: 5 is printed, 0 is in effect.
How to reproduce:
1. Use passwd compiled without PAM support
2. Do not specify PASS_MIN_LEN in login.defs
3. Run passwd as a user and enter your old password, then
- you will see that 5 characters are expected
- you can just press enter twice
The getdef_num implementation allows -1 to be specified in login.defs.
In general, -1 should be treated the same way as "not specified". In
this case, casting -1 to size_t leads to every password being "too
short."
The check_fds function is supposed to ensure that fds 0, 1, and 2 are
opened in a well-defined state, i.e. either they are already connected
to supposed input/output files or will be connected to /dev/null if not.
Opening the audit socket before checking the fds allows the audit socket
to get one of these numbers.
Avoid this by opening the audit socket after the check.
In general, this check is already covered by system libraries, but this
proof of concept works for root user. Note the different states of the
file descriptor 2.
In bash or another shell that interprets `2>&-` as closing stderr with
shadow + audit support, e.g. Arch Linux:
```
sg bin 'ls -l /proc/self/fd'
sg bin 'ls -l /proc/self/fd' 2>/dev/null
sg bin 'ls -l /proc/self/fd' 2>&-
```
The `PASS_MAX_LEN` is effectively only used for DES. Do not describe it
in a way that makes it sound like `MD_CRYPT_ENAB=yes` is required to
disable it. Any other `ENCRYPT_METHOD` disables it as well.
Also, even for DES, `PASS_MAX_LEN` requires `OBSCURE_CHECKS_ENAB` to
have any effect.
Even more, `PASS_MIN_LEN` and `PASS_MAX_LEN` are only used for
user passwords. Group passwords are not checked.
Note: All of this is actually true even if compiled with PAM if command
line arguments change root. But if compiled with PAM support, this
section is not added to manual pages... Since this is true for some
more files, it's not part of this commit.
Link to source files:
- lib/obscure.c line 133 stops further checks, including max length,
if OBSCURE_CHECS_ENAB is not yes
- lib/obscure.c line 172 is only reached in case of DES
- src/passwd.c line 248 duplicates the check for output
- src/gpasswd.c has no reference to obscure
- The total number of password change tries can be configured
- Except min length, password strength checks can be disabled
- Even the root user can have password strength checks...
- ... except in some cases (stdin, command line arguments)
In general, this code does not run for PAM, except root directory
is modified through command line arguments by root user.
The passwd tool checks if the password of a user may be changed before
locking the passwd/shadow files. This leaves a time window to perform
the same action twice (e.g. circumventing PASS_MIN_DAYS limit) or to
circumvent a locked password by an administrator.
Perform the check after the lock again. This keeps the behavior as it
is today for a user and also prevents the race condition.
Always use the name in shadow entry for logging. This reduces the
amount of data retrieved from password entry to bare minimum, i.e.
passing through into library call.
Make sure that passwd and shadow are always opened in the correct
order to avoid possible dead locks with other tools:
- Lock passwd first, then shadow
- Unlock shadow first, then passwd
The passwd utility may work without a shadow entry. In that case, it
operates on the passwd file. But to figure this out, the shadow file
must have been opened and thus locked already. Unconditionally open the
passwd file first, even though it's not needed most of the time.
At this point, shadow might be already locked if update_noshadow is
called as fallback within update_shadow. Make sure that unlock is
called before exit.
newusers: Allow creation without aging information
If PASS_MAX_DAYS is not set, newusers falls back to 10000 days, which is
considered "unlimited" in some parts of the source tree. All other tools
fall back to -1, which truely implies unlimited.
Sync newusers with all other shadow tools.
How to reproduce:
1. Remove or comment out PASS_MAX_DAYS from /etc/login.defs
2. Run `newusers <<< user:pass:1234:1234::/home/user:/bin/bash`
3. Check user line in /etc/shadow
```
/etc/shadow:user:HASH:19721:0:10000:7:::
```
Max days are set to 10000. Instead, this should be:
The pwd_to_spwd routine claims that fields without corresponding
information in the password file are set to uninitialized values,
but sets some aging information. These cannot be available in
struct passwd.
Also, the code is only used in passwd to temporarily hold the
new password. All other values are copied from an existing entry
later on. If no entry exists, all values are dismissed anyway.
Clarify that everything is uninitialized except name and password.
Gets rid of magic value 10000 for sp_max.
Mike Gilbert [Sat, 13 Dec 2025 20:07:02 +0000 (15:07 -0500)]
lib/xgetXXbyYY.c: include stdint.h for SIZE_MAX
Fixes build failure:
```
In file included from xgetgrnam.c:40:
xgetXXbyYY.c: In function ‘xgetgrnam’:
xgetXXbyYY.c:83:31: error: ‘SIZE_MAX’ undeclared (first use in this function)
83 | if (length == SIZE_MAX) {
| ^~~~~~~~
```
Compound literals also have the ability of converting values, but they
don't have the unwanted effects on safety --casts disable most useful
diagnostics--.
Compound literals are lvalues, which means their address can be taken,
and they can also be assigned to. To avoid this, we force lvalue
conversion through a statement expression.
Iker Pedrosa [Tue, 19 Aug 2025 09:26:18 +0000 (11:26 +0200)]
tests/system/tests/test_groupmems.py: add user to group as root user
This is the transformation to Python of the test located in
`tests/grouptools/groupmems/01_groupmems_root_add_user/groupmems.test`,
which checks that `groupmems` is able to add a user to a group running
as the root user.
Iker Pedrosa [Fri, 8 Aug 2025 15:08:20 +0000 (17:08 +0200)]
tests/system/tests/test_newusers.py: create multiple users using file input
This is the transformation to Python of the test located in
`tests/newusers/20_multiple_users/newusers.test`, which checks that
`newusers` is able to create new users from file.
Iker Pedrosa [Fri, 8 Aug 2025 15:05:42 +0000 (17:05 +0200)]
tests/system/tests/test_newusers.py: create multiple users from stdin
This is the transformation to Python of the test located in
`tests/newusers/35_read_from_stdin/newusers.test`, which checks that
`newusers` is able to create new users from stdin.
[Serge: preference to keep the names as a comment, but ok] Reviewed-by: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
lib/, src/ tests/: Unname unused parameters in callbacks
[Serge: preference to keep the names as a comment, but ok] Reviewed-by: Serge Hallyn <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
Antonio Terceiro [Sun, 24 Aug 2025 13:59:07 +0000 (10:59 -0300)]
newusers: allow not passing a password
A possible use case for this is wanting to add subuid/subgid entries for
an existing user. This change makes it possible to pass `username::::::`
to newusers; the empty password will be ignored an everything else will
be done. Currently this fails miserably, as PAM errors out on a empty
password.
Signed-off-by: Antonio Terceiro <terceiro@debian.org>
This macro is like typeof(), but requires that the input is a type name.
This macro is useful because it parenthesizes the type name, which
allows one to create pointers to arbitrary types. The following code
works for simple types:
T x;
T *p;
For example, that would work fine for 'int':
int x;
int *p;
However, that wouldn't work for types such as 'int[3]':
int[3] x;
int[3] *p;
But if we do instead
typeas(T) x;
typeas(T) *p;
now we can use 'int[3]' just fine:
typeof((int[3]){}) x;
typeof((int[3]){}) *p;
To understand this, we create a compound literal of type int[3], with
default values (zero, zero, zero), then get it's type, which is
obviously int[3]. And then we use that to declare a variable x of that
type, and a pointer p, which has type 'int(*)[3]'.
This would also work with something simpler. One could use typeof(T)
directly:
typeof(T) x;
typeof(T) *p;
However, typeof() doesn't require that the input is a type; it accepts
arbitrary input. The following is valid C:
typeof(42) x;
For our macro MALLOC(), where we want the second argument to be a type
name, we want to require that the argument is a type name. For that, we
need to use typeas().
src/: Fix uninitialized flags, and use const appropriately
In all these functions, we were setting the flags to 'true' in
process_flags() if the appropriate command-line flag was specified.
However, they were never being defined to 'false', which should be the
default value. Initialize these flags to false.
Initialize option_flags structure to prevent garbage memory values in
`flags.chroot` and `flags.prefix` fields. Uninitialized memory caused
architecture-specific failures where process_selinux evaluation
differed between x86_64 and aarch64, leading to `pw_close()` failures
when SELinux contexts weren't properly managed.
src/usermod.c: $user_newhome: Remove all trailing '/'s
FTR: I'm not entirely sure if an empty string can arrive here. It might
be that the streq() check is dead code, but I'm not sure, so I put it.
It also makes the code more robust.
configure.ac, Makefile.am: Propagate ./configure flags to 'distcheck'
'make distcheck' runs ./configure (among other things). That command
should inherit the flags passed on the command line, as they are the
flags necessary to build in the current system.
This also allows testing different configurations in the same system.
Until now, we only tested the default automagic configuration. With
this change, one can test the same default automagic configuration, by
not passing any flags to ./configure, or they can test more specific
configurations, by passing flags.
This allows removing the hardcoded AM_DISTCHECK_CONFIGURE_FLAGS in the
"Makefile.am".
memcpy(3) is overkill, and much more dangerous than simple assignment.
Simple assignment adds type safety, and removes any possibility of
buffer overflow due to accidentally specifying a wrong size.
tests/unit/test_xaprintf.c: Use assert_string_equal()
This produces more useful test results.
With assert_true(streq(...)), we only see the line of code that
triggered the failure, while assert_string_equal() shows the contents
of the strings. See the following example:
The differences between the actual patch and the approximation via the
semantic patch from above are includes, whitespace, braces, and a case
where there was an implicit pointer-to-bool comparison which I made
explicit. When reviewing, it'll be useful to use git-diff(1) with '-w'
and '--color-words=.'.
The differences between the actual patch and the approximation via the
semantic patch from above are whitespace only. When reviewing, it'll
be useful to diff with '-w'.
Iker Pedrosa [Tue, 26 Aug 2025 08:48:15 +0000 (10:48 +0200)]
doc/contributions/tests.md: add Python system tests
Document the new Python system tests:
- Benefits
- Contribution guidance
- How to setup the testing environment
- Test configuration and execution
- Advanced testing features
- Development patterns
- Debugging information
- Troubleshooting & FAQs
This name better reflects that it handles arrays, and doesn't shout.
This case is slightly different, as this macro does a little bit more
than just enforcing arrays. It changes the return value too. However,
that is related-enough to the handling of arrays that I'm inclined to
accept it as a minor inconsistency.
projects / thirdparty / shadow.git / log
