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.
lib/: Merge directories "lib/string/ctype/*" into unified files
The APIs defined under each of those subdirs are too similar and related
that it makes more sense to define them in the same files. (BTW, we
only had one API per subdir, except in one subdir that had two APIs, so
in the end, we have almost the same separation.)
Artem Semenov [Mon, 1 Jun 2026 12:57:04 +0000 (15:57 +0300)]
fix: memory leak: free path/rel in shadowtcb_remove()
In function shadowtcb_remove() (lib/tcbfuncs.c), memory allocated
for 'path' and 'rel' pointers was not released, causing a memory leak.
Added explicit free(path) and free(rel) calls before returning.
aborah-sudo [Wed, 3 Jun 2026 05:39:03 +0000 (11:09 +0530)]
Tests: Add two users with same UID using -o flag
This is the transformation to Python of the test located in
`tests/usertools/01/04_useradd_add_user_with_existing_UID_with_-o.test`
which checks that `useradd` add two users with same UID using -o flag
On Linux, userdel/usermod check all /proc/<pid> status files to ensure a
to-be-modified user has no more running tasks, or abort modification
otherwise.
However, the check failed to detect threads running as the user if the
corresponding main thread ran as a different user. The user is deleted
despite still being busy. This is due to passing a wrong value to
check_status. The caller passed "<pid>/task", rather than
"<pid>/task/<tid>". In consequence check_status tried to open
"/proc/<pid>/task/status" - a wrong path that never exists - open fails,
and check_status always returns 0. The correct status file name would
have been "/proc/<pid>/task/<tid>/status" instead.
The bug can only be reproduced by rather exotic code using raw syscalls.
POSIX does not allow threads to have different UIDs.
To fix it, construct the correct path to the tid status file in the
caller, before passing it to check_status.
The old name was too complex, and is inconsistent with all other
sprintf(3)-based APIs having just one letter for differentiation.
This allows breaking less lines.
The original name was chosen for differentiation with the buggy Plan9
API seprint(2). However, 9front (the current fork where Plan9 is mainly
developed) has acknowledged the bug. There's still no decision on
fixing the bug or not, due to the age of their code base, and the
projects depending on their library. It is under consideration
inventing something like a seprint2(2) in 9front for replacement of
seprint(2), but there's no decision yet either.
Considering that 9front acknowledges their bug, and that they *may*
release a fixed API with a similar name, we may as well claim that our
seprintf() is also a fixed version of Plan9's seprint(2). It has a
different name, after all (we terminate in 'f').
This commit was partially scripted with
$ find * -type f \
| xargs grep -l stpeprintf \
| xargs sed -i 's/stpeprintf/seprintf/g';
Instead of having three possible returns (a pointer to the NUL byte, the
end of the array, or NULL), reduce it to two possible ones: one for
success, and one for error.
Use errno, which is a common way to signal the specific error, and thus
treat truncation as any other error. This simplifies error handling
after these calls. Also, if one misuses a pointer after truncation, the
results are better if the pointer is NULL: the program will easily
abort. If we returned 'end', the program could more easily produce a
buffer overrun.
Suggested-by: Douglas McIlroy <douglas.mcilroy@dartmouth.edu> Signed-off-by: Alejandro Colomar <alx@kernel.org>
projects / thirdparty / shadow.git / log
