Michael Orlitzky [Sun, 17 Jun 2018 23:43:21 +0000 (19:43 -0400)]
src/libadacl.c: add a special case for the path "." as an argument.
There's a bug (exposed by the most recent test case) in the way the
path "." is handled. Specifically, the dirname() function miscomputes
its parent path as ".", which is clearly not correct.
In this commit, a special case is added for the path ".", and the test
suite passes once more. The implementation is a bit of a hack, however,
and will be improved once the same issue with ".." has been dealt with.
Michael Orlitzky [Thu, 29 Mar 2018 01:59:51 +0000 (21:59 -0400)]
src/libadacl.c: fix a memory leak found by clang-tidy.
There was an error path in apply_default_acl() that was returning
directly rather than jumping to the cleanup function where memory is
freed. Thanks, clang-tidy.
Michael Orlitzky [Thu, 29 Mar 2018 01:41:31 +0000 (21:41 -0400)]
src/libadacl.c: cast two fgetxattr() and fsetxattr() params to size_t.
The two functions fgetxattr() and fsetxattr() take an unsigned "size"
parameter as arguments. We are passing them signed integers that we
happen to know are nonnegative, since we have ruled out the one
possible negative value -- but the compiler doesn't know that. To
avoid a warning from clang, we now cast the parameters to the
(unsigned) size_t type.
Michael Orlitzky [Wed, 28 Mar 2018 03:50:41 +0000 (23:50 -0400)]
Drop the "--no-exec-mask" flag and function parameters.
Nobody needs the "--no-exec-mask" flag, and it's uglying up the
library's API. Sayonara:
* Update the man page:
* Remove all mentions of the flag.
* Update the algorithm description.
* Reword the general description.
* Remove all --no-exec-mask tests.
* Bump the program version in configure.ac.
* Make apply_default_acl() work as if no_exec_mask == false.
* Remove all no_exec_mask function parameters.
* Bump the soname major version in src/Makefile.am.
Michael Orlitzky [Wed, 28 Mar 2018 01:03:01 +0000 (21:03 -0400)]
src/libadacl.c: use asprintf() instead of snprintf() for paths.
When constructing a path, there is an ancient problem: how do you
ensure that your path-name buffer is large enough, and what do you do
if it isn't? The existing solution was to use the PATH_MAX constant
from limits.h, which is often a big number, but need not actually be
defined. If a path exceeded PATH_MAX bytes, we would fail.
Now the GNU/BSD extension asprintf() is used instead. The path-name
buffer is constructed on-the-fly to be as large as necessary, and if
allocation fails, an error is returned. This solution is a little
cleaner, and is not too much less portable considering that we only
work on Linux anyway.
Michael Orlitzky [Wed, 28 Mar 2018 00:23:16 +0000 (20:23 -0400)]
doc: document the apply-default-acl algorithm.
It's nice to have a high-level overview of what the ACL application
actually does, so I have added one to the man page in a new section
titled "ALGORITHM". The manual now also explains how apply-default-acl
differs from the kernel when, for example, you "touch" a file in a
directory with a default ACL.
autotools: replace my busted header checks with something that works.
My existing AC_CHECK_HEADERS checks were failing silently. Oops. I've
now defined my own macro in m4/ac_header_required.m4 that successfully
fails when a required header is missing.
Replace nftw() with manual recursion in apply_default_acl().
The nftw() tree walk worked well for a while; in particular, before we
handled symlinks safely, it was empirically faster than a hand-written
recursive descent. But recently, the very slow safe_open() function
was being called on the path that was passed to apply_default_acl(),
and nftw() fed that function a whole bunch of paths.
The apply_default_acl() function now takes a third "recusive"
parameter, and implements the recursion on its own. This lets us pass
down the old child file descriptor as the new parent file descriptor,
and avoid calling safe_open() more than once when we're operating
recursively. The result is a big speed improvement with --recursive,
tested for example on the Linux kernel source tree.
The hand-written recursion also allows us to fix a lingering exit code
bug. Now --recursive acts as if all of the targets were passed (in the
right order) on the command-line.
The new parameter affects the public API, so in the next release the
library will get a new version. The upside to this is that now it's
easy for other programs to operate recursively, simply by passing
"true" as the third parameter to apply_default_acl().
Update docs and tests for the --recursive exit code.
With the nftw() implementation, there was some bugginess in our exit
code that was both documented and tested. Well now I plan on fixing
that, so the documentation has been updated to state what the exit
code _should_ be, and the tests now check for the correct behavior
(meaning that they fail, for the moment).
The apply_default_acl_ex() function was an "extended" version of the
apply_default_acl() function that, in addition, took a stat structure
pointer to the target path. The extended function was used by nftw(),
which usually has such a stat structure handy. However, the provenance
of that stat structure is not clear: does nftw() obtain it in a safe
way?
Since I don't know the answer to that question, I would rather stat()
the descriptor that I know was obtained safely. Thus there's no reason
to take the pointer as an argument, and then no reason to keep the
extended function around at all.
src/libadacl.c: remove two NULL checks around acl_free() calls.
The acl_free() function will return ACL_ERROR and set errno to EINVAL
if we pass it a null pointer; but aside from that, nothing bad
happens, and removing the checks makes the code marginally cleaner.
src/libadacl.c: rewrite acl_set_entry() as acl_update_entry().
It turns out we only need to update existing entries in our ACLs, to
mask the execute permissions. Since we never need to create new
entries, the name "acl_set_entry" was not quite right. The
new-entry-creation code has been removed from the bottom half of the
function, and it has been renamed to "acl_update_entry".
Michael Orlitzky [Wed, 28 Feb 2018 22:33:17 +0000 (17:33 -0500)]
Eliminate the last bit of pathname usage.
A lot of work has been done recently to make apply-default-acl safe
from symlink and hardlink attacks. A big part of that work was the
recent switch to using file descriptors instead of pathnames; but,
pathnames still lingered in a few places due to a shortcoming in
libacl. Through the use of a new function, acl_copy_xattr(), I've
finally eliminated those last few bits.
The apply_default_acl_ex() function now uses path names only as
arguments to safe_open(), which hopefully is safe. Afterwards, the
file descriptors obtained from safe_open() are used. Thus the hard and
symlink attacks should finally be fixed, modulo a tiny race condition
between safe_open() and fstat() that has no known solution.
These changes rely on the Linux xattr implementation and kill our
portability, but I don't think we ever had any to begin with.
Michael Orlitzky [Mon, 26 Feb 2018 18:27:18 +0000 (13:27 -0500)]
Rename apply_default_acl() to apply_default_acl_ex() and add a wrapper.
The old apply_default_acl() function has a weird second argument that
will usually be NULL for other users of the library. Instead of making
them deal with that design choice, the old apply_default_acl()
function was renamed t apply_default_acl_ex(), and a new
apply_default_acl() was added with no second argument to wrap the
former.
Michael Orlitzky [Mon, 26 Feb 2018 03:11:47 +0000 (22:11 -0500)]
Add safe_open() function to fix symlink traversal in non-terminal components.
The standard library provides lots of ways to avoid symlinks in the
"baz" component of "foo/bar/baz", but very few (i.e. zero) ways to
avoid them in the "bar" component. Of course, they're just as
dangerous in either place, so it would be cool if we could ignore
symlinks entirely.
This commit adds a safe_open() function, which looks just like open()
to the caller, but which starts at the root and calls openat() one
component at-a-time. Thus if you use O_NOFOLLOW, nobody can trick you
with an intermediate component: there are no intermediate components;
it works one at-a-time. This slows things down a bit, but not fatally.
Michael Orlitzky [Mon, 26 Feb 2018 02:47:25 +0000 (21:47 -0500)]
Eliminate the is_path_directory() function.
The is_path_directory() function was only used in two places:
1. To avoid involving nftw when the target is a file.
2. To avoid setting (or trying to) a default ACL on a file.
The first one doesn't really matter, and the correct behavior is
tested, so the additional "is it a directory?" check has been
dropped. In the second case, we still don't want to try to set default
ACLs on files, but now the "is it a directory?" check is done at the
call site, where a stat structure is already available.
With those two uses gone, the function has been removed.
Michael Orlitzky [Mon, 26 Feb 2018 02:35:02 +0000 (21:35 -0500)]
Allow apply_default_acl() to take a stat pointer for its path argument.
When operating recursively, nftw has already called stat() on the path
that it feeds to apply_default_acl(). Rather than re-stat it, we can
now pass a stat struct pointer in as the second argument to
apply_default_acl(). When not operating recursively, you just pass it
NULL instead, and apply_default_acl() will do the fstat as before.
Michael Orlitzky [Mon, 26 Feb 2018 01:11:54 +0000 (20:11 -0500)]
Eliminate the one remaining use of is_directory().
The is_directory() function was called once, in a place where we
already had access to its argument's stat structure. Instead, we now
just use S_ISDIR, and the is_directory() function has met its end.
Michael Orlitzky [Mon, 26 Feb 2018 01:10:04 +0000 (20:10 -0500)]
Move the "or_dir" out of any_can_execute_or_dir().
The any_can_execute_or_dir() function checked two things; whether or
not anyone could execute something, and whether or not that thing was
a directory. It's cleaner to have the "is it directory?" check outside
these days, so this commit renames that function to any_can_execute()
and the one place it's used now checks whether or not the argument is
a directory itself.
Michael Orlitzky [Mon, 26 Feb 2018 01:03:44 +0000 (20:03 -0500)]
Inline the is_hardlink_safe() and is_regular_file() functions.
These two functions were only called in one place, and they were
called right after one another. Both functions merely peek the stat
structure for a file descriptor, so it's easier to stat the thing once
and then just inline the two checks rather than setup/teardown
everything twice.