Commit Graph

2965 Commits

Author SHA1 Message Date
Serge Hallyn
aa15bc445e libsubid: include alloc.h
Fixes: efbbcade43: Use safer allocation macros
Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-02-24 21:27:02 -06:00
Serge Hallyn
9a9e163e71 run_some: log stderr
Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-02-24 20:11:14 -06:00
Vinícius dos Santos Oliveira
05e2adf509 Validate fds created by the user
write_mapping() will do the following:

openat(proc_dir_fd, map_file, O_WRONLY);

An attacker could create a directory containing a symlink named
"uid_map" pointing to any file owned by root, and thus allow him to
overwrite any root-owned file.
2023-02-24 16:20:57 -06:00
Serge Hallyn
7ff33fae6f get_pidfd_from_fd: return -1 on error, not 0
Fixes: 6974df39a: newuidmap and newgidmap: support passing pid as fd
Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-02-24 13:54:54 -06:00
Serge Hallyn
17efd59252 g-h-a workflow: workaround
Skip updating grub packages that are currently breaking
apt-get dist-upgrade.

Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-02-24 13:27:57 -06:00
Serge Hallyn
e4e3212122 Fix regression in some translation strings
Fixes: d80df2c8a: Update translation
Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-02-24 12:56:05 -06:00
Iker Pedrosa
3b3d3e5cd4 lib: bit_ceil_wrapul(): stop recursion
It should call bit_ceilul() instead of itself.

Fixes: 0712b236c3 ("Add bit manipulation functions")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-02-24 12:44:14 -06:00
Iker Pedrosa
21d88b4525 lib: define ULONG_WIDTH if non-existent
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
2023-02-24 12:44:14 -06:00
maqi
d80df2c8a0 Update translation 2023-02-24 12:41:50 -06: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
7668f77439 Fix use-after-free of pointer after realloc(3)
We can't use a pointer that was input to realloc(3), nor any pointers
that point to reallocated memory, without making sure that the memory
wasn't moved.  If we do, the Behavior is Undefined.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -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
6e58c12752 libmisc: Add safer allocation macros
This macros have several benefits over the standard functions:

-  The type of the allocated object (not the pointer) is specified as an
   argument, which improves readability:
   -  It is directly obvious what is the type of the object just by
      reading the macro call.
   -  It allows grepping for all allocations of a given type.

   This is admittedly similar to using sizeof() to get the size of the
   object, but we'll see why this is better.

-  In the case of reallocation macros, an extra check is performed to
   make sure that the previous pointer was compatible with the allocated
   type, which can avoid some mistakes.

-  The cast is performed automatically, with a pointer type derived from
   the type of the object.  This is the best point of this macro, since
   it does an automatic cast, where there's no chance of typos.

   Usually, programmers have to decide whether to cast or not the result
   of malloc(3).  Casts usually hide warnings, so are to be avoided.
   However, these functions already return a void *, so a cast doesn't
   really add much danger.  Moreover, a cast can even add warnings in
   this exceptional case, if the type of the cast is different than the
   type of the assigned pointer.  Performing a manual cast is still not
   perfect, since there are chances that a mistake will be done, and
   even ignoring accidents, they clutter code, hurting readability.
   And now we have a cast that is synced with sizeof.

-  Whenever the type of the object changes, since we perform an explicit
   cast to the old type, there will be a warning due to type mismatch in
   the assignment, so we'll be able to see all lines that are affected
   by such a change.  This is especially important, since changing the
   type of a variable and missing to update an allocation call far away
   from the declaration is easy, and the consequences can be quite bad.

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
f332379ea0 Use xreallocarray() instead of its pattern
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Alejandro Colomar
190a702225 Use reallocarrayf() instead of its pattern
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
727275a027 Use xcalloc(3) instead of its pattern
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Alejandro Colomar
d81506de1e libmisc: Add safer allocation functions
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
Alejandro Colomar
a578617cc0 Use calloc(3) instead of its pattern
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Alejandro Colomar
1aa22c1467 Use reallocarray(3) instead of its pattern
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Alejandro Colomar
45c0003e53 Use reallocf(3) instead of its pattern
In addition, don't set local variables just before return.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Alejandro Colomar
56e4842db0 malloc(3) already sets errno to ENOMEM
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Alejandro Colomar
0e1d017993 Rely on realloc(NULL, ...) being equivalent to malloc(...)
This is guaranteed by ISO C.  Now that we require ISO C (and even POSIX)
to compile, we can simplify this code.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-23 20:28:43 -06:00
Alejandro Colomar
5c5dc75641 libmisc: agetpass(): Fix bug detecting truncation
On 2/19/23 18:09, David Mudrich wrote:
> I am working on a RAM based Linux OS from source, and try to use
> latest versions of all software.  I found shadow needs libbsd's
> readpassphrase(3) as superior alternative to getpass(3).  While
> considering if I a) include libbsd, or include libbsd's code of
> readpassphrase(3) into shadow, found, that libbsd's readpassphrase(3)
> never returns \n or \r
> <https://cgit.freedesktop.org/libbsd/tree/src/readpassphrase.c>
> line 122, while agetpass() uses a check for \n in agetpass.c line 108.
> I assume it always fails.

Indeed, it always failed.  I made a mistake when writing agetpass(),
assuming that readpassphrase(3) would keep newlines.

>
> I propose a check of len == PASS_MAX - 1, with false positive error for
> exactly PASS_MAX - 1 long passwords.

Instead, I added an extra byte to the allocation to allow a maximum
password length of PASS_MAX (which is the maximum for getpass(3), which
we're replacing.

While doing that, I notice that my previous implementation also had
another bug (minor): The maximum password length was PASS_MAX - 1
instead of PASS_MAX.  That's also fixed in this commit.

Reported-by: David Mudrich <dmudrich@gmx.de>
Fixes: 155c9421b9 ("libmisc: agetpass(), erase_pass(): Add functions for getting passwords safely")
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-20 12:16:01 +01:00
Martin Kletzander
baae5b4a06 find_new_[gu]id(): Skip over IDs that are reserved for legacy reasons
Some programs don't support `(uint16_t) -1` or `(uint32_t) -1` as user
or group IDs.  This is because `-1` is used as an error code or as an
unspecified ID, e.g. in `chown(2)` parameters, and in the past, `gid_t`
and `uid_t` have changed width.  For legacy reasons, those values have
been kept reserved in programs today (for example systemd does this; see
the documentation in the link below).

This should not be confused with catching overflow in the ID values,
since that is already caught by our ERANGE checks.  This is about not
using reserved values that have been reserved for legacy reasons.

Link: <https://systemd.io/UIDS-GIDS/>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
2023-02-20 12:10:02 +01:00
Samanta Navarro
0dfeb9e674 Fix comments
These comments should indicate which functions they really wrap.
An alternative would be to remove the line completely to avoid
future copy&paste mistakes.

Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
2023-02-16 13:23:08 -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
b8ea76ba72 Fix typo
It should be "if" not "is".

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
709e6b4497 Add stpecpy()
strncat(3), strlcpy(3), and many other functions are often misused for
catenating strings, when they should never be used for that.  strlcat(3)
is good.  However, there's no equivalent to strlcat(3) similar to
snprintf(3).  Let's add stpecpy(), which is similar to strlcat(3), but
it is also the only function compatible with stpeprintf(), which makes
it more useful than strlcat(3).

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-16 11:29:33 +01:00
Alejandro Colomar
e0e9e57a72 Add mempcpy(3)
We'll use it for implementing stpecpy(), and may be interesting to have
it around.

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
Alejandro Colomar
7e213cfb50 Add stpeprintf()
[v]stpeprintf() are similar to [v]snprintf(3), but they allow chaining.
[v]snprintf(3) are very dangerous for catenating strings, since the
obvious ways to do it invoke Undefined Behavior, and the ways that avoid
UB are very error-prone.

Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-16 11:29:33 +01:00
Alejandro Colomar
a187ad8e9e agetpass.c: Use SPDX tags
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
5da8388fc6 ttytype(): Fix race
The intention of the code is just to not report an error message when
'typefile' doesn't exist.  If we call access(2) and then fopen(2),
there's a race.  It's not a huge problem, and the worst thing that can
happen is reporting an error when the file has been removed after
access(2).  It's not a problem, but we can fix the race and at the same
time clarify the intention of not warning about ENOENT and also remove
one syscall.  Seems like a win-win.

Suggested-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-09 10:03:03 -06: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
Serge Hallyn
66daa74232 run on github runner 2023-02-09 09:55:04 -06:00
Serge Hallyn
8728bd87ed tests: print default timeout message to stderr
Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-02-09 09:55:04 -06:00
Serge Hallyn
6a51e6893e use self-hosted runner for testsuite
Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-02-09 09:55:04 -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
b34387745a lib/defines.h: Add NORETURN attribute macro
We could use the standard (C11) _Noreturn qualifier, but it will be
deprecated in C23, and replaced by C++'s [[noreturn]], which is
compatible with the GCC attribute, so let's directly use the attribute,
and in the future we'll be able to switch to [[]].

Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-08 22:01:01 -06:00
Alejandro Colomar
e762ab8b54 Assume getutent(3) exists (remove dead code)
Recently, we removed support for 'struct utmpx'.  We did it because utmp
and utmpx are identical, and while POSIX specifies utmpx (and not utmp),
GNU/Linux documentation seems to favor utmp.  Also, this project
defaulted to utmp, so changing to utmpx would be more dangerous than
keeping old defaults, even if it's supposed to be the same.

Now, I just found more code that didn't make much sense: lib/utent.c
provides definitions for getutent(3) and friends in case the system
doesn't provide them, but we don't provide prototypes for those
definitions, so code using the functions would have never compiled.

Let's just remove these definitions as dead code.

Fixes: 3be7b9d75a ("Remove traces of utmpx")
Fixes: 170b76cdd1 ("Disable utmpx permanently")
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-08 17:21:34 +01: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
82480995b4 Remove unused function: gr_append_member()
Reported-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
2023-02-03 22:03:38 -06:00
Serge Hallyn
1058872a0b Improve TTYGROUP description in login.defs manpage
Closes #457

The existing prose was confusing, or simply wrong.  Make it clear
that only the group ownership of the tty is affected, and how.
Also move the paragraph about defaults after the discussion of
acceptable TTYGROUPs, as this seems more natural.

Signed-off-by: Serge Hallyn <serge@hallyn.com>
2023-02-02 22:03:45 -06:00