useradd: don't mask user_id variable in check_uid_range
While it's not "wrong" right now, it could be confusing, and cause
trouble later.
I'm not a fan of global variables in the first place, but restructuring
venerable code like this tends to be asking for trouble.
Also rename rflg as sys_user. And fix its type, as it is a bool, not
int.
Also a note on commit 764e56e9ec: useradd: fix subuid allocation,
which is in a PR comment. Putting it here so it'll be in the git
tree itself:
> For posterity: this was apparently always broken, but the opposite way. The
> user_id part of the check was always true if !user_id, and user_id was always
> 0, so that was ignored. Alex fixed that accidentally by switching to if
> (user_id == 0) return false, in 8508d61.
If someone has a subuid range with count==0, the variable last is
calculated as first + count - 1, meaning last == first - 1. Since
the calculated range wraps around, this can invalidate later checks.
Check for this case explicitly.
Because of other checks, this can only lead to erroneous ranges
being created if the first loweruid is 0. This means that the
subid range has to be user:0:0, which is doubly wrong, and should
never be seen in the real world.
A note on formatting: the calculations of first and last are done in
their declarations. If range->count == 0, these go unused. The
alternatives would be to declare the variables later in the scope -
which we do not do - or to assign them later, adding 3 more lines in
each scope.
Since this case (again) should never happen, it's not worth avoiding
the extra assignments in this error case.
Signed-off-by: Serge Hallyn <serge@hallyn.com> Reported-by: Mohammad Hossein Abedini <mhmd.abedinii@gmail.com>
man/login.defs.5.xml: Remove references to PASS_MAX_LEN
We removed the configuration variable. The documentation of
PASS_MIN_LEN was moved to a new file of its own name, and removed the
file that documented PASS_MAX_LEN.
Historically, if a user is a member of a group with no name, id(1)
would show that as 424243, not 424243(424243). But id(1) from the rust
based uutils prints that group number. Rather than try to guess at
the parsing, just add the group name in the testcases's group file,
and expect 424243(testsuite).
tests/libsubid: fix the 03_add_remove tests so they pass
Fix src/new_subid_range.c, which is only used by this test case. It
was not honoring the SUB_UID_MIN in login.defs. Then fix the testcase's
expected end results.
Remove the original shell based test
`tests/usertools/62_usermod_remove_supplementary_groups/` and its
references from test runner scripts. The test was failing intermittently
and has been successfully transformed to Python framework with improved
reliability.
tests/system/tests/test_usermod.py: remove user from supplementary group they don't belong to
Transform second test case from
`tests/usertools/62_usermod_remove_supplementary_groups/usermod.test`
to Python framework. Tests `usermod -rG` graceful handling when
attempting to remove a user from a supplementary group they're not
actually a member of.
tests/system/tests/test_usermod.py: remove user from existing supplementary group
Transform first test case from
`tests/usertools/62_usermod_remove_supplementary_groups/usermod.test`
to Python framework. Tests `usermod -rG` functionality to remove a user
from an existing supplementary group while preserving membership in
other groups.
In the last commit, PASS_MAX_LEN became unused. As a consequence,
obscure_get_range()'s $2 is unconditionally set to -1. Remove all code
that depends on this value.
The decision on whether to allocate a subuid is dependent
on user_id != 0, but user_id is not filled in until after
the call to this. I spent a few minutes trying to track
down where this ordering changed, and haven't found it yet.
We can add a fixes tag if we track it down.
Move the decision on whether to calculate it to after we
calculate user_id. Then, because that decision is used to
decide whether to open the subuid_db, and pull the subuid
db opening out of open_files and run it later.
Having such long and complex format strings and variadic arguments is
error-prone, as can be seen in the previous commit, which fixes a bug of
this kind.
like the fprintf(3) wrappers, these print an error message. SYSLOGE()
uses errno, and SYSLOGEC() uses an errno-like error code.
This time we need to be careful to name the local copy of errno
differently than within SYSLOG() --which we call--. Let's use a double
underscore. In the future, C might have function literals (similar to
lambdas), which will solve this issue.
fprinte() is like fprintec(), but uses errno instead of an error code.
It is implemented as a macro so that it can read the value in errno
before evaluating any of its arguments, thus not corrupting it.
It also preserves errno so that the error can be printed more than once.
This is useful because we often print to stderr or log_get_logfd(), and
immediately after print to the system log with syslog(3).
These functions print a formatted string, followed by a colon and
a space, followed by an error string produced by strerror($2). That is,
the output is as follows:
fmt: error message string
For example,
fprintec(stderr, ENOMEM, "foo(%d)", 42);
prints
foo(42): Cannot allocate memory
These functions return the number of characters printed, or a negative
value on error, like fprintf(3).
In some cases, we print strerrno() with this macro. Because this is
a macro, and internal calls such as strdup(3) may set errno, that value
could be corrupted when we arrive at syslog(3). While we could solve
this here, it's not robust. Instead, we'll use a dedicated wrapper for
that, which will be added in the following commits: SYSLOGE().
What we'll do here is preserve errno when we exit from this macro, as we
often follow SYSLOG() calls with fprintf(stderr,) calls, which also use
the errno value, and we don't want to pollute that.
src/newusers.c: Don't allow bad names in group names
Other programs handling groups don't allow this.
I believe this was only accidentally allowed.
Fixes: a2cd3e9ef03a (2019-10-04; "chkname.c, pwck.c, useradd.c, usermod.c, newusers.c: Allow names that do not conform to standards") Cc: ed <ed@s5h.net> Signed-off-by: Alejandro Colomar <alx@kernel.org>
These programs don't add new users. Because they handle existing users,
they should handle them with whatever names they already have, including
bad names.
A recent commit that added some username syntax validation blindly
removed support for bad names in these programs. Fix that regression.
It's just the obvious thin wrapper around strncpy(3); let's trust it's
ok.
The reason for removing this test is that GCC has bogus diagnostics
for strncpy(3). Its diagnostics are geared towards helping people
that abuse strncpy(3) as a poor-man's strlcpy(3) not write exploitable
code as easily. Using strncpy(3) for that purpose is brain damaged, and
those programs should be audited to stop using this API for that. And
most importantly, GCC should stop encouraging writing bad code that
calls strncpy(3) as that results in diagnostics that are actively
harmful for us, legitimate users of strncpy(3). Those false positives
should certainly be out of -Wall.
We could fill the tests with pragmas, but let's just remove the tests.
I don't feel like maintaining code for ignoring GCC's brain bamage.
If people want to write a function for truncating strings (because they
can't rely on strlcpy(3) being available, or because they don't like
it), they certainly should write such an API. strncpy(3) isn't that
API. strncpy(3) is a function that takes a string and writes it into a
utmpx(5) member, which is NOT a string. (And I heard it might also be
useful for implementing tar(1), which also uses non-terminated character
arrays, but I never looked at that code.)
Whenever we were reading it, let's assume it contains a -1 (the integer
representation of an empty field). Whenever we were writing it, let's
write a -1.
I think this would be worth a CVE. I've contacted Debian, in case they
can issue a CVE. Here's the draft I've sent them for the CVE:
shadow-utils: inability to change a compromised password
Description
A flaw was found in /etc/shadow, and the programs that handle
the file.
The /etc/shadow database (documented in shadow(5)) entries
contain a "minimum password age" field. This field imposes
a minimum password age before the password can be changed.
If a user's password is compromised before that period expires,
the user must contact the administrator to be able to change the
password. This can usually take hours or days, and during these
hours or days, an attacker would have unlimited access to the
credentials.
One option of the user would be to lock its own password with
'passwd -l', thus being locked out of its own account.
The history of this 4th field of /etc/shadow entries is
considered dubious in retrospective, and the recommended action
is to never set this field. If the field is set, the
recommended action is to ignore the field, and if possible,
remove it.
It makes no sense to limit the frequency of password change. If one
changes its password, and 5 minutes later the password is leaked, one
should be able to change the password immediately.
Hadi Chokr [Thu, 9 Jul 2026 13:28:41 +0000 (13:28 +0000)]
subids: find_free_range(): skip empty ranges
Empty (count == 0) ranges constrain nothing, so skip them instead of
processing them. Besides documenting that invariant, this keeps the
range-end computation, first - 1LL + range->count, away from the
count == 0 corner, where for first == 0 it would produce -1 through
unsigned long wrap-around and an implementation-defined conversion on
64-bit systems.
Such entries exist in the wild, since old useradd versions used to
create them, so they are still accepted by the parser; they are only
ignored during allocation.
Co-authored-by: Dan Anderson <https://github.com/MillaFleurs> Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Hadi Chokr <hadichokr@icloud.com>
Hadi Chokr [Thu, 9 Jul 2026 07:30:36 +0000 (07:30 +0000)]
subids: reject subid file entries that do not fit in id_t
range->start and range->count are parsed from /etc/subuid and
/etc/subgid as unsigned long, so a corrupt or malicious entry was not
bounded by id_t: find_free_range() assigned such a start to intmax_t,
which is implementation-defined above INTMAX_MAX, and the range-end
computation could exceed INTMAX_MAX.
Validate the values where they are parsed, instead of hardening each
consumer: bound start to maxof(id_t) in subordinate_parse(), and
bound count so that the last ID of the range, start + count - 1, fits
in id_t as well; the MIN() caps that bound at maxof(id_t), which
also keeps it representable in unsigned long on 32-bit systems.
a2ul() reports ERANGE for out-of-range values.
Rejected entries are skipped when iterating the database
(commonio_next() skips entries whose parse failed), while the raw line
is still preserved if the file is rewritten. This protects all
consumers of these fields, not just allocation.
After this, every visible range satisfies
0 <= start <= start + count - 1 <= (id_t)-1, so the intmax_t
arithmetic in find_free_range() is exact and cannot overflow, and
oversized values no longer need to be handled there.
This is a separate, pre-existing problem from the regression fixed in
the previous commit: before de7f1c78f70f8df4b2956f7363da3774348bc58e
this arithmetic was done in unsigned long and silently wrapped for
such values.
Co-authored-by: Dan Anderson <https://github.com/MillaFleurs> Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Hadi Chokr <hadichokr@icloud.com>
Hadi Chokr [Mon, 6 Jul 2026 12:29:13 +0000 (14:29 +0200)]
subids: fix security regression which can cause overlapping ranges
Fix bug from de7f1c78f70f8df4b2956f7363da3774348bc58e that changed from
using unsigned long to id_t. new_subid_range() returns the same value
as the ceiling allowing potentially overlapping subordinate ID ranges.
In a nutshell, the commit updated lib/subordinateio.c which now allows
creation of overlapping subordinate ID ranges. This is caused because
the commit changed from unsigned long to id_t. The bug is that the
internal algorithm still used an exclusive endpoint expression, max + 1,
which was only safe while the intermediate type was wide enough. So
find_free_range() accepts max as an inclusive ID ceiling, but internally
compares holes against max + 1.
When max is (id_t)-1 on an unsigned-ID build, max + 1 wraps to zero.
new_subid_range() passes exactly that value as the ceiling.
Fix it by doing the internal arithmetic in signed types that are
strictly wider than id_t, so that max + 1, last + 1, and the hole
computations cannot wrap:
- The static_assert checks that long long is wider than id_t. That
makes max + 1LL exact, and since intmax_t is at least as wide as
long long, it also guarantees that the intmax_t variables are wider
than id_t.
- count is converted up front into the intmax_t variable n, which the
input check and the later size comparisons use directly, without
casts. The input check is done in signed arithmetic, as
n > max - min + 1LL: max >= min holds at that point, so max - min is
in the range 0 .. (id_t)-1, and adding 1LL to that fits in long long.
- The end of each range is computed as first - 1LL + range->count,
which likewise needs no cast.
- The final check on the remaining unclaimed area is written as
max - low >= n - 1 rather than (max - low) + 1 >= n: low <= max and
n >= 1 hold there, so neither side of the comparison can overflow.
This was found with N184, an open source bug and security vulnerability
scanner: https://github.com/MillaFleurs/N184
Closes: <https://github.com/shadow-maint/shadow/issues/1629> Co-authored-by: Dan Anderson <https://github.com/MillaFleurs> Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Hadi Chokr <hadichokr@icloud.com>
Serge Hallyn [Tue, 9 Jun 2026 01:08:04 +0000 (20:08 -0500)]
Avoid falling prey to tiocsti
We've long known (see
https://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/
and https://jdebp.uk/FGA/dont-abuse-su-for-dropping-privileges.html)
that su should be used to gain but not drop privilege. Much more
recently, linux added the ability to prevent TIOCSTI through a
configurable /proc/sys/dev/tty/legacy_tiocsti setting.
If /proc/sys/dev/tty/legacy_tiocsti is set to 0, then we are protected
from the callee injecting commands on caller's tty through TIOCSTI.
If it's 1, or doesn't exist, then we are not. That can be dangerous
if caller is root. We currently give up the controlling terminal
for non-interactive uses of su (-c). Let's do that for interactive
calls as well, only in the dangerous case.
The Czech Makefile.am was accidentally truncated during the groupmems
removal. This caused the Czech man pages to be excluded from the release
tarballs.
Restoring the Makefile.am and regenerating the release fixes the
problem.
Tests: Force creation of existing group using -f flag
This is Python transformation of the test located in
'tests/grouptools/groupadd/06_groupadd_-f_add_existing_group/groupadd.test'
which checks that 'groupadd' completes successfully while force-creating
an existing group using -f flag
Akshay Sakure [Tue, 30 Jun 2026 14:09:56 +0000 (19:39 +0530)]
Tests: Add group with password using -p flag
This is Python transformation of the test located in
'tests/grouptools/groupadd/04_groupadd_set_password/groupadd.test'
which checks that 'groupadd' can create group with password using
-p flag
Akshay Sakure [Tue, 23 Jun 2026 13:45:43 +0000 (19:15 +0530)]
Tests: Add group with specified GID using -g flag
This is Python transformation of the test located in
`tests/grouptools/groupadd/05_groupadd_set_GID/groupadd.test`
which checks that `groupadd` can create group with sepcified GID
using -g flag
aborah-sudo [Tue, 30 Jun 2026 07:43:49 +0000 (13:13 +0530)]
Tests: Change multiple user attributes at once
This is the transformation to Python of the test located in
`tests/usertools/01/09_usermod_change_user_info.test`
which checks that `usermod` can change multiple user attributes at once
Iker Pedrosa [Tue, 2 Jun 2026 13:18:31 +0000 (15:18 +0200)]
passwd: add UPN validation support
Add User Principal Name (UPN) validation to allow passwd command to
accept usernames in user@domain.com format. Currently, passwd will
accept both traditional usernames and UPN format.
Iker Pedrosa [Tue, 2 Jun 2026 11:15:42 +0000 (13:15 +0200)]
lib/chkname.*: Add UPN validation support
Add is_valid_upn() function to validate User Principal Name format. UPN
validation splits on @ and validates the prefix using existing username
rules and suffix part using RFC 1035/1123 compliant domain name
validation.
doc/contributions/: Add guidelines severely restricting use of AI for contributing
This policy has been derived from Gentoo. I added a requirement that
use of AI is disclosed. And changes resulting from said use should be
disclosed in detail. Also, I left a note saying we'll reject
non-negligible use of AI, which is a bit of an escape allowing us to
just say "too much".
lib/string/strspn/: strr[c]spn(): Return the length counting from the end
Instead of returning the offset from the start of the string, return the
length of the span counting from the end.
This makes the name of the function more representative of its behavior,
and also makes the functions more useful: one can use them as booleans
to determine whether a string ends in a set of characters or not.
if (strrspn(s, "xyz")) // Does 's' end in any chars of "xyz"?
Currently, the only uses of strr[c]spn() are for implementing
stpr[c]spn(), so we had to update those too.
Serge Hallyn [Wed, 17 Jun 2026 18:55:07 +0000 (13:55 -0500)]
man/po: keep tmpdir out of po files
When we pass every filename to the itstool call as $tmpdir/$base.out,
the file:line listed in the .po includes the $tmpdir. That means
every update-po changes every .po.
Serge Hallyn [Tue, 16 Jun 2026 17:35:03 +0000 (12:35 -0500)]
Make man/po update-po more robust
There were some inconsistencies in how remove-potcdate was named.
It exists as po/remove-potcdate.sin. The man/po tried to copy that
in, but it referred to it as remove-potcdate.sed.
Just copy it in as remove-potcdate.sed. I think we can do better
than this, but for now this should make update-po more robust.
This error message didn't make much sense. After inspecting the commit
in which it was introduced, it seems the intention was to diagnose if
the line was empty after ignoring white space. It was incorrectly
written then, so fix it now.
Rewrite it in the following way:
- If there's not a '\n', the entire line is bogus. Fail, and report an
appropriate diagnostic.
- Then, break the string at the first white space, as we were doing
before. No error handling is appropriate here.
- Then, diagnose if the remaining string is empty.
Having trailing white space in a line doesn't remove the need for
a trailing '\n'. Let's fail if a line doesn't have it, regardless of
how much trailing white space there is.
It is not intuitive or clear what the right behavior should be for an
empty string. If we define these APIs as "return true if all characters
in the string belong to the specified character set", then an empty
string should return true. On the other hand, if you ask me if an empty
string is a numeric string, I might naively say no.
It is irrelevant whether we return true or false for an empty string.
All of the callers already handle correctly the case of an empty string.
This makes the implementation simpler, using the argument only once.
This allows implementing these as macros.
projects / thirdparty / shadow.git / log
