Re: [PATCH v5 0/4] landlock: truncate support

["Mickaël Salaün" <mic@digikod.net>]

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).