All the string-copying functions called above do terminate the strings
they create with a NUL byte. Writing it again at the end of the buffer
is unnecessary paranoid code. Let's remove it.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This function allows reducing error checking (since errors are
propagated across chained calls), and also simplifies the calculation of
the start and end of the buffer where the string should be written.
Moreover, the new code is more optimized, since many calls to strlen(3)
have been removed.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
[v]stpeprintf() are similar to [v]snprintf(3), but they allow chaining.
[v]snprintf(3) are very dangerous for catenating strings, since the
obvious ways to do it invoke Undefined Behavior, and the ways that avoid
UB are very error-prone.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
When trying to build shadow in a different directory I stumbled upon few
issues, this commit aims to fix all of them:
- The `subid.h` file is generated and hence in the build directory and
not in the source directory, so use `$(builddir)` instead of
`$(srcdir)`.
- Using `$<` instead of filenames utilises autotools to locate the files
in either the source or build directory automatically.
- `xsltproc` needs to access the files in login.defs.d in either the
source directory or the symlink in a language subdirectory, but it
does not interpret the `--path` as prefix of the entity path, but
rather a path under which to locate the basename of the entity
from the XML file. So specify the whole path to login.defs.d.
- The above point could be used to make the symlinks of login.defs.d
and entity path specifications in the XMLs obsolete, but I trying
not to propose possibly disrupting patches, so for the sake of
simplicity just specify `$(srcdir)` when creating the symlink.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
The intention of the code is just to not report an error message when
'typefile' doesn't exist. If we call access(2) and then fopen(2),
there's a race. It's not a huge problem, and the worst thing that can
happen is reporting an error when the file has been removed after
access(2). It's not a problem, but we can fix the race and at the same
time clarify the intention of not warning about ENOENT and also remove
one syscall. Seems like a win-win.
Suggested-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Every non-const pointer converts automatically to void *.
- Every pointer converts automatically to void *.
- void * converts to any other pointer.
- const void * converts to any other const pointer.
- Integer variables convert to each other.
I changed the declaration of a few variables in order to allow removing
a cast.
However, I didn't attempt to edit casts inside comparisons, since they
are very delicate. I also kept casts in variadic functions, since they
are necessary, and in allocation functions, because I have other plans
for them.
I also changed a few casts to int that are better as ptrdiff_t.
This change has triggered some warnings about const correctness issues,
which have also been fixed in this patch (see for example src/login.c).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We could use the standard (C11) _Noreturn qualifier, but it will be
deprecated in C23, and replaced by C++'s [[noreturn]], which is
compatible with the GCC attribute, so let's directly use the attribute,
and in the future we'll be able to switch to [[]].
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Recently, we removed support for 'struct utmpx'. We did it because utmp
and utmpx are identical, and while POSIX specifies utmpx (and not utmp),
GNU/Linux documentation seems to favor utmp. Also, this project
defaulted to utmp, so changing to utmpx would be more dangerous than
keeping old defaults, even if it's supposed to be the same.
Now, I just found more code that didn't make much sense: lib/utent.c
provides definitions for getutent(3) and friends in case the system
doesn't provide them, but we don't provide prototypes for those
definitions, so code using the functions would have never compiled.
Let's just remove these definitions as dead code.
Fixes: 3be7b9d75a ("Remove traces of utmpx")
Fixes: 170b76cdd1 ("Disable utmpx permanently")
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Closes#457
The existing prose was confusing, or simply wrong. Make it clear
that only the group ownership of the tty is affected, and how.
Also move the paragraph about defaults after the discussion of
acceptable TTYGROUPs, as this seems more natural.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
In variadic functions we still do the cast. In POSIX, it's not
necessary, since NULL is required to be of type 'void *', and 'void *'
is guaranteed to have the same alignment and representation as 'char *'.
However, since ISO C still doesn't mandate that, and moreover they're
doing dubious stuff by adding nullptr, let's be on the cautious side.
Also, C++ requires that NULL is _not_ 'void *', but either plain 0 or
some magic stuff.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If lines start with '\0' then it is possible to trigger out of
boundary accesses.
Check if indices are valid before accessing them.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
If the file referenced by ENV_TZ has a zero length string, then an out
of boundary write occurs. Also the result can be wrong because it is
assumed that the file will always end with a newline.
Only override a newline character with '\0' to avoid these cases.
This cannot be considered to be security relevant because login.defs
and its contained references to system files should be trusted to begin
with.
Proof of Concept:
1. Compile shadow's su with address sanitizer and --without-libpam
2. Setup your /etc/login.defs to contain ENV_TZ=/etc/tzname
3. Prepare /etc/tzname to contain a '\0' byte at the beginning
`python -c "print('\x00')" > /etc/tzname`
4. Use su
`su -l`
You can see the following output:
`tz.c:45:8: runtime error: index 18446744073709551615 out of bounds for type 'char [8192]'`
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
__int128, which is needed for optimizing that part of the range, is not
always available. We need the unoptimized version for portability
reasons.
Closes: <https://github.com/shadow-maint/shadow/issues/634>
Fixes: 1a0e13f94e ("Optimize csrand_uniform()")
Reported-by: Adam Sampson <ats@offog.org>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We do need the unoptimized version of csrand_uniform() for high values
of `n`, since the optimized version depends on having __int128, and it's
not available on several platforms, including ARMv7, IA32, and MK68k.
This reverts commit 848f53c1d3c1362c86d3baab6906e1e4419d2634; however,
I applied some tweaks to the reverted commit.
Reported-by: Adam Sampson <ats@offog.org>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Now that we optimized csrand_uniform(), we don't need these functions.
This reverts commit 7c8fe291b1260e127c10562bfd7616961013730f.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Use a different algorithm to minimize rejection. This is essentially
the same algorithm implemented in the Linux kernel for
__get_random_u32_below(), but written in a more readable way, and
avoiding microopimizations that make it less readable.
Which (the Linux kernel implementation) is itself based on Daniel
Lemire's algorithm from "Fast Random Integer Generation in an Interval",
linked below. However, I couldn't really understand that paper very
much, so I had to reconstruct the proofs from scratch, just from what I
could understand from the Linux kernel implementation source code.
I constructed some graphical explanation of how it works, and why it
is optimal, because I needed to visualize it to understand it. It is
published in the GitHub pull request linked below.
Here goes a wordy explanation of why this algorithm based on
multiplication is better optimized than my original implementation based
on masking.
masking:
It discards the extra bits of entropy that are not necessary for
this operation. This works as if dividing the entire space of
possible csrand() values into smaller spaces of a size that is
a smaller power of 2. Each of those smaller spaces has a
rejection band, so we get as many rejection bands as spaces
there are. For smaller values of 'n', the size of each
rejection band is smaller, but having more rejection bands
compensates for this, and results in the same inefficiency as
for large values of 'n'.
multiplication:
It divides the entire space of possible random numbers in
chunks of size exactly 'n', so that there is only one rejection
band that is the remainder of `2^64 % n`. The worst case is
still similar to the masking algorithm, a rejection band that is
almost half the entire space (n = 2^63 + 1), but for lower
values of 'n', by only having one small rejection band, it is
much faster than the masking algorithm.
This algorithm, however, has one caveat: the implementation
is harder to read, since it relies on several bitwise tricky
operations to perform operations like `2^64 % n`, `mult % 2^64`,
and `mult / 2^64`. And those operations are different depending
on the number of bits of the maximum possible random number
generated by the function. This means that while this algorithm
could also be applied to get uniform random numbers in the range
[0, n-1] quickly from a function like rand(3), which only
produces 31 bits of (non-CS) random numbers, it would need to be
implemented differently. However, that's not a concern for us,
it's just a note so that nobody picks this code and expects it
to just work with rand(3) (which BTW I tried for testing it, and
got a bit confused until I realized this).
Finally, here's some light testing of this implementation, just to know
that I didn't goof it. I pasted this function into a standalone
program, and run it many times to find if it has any bias (I tested also
to see how many iterations it performs, and it's also almost always 1,
but that test is big enough to not paste it here).
int main(int argc, char *argv[])
{
printf("%lu\n", csrand_uniform(atoi(argv[1])));
}
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 1 | wc -l
341
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 1 | wc -l
339
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 1 | wc -l
338
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 2 | wc -l
336
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 2 | wc -l
328
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 2 | wc -l
335
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 0 | wc -l
332
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 0 | wc -l
331
$ seq 1 1000 | while read _; do ./a.out 3; done | grep 0 | wc -l
327
This isn't a complete test for a cryptographically-secure random number
generator, of course, but I leave that for interested parties.
Link: <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e9a688bcb19348862afe30d7c85bc37c4c293471>
Link: <https://github.com/shadow-maint/shadow/pull/624#discussion_r1059574358>
Link: <https://arxiv.org/abs/1805.10941>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Björn Esser <besser82@fedoraproject.org>
Cc: Yann Droneaud <ydroneaud@opteya.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Sam James <sam@gentoo.org>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
[Daniel Lemire: Added link to research paper in source code]
Cc: Daniel Lemire <daniel@lemire.me>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It is common to use the expression 'sizeof(x) * CHAR_BIT' to mean the
width in bits of a type or object. Now that there are _WIDTH macros for
some types, indicating the number of bits that they use, it makes sense
to wrap this calculation in a macro of a similar name.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The old code didn't produce very good random numbers. It had a bias.
And that was from performing some unnecessary floating-point
calculations that overcomplicate the problem.
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Björn Esser <besser82@fedoraproject.org>
Cc: Yann Droneaud <ydroneaud@opteya.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Sam James <sam@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This API is similar to arc4random_uniform(3). However, for an input of
0, this function is equivalent to csrand(), while arc4random_uniform(0)
returns 0.
This function will be used to reimplement csrand_interval() as a wrapper
around this one.
The current implementation of csrand_interval() doesn't produce very good
random numbers. It has a bias. And that comes from performing some
unnecessary floating-point calculations that overcomplicate the problem.
Looping until the random number hits within bounds is unbiased, and
truncating unwanted bits makes the overhead of the loop very small.
We could reduce loop overhead even more, by keeping unused bits of the
random number, if the width of the mask is not greater than
ULONG_WIDTH/2, however, that complicates the code considerably, and I
prefer to be a bit slower but have simple code.
BTW, Björn really deserves the copyright for csrand() (previously known
as read_random_bytes()), since he rewrote it almost from scratch last
year, and I kept most of its contents. Since he didn't put himself in
the copyright back then, and BSD-3-Clause doesn't allow me to attribute
derived works, I won't add his name, but if he asks, he should be put in
the copyright too.
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Björn Esser <besser82@fedoraproject.org>
Cc: Yann Droneaud <ydroneaud@opteya.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Sam James <sam@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions implement bit manipulation APIs, which will be added to
C23, so that in the far future, we will be able to replace our functions
by the standard ones, just by adding the stdc_ prefix, and including
<stdbit.h>.
However, we need to avoid UB for an input of 0, so slightly deviate from
C23, and use a different name (with _wrap) for distunguishing our API
from the standard one.
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Yann Droneaud <ydroneaud@opteya.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
A set of APIs similar to arc4random(3) is complex enough to deserve its
own file.
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Björn Esser <besser82@fedoraproject.org>
Cc: Yann Droneaud <ydroneaud@opteya.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
arc4random(3) returns a number.
arc4random_buf(3) fills a buffer.
arc4random_uniform(3) returns a number less than a bound.
and I'd add a hypothetical one which we use:
*_interval() should return a number within the interval [min, max].
In reality, the function being called csrand() in this patch is not
really cryptographically secure, since it had a bias, but a subsequent
patch will fix that.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We were always casting the result to u_long. Better just use that type
in the function. Since we're returning u_long, it makes sense to also
specify the input as u_long. In fact, that'll help for doing bitwise
operations inside this function.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
I have plans to split this function in smaller functions that implement
bits of this functionallity, to simplify the implementation. So, let's
use names that distinguish them.
This one produces a number within an interval, so make that clear. Also
make clear that the function produces cryptographically-secure numbers.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strlcpy(3) might not be visible since it is declared in <bsd/string.h>.
This can lead to warnings, like:
fields.c: In function 'change_field':
fields.c:103:17: warning: implicit declaration of function 'strlcpy'; did you mean 'strncpy'? [-Wimplicit-function-declaration]
103 | strlcpy (buf, cp, maxsize);
| ^~~~~~~
| strncpy
../lib/fields.c:103:17: warning: type of 'strlcpy' does not match original declaration [-Wlto-type-mismatch]
103 | strlcpy (buf, cp, maxsize);
| ^
/usr/include/bsd/string.h:44:8: note: return value type mismatch
44 | size_t strlcpy(char *dst, const char *src, size_t siz);
| ^
/usr/include/bsd/string.h:44:8: note: type 'size_t' should match type 'int'
/usr/include/bsd/string.h:44:8: note: 'strlcpy' was previously declared here
/usr/include/bsd/string.h:44:8: note: code may be misoptimized unless '-fno-strict-aliasing' is used
Comparisons if different signedness can result in unexpected results.
Add casts to ensure operants are of the same type.
gettime.c: In function 'gettime':
gettime.c:58:26: warning: comparison of integer expressions of different signedness: 'long long unsigned int' and 'time_t' {aka 'long int'} [-Wsign-compare]
58 | } else if (epoch > fallback) {
| ^
Cast to time_t, since epoch is less than ULONG_MAX at this point.
idmapping.c: In function 'write_mapping':
idmapping.c:202:48: warning: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Wsign-compare]
202 | if ((written <= 0) || (written >= (bufsize - (pos - buf)))) {
| ^~
newgidmap.c: In function ‘main’:
newgidmap.c:178:40: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
178 | if ((written <= 0) || (written >= sizeof(proc_dir_name))) {
| ^~
newuidmap.c: In function ‘main’:
newuidmap.c:107:40: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
107 | if ((written <= 0) || (written >= sizeof(proc_dir_name))) {
| ^~
Instead of using volatile pointers to prevent the compiler from
optimizing the call away, use a memory barrier.
This requires support for embedded assembly, which should be fine after
the recent requirement bumps.
memset_s() has a different signature than memset(3) or explicit_bzero(),
thus the current code would not compile. Also memset_s()
implementations are quite rare.
Use the C23 standardized version memset_explicit(3).
Fixes: 7a799ebb ("Ensure memory cleaning")
arc4random(3) without kernel support is unsafe, as it can't know when to
drop the buffer. Since we depend on libbsd since recently, we have
arc4random(3) functions always available, and thus, this code would have
always called arc4random_buf(3bsd), which is unsafe. Put it after some
better alternatives, at least until in a decade or so all systems have a
recent enough glibc.
glibc implements arc4random(3) safely, since it's just a wrapper around
getrandom(2).
Link: <https://inbox.sourceware.org/libc-alpha/20220722122137.3270666-1-adhemerval.zanella@linaro.org/>
Link: <https://inbox.sourceware.org/libc-alpha/5c29df04-6283-9eee-6648-215b52cfa26b@cs.ucla.edu/T/>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Guillem Jover <guillem@hadrons.org>
Cc: Björn Esser <besser82@fedoraproject.org>
Reviewed-by: "Jason A. Donenfeld" <Jason@zx2c4.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
