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>
Moreover, include checks to prevent writing entries longer than the
length limit.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1422497
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>
This tweaks the database locking logic so that failures in the
link-checking paths are more detailed.
The rationale for this is that I've experienced a non-deterministic
bug which seems to be coming from this logic, and I'd like to get
more details about the actual failing condition.
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.)
IDs already populate /etc/subuid and /etc/subgid files so it's necessary
not only to check for the owner name but also for the owner ID of a
given range.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2093311
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
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>
If /etc/nsswitch.conf doesn't exist podman crashes because shadow_logfd
is NULL. In order to avoid that load the log file descriptor with the
log_get_logfd() helper function.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2038811
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Systems on which <sys/time.h> conflicted with <time.h> are obsolete.
This macro has been marked as obsolete by autoconf documentation.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
As autoconf documentation says, this macro is obsolescent, as no
current systems have the bug in S_ISDIR, S_ISREG, etc..
The affected systems were Tektronix UTekV, Amdahl UTS, and
Motorola System V/88.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
GNU autoconf documentation marks this macro as obsolescent, as
current systems are compatible with POSIX.
Simplify code to unconditionally include <sys/wait.h>, and don't
redefine WIFEXITSTATUS() and WIFEXITED(), since they are mandated
by POSIX.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
POSIX.1-2001 defines 'struct dirent' in <dirent.h>. It replaces
the old 'struct direct' found in BSDs. All of the systems that I
checked (including FreeBSD, NetBSD, and OpenBSD), now provide
<dirent.h> with 'struct dirent', as mandated by POSIX.
Since autoconf first checks <dirent.h> and only if it's missing it
checks other header files, it's clear that it will always find
<dirent.h>, so let's simplify.
GNU autoconf documentation declares this macro as obsolescent, and
acknowledges that all current systems with directory libraries
have <dirent.h>:
<https://www.gnu.org/software/autoconf/manual/autoconf-2.70/html_node/Particular-Headers.html>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
Compilers are allowed to and do optimize memset(3) calls away for
pointers not accessed in the future. Since the memzero wrappers purpose
is exactly to unconditionally override memory (e.g. for stored
passwords) do not implement via regular memset(3), but via either
memset_s(3), explicit_bzero(3) or a hand written implementation using
volatile pointers.
See https://wiki.sei.cmu.edu/confluence/display/c/MSC06-C.+Beware+of+compiler+optimizations
run_part() and run_parts() do not modify their directory, name and
action arguments.
Also include the header in the implementation to provide the prototypes.
useradd.c:2495:59: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
2495 | if (run_parts ("/etc/shadow-maint/useradd-pre.d", (char*)user_name,
| ^
useradd.c:2495:24: warning: passing argument 1 of ‘run_parts’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
2495 | if (run_parts ("/etc/shadow-maint/useradd-pre.d", (char*)user_name,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from useradd.c:45:
../lib/run_part.h:2:22: note: expected ‘char *’ but argument is of type ‘const char *’
2 | int run_parts (char *directory, char *name, char *action);
| ~~~~~~^~~~~~~~~
useradd.c:2496:25: warning: passing argument 3 of ‘run_parts’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
2496 | "useradd")) {
| ^~~~~~~~~
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.
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>
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>
memset(3) has been in standard C since C89. It is also in
POSIX.1-2001, in SVr4, and in 4.3BSD (see memset(3) and memset(3p)).
We can assume that this function is always available.
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>
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>
The build was failing with duplicate symbol errors with -fno-common.
This is the default in GCC 10 and later, and explicitly enabled in some
distributions to catch problems like this. There were two causes:
- Prog and shadow_logfd were defined in a header file that was included
in multiple other files. Fix this by defining them once in
shadowlog.c, and having extern declarations in the header.
- Most of the tools (except id/nologin) also define a Prog variable,
which is not intended to alias the one in the library. Fix
this by renaming Prog in the library to shadow_progname, which also
matches the new accessor functions for it.
Add an additional NULL check condition in spw_free() and pw_free() to
avoid freeing an already empty pointer.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
During shadowtcb_move() the directory is temporarily changed to be
owned by root:root with permissions 0700. After the change is done,
the ownership and permissions were supposed to be restored. The
call for chown() was there, but the chmod() call was missing. This
resulted in the broken TCB functionality. The added chmod() fixes
the issue.
Close the selabel handle to update the file_context. This means that the
file_context will be remmaped and used by selabel_lookup() to return
the appropriate context to label the home folder.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1993081
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
