Implement it as an inline function, and add restrict and ATTR_STRING()
and ATTR_ACCESS() as appropriate.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Implement it as an inline function, and add restrict and ATTR_STRING()
and ATTR_ACCESS() as appropriate.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Implement it as an inline function, and add restrict and ATTR_STRING()
and ATTR_ACCESS() as appropriate.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Adding function check_fds to new file fd.c. The function check_fds
should be called in every setuid/setgid program.
Co-developed-by: Alejandro Colomar <alx@kernel.org>
These functions (e.g., gr_free()), explicitly dereference the pointer
and read the pointee.
The /@out@/ comment, which is (almost) analogous to the
[[gnu::access(write_only, ...)]] attribute, means that the pointee can
be uninitialized, since it won't read it. There's a difference between
/@out@/ and the GCC attribute: the attribute doesn't require that the
call writes to the pointee, while /@out@/ requires that the pointee be
fully initialized after the call, so it _must_ write to it.
A guess of why it was used is that these functions are similar to
free(3), which does not read the memory it frees, and so one would
assume that if it doesn't read, write_only (or equivalents) are good.
That's wrong in several ways:
- free(3) does not read _nor_ write to the memory, so it would
be slightly inappropriate to use write_only with it. It wouldn't be
"wrong", but [[gnu::access(none, ...)]] would be more appropriate.
- Because /@out@/ requires that the call writes to the pointee, it
would be wrong to use it in free(3), which doesn't write to the
pointee.
- Our functions are similar to free(3) conceptually, but they don't
behave like free(3), since they do read the memory (pointee) (and
also write to it), and thus they're actually read_write.
Link: <https://splint.org/manual/manual.html#undefined>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Documentation:
- Correct the comment documenting the function:
write_full() doesn't write "up to" count bytes (which is write(2)'s
behavior, and exactly what this function is designed to avoid), but
rather exactly count bytes (on success).
- While fixing the documentation, take the time to add a man-page-like
comment as in other APIs. Especially, since we'll have to document
a few other changes from this patch, such as the modified return
values.
- Partial writes are still possible on error. It's the caller's
responsibility to handle that possibility.
API:
- In write(2), it's useful to know how many bytes were transferred,
since it can have short writes. In this API, since it either writes
it all or fails, that value is useless, and callers only want to know
if it succeeded or not. Thus, just return 0 or -1.
Implementation:
- Use `== -1` instead of `< 0` to check for write(2) syscall errors.
This is wisdom from Michael Kerrisk. This convention is useful
because it more explicitly tells maintainers that the only value
which can lead to that path is -1. Otherwise, a maintainer of the
code might be confused to think that other negative values are
possible. Keep it simple.
- The path under `if (res == 0)` was unreachable, since the loop
condition `while (count > 0)` precludes that possibility. Remove the
dead code.
- Use a temporary variable of type `const char *` to avoid a cast.
- Rename `res`, which just holds the result from write(2), to `w`,
which more clearly shows that it's just a very-short-lived variable
(by it's one-letter name), and also relates itself more to write(2).
I find it more readable.
- Move the definition of `w` to the top of the function. Now that the
function is significantly shorter, the lifetime of the variable is
clearer, and I find it more readable this way.
Use:
- Also use `== -1` to check errors.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Remove `utmp` structure as an argument and include its logic inside the
function. This will help remove any reference to utmp from login.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
The functionality from this function is related to utmp. Restrict access
to `setutmp()` to the same file.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
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>
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>
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>
This API is similar to arc4random_uniform(3). However, for an input of
0, this function is equivalent to csrand(), while arc4random_uniform(0)
returns 0.
This function will be used to reimplement csrand_interval() as a wrapper
around this one.
The current implementation of csrand_interval() doesn't produce very good
random numbers. It has a bias. And that comes from performing some
unnecessary floating-point calculations that overcomplicate the problem.
Looping until the random number hits within bounds is unbiased, and
truncating unwanted bits makes the overhead of the loop very small.
We could reduce loop overhead even more, by keeping unused bits of the
random number, if the width of the mask is not greater than
ULONG_WIDTH/2, however, that complicates the code considerably, and I
prefer to be a bit slower but have simple code.
BTW, Björn really deserves the copyright for csrand() (previously known
as read_random_bytes()), since he rewrote it almost from scratch last
year, and I kept most of its contents. Since he didn't put himself in
the copyright back then, and BSD-3-Clause doesn't allow me to attribute
derived works, I won't add his name, but if he asks, he should be put in
the copyright too.
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Björn Esser <besser82@fedoraproject.org>
Cc: Yann Droneaud <ydroneaud@opteya.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Sam James <sam@gentoo.org>
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>
Clang doesn't implement this attribute and reports an error. Work
around it by hiding it in a macro that will be empty in clang.
Reported-by: 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>
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>
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;
| ^
