Commit Graph

1207 Commits

Author SHA1 Message Date
pepe
1f0ac29e17 :( 2023-05-28 03:50:23 +00:00
Martin Kletzander
8665fe1957 usermod: Small optimization using memmove for password unlock
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
2023-05-26 15:14:02 -05:00
Alejandro Colomar
e3b7058110 Reorder logic to improve comprehensibility
-  Don't else after return or fail_exit().
-  Prefer == over != (negated logic is more complex to think about it).
-  Reduce nesting when reasonable.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-05-25 21:22:08 -05:00
Alejandro Colomar
5b117d5526 newusers: Fail early
There's no reason to report all errors.  Bail out at the first one,
which is simpler.

Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-05-25 21:22:08 -05:00
Alejandro Colomar
1957c8c881 newusers: Add missing error handling
Some errors were being reported in stderr, but then they weren't really
being treated as errors.

If mkdir(2) for EEXIST, it's possible that the sysadmin pre-created the
user dir; don't fail.  However, let's keep a log line, for having some
notice that it happened.

Also, run chmod(2) if mkdir(2) failed for EEXIST (so transform the
'else if' into an 'if').

Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-05-25 21:22:08 -05:00
Samanta Navarro
a116e20c76 su: Prevent stack overflow in check_perms
This is no real world security fix.

The overflow could occur if too many layered subsystems are encountered
because the function check_perms calls itself recursively.

It would already take a misconfigured system for this to achieve it.

Use an iterative approach by calling the do_check_perms in a loop
instead of calling itself recursively.

As a side note: At least GCC 13 optimized this code and already uses
a jmp in its assembler code. I could only see the stack overflow by
activating address sanitizer which prevented the optimization.

Co-developed-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-25 08:25:42 -05:00
Tobias Stoeckmann
8175b1532e Plug econf memory leaks
You can see the memory leaks with address sanitizer if shadow is
compiled with `--enable-vendordir=/usr/etc`.

How to reproduce:

1. Prepare a custom shell file as root
```
mkdir -p /etc/shells.d
echo /bin/myshell > /etc/shells.d/custom
```

2. Run chsh as regular user
```
chsh
```

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
2023-05-19 08:02:24 -05:00
Samanta Navarro
7321ceaf69 chsh: Verify that login shell path is absolute
The getusershell implementation of musl returns every line within the
/etc/shells file, which even includes comments. Only consider absolute
paths for login shells.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-18 16:03:41 +02:00
Samanta Navarro
666468cc36 Remove some static char arrays
Some strings are first written into static char arrays before passed to
functions which expect a const char pointer anyway.

It is easier to pass these strings directly as arguments.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-11 11:05:29 -05:00
Samanta Navarro
7109b7c066 login_prompt: Simplify login_prompt API
The only user of login_prompt is the login tool. This implies that the
first argument is always the same.

It is much easier to verify printf's format string and its argument if
both are next to each other.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-05-03 07:54:28 -05:00
ed neville
4e1f674c41 run_parts for groupadd and groupdel
run_parts currently exists in useradd and userdel, this commit mirrors
the functionality with groupadd and groupdel

Hook for group{add,del} to include killing processes that have group
membership that would no longer exist to avoid membership ID reuse.
2023-04-26 17:38:24 -05:00
Samanta Navarro
7b686d8bd6 newgrp/useradd: always set SIGCHLD to default
The tools newgrp and useradd expect waitpid to behave as described in
its manual page. But the notes indicate that if SIGCHLD is ignored,
waitpid behaves differently.

A user could set SIGCHLD to ignore before starting newgrp through exec.
Children of newgrp would not become zombies and their PIDs could be
reassigned before newgrp could call kill with the child pid and SIGCONT.

The useradd tool is not installed setuid, but I have added the default
there as well (copied from vipw).

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-04-26 08:27:45 -05:00
Christian Göttsche
c80788a3ac useradd/usermod: add --selinux-range argument
Add a command line argument to useradd(8) and usermod(8) to specify the
MLS range for a SELinux user mapping.

Improves: #676
2023-04-19 09:19:19 +02:00
Michael Vetter
b44d7f78e0 Fix typo in groupadd usage 2023-03-31 16:04:42 +02:00
Mike Gilbert
bd2d0079c9 usermod: respect --prefix for --gid option
The --gid option accepts a group name or id. When a name is provided, it
is resolved to an id by looking up the name in the group database
(/etc/group).

The --prefix option overides the location of the passwd and group
databases. I suspect the --gid option was overlooked when wiring up the
--prefix option.

useradd --gid already respects --prefix; this change makes usermod
behave the same way.

Fixes: b6b2c756c9
Signed-off-by: Mike Gilbert <floppym@gentoo.org>
2023-03-29 09:05:23 +02:00
Alejandro Colomar
4c210a29bc Fix su(1) silent truncation
*  src/su.c (check_perms): Do not silently truncate user name.

Reported-by: Paul Eggert <eggert@cs.ucla.edu>
Co-developed-by: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-03-28 13:00:38 +02:00
Serge Hallyn
6974df39a7 newuidmap and newgidmap: support passing pid as fd
Closes #635

newuidmap and newgidmap currently take an integner pid as
the first argument, determining the process id on which to
act.  Accept also "fd:N", where N must be an open file
descriptor to the /proc/pid directory for the process to
act upon.  This way, if you

exec 10</proc/99
newuidmap fd:10 100000 0 65536

and pid 99 dies and a new process happens to take pid 99 before
newuidmap happens to do its work, then since newuidmap will use
openat() using fd 10, it won't change the mapping for the new
process.

Example:

// terminal 1:
serge@jerom ~/src/nsexec$ ./nsexec -W -s 0 -S 0 -U
about to unshare with 10000000
Press any key to exec (I am 129176)

// terminal 2:
serge@jerom ~/src/shadow$ exec 10</proc/129176
serge@jerom ~/src/shadow$ sudo chown root src/newuidmap src/newgidmap
serge@jerom ~/src/shadow$ sudo chmod u+s src/newuidmap
serge@jerom ~/src/shadow$ sudo chmod u+s src/newgidmap
serge@jerom ~/src/shadow$ ./src/newuidmap fd:10 0 100000 10
serge@jerom ~/src/shadow$ ./src/newgidmap fd:10 0 100000 10

// Terminal 1:
uid=0(root) gid=0(root) groups=0(root)

Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-02-24 12:35:49 -06:00
Alejandro Colomar
efbbcade43 Use safer allocation macros
Use of these macros, apart from the benefits mentioned in the commit
that adds the macros, has some other good side effects:

-  Consistency in getting the size of the object from sizeof(type),
   instead of a mix of sizeof(type) sometimes and sizeof(*p) other
   times.

-  More readable code: no casts, and no sizeof(), so also shorter lines
   that we don't need to cut.

-  Consistency in using array allocation calls for allocations of arrays
   of objects, even when the object size is 1.

Cc: Valentin V. Bartenev <vbartenev@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Alejandro Colomar
191f04f7dc Use *array() allocation functions where appropriate
This prevents overflow from multiplication.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Alejandro Colomar
881c1d63a1 libmisc: Move xmalloc.c to alloc.c
We'll expand the contents in a following commit, so let's move the file
to a more generic name, have a dedicated header, and update includes.

Signed-off-by: Alejandro Colomar <alx@kernel.org>

Use the new header for xstrdup()

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Samanta Navarro
c53b36fe85 Fix grammar
Use proper grammar (third-person singular).

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-02-16 13:23:08 -06:00
Samanta Navarro
d5d1932370 Fix typos
It is a user, not an user.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-02-16 13:23:08 -06:00
Alejandro Colomar
5956cea1d1 Use stpecpy() where appropriate
This function simplifies the calculation of the bounds of the buffer for
catenating strings.  It would also reduce error checking, but we don't
care about truncation in this specific code. :)

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-16 11:29:33 +01:00
Alejandro Colomar
8a9285aacb Remove unnecessary NUL terminators
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>
2023-02-16 11:29:33 +01:00
Alejandro Colomar
46610792e9 Use stpeprintf() where appropriate
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>
2023-02-16 11:29:33 +01:00
Martin Kletzander
ca9e309d30 Fix VPATH build
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>
2023-02-13 10:01:17 +01:00
Alejandro Colomar
bddcd9b095 Remove superfluous casts
-  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>
2023-02-09 10:03:03 -06:00
Alejandro Colomar
416707b087 Use the noreturn attribute, rather than comments
This will allow the compiler to understand these functions better.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-08 22:01:01 -06:00
Alejandro Colomar
f301a4ca19 Handle reallocf(3) errors
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-03 22:03:38 -06:00
Alejandro Colomar
0ec157d579 Fix memory leaks by replacing realloc(3) with reallocf(3)
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-03 22:03:38 -06:00
Alejandro Colomar
62172f6fb5 Call NULL by its name
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>
2023-02-02 13:08:30 -06:00
Samanta Navarro
8e0ad48c21 Prevent out of boundary access
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>
2023-02-01 15:47:35 -06:00
Stefan Schubert
a27d5c51f1 Supporting vendor given -shells- configuration file 2023-01-26 22:45:32 -06:00
Samanta Navarro
b312bc0b4d Fix typos
Typos found with codespell.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-01-26 22:44:39 -06:00
Christian Göttsche
89be7c0465 Provide strlcpy declaration
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
2023-01-25 12:31:17 +01:00
Christian Göttsche
c99d8d0a08 Avoid comparisons of different signs
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))) {
          |                                        ^~
2023-01-25 12:31:17 +01:00
Christian Göttsche
43508ac476 Drop redundant declaration
environ is exported in <unistd.h>.

    env.c:29:15: warning: redundant redeclaration of 'environ' [-Wredundant-decls]
       29 | extern char **environ;
          |               ^~~~~~~
    login.c:92:15: warning: redundant redeclaration of ‘environ’ [-Wredundant-decls]
       92 | extern char **environ;
          |               ^~~~~~~
    sulogin.c:40:15: warning: redundant redeclaration of ‘environ’ [-Wredundant-decls]
       40 | extern char **environ;
          |               ^~~~~~~
    newgrp.c:32:15: warning: redundant redeclaration of ‘environ’ [-Wredundant-decls]
       32 | extern char **environ;
          |               ^~~~~~~
2023-01-25 12:31:17 +01:00
Alejandro Colomar
b2bed465e8 Use getnameinfo(3) instead of our own equivalent
I didn't know getnameinfo(3) existed, so I implemented it, or something
similar to it called inet_sockaddr2str().  Let's use the standard API.

Link: <https://inbox.sourceware.org/libc-alpha/0f25d60f-f183-b518-b6c1-6d46aa63ee57@gmail.com/T/>
Link: <https://stackoverflow.com/a/42190913/6872717>
Link: <https://github.com/shadow-maint/shadow/pull/617>
Link: <https://software.codidact.com/posts/287748>
Cc: Zack Weinberg <zack@owlfolio.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-01-20 10:23:03 -06:00
SoumyaWind
670cae8348 shadow: Fix can not print full login timeout message
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.
2023-01-12 18:30:32 -06:00
Alejandro Colomar
609c641323 Call inet_sockaddr2str() instead of inet_ntop(3)
To simplify.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-01-02 08:20:43 +01:00
Alejandro Colomar
eec5f9fccc Replace gethostbyname(3) by getaddrinfo(3)
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>
2023-01-02 08:20:43 +01:00
Serge Hallyn
bc18c184e5 chfn: new_fields: fix wrong fields printed
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>
2022-12-23 09:04:02 +01:00
Alejandro Colomar
220b352b70 Use strlcpy(3) instead of its pattern
-  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>
2022-12-22 18:03:39 -06:00
Alejandro Colomar
5d7a3b80e9 Remove USE_SYSLOG preprocessor conditional, which was always defined
Reported-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2022-12-22 11:44:36 +01:00
Alejandro Colomar
350b1e8683 Remove dead code
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2022-12-22 11:44:36 +01:00
Alejandro Colomar
e2df287aad Don't redefine errno(3)
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>
2022-12-22 11:43:29 +01:00
Alejandro Colomar
ed69feaaff Fix typos in length calculations
Link: <https://github.com/shadow-maint/shadow/pull/607>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2022-12-22 10:34:04 +01:00
Alejandro Colomar
06c30450ce Use 'uintmax_t' to print 'gid_t'
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>
2022-12-22 10:34:04 +01:00
Alejandro Colomar
587ce83e3f Fix off-by-one mistakes
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>
2022-12-22 10:34:04 +01:00
Alejandro Colomar
b990b167d4 Cosmetic fixes
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>
2022-12-22 10:31:43 +01:00