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.
The groupadd from shadow does not allow upper case group names, the
same is true for the upstream shadow. But distributions like
Debian/Ubuntu/CentOS has their own way to cope with this problem,
this patch is picked up from Fedora [1] to relax the usernames
restrictions to allow the upper case group names, and the relaxation is
POSIX compliant because POSIX indicate that usernames are composed of
characters from the portable filename character set [A-Za-z0-9._-].
[1] https://src.fedoraproject.org/rpms/shadow-utils/blob/rawhide/f/shadow-4.8-goodname.patch
Signed-off-by: Alexander Kanavin <alex@linutronix.de>
Use *at() functions to pin the directory operating in to avoid being
redirected by unprivileged users replacing parts of paths by symlinks to
privileged files.
Introduce a path_info struct with the full path and dirfd and name
information for *at() functions, since the full path is needed for link
resolution, SELinux label lookup and ACL attributes.
Use *at() functions to pin the directory operating in to avoid being
redirected by unprivileged users replacing parts of paths by symlinks to
privileged files.
Use *at() functions to pin the directory operating in to avoid being
redirected by unprivileged users replacing parts of paths by symlinks to
privileged files.
Allow the compiler to verify the format string against the supplied
arguments.
chage.c:239:51: warning: format not a string literal, format string not checked [-Wformat-nonliteral]
239 | (void) strftime (buf, sizeof buf, format, tp);
| ^~~~~~
salt.c:102:22: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
102 | static /*@observer@*/const unsigned long SHA_get_salt_rounds (/*@null@*/int *prefered_rounds);
| ^~~~~
salt.c:110:22: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
110 | static /*@observer@*/const unsigned long YESCRYPT_get_salt_cost (/*@null@*/int *prefered_cost);
| ^~~~~
subordinateio.c:160:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
160 | static const bool range_exists(struct commonio_db *db, const char *owner)
| ^~~~~
Compilers are free to ignore the indented hint and modern optimizations
should create good code by themself.
(As such it is for example deprecated in C++17.)
On systems with Linux kernel < 3.17, getentropy() and getrandom() may
exist but return ENOSYS. Use /dev/urandom as a fallback to avoid a hard
requirement on Linux kernel version.
Fixes#512.
Signed-off-by: Xi Ruoyao <xry111@xry111.site>
In order to remove some of the FIXMEs it was necessary to change the
code and call getulong() instead of getlong().
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
C89 and POSIX.1-2001 define signal(2) as returning a pointer to a
function returning 'void'. K&R C signal(2) signature is obsolete.
Use 'void' directly.
Also, instead of writing the function pointer type explicitly, use
POSIX's 'sighandler_t'.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
nss_init() does not modify its path argument, thus declare it const.
Also drop superfluous prototype.
nss.c:54:31: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
54 | nsswitch_path = NSSWITCH;
| ^
Function declarations with no argument declare functions taking an
arbitrary number of arguments. Use the special type void to declare
functions taking no argument.
C89 defined isdigit as a function that tests for any decimal-digit
character, defining the decimal digits as 0 1 2 3 4 5 6 7 8 9.
I don't own a copy of C89 to check, but check in C17:
7.4.1.5
5.2.1
More specifically:
> In both the source and execution basic character sets, the value
> of each character after 0 in the above list of decimal digits
> shall be one greater than the value of the previous.
And since in ascii(7), the character after '9' is ':', it's highly
unlikely that any implementation will ever accept any
_decimal digit_ other than 0..9.
POSIX simply defers to the ISO C standard.
This is exactly what we wanted from ISDIGIT(c), so just use it.
Non-standard implementations might have been slower or considered
other characters as digits in the past, but let's assume
implementations available today conform to ISO C89.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
It wasn't being used at all. Let's remove it.
Use isdigit(3) directly in comments that referenced it.
Also, in those comments, remove an outdated reference to the fact
that ISDIGIT_LOCALE(c) might evaluate its argument more than once,
which could be true a few commits ago, until
IN_CTYPE_DEFINITION(c) was removed. Previously, the definition
for ISDIGIT_LOCALE(c) was:
#if defined (STDC_HEADERS) || (!defined (isascii) && !defined (HAVE_ISASCII))
# define IN_CTYPE_DOMAIN(c) 1
#else
# define IN_CTYPE_DOMAIN(c) isascii(c)
#endif
#define ISDIGIT_LOCALE(c) (IN_CTYPE_DOMAIN (c) && isdigit (c))
Which could evaluate 'c' twice on pre-C89 systems (which I hope
don't exist nowadays).
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
Due to the recent removal of IN_CTYPE_DOMAIN(), the uppercase
macros that wrapped these standard calls are now defined to be
equivalent. Therefore, there's no need for the wrappers, and it
is much more readable to use the standard calls directly.
However, hold on with ISDIGIT*(), since it's not so obvious what
to do with it.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
The recent removal of STDC_HEADERS made IN_CTYPE_DOMAIN be defined
to 1 unconditionally. Remove the now unnecessary definition, and
propagate its truthness to expressions where it was used.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
We're in 2021. C89 is everywhere; in fact, there are many other
assumptions in the code that wouldn't probably hold on
pre-standard C environments. Let's simplify and assume that C89
is available.
The specific assumptions are that:
- <string.h>, and <stdlib.h> are available
- strchr(3), strrchr(3), and strtok(3) are available
- isalpha(3), isspace(3), isdigit(3), and isupper(3) are available
I think we can safely assume we have all of those.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
memcpy(3) has been in standard C since C89. It is also in
POSIX.1-2001, in SVr4, and in 4.3BSD (see memcpy(3) and memcpy(3p)).
We can assume that this function is always available.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
strftime(3) has been in standard C since C89. It is also in
POSIX.1-2001, and in SVr4 (see strftime(3) and strftime(3p)).
We can assume that this function is always available.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
PARAMETERS:
According to the C2x charter, I reordered the parameters 'size'
and 'buf' from previously existing date_to_str() definitions.
C2x charter:
> 15. Application Programming Interfaces (APIs) should be
> self-documenting when possible. In particular, the order of
> parameters in function declarations should be arranged such that
> the size of an array appears before the array. The purpose is to
> allow Variable-Length Array (VLA) notation to be used. This not
> only makes the code's purpose clearer to human readers, but also
> makes static analysis easier. Any new APIs added to the Standard
> should take this into consideration.
I used 'long' for the date parameter, as some uses of the function
need to pass a negative value meaning "never".
FUNCTION BODY:
I didn't check '#ifdef HAVE_STRFTIME', which old definitions did,
since strftime(3) is guaranteed by the C89 standard, and all of
the conversion specifiers that we use are also specified by that
standard, so we don't need any extensions at all.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
When using groupdel with a prefix, groupdel will attempt to read a
passwd file to look for any user in the group. When the file does not
exist it cores with segmentation fault.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1986111
If a line in hushlogins file, e.g. /etc/hushlogins, starts with
'\0', then current code performs an out of boundary write.
If the line lacks a newline at the end, then another character is
overridden.
With strcspn both cases are solved.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
If SHA_CRYPT_MIN_ROUNDS and SHA_CRYPT_MAX_ROUNDS are both unspecified,
use SHA_ROUNDS_DEFAULT.
Previously, the code fell through, calling shadow_random(-1, -1). This
ultimately set rounds = (unsigned long) -1, which ends up being a very
large number! This then got capped to SHA_ROUNDS_MAX later in the
function.
The new behavior matches BCRYPT_get_salt_rounds().
Bug: https://bugs.gentoo.org/808195
Fixes: https://github.com/shadow-maint/shadow/issues/393
There's a better way to do this, and I hope to clean that up,
but this fixes out of tree builds for me right now.
Closes#386
Signed-off-by: Serge Hallyn <serge@hallyn.com>
In 9eb191edc4 I included a free() that
frees the members variable, which in turn causes the comma_to_list()
function to return an array of empty elements. The array variable holds
a list of pointers that point to offsets of the members variable. When
the function succeeds freeing members variable causes the elements of
the array variable to point to an empty string.
This is causing several regressions in our internal testing environment.
So, I'm reverting the change.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Most Linux distributions, including Fedora and RHEL 8, are shipping
with libxcrypt >= 4.0.
Since that version of libxcrypt the provided family of crypt_gensalt()
functions are able to use automatic entropy drawn from secure system
ressources, like arc4random(), getentropy() or getrandom().
Anyways, the settings generated by crypt_gensalt() are always
guaranteed to works with the crypt() function.
Using crypt_gensalt() is also needed to make proper use of newer
hashing methods, like yescrypt, provided by libxcrypt.
Signed-off-by: Björn Esser <besser82@fedoraproject.org>
In a previous commit we introduced /dev/urandom as a source to obtain
random bytes from. This may not be available on all systems, or when
operating inside of a chroot.
Almost all systems provide functions to obtain random bytes from
secure system ressources. Thus we should prefer to use these, and
fall back to /dev/urandom, if there is no such function present, as
a last resort.
Signed-off-by: Björn Esser <besser82@fedoraproject.org>
