On 3/4/25 19:49, Mickaël Salaün wrote:
On Tue, Mar 04, 2025 at 01:13:01AM +0000, Tingmao Wang wrote:
[...]
+ /**
+ * @cookie: Opaque identifier to be included in the response.
+ */
+ __u32 cookie;
I guess we could use a __u64 index counter per layer instead. That
would also help to order requests if they are treated by different
supervisor threads.
I don't immediately see a use for ordering requests (if we get more than
one event at once, they are coming from different threads anyway so
there can't be any dependencies between them, and the supervisor threads
can use timestamps), but I think making it a __u64 is probably a good
idea regardless, as it means we don't have to do some sort of ID
allocation, and can just increment an atomic.
+};
+
+struct landlock_supervise_event {
+ struct landlock_supervise_event_hdr hdr;
+ __u64 access_request;
+ __kernel_pid_t accessor;
+ union {
+ struct {
+ /**
+ * @fd1: An open file descriptor for the file (open,
+ * delete, execute, link, readdir, rename, truncate),
+ * or the parent directory (for create operations
+ * targeting its child) being accessed. Must be
+ * closed by the reader.
+ *
+ * If this points to a parent directory, @destname
+ * will contain the target filename. If @destname is
+ * empty, this points to the target file.
+ */
+ int fd1;
+ /**
+ * @fd2: For link or rename requests, a second file
+ * descriptor for the target parent directory. Must
+ * be closed by the reader. @destname contains the
+ * destination filename. This field is -1 if not
+ * used.
+ */
+ int fd2;
Can we just use one FD but identify the requested access instead and
send one event for each, like for the audit patch series?
I haven't managed to read or test out the audit patch yet (I will do),
but I think having the ability to specifically tell whether the child is
trying to move / rename / create a hard link of an existing file, and
what it's trying to use as destination, might be useful (either for
security, or purely for UX)?
For example, imagine something trying to link or move ~/.ssh/id_ecdsa to
/tmp/innocent-tmp-file then read the latter. The supervisor can warn the
user on the initial link attempt, and the shenanigan will probably be
stopped there (although still, being able to say "[program] wants to
link ~/.ssh/id_ecdsa to /tmp/innocent-tmp-file" seems better than just
"[program] wants to create a link for ~/.ssh/id_ecdsa"), but even if
somehow this ends up allowed, later on for the read request it could say
something like
[program] wants to read /tmp/innocent-tmp-file
(previously moved from ~/.ssh/id_ecdsa)
Maybe this is a bit silly, but there might be other use cases for
knowing the exact details of a rename/link request, either for
at-the-time decision making, or tracking stuff for future requests?
I will try out the audit patch to see how things like these appears in
the log before commenting further on this. Maybe there is a way to
achieve this while still simplifying the event structure?
+ /**
+ * @destname: A filename for a file creation target.
+ *
+ * If either of fd1 or fd2 points to a parent
+ * directory rather than the target file, this is the
+ * NULL-terminated name of the file that will be
+ * newly created.
+ *
+ * Counting the NULL terminator, this field will
+ * contain one or more NULL padding at the end so
+ * that the length of the whole struct
+ * landlock_supervise_event is a multiple of 8 bytes.
+ *
+ * This is a variable length member, and the length
+ * including the terminating NULL(s) can be derived
+ * from hdr.length - offsetof(struct
+ * landlock_supervise_event, destname).
+ */
+ char destname[];
I'd prefer to avoid sending file names for now. I don't think it's
necessary, and that could encourage supervisors to filter access
according to names.
This is also motivated by the potential UX I'm thinking of. For example,
if a newly installed application tries to create ~/.app-name, it will be
much more reassuring and convenient to the user if we can show something
like
[program] wants to mkdir ~/.app-name. Allow this and future
access to the new directory?
rather than just "[program] wants to mkdir under ~". (The "Allow this
and future access to the new directory" bit is made possible by the
supervisor knowing the name of the file/directory being created, and can
remember them / write them out to a persistent profile etc)
Note that this is just the filename under the dir represented by fd -
this isn't a path or anything that can be subject to symlink-related
attacks, etc. If a program calls e.g.
mkdirat or openat (dfd -> "/some/", pathname="dir/stuff", O_CREAT)
my understanding is that fd1 will point to /some/dir, and destname would
be "stuff"
Actually, in case your question is "why not send a fd to represent the
newly created file, instead of sending the name" -- I'm not sure whether
you can open even an O_PATH fd to a non-existent file.
+ };
+ struct {
+ __u16 port;
+ };
+ };
+};
+
[...]
