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>
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))) {
| ^~
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>
- Since strncpy(3) is not designed to write strings, but rather
(null-padded) character sequences (a.k.a. unterminated strings), we
had to manually append a '\0'. strlcpy(3) creates strings, so they
are always terminated. This removes dependencies between lines, and
also removes chances of accidents.
- Repurposing strncpy(3) to create strings requires calculating the
location of the terminating null byte, which involves a '-1'
calculation. This is a source of off-by-one bugs. The new code has
no '-1' calculations, so there's almost-zero chance of these bugs.
- strlcpy(3) doesn't padd with null bytes. Padding is relevant when
writing fixed-width buffers to binary files, when interfacing certain
APIs (I believe utmpx requires null padding at lease in some
systems), or when sending them to other processes or through the
network. This is not the case, so padding is effectively ignored.
- strlcpy(3) requires that the input string is really a string;
otherwise it crashes (SIGSEGV). Let's check if the input strings are
really strings:
- lib/fields.c:
- 'cp' was assigned from 'newft', and 'newft' comes from fgets(3).
- lib/gshadow.c:
- strlen(string) is calculated a few lines above.
- libmisc/console.c:
- 'cons' comes from getdef_str, which is a bit cryptic, but seems
to generate strings, I guess.1
- libmisc/date_to_str.c:
- It receives a string literal. :)
- libmisc/utmp.c:
- 'tname' comes from ttyname(3), which returns a string.
- src/su.c:
- 'tmp_name' has been passed to strcmp(3) a few lines above.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Since the project is supposed to be POSIX.1-2001 compliant it doesn't
make sense to have that added conditionally.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
It is Undefined Behavior to declare errno (see NOTES in its manual page).
Instead of using the errno dummy declaration, use one that doesn't need
a comment.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Previous commits, to keep readability of the diffs, left the code that
was previously wrapped by preprocessor coditionals untouched. Apply
some minor cosmetic changes to merge it in the surrounding code.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
On Linux, utmpx and utmp are identical. However, documentation (manual
pages) covers utmp, and just says about utmpx that it's identical to
utmp. It seems that it's preferred to use utmp, at least by reading the
manual pages.
Moreover, we were defaulting to utmp (utmpx had to be explicitly enabled
at configuration time). So, it seems safer to just make it permanent,
which should not affect default builds.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The OSes that are referred to by these comments, are extinct, but
their comments survived, fossilized in amber.
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
All of the macros we're using are required by POSIX.1-2001.
Cc: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The function is obsolete. It is recommended to use getrlimit(2) instead
(see the manual page for ulimit(3) or the POSIX manual for it). Since
getrlimit(2) is required by POSIX.1-2001, we can rely on it.
Cc: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There are several issues with getpass(3).
Many implementations of it share the same issues that the infamous
gets(3). In glibc it's not so terrible, since it's a wrapper
around getline(3). But it still has an important bug:
If the password is long enough, getline(3) will realloc(3) memory,
and prefixes of the password will be laying around in some
deallocated memory.
See the getpass(3) manual page for more details, and especially
the commit that marked it as deprecated, which links to a long
discussion in the linux-man@ mailing list.
So, readpassphrase(3bsd) is preferrable, which is provided by
libbsd on GNU systems. However, using readpassphrase(3) directly
is a bit verbose, so we can write our own wrapper with a simpler
interface similar to that of getpass(3).
One of the benefits of writing our own interface around
readpassphrase(3) is that we can hide there any checks that should
be done always and which would be error-prone to repeat every
time. For example, check that there was no truncation in the
password.
Also, use malloc(3) to get the buffer, instead of using a global
buffer. We're not using a multithreaded program (and it wouldn't
make sense to do so), but it's nice to know that the visibility of
our passwords is as limited as possible.
erase_pass() is a clean-up function that handles all clean-up
correctly, including zeroing the entire buffer, and then
free(3)ing the memory. By using [[gnu::malloc(erase_pass)]], we
make sure that we don't leak the buffers in any case, since the
compiler will be able to enforce clean up.
Link: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit?id=7ca189099d73bde954eed2d7fc21732bcc8ddc6b>
Reported-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The minimum id allocation for system accounts shouldn't be 0 as this is
reserved for root.
Signed-off-by: Tomáš Mráz <tm@t8m.info>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
glibc, musl, FreeBSD, and OpenBSD define the MAX() and MIN()
macros in <sys/param.h> with the same definition that we use.
Let's not redefine it here and use the system one, as it's
effectively the same as we define (modulo whitespace).
See:
shadow (previously):
alx@asus5775:~/src/shadow/shadow$ grepc -ktm MAX
./lib/defines.h:318:#define MAX(x,y) (((x) > (y)) ? (x) : (y))
glibc:
alx@asus5775:~/src/gnu/glibc$ grepc -ktm -x 'sys/param.h$' MAX
./misc/sys/param.h:103:#define MAX(a,b) (((a)>(b))?(a):(b))
musl:
alx@asus5775:~/src/musl/musl$ grepc -ktm -x 'sys/param.h$' MAX
./include/sys/param.h:19:#define MAX(a,b) (((a)>(b))?(a):(b))
OpenBSD:
alx@asus5775:~/src/bsd/openbsd/src$ grepc -ktm -x 'sys/param.h$' MAX
./sys/sys/param.h:193:#define MAX(a,b) (((a)>(b))?(a):(b))
FreeBSD:
alx@asus5775:~/src/bsd/freebsd/freebsd-src$ grepc -ktm -x 'sys/param.h$' MAX
./sys/sys/param.h:333:#define MAX(a,b) (((a)>(b))?(a):(b))
Signed-off-by: Alejandro Colomar <alx@kernel.org>
free(3) accepts NULL, since the oldest ISO C. I guess the
paranoid code was taking care of prehistoric implementations of
free(3). I've never known of an implementation that doesn't
conform to this, so let's simplify this.
Remove xfree(3), which was effectively an equivalent of free(3).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The setuid, setgid, and sticky bits are not copied during copy_tree.
Also start with very restrictive permissions before setting ownerships.
This prevents situations in which users in a group with less permissions
than others could win a race in opening the file before permissions are
removed again.
Proof of concept:
$ echo $HOME
/home/uwu
$ install -o uwu -g fandom -m 604 /dev/null /home/uwu/owo
$ ls -l /home/uwu/owo
-rw----r-- 1 uwu fandom 0 Sep 4 00:00 /home/uwu/owo
If /tmp is on another filesystem, then "usermod -md /tmp/uwu uwu" leads
to this temporary situation:
$ ls -l /tmp/uwu/owo
-rw----r-- 1 root root 0 Sep 4 00:00 /tmp/uwu/owo
This means that between openat and chownat_if_needed a user of group
fandom could open /tmp/uwu/owo and read the content when it is finally
written into the file.
Fixes regression introduced in faeab50e71.
If a directory contains fifos, then openat blocks until the other side
of the fifo is connected as well.
This means that users can prevent "usermod -m" from completing if their
home directories contain at least one fifo.
