On 02/09/2022 18:34, Günther Noack wrote:
On Fri, Sep 02, 2022 at 10:40:57AM +0200, Mickaël Salaün wrote:
On 02/09/2022 08:16, Günther Noack wrote:
On Fri, Sep 02, 2022 at 07:32:49AM +0200, Günther Noack wrote:
On Thu, Sep 01, 2022 at 07:10:38PM +0200, Mickaël Salaün wrote:
Hmm, I think there is an issue with this series. Landlock only enforces
restrictions at open time or when dealing with user-supplied file paths
(relative or absolute).
Argh, ok. That sounds like a desirable property, although it would
mean reworking the patch set.
The use of the path_truncate hook in this series
doesn't distinguish between file descriptor from before the current sandbox
or from after being sandboxed. For instance, if a file descriptor is
received through a unix socket, it is assumed that this is legitimate and no
Landlock restriction apply on it, which is not the case with this series
anymore. It is the same for files opened before the process sandbox itself.
To be able to follow the current semantic, I think we should control the
truncate access at open time (or when dealing with a user-supplied path) but
not on any file descriptor as it is currently done.
OK - so let me try to make a constructive proposal. We have previously
identified a few operations where a truncation happens, and I would
propose that the following Landlock rights should be needed for these:
* truncate() (operating on char *path): Require LL_ACCESS_FS_TRUNCATE
* ftruncate() (operating on fd): No Landlock rights required
* open() for reading with O_TRUNC: Require LL_ACCESS_FS_TRUNCATE
* open() for writing with O_TRUNC: Require LL_ACCESS_FS_WRITE_FILE
Thinking about it again, another alternative would be to require
TRUNCATE as well when opening a file for writing - it would be
logical, because the resulting FD can be truncated. It would also
require people to provide the truncate right in order to open files
for writing, but this may be the logical thing to do.
Another alternative would be to keep the current semantic but ignore file
descriptors from not-sandboxed processes. This could be possible by
following the current file->f_mode logic but using the Landlock's
file->f_security instead to store if the file descriptor was opened in a
context allowing it to be truncated: file opened outside of a landlocked
process, or in a sandbox allowing LANDLOCK_ACCESS_FS_TRUNCATE on the related
path.
I'm not convinced that it'll be worth distinguishing between a FD
opened for writing and a FD opened for writing+truncation. And whether
the FD is open for writing is already tracked by default and
ftruncate() checks that.
That might be a misunderstanding. What I'm proposing is to keep the same
semantic as this fifth patch series, only to keep scoped Landlock
restrictions and propagate them (which is already how Landlock works).
The layout1.truncate tests should work the same except that
test_ftruncate(file_*_fd) will always be allowed because such FD they
are opened before the thread being sandboxed.
I'm having a hard time constructing a scenario where write() should be
permitted on an FD but ftruncate() should be forbidden. It seems that
write() is the more dangerous operation of the two, with more
potential to modify a file to one's liking, whereas the modifications
possible through TRUNCATE are relatively benign?
I don't understand, this is how this fifth series already restrict
truncate. I'm not proposing to change the FD minimal requirement to be
"truncatable", and it would not be possible with the LSM framework anyway.
The opposite scenario (where ftruncate() is permitted and write() is
forbidden) simply can't exist because an FD must already be writable
in order to use ftruncate(). (see man page)
Right, and I'm not proposing to change that. Currently, the kernel
tracks how a FD was opened (e.g. read or write mode). I'm proposing to
add another *complementary* Landlock-specific mode for truncate because
it is a more fine-grained access right with Landlock, hence this patch
series.
Additionally, if we recall previous discussions on the truncate patch
sets, there is the very commonly used creat() syscall (a.k.a. open()
with O_CREAT|O_WRONLY|O_TRUNC), which anyway requires the Landlock
truncate right in many cases. So I still think you can't actually use
LANDLOCK_ACCESS_FS_FILE_WRITE without also providing the
LANDLOCK_ACCESS_FS_TRUNCATE right?
Hmm, that is definitely not a common case, but this series permit that,
see test_truncate(file_t).
In conclusion, I'd be in favor of not tracking the truncate right
separately as a property of an open file descriptor. Does that
rationale sound reasonable?
No, but I think there is a misunderstanding. :)
The idea is first to change hook_file_open() to set
landlock_file(file)->access = LANDLOCK_ACCESS_FS_TRUNCATE if it is
allowed by the policy: current thread being either not in a sandbox, or
in a sandbox that allows truncate.
Then, in hook_path_truncate(), we allow the operation if `file &&
(landlock_file(file)->access & LANDLOCK_ACCESS_FS_TRUNCATE)`. Otherwise,
it there is only a path available (because it comes from truncate(2)),
we (almost) call current_check_access_path(path,
LANDLOCK_ACCESS_FS_TRUNCATE).
