| Message ID | cde6bbf0b52710b33170f2787fdcb11538e40813.1741047969.git.m@maowtm.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Landlock supervise: a mechanism for interactive permission requests | expand |
On Tue, Mar 04, 2025 at 01:13:01AM +0000, Tingmao Wang wrote: > The two structures are designed to be passed via read and write > to the supervisor-fd. Compile time check for no holes are added > to build_check_abi. > > The event structure will be a dynamically sized structure with > possibly a NULL-terminating filename at the end. This is so that > we can pass a raw filename to the supervisor for file creation > requests, without having the trouble of not being able to open a > fd to a file that has not been created. > > NOTE: despite this patch having a new uapi, I'm still very open to e.g. > re-using fanotify stuff instead (if that makes sense in the end). This is > just a PoC. > > Signed-off-by: Tingmao Wang <m@maowtm.org> > --- > include/uapi/linux/landlock.h | 107 ++++++++++++++++++++++++++++++++++ > security/landlock/syscalls.c | 28 +++++++++ > 2 files changed, 135 insertions(+) > > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h > index 7bc1eb4859fb..b5645fdd998d 100644 > --- a/include/uapi/linux/landlock.h > +++ b/include/uapi/linux/landlock.h > @@ -318,4 +318,111 @@ struct landlock_net_port_attr { > #define LANDLOCK_SCOPE_SIGNAL (1ULL << 1) > /* clang-format on*/ > > +/** > + * DOC: supervisor > + * > + * Supervise mode > + * ~~~~~~~~~~~~~~ > + * > + * TODO > + */ > + > +typedef __u16 landlock_supervise_event_type_t; > +/* clang-format off */ > +#define LANDLOCK_SUPERVISE_EVENT_TYPE_FS_ACCESS 1 > +#define LANDLOCK_SUPERVISE_EVENT_TYPE_NET_ACCESS 2 > +/* clang-format on */ > + > +struct landlock_supervise_event_hdr { > + /** > + * @type: Type of the event. > + */ > + landlock_supervise_event_type_t type; > + /** > + * @length: Length of the entire struct > + * landlock_supervise_event including this header. > + */ > + __u16 length; > + /** > + * @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. > +}; > + > +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? > + /** > + * @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. > + }; > + struct { > + __u16 port; > + }; > + }; > +}; > + [...]
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; >> + }; >> + }; >> +}; >> + > > [...]
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h index 7bc1eb4859fb..b5645fdd998d 100644 --- a/include/uapi/linux/landlock.h +++ b/include/uapi/linux/landlock.h @@ -318,4 +318,111 @@ struct landlock_net_port_attr { #define LANDLOCK_SCOPE_SIGNAL (1ULL << 1) /* clang-format on*/ +/** + * DOC: supervisor + * + * Supervise mode + * ~~~~~~~~~~~~~~ + * + * TODO + */ + +typedef __u16 landlock_supervise_event_type_t; +/* clang-format off */ +#define LANDLOCK_SUPERVISE_EVENT_TYPE_FS_ACCESS 1 +#define LANDLOCK_SUPERVISE_EVENT_TYPE_NET_ACCESS 2 +/* clang-format on */ + +struct landlock_supervise_event_hdr { + /** + * @type: Type of the event. + */ + landlock_supervise_event_type_t type; + /** + * @length: Length of the entire struct + * landlock_supervise_event including this header. + */ + __u16 length; + /** + * @cookie: Opaque identifier to be included in the response. + */ + __u32 cookie; +}; + +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; + /** + * @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[]; + }; + struct { + __u16 port; + }; + }; +}; + +/* clang-format off */ +#define LANDLOCK_SUPERVISE_DECISION_DENY 0 +#define LANDLOCK_SUPERVISE_DECISION_ALLOW 1 +/* clang-format on */ + +struct landlock_supervise_response { + /** + * @length: Size of this structure. + */ + __u16 length; + /** + * @decision: Whether to allow the request. + */ + __u8 decision; + /** + * @pad: Reserved, must be zero. + */ + __u8 _reserved; + /** + * @cookie: Cookie previously received in the request. + */ + __u32 cookie; +}; + #endif /* _UAPI_LINUX_LANDLOCK_H */ diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c index adf7e77023b5..f1080e7de0c7 100644 --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -91,6 +91,9 @@ static void build_check_abi(void) struct landlock_path_beneath_attr path_beneath_attr; struct landlock_net_port_attr net_port_attr; size_t ruleset_size, path_beneath_size, net_port_size; + struct landlock_supervise_event *event; + struct landlock_supervise_response response; + size_t supervise_evt_size, supervise_response_size; /* * For each user space ABI structures, first checks that there is no @@ -114,6 +117,31 @@ static void build_check_abi(void) net_port_size += sizeof(net_port_attr.port); BUILD_BUG_ON(sizeof(net_port_attr) != net_port_size); BUILD_BUG_ON(sizeof(net_port_attr) != 16); + + /* Check that anything before the destname does not have holes */ + supervise_evt_size = sizeof(event->hdr.type); + supervise_evt_size += sizeof(event->hdr.length); + supervise_evt_size += sizeof(event->hdr.cookie); + BUILD_BUG_ON(offsetofend(typeof(*event), hdr) != 8); + supervise_evt_size += sizeof(event->access_request); + supervise_evt_size += sizeof(event->accessor); + supervise_evt_size += sizeof(event->fd1); + supervise_evt_size += sizeof(event->fd2); + BUILD_BUG_ON(offsetof(typeof(*event), destname) != supervise_evt_size); + BUILD_BUG_ON(offsetof(typeof(*event), destname) != 28); + + /* + * Make sure this struct does not end up with stricter + * alignment than 8 + */ + BUILD_BUG_ON(__alignof__(typeof(*event)) != 8); + + supervise_response_size = sizeof(response.length); + supervise_response_size += sizeof(response.decision); + supervise_response_size += sizeof(response._reserved); + supervise_response_size += sizeof(response.cookie); + BUILD_BUG_ON(sizeof(response) != supervise_response_size); + BUILD_BUG_ON(sizeof(response) != 8); } /* Ruleset handling */
The two structures are designed to be passed via read and write to the supervisor-fd. Compile time check for no holes are added to build_check_abi. The event structure will be a dynamically sized structure with possibly a NULL-terminating filename at the end. This is so that we can pass a raw filename to the supervisor for file creation requests, without having the trouble of not being able to open a fd to a file that has not been created. NOTE: despite this patch having a new uapi, I'm still very open to e.g. re-using fanotify stuff instead (if that makes sense in the end). This is just a PoC. Signed-off-by: Tingmao Wang <m@maowtm.org> --- include/uapi/linux/landlock.h | 107 ++++++++++++++++++++++++++++++++++ security/landlock/syscalls.c | 28 +++++++++ 2 files changed, 135 insertions(+)