BUGS
Error returns from this system call are indistinguishable from
successful reports that the time is a few seconds before the
Epoch, so the C library wrapper function never sets errno as a re‐
sult of this call.
The tloc argument is obsolescent and should always be NULL in new
code. When tloc is NULL, the call cannot fail.
tests/unit/test_xasprintf.c: Fix use of volatile pointer
volatile needs to be casted away behind a [[gnu::noipa]] function, to
make that invisible to the compiler. Otherwise, the compiler can see
that it is being discarded, and is free to abuse Undefined Behavior.
Closes: <https://github.com/shadow-maint/shadow/issues/1028> Reported-by: Chris Hofstaedtler <zeha@debian.org> Tested-by: Chris Hofstaedtler <zeha@debian.org> Reviewed-by: Chris Hofstaedtler <zeha@debian.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>
Serge Hallyn [Sat, 25 May 2024 13:40:11 +0000 (08:40 -0500)]
tests/: Support run_some from exported tarball
common/config.sh currently tries to find the top directory by looking
for .git. There are also many places under tests/ where we use
hard-coded ../../.. to find things like ${TOP_DIR}/lib.
We don't actually ship the tests with 'make dist'. So we will
be exporting tests/ as a separate tarball. In particular, I want
to then import this in the debian package. However, there it will
be under shadow.git/debian/tests, not shadow.git/tests.
To support this, accept the environment variable BUILD_BASE_DIR,
which should point to shadow.git.
An alternative would be to move the tests to their own git
tree. However, keeping tests in separate git tree tends to
lead to repos getting out of sync. And we'd still need to accept
something like BUILD_BASE_DIR.
Note there are a lot of tests under run-all, which I'm not converting
as they currently are not being run in CI, so I'm more likely to
break something.
Changelog:
2024 05 26: Incorporate feedback from alejandro-colomar
lib/csrand.c: Fix the lower part of the domain of csrand_uniform()
I accidentally broke this code during an un-optimization. We need to
start from a random value of the width of the limit, that is, 32 bits.
Thanks to Jason for pointing to his similar code in the kernel, which
made me see my mistake.
Fixes: 2a61122b5e8f ("Unoptimize the higher part of the domain of csrand_uniform()") Closes: <https://github.com/shadow-maint/shadow/issues/1015> Reported-by: Michael Brunnbauer <https://github.com/michaelbrunnbauer>
Link: <https://git.zx2c4.com/linux-rng/tree/drivers/char/random.c#n535> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Link: <https://github.com/shadow-maint/shadow/pull/638>
Link: <https://github.com/shadow-maint/shadow/issues/634>
Link: <https://github.com/shadow-maint/shadow/pull/624> Tested-by: Michael Brunnbauer <https://github.com/michaelbrunnbauer> Reviewed-by: Michael Brunnbauer <https://github.com/michaelbrunnbauer> Signed-off-by: Alejandro Colomar <alx@kernel.org>
lixinyun [Tue, 28 May 2024 22:53:02 +0000 (06:53 +0800)]
src/groupmod.c: delete gr_free_members(&grp) to avoid double free
Groupmod -U may cause crashes because of double free. If without -a, the first free of (*ogrp).gr_mem is in gr_free_members(&grp), and then in gr_update without -n or gr_remove with -n.
Considering the minimal impact of modifications on existing code, delete gr_free_members(&grp) to avoid double free.Although this may seem reckless, the second free in two different positions will definitely be triggered, and the following two test cases can be used to illustrate the situation :
This case would free (*ogrp).gr_mem in gr_free_members(&grp) due to assignment statements grp = *ogrp, then in if (nflg && (gr_remove (group_name) == 0)), which finally calls gr_free_members(grent) to free (*ogrp).gr_mem again.
The other case would free (*ogrp).gr_mem in gr_free_members(&grp) too, then in if (gr_update (&grp) == 0), which finally calls gr_free_members(grent) too to free (*ogrp).gr_mem again.
So the first free is unnecessary, maybe we can drop it.
Fixes: 342c934a3590 ("add -U option to groupadd and groupmod") Closes: <https://github.com/shadow-maint/shadow/issues/1013>
Link: <https://github.com/shadow-maint/shadow/pull/1007>
Link: <https://github.com/shadow-maint/shadow/pull/271>
Link: <https://github.com/shadow-maint/shadow/issues/265> Cc: "Serge E. Hallyn" <serge@hallyn.com> Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: lixinyun <li.xinyun@h3c.com>
Serge Hallyn [Wed, 5 Jun 2024 13:02:27 +0000 (08:02 -0500)]
man/lastlog: remove wrong use of keyword term
Per https://tdg.docbook.org/tdg/4.5/term, term is a word being
defined in a varlistentry. The 'high uid' description is not a
varlistentry, so <term> and </term> show up in the processed
manpage. See debian Bug#1072297.
Iker Pedrosa [Mon, 27 May 2024 13:33:45 +0000 (15:33 +0200)]
configure: move cmocka library detection
`PKG_CONFIG` variable needs to be set for `PKG_CHECK_MODULES` to
succeed, but this wasn't happening in Fedora because the first
appearance of `PKG_CHECK_MODULES` was conditionally skipped because this
distribution is compiled without `libbsd` support. Thus, moving the
cmocka library detection before libbsd fixes the problem.
lib/gettime.c: gettime(): Call a2i() instead of strtoull_noneg()
time_t isn't necessarily unsigned (in fact, it's likely to be signed.
Therefore, parse the number as the right type, via a2i(time_t, ...).
Still, reject negative numbers, just to be cautious. It was done
before (strtoull_noneg()), so it shouldn't be a problem. (However,
strtoull_noneg() was only introduced recently, and before that we called
strtoull(3), which silently accepted negative values.)
Remove the limitation of ULONG_MAX, which seems arbitrary. It probably
was written in times where 'time_t' had the same length of 'long', and
this was thus a test that the value didn't overflow 'time_t'. Such a
test is implicit in the a2i() call, so forget about it.
Unify the error messages into a single one that provides all the info
(except the value of 'fallback').
This was already true in the commit that introduced the code:
$ git show 45c6603cc:src/usermod.c \
| grepc update_gshadow \
| grep -e changed -e goto -e break -e continue -e '\<if\>' -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
int changed;
changed = 0;
while ((sgrp = sgr_next())) {
* See if the user was a member of this group
* See if the user was an administrator of this group
* See if the user specified this group as one of their
if (!was_member && !was_admin && !is_member)
continue;
if (was_admin && lflg) {
changed = 1;
}
if (was_member && (!Gflg || is_member)) {
if (lflg) {
changed = 1;
}
} else if (was_member && Gflg && !is_member) {
changed = 1;
} else if (!was_member && Gflg && is_member) {
changed = 1;
}
if (!changed)
continue;
changed = 0;
}
}
Report:
> shadow-4.15.0/src/usermod.c:864:3: alloc_fn: Storage is returned from allocation function "__sgr_dup".
> shadow-4.15.0/src/usermod.c:864:3: var_assign: Assigning: "nsgrp" = storage returned from "__sgr_dup(sgrp)".
> shadow-4.15.0/src/usermod.c:964:1: leaked_storage: Variable "nsgrp" going out of scope leaks the storage it points to.
> 962| free (nsgrp);
> 963| }
> 964|-> }
> 965| #endif /* SHADOWGRP */
> 966|
Report:
> shadow-4.15.0/src/usermod.c:734:3: alloc_fn: Storage is returned from allocation function "__gr_dup".
> shadow-4.15.0/src/usermod.c:734:3: var_assign: Assigning: "ngrp" = storage returned from "__gr_dup(grp)".
> shadow-4.15.0/src/usermod.c:815:1: leaked_storage: Variable "ngrp" going out of scope leaks the storage it points to.
> 813| gr_free(ngrp);
> 814| }
> 815|-> }
> 816|
> 817| #ifdef SHADOWGRP
Report:
> shadow-4.15.0/src/useradd.c:575:2: alloc_fn: Storage is returned from allocation function "fdopen".
> shadow-4.15.0/src/useradd.c:575:2: var_assign: Assigning: "ofp" = storage returned from "fdopen(ofd, "w")".
> shadow-4.15.0/src/useradd.c:734:2: leaked_storage: Variable "ofp" going out of scope leaks the storage it points to.
> 732| }
> 733|
> 734|-> return ret;
> 735| }
> 736|
src/useradd.c: Add fmkstemp() to fix file-descriptor leak
This function creates a temporary file, and returns a FILE pointer to
it. This avoids dealing with both a file descriptor and a FILE pointer,
and correctly deallocating the resources on error.
The code before this patch was leaking the file descriptor if fdopen(3)
failed.
src/useradd.c: set_defaults(): Do not free(3) the result of asprintf(3) if it failed
See asprintf(3):
RETURN VALUE
When successful, these functions return the number of bytes
printed, just like sprintf(3). If memory allocation wasn’t possi‐
ble, or some other error occurs, these functions will return -1,
and the contents of strp are undefined.
Iker Pedrosa [Wed, 15 May 2024 10:25:51 +0000 (12:25 +0200)]
port: fix OVERRUN (CWE-119)
```
shadow-4.15.0/lib/port.c:154:2: alias: Assigning: "port.pt_names" = "ttys". "port.pt_names" now points to element 0 of "ttys" (which consists of 65 8-byte elements).
shadow-4.15.0/lib/port.c:155:2: cond_const: Checking "j < 64" implies that "j" is 64 on the false branch.
shadow-4.15.0/lib/port.c:175:2: overrun-local: Overrunning array of 65 8-byte elements at element index 65 (byte offset 527) by dereferencing pointer "port.pt_names + (j + 1)".
173| *cp = '\0';
174| cp++;
175|-> port.pt_names[j + 1] = NULL;
176|
177| /*
```
These overloaded macros allow passing either a const or a non-const
endp, and will call the appropriate function. This kind of const
overloading has prior art in C23's string functions, such as memchr(3).
Martin suggested using an artificial function pointer in _Generic(3); it
allows switching on various types at the same time.
Also add a comment referring to liba2i's PDF manual for documentation.
lib/alloc.h: Reimplement [X]REALLOC[F]() macros with _Generic(3)
Instead of GNU builtins and extensions, these macros can be implemented
with C11's _Generic(3), and the result is much simpler (and safer, since
it's now an error, not just a warning).
'endptr' is appropriate internally in strtol(3) because it's a pointer
to 'end', and 'end' itself is a pointer to one-after-the-last character
of the numeric string. In other words,
endptr == &end
However, naming the pointer whose address we pass to strtol(3)'s
'endptr' feels wrong, and causes me trouble while parsing the code; I
need to double check the number of dereferences, because something feels
wrong in my head.
This means we set the pointees on error, which we didn't do before, but
since we return -1 on error and ignore (don't use) the pointees at call
site, that's fine.
All 3 non-error paths in the second part resulted in *has_min = true.
Set in once before the switch(), to simplify.
This means we set this variable on error, which we didn't do before,
but since we return -1 on error and ignore (don't use) the pointees at
call site, that's fine.
Also, move a couple of *has_max = true statements to before a comment,
in preparation for future commits.
Set *has_{min,max} = false at the begining, so we only need to set them
to true later.
This means we set these variables on error, which we didn't do before,
but since we return -1 on error and ignore (don't use) the pointees at
call site, that's fine.
share/containers: update build flags for fedora 40
libpam is enabled to provide `passwd` binary from this package, as there
are several password quality checks that are enabled through a PAM
module. Same reason to disable account-tools-setuid.
sssd is disabled because `files provider` has been removed in sssd, and
the underlying functionality in shadow isn't needed anymore.
libcrack dependency was disabled some time ago, but the upstream repo
wasn't updated. Doing it now.
newuidmap and newgidmap manpages: fix fd description
The manpages for newuidmap and newgidmap had a typo "[pid[" instead
of "[pid]". They were also unclear about what the /proc/pid fd should
be. Fix both.
projects / thirdparty / shadow.git / log
