getgroups(0, NULL) returns the number of groups, so that we can allocate
at once. This might fail if there's a race and the number of users
grows while we're allocating, but if that happens, failing is probably a
good thing to do.
There was some comment saying it doesn't work on some systems, but
according to gnulib, that's only NeXTstep 3.2, which we don't support.
This originates from a typo in this project, which was later copied by
glibc, and so the typo was set in stone. The typo was eventually fixed
in shadow, but glibc had already set the name in stone, so we should
just learn to live with it.
$ grep -rn -C3 sg_name ChangeLog
1607-
1608-2011-07-30 Nicolas François <nicolas.francois@centraliens.net>
1609-
1610: * src/chgpasswd.c: Fix typo sp -> sg. sg_namp -> sg_name
1611- * src/chgpasswd.c: Always update the group file when SHADOWGRP is
1612- not enabled.
1613-
This is a scripted change:
$ find lib* src -type f \
| xargs sed -i 's/\<sg_name\>/sg_namp/g';
If we are not interested in the amount of errors but only if errors
exist, use a flag instead of a counter. This eliminates the chance of
signed integer overflows and better reflects the meaning of variable.
Keeping variable name and basically copied from src/faillog.c.
If crypt fails, pw_encrypt calls exit. This has the consequence that the
plaintext password is not cleared.
A valid password can fail if the underlying library does not support it.
One such example is SHA512, for which the password must not be longer
than 256 characters on musl. A password longer than this with glibc
works, so it is actually possible that a user, running passwd, tries to
enter the old password but the musl-based passwd binary simply exits.
Let passwd clear the password before exiting.
src/login_nopam.c: list_match(): Use iteration instead of recursion
The recursive nature of list_match() triggered regression during
refactoring. In Linux-PAM, the same code exists which could lead to
stack overflow because <access.conf> could be arbitrarily long.
Use an iterative approach for easier refactoring, to support long
lines in the future and to stay in sync with Linux-PAM.
If passwd is called with -P, then PAM handling is disabled
(src/passwd.c line 749). The manual page claims that host files would
be used, which is not true.
The PAM support was only enabled with configure option
--enable-account-tools-setuid. The other account tools would use PAM
then to verify that the user is granted elevated permissions for
actions which normally only root can do.
In chage, however, any non-root user who does not specify the -l
command line option is denied access in check_perms. The check for
being root or not is done with getuid, so non-root users cannot
change user account's aging information in any possible way since
more than 18 years by now.
It's safe to say that nobody misses this non-existing feature. Biggest
benefit is to get chage out of the ACCT_TOOLS_SETUID group of tools.
The nusers variable could, in theory, overflow and trigger an out of
boundary access if a huge amount of entries is added. Realistically,
this is not possible with current systems because way too much data
would be involved.
But let's better be safe than sorry and use correct data types.
Huge files could trigger signed integer overflows if enough lines are
within the file. Use intmax_t which is at least 64 bit to move this
event far into the future.
Support for /etc/suauth only exists if su is installed without
PAM support. If su is not installed (--without-su) or if PAM
support is enabled (default), do not install suauth.5 manual
page.
The SU_ACCESS preprocessor definition is used to decide if
feature exists or not. See links for more details.
Serge Hallyn [Sat, 11 Jan 2025 21:35:01 +0000 (15:35 -0600)]
add and use a login.defs.test with CREATE_HOME set
I suspect this is not a big deal, and most distributions just ship their own
version verbatim like debian/login.defs. But if there is a distro - or even a
person - using this as is from upstream, then we dont' want to break them. So
let's undo this and use an etc/login.defs.test for the testing if needed.
Changelog: 01/13: move etc/login.defs.test to tests/system/etc/login.defs per
suggestion.
Iker Pedrosa [Fri, 22 Nov 2024 09:28:48 +0000 (10:28 +0100)]
etc/login.defs: enable CREATE_HOME
In order to have consistent behaviour among all distributions, the same
configuration needs to be shared. That is why we are going to use the
`etc/login.defs` file and enable CREATE_HOME so that the home dir is
created automatically. This is not the default configuration used in all
distributions, but it is the most common one.
Iker Pedrosa [Wed, 20 Nov 2024 09:41:10 +0000 (10:41 +0100)]
tests: basic group deletion
This is the transformation to Python of the test located in
`tests/grouptools/groupdel/01_groupdel_delete_group/groupdel.test`,
which checks that `groupdel` is able to delete a group.
Iker Pedrosa [Wed, 20 Nov 2024 09:13:33 +0000 (10:13 +0100)]
tests: change GID of a group
This is the transformation to Python of the test located in
`tests/grouptools/groupmod/01_groupmod_change_gid/groupmod.test`, which
checks that `groupmod` is able to change the GID of a group.
Iker Pedrosa [Tue, 19 Nov 2024 15:18:45 +0000 (16:18 +0100)]
tests: basic group creation
This is the transformation to Python of the test located in
`tests/grouptools/groupadd/02_groupadd_add_group_GID_MIN/groupadd.test`,
which checks that `groupadd` is able to create a new group.
Iker Pedrosa [Tue, 19 Nov 2024 09:19:09 +0000 (10:19 +0100)]
tests: delete user and homedir
This is the transformation to Python of the test located in
`tests/usertools/01/18_userdel_remove_homedir.test`, which checks that
`userdel` is able to delete a user and its homedir. The test checks that
the user, the group and the home folder don't exist.
Iker Pedrosa [Wed, 13 Nov 2024 15:24:55 +0000 (16:24 +0100)]
tests: rename user
This is the transformation to Python of the test located in
`tests/usertools/01/10_usermod_rename_user.test`, which checks that
`usermod` is able to rename a user. The test checks that the new user,
the group and home folder exists.
Iker Pedrosa [Wed, 20 Nov 2024 13:58:54 +0000 (14:58 +0100)]
tests: recreate deleted user
This is the transformation to Python of the test located in
`tests/usertools/01/02_useradd_recreate_deleted_user.test`, which checks
that `useradd` is able to create again a removed user.
Iker Pedrosa [Fri, 8 Nov 2024 11:15:52 +0000 (12:15 +0100)]
tests: basic user creation
This is the transformation to Python of the test located in
`tests/usertools/01/01_useradd_add_user.test`, which checks that
`useradd` is able to create a new user and its corresponding group and
home folder.
Iker Pedrosa [Mon, 7 Oct 2024 13:44:17 +0000 (15:44 +0200)]
Tests: implement system test framework
As discussed at length, this is the implementation of the new system
tests framework for shadow. This is a proof of concept that contains the
key elements to be able to run basic user (i.e. useradd, usermod) and
group (i.e. usermod) tests. If you like the framework the rest of the
functionality will be added in the future.
Some useful facts:
* It is implemented in python
* It is based on pytest and pytest-mh
* It works on all the distributions that are part of our CI
* It can be run in the cloud (VM or container) as well as on-premises
* After the execution of each test the environment is cleaned up
* Logs and other artifacts for failed tests are collected
* It has a rich API that can be extended and extended to cover new
functionalities
With glibc we can use "e" in mode argument to set O_CLOEXEC on
opened files. The /etc/shadow and /etc/gshadow file handles should
be protected to make sure that they are never passed to child
processes by accident.
Calling exit might trigger cleanup functions registered through
atexit. Since some programs use this mechanism, be extra cautious to
never release passwd/group locks too early.
The list_match function handles EXCEPT entries through recursive
calls. It calls itself with NULL, which was then passed to strtok so
parsing continued at current position.
Replacing strtok with strsep, this means that EXCEPT entries never
match, because strsep(NULL, ...) always returns NULL, i.e. the
code treats everything after EXCEPT as non-existing.
Fix this by passing current list pointer to recursive call.
Fixes: 90afe61003ef (2024-07-04; "lib/, src/: Use strsep(3) instead of strtok(3)") Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
lib/chkname.c: login_name_max_size(): Put limits for LOGIN_NAME_MAX and sysconf(_SC_LOGIN_NAME_MAX)
GNU Hurd doesn't define LOGIN_NAME_MAX. GNU Hurd recommends having no
system limits. When a program needs a limit, because it needs to
validate user input, it is recommended that each program defines its own
limit macros. The rationale is that this avoids hard-coded limits in
ABIs, which cannot be modified ever.
However, that doesn't mean that programs should have no limits at all.
We use this limit for validating user input, and so we shouldn't allow
anything just because the system doesn't want to set a limit.
So, when sysconf(2) returns -1, either due to an error or due to a claim
for no limits, we must fall back to the LOGIN_NAME_MAX value. And if
the system doesn't define that value, we must define it ourselves (we're
more or less free to choose any value, so let's pick the one that glibc
provides nowadays).
Fixes: 6a1f45d932c8 (2024-02-04; "lib/chkname.c: Support unlimited user name lengths") Closes: <https://github.com/shadow-maint/shadow/issues/1166> Cc: Chris Hofstaedtler <zeha@debian.org> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> Reviewed-by: Tobias Stoeckmann <tobias@stoeckmann.org> Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
Launch a login shell again if requested through "su -" or "su -l".
Fixes: d9923431eb38 ("src/: Use xasprintf() instead of its pattern") Closes: <https://github.com/shadow-maint/shadow/issues/1160> Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Except for the added (and sorted) includes, the removal of redundant
parentheses, and a few non-string cases that I've left out of the
change, this patch can be approximated with the following semantic
patch:
Except for the added (and sorted) includes, the removal of redundant
parentheses, a few cases that have been refactored for readability, and
a couple of non-string cases that I've left out of the change, this
patch can be approximated with the following semantic patch:
We were reusing a leftover from parsing a previous line if
(i == NFIELDS-1). A few lines below this check, we use read the element
in [3] (that is, [NFIELDS-1]), without having written it in this call.
Be stricter, and require that all NFIELDS fields are found.
Instead of reallocating 1 more meber per iteration, calculate the total
amount that we want by counting the number of commas (delimiters) in the
string, plus one for the last element, plus one for the terminating
NULL.
This might result in overallocation of one element if the string is an
empty string, or if there's a trailing comma; however, that's not an
issue. We can afford overallocating one element in certain cases, and
we get in exchange a much simpler function.
lib/gshadow.c: build_list(): Remove second parameter
We've simplified the function so much in the previous commits, that now
$2 is rather useless. It only sets the output parameter to the same
value that the function returns. It's simpler if the caller just sets
it itself after the call.
This removes the only 3-star pointer in the entire project. :)
lib/gshadow.c: sgetsgent(): Be consistent using NULL
0 is a horrible null-pointer constant. Don't use it.
Especially, when just a few lines above, in the same function,
we've used NULL for the same thing.
lib/gshadow.c: Move zeroing to within build_list()
This makes build_list() less dependent on the context.
It starts from clean, whatever the state before the call was.
I was having a hard time understanding the reallocation,
until I saw that we were zeroing everything right before the call.
lib/gshadow.c: build_list(): Improve variable and parameter names
It was hard to understand what each variable is. Use a consistent
scheme, where a 'p' means a pointer, 'l' means list, and 'n' means
number of elements. Those should be obvious from the name of the
function and the context, and will make it easier to read the code.
Also, the shorter names will allow focusing on the rest of the code.
lib/gshadow.c: build_list(): Fix type of parameter
list ($2) is a pointer to a list of strings. We were declaring it as an
array of pointers to strings, which was bogus. It worked out of luck,
because array parameters are transformed into pointers by the compiler,
but it was incorrect. Just look at how we're calling this function.
strsep(3) is stateless, and so is easier to reason about.
It also has a slight difference: strtok(3) jumps over empty fields,
while strsep(3) respects them as empty fields. In most of the cases
where we were using strtok(3), it makes more sense to respect empty
fields, and this commit probably silently fixes a few bugs.
In other cases (most notably filesystem paths), contiguous delimiters
("//") should be collapsed, so strtok(3) still makes more sense there.
This commit doesn't replace such strtok(3) calls.
While at this, remove some useless variables used by these calls, and
reduce the scope of others.
Except for the added (and sorted) includes, and the removal of redundant
parentheses, and one special case, this patch can be approximated with
the following semantic patch:
$ cat ~/tmp/spatch/strneq.sp;
@@
expression a, b;
@@
contrib/, lib/, src/: Use streq() instead of its pattern
Except for the added (and sorted) includes, and the removal of redundant
parentheses, this patch can be approximated with the following semantic
patch:
$ cat ~/tmp/spatch/streq.sp;
@@
expression a, b;
@@
The instructions are written so that this script should be run from the
root of the repository. Specify the path from the root of the repo.
Before this fix, the command needed to be run from within <share/>.
The get_map_ranges function shall support the whole accepted range
as specified in user_namespaces(7), i.e. upper and lower from 0 to
UINT_MAX - 1 as well as range from 1 to UINT_MAX. The actual limit of
range depends on values of upper and lower and adding the range
to either upper or lower shall never overflow UINT_MAX.
Fixes: 7c43eb2c4ea6 (2024-07-11, "lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call") Fixes: ff2baed5dbf8 (2016-08-14, "idmapping: add more checks for overflow") Fixes: 94da3dc5c853 (2016-08-14, "also check upper for wrap") Fixes: 7f5a14817d30 (2016-07-31, "get_map_ranges: check for overflow") Co-authored-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>
lib/, src/: Use NULL instead of 0 as a null pointer constant
GCC 15 will add -Wzero-as-null-pointer-constant for deprecating it,
and I'm working on a paper for deprecating it from ISO C too.
Let's remove any uses in our code base.
I've done this change by building GCC from master, adding
-Werror=zero-as-null-pointer-constant to ./autogen.sh, and fixing every
error that showed up.
/*
* Copy string to local buffer. It has to be tokenized and we
* have to do that to our private copy.
*/
- if (strlen (string) >= sizeof spwbuf) {
- fprintf (shadow_logfd,
- "%s: Too long passwd entry encountered, file corruption?\n",
- shadow_progname);
- return NULL; /* fail if too long */
- }
+ if (strlen (string) >= sizeof spwbuf)
+ return 0;
strcpy (spwbuf, string);
stpsep(spwbuf, "\n");
@@ -30,14 +26,16 @@
fields[i] = strsep(&cp, ":");
if (i == (FIELDS - 1))
- fields[i++] = "";
+ fields[i++] = empty;
if (cp != NULL || (i != FIELDS && i != OFIELDS))
- return NULL;
+ return 0;
/*
* Start populating the structure. The fields are all in
- * static storage, as is the structure we pass back.
+ * static storage, as is the structure we pass back. If we
+ * ever see a name with '+' as the first character, we try
+ * to turn on NIS processing.
*/
spwd.sp_namp = fields[0];
@@ -46,13 +44,13 @@
/*
* Get the last changed date. For all of the integer fields,
* we check for proper format. It is an error to have an
- * incorrectly formatted number.
+ * incorrectly formatted number, unless we are using NIS.
*/
/*
* Get the minimum period between password changes.
@@ -61,7 +59,7 @@
if (fields[3][0] == '\0')
spwd.sp_min = -1;
else if (a2sl(&spwd.sp_min, fields[3], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the maximum number of days a password is valid.
@@ -70,7 +68,7 @@
if (fields[4][0] == '\0')
spwd.sp_max = -1;
else if (a2sl(&spwd.sp_max, fields[4], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* If there are only OFIELDS fields (this is a SVR3.2 /etc/shadow
@@ -93,7 +91,7 @@
if (fields[5][0] == '\0')
spwd.sp_warn = -1;
else if (a2sl(&spwd.sp_warn, fields[5], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the number of days of inactivity before an account is
@@ -103,7 +101,7 @@
if (fields[6][0] == '\0')
spwd.sp_inact = -1;
else if (a2sl(&spwd.sp_inact, fields[6], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the number of days after the epoch before the account is
@@ -113,7 +111,7 @@
if (fields[7][0] == '\0')
spwd.sp_expire = -1;
else if (a2sl(&spwd.sp_expire, fields[7], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* This field is reserved for future use. But it isn't supposed
@@ -123,8 +121,7 @@
if (fields[8][0] == '\0')
spwd.sp_flag = SHADOW_SP_FLAG_UNSET;
else if (str2ul(&spwd.sp_flag, fields[8]) == -1)
- return NULL;
+ return 0;
src/login_nopam.c: Rely on the system's MAXHOSTNAMELEN
The reason for that code seems to be some ancient AIX version that
defined a value that was too small (32). We don't support such systems.
In the link below, I found the following comment and code:
/*
* Some AIX versions advertise a too small MAXHOSTNAMELEN value (32).
* Result: long hostnames would be truncated, and connections would be
* dropped because of host name verification failures. Adrian van Bloois
* (A.vanBloois@info.nic.surfnet.nl) figured out what was the problem.
*/
This fix addresses an issue in is_valid_user_list() where the free
operation was attempted on an address not allocated with malloc(). By
duplicating the pointer with xstrdup(users) into dup, and using dup as
the original pointer, we ensure that only the valid pointer is freed,
avoiding an invalid free operation.
This bug was introduced when changing some code that used strchrnul(3)
to use strsep(3) instead. strsep(3) advances the pointer, unlike the
previous code.
This unconditionally leads to a bug:
- Passing NULL to free(3), if the last field in the
colon-separated-value list is non-empty. This results in a memory
leak.
- Passing a pointer to the null byte ('\0') that terminates the string,
if the last element of the colon-separated-value list is empty. The
most obvious reproducer of such a bogus free(3) call is:
free(strdup("foo:") + 4);
This results in Undefined Behavior, and could result in allocator
data corruption.
Fixes: 16cb66486554 (2024-07-01, "lib/, src/: Use strsep(3) instead of its pattern") Suggested-by: <https://github.com/frostb1ten> Reported-by: <https://github.com/frostb1ten> Reviewed-by: Serge Hallyn <serge@hallyn.com> Reviewed-by: Alejandro Colomar <alx@kernel.org> Cc: Iker Pedrosa <ipedrosa@redhat.com> Cc: Christian Brauner <christian@brauner.io>
projects / thirdparty / shadow.git / log
