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>
Login timed out message prints only first few bytes when write is immediately followed by exit.
Calling exit from new handler provides enough time to display full message.
Commit 90424e7c ("Don't warn when failed to open /etc/nsswitch.conf")
removed the logging for failing to read /etc/nsswitch.conf to reduce the
noise in the case the file does not exists (e.g. musl based systems).
Reintroduce a warning if /etc/nsswitch.conf exists but we failed to read
it (e.g. permission denied).
Improves: 90424e7c ("Don't warn when failed to open /etc/nsswitch.conf")
gethostbyname(3) was removed in POSIX.1-2008. It has been obsoleted,
and replaced by getaddrinfo(3), which is superior in several ways:
- gethostbyname(3) is not reentrant. There's a GNU extension,
gethostbyname_r(3) which is reentrant, but it's not likely to be
standardized for the following reason. And we don't care too much
about this point either.
- gethostbyname(3) only supports IPv4, but getaddrinfo(3) supports both
IPv4 and IPv6 (and may support other address families in the future).
We don't care about reentrancy, so for keeping the code simple (i.e.,
not touch call site to add code to free(3) an allocated buffer), I added
a static buffer for inet_ntop(3). We could address that in the future,
but I don't think it's worth it.
BTW, we also replace inet_ntoa(3) by inet_ntop(3), as a consequence of
using getaddrinfo(3). inet_ntoa(3) is also marked as deprecated, but
that deprecation seems to have been documented only in the manual page,
and POSIX doesn't mark it as deprecated. The deprecation notice goes
back to when the inet_ntop(3) manual page was added by Sam Varshavchik
to the Linux man-pages in version 1.30 (year 2000).
So, this, apart from updating the code to POSIX.1-2008, is also adding
support for IPv6 :) Although, probably many other parts of the code are
written for IPv4 only, so I wouldn't yet claim support for it.
A few notes:
- I didn't check the return value of inet_ntop(3), since it can't fail
for the given input:
- EAFNOSUPPORT: We only call it with AF_INET and AF_INET6.
- ENOSPC: We calculate the size of the buffer to be wide enough:
MAX(INET_ADDRSTRLEN, INET6_ADDRSTRLEN) so it always fits.
Cc: Dave Hagewood <admin@arrowweb.com>
Cc: Sam Varshavchik
Cc: Jakub Jelinek <jakub@redhat.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Systems can suffer power interruptions whilst .lock files are in /etc,
preventing scripts and other automation tools from updating shadow's
files which persist across boots.
This commit replaces that mechanism with file locking to avoid problems
of power interruption/crashing.
Minor tweak to groupmems man page, requested by 'xx' on IRC.
Signed-off-by: ed neville <ed@s5h.net>
When the caller may not change the room number, work phone, or
home number, then rather than prompting for the new one it will
print the existing one. But due to a typo it printed the full name
in place of each of those.
Fix the fields being printed.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
- 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 has been a requirement since at least C90, according to tm(3type).
Link: <https://github.com/shadow-maint/shadow/pull/600>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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>
This is shorter to write than 'unsigned long int', so we can collapse
some lines. It is guaranteed by C99.
Link: <https://github.com/shadow-maint/shadow/pull/607>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The buffers have a size of 512 (see xmalloc() above), which is what
snprintf(3) expects.
Link: <https://github.com/shadow-maint/shadow/pull/607>
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>
- USER_NAME_MAX_LENGTH was being calculated in terms of utmpx. Do it
in terms of utmp.
- Remove utmpx support from the whishlist.
- Remove unused tests about utmpx members.
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>
We already made that assumption in commit b47aa1e9aa. While the
header is not required by POSIX (it is an XSI extension), it is defined
in systems that are of interest to this project (GNU/Linux).
Fixes: b47aa1e9aa ("Assume <utmpx.h> exists")
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We don't know what it was for. If anyone cares, it's in git history.
In my distro, there seem to be no traces of it:
alx@debian:~$ apt-file find pwdauth
alx@debian:~$
Link: <https://codesearch.debian.net/search?q=pwdauth&literal=1>
Link: <https://github.com/shadow-maint/shadow/pull/612>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
When useradd sends its ADD_USER event, it is filling in the id field. This is not yet written to disk. When auditd sees the event and the log format is enriched, auditd tries to lookup the user name but it does not exist. This causes the event to never be resolvable since ausearch relies on the lookup information attached by auditd.
The fix is to not send the id information for any event until after close_files() is called. Just the acct field is all that is
Patch by Steve Grubb (afaik).
Reported at https://bugzilla.redhat.com/show_bug.cgi?id=1713432
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>
In a previous commit, we made USE_TERMIOS unconditionally defined.
Let's just remove it, and remove the condition everywhere.
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>
The definition for this macro was removed in a previous commit.
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>
It is required by POSIX.1-2001.
Link: <https://github.com/shadow-maint/shadow/pull/600>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It is required by POSIX.1-2001.
Link: <https://github.com/shadow-maint/shadow/pull/600>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
