| Message ID | 20240719150618.197991-1-mic@digikod.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Use socket's Landlock domain | expand |
Hello Mickaël! On Fri, Jul 19, 2024 at 05:06:15PM +0200, Mickaël Salaün wrote: > While the current approach works, I think we should change the way > Landlock restricts network actions. Because this feature is relatively > new, we can still fix this inconsistency. In a nutshell, let's follow a > more capability-based model. Please let me know what you think. Thanks for sending the patch. The implementation with ->f_cred is much simpler than I had thought it would be. Some higher level questions: * I assume that the plan is to backport this as a fix to older kernels that already have the feature? (Otherwise, we would potentially have backwards compatibility issues.) * I believe it clashes a little bit with the TCP server example [1], which I found a useful use case for the TCP connect/bind and socket restriction features. * accept(2) on a passive (listen-mode) socket will give you a new socket FD -- does that new socket inherit its f_cred from the server socket, or does it inherit its f_cred from the thread? Regarding the TCP server example, the current implementation is *very* simple, and does the following steps: 1. create socket with socket(2) 2. bind(2) the socket to the desired port 3. enforce a Landlock ruleset that disables all networking features (TCP bind, TCP connect and socket creation with the new patch set) 4. listen(2) on the socket 5. go into the accept(2) loop With the old behaviour, step 3 is going to affect the existing passive socket: It will not be possible any more to bind(2) that passive socket to another port. With the new behaviour (after your patch), step 3 does *not* affect the existing socket, and the server socket can be reused to bind(2) to other ports. Or, in other words: If the relevant domain is tied to the socket at creation time, that means that a future client connection which takes over the process might be able to use that socket's Landlock domain, which potentially grants more permissions than the thread's domain I think it would be nice if a use case like in the TCP server example would still be possible with Landlock; there are multiple ways to reach that: - We could enforce two layers of Landlock rules, one before socket creation that restricts bind(2) to a given port, and one after socket creation that restricts other bind(2), create(2) and socket(2) operations. Drawbacks: - One Landlock layer more, and needs to add a Landlock rule: This is a bit more complicated to implement. - The bind(2) restriction on the socket is still only per port *number*, but it is still possible to bind with that port number on different IP addresses. - Alternatively, I wish we could just lock the passive server socket in, so that it can't be made to reconnect anywhere else. After all, the socket disassociation with connect(2) with AF_UNSPEC seems to be a somewhat obscure and seldomly used feature - if we could just disallow that operation, we could ensure that this socket gets reused for such a nefarious purpose. It would still require two nested Landlock rulesets to make the TCP server example work, but they would be a bit simpler than in the alternative above. - There are probably more alternatives...? What do you think? —Günther [1] https://wiki.gnoack.org/LandlockTcpServerExample
On Tue, Jul 23, 2024 at 03:07:53PM +0200, Günther Noack wrote: > Hello Mickaël! > > On Fri, Jul 19, 2024 at 05:06:15PM +0200, Mickaël Salaün wrote: > > While the current approach works, I think we should change the way > > Landlock restricts network actions. Because this feature is relatively > > new, we can still fix this inconsistency. In a nutshell, let's follow a > > more capability-based model. Please let me know what you think. > > Thanks for sending the patch. The implementation with ->f_cred is much simpler > than I had thought it would be. Some higher level questions: > > * I assume that the plan is to backport this as a fix to older kernels that > already have the feature? (Otherwise, we would potentially have backwards > compatibility issues.) Correct, if this patch is merged it must be backported too, but there might be better alternatives, or we might just stick to the initial approach. > > * I believe it clashes a little bit with the TCP server example [1], > which I found a useful use case for the TCP connect/bind and socket > restriction features. Indeed, because the socket is created before the sandboxing, the socket could be reused to bind (or connect) to other ports. This is a good example of why using current's instead of socket's credentials may be less surprising. From my point of view, the main issue is that a socket can be reconfigured. > > * accept(2) on a passive (listen-mode) socket will give you a new socket FD > -- does that new socket inherit its f_cred from the server socket, > or does it inherit its f_cred from the thread? According to sock_alloc_file(), the newly created socket inherits the caller's credentials, which is similar to a call to openat2(2) with a directory file descriptor. > > Regarding the TCP server example, the current implementation is *very* simple, > and does the following steps: > > 1. create socket with socket(2) > 2. bind(2) the socket to the desired port > 3. enforce a Landlock ruleset that disables all networking features > (TCP bind, TCP connect and socket creation with the new patch set) > 4. listen(2) on the socket > 5. go into the accept(2) loop > > With the old behaviour, step 3 is going to affect the existing passive socket: > It will not be possible any more to bind(2) that passive socket to another port. > > With the new behaviour (after your patch), step 3 does *not* affect the existing > socket, and the server socket can be reused to bind(2) to other ports. > > Or, in other words: If the relevant domain is tied to the socket at creation > time, that means that a future client connection which takes over the process > might be able to use that socket's Landlock domain, which potentially grants > more permissions than the thread's domain Yes, that's why it might be more risky, but I wanted to have this discussion. Whatever the outcome, it should be explained in Documentation/security/landlock.rst One thing to keep in mind and that contrary to other LSMs, Landlock, like seccomp, enables processes to (only) drop privileges, and this is done by the process itself (not at execve time). This was also one argument for the initial approach. A thing that bothered me was related to the restrictions of sockets, especially with the WIP scoping feature. Datagram (unix) sockets could work before sandboxing, and suddenly become broken after sandboxing (because the security check would be done at send time instead of connect time). This kind of issue should be identified quite early and easily though. However, there is still an inconsistency between connected stream sockets and datagram sockets. From a security point of view, this looks like a good thing though. > > I think it would be nice if a use case like in the TCP server example would > still be possible with Landlock; there are multiple ways to reach that: > > - We could enforce two layers of Landlock rules, one before socket creation > that restricts bind(2) to a given port, and one after socket creation that > restricts other bind(2), create(2) and socket(2) operations. > > Drawbacks: > > - One Landlock layer more, and needs to add a Landlock rule: > This is a bit more complicated to implement. Right, I think it's too complex for users. > - The bind(2) restriction on the socket is still only per port *number*, > but it is still possible to bind with that port number on different IP > addresses. Good point. That's another argument for the initial approach and the way you sandboxed the example: dropping the *_TCP access rights, it is not possible to rebind or reconnect a socket (to another address). Actually, I'm not sure if using the socket's credential would not confuse users to understand why an access is denied (or allowed). > > - Alternatively, I wish we could just lock the passive server socket in, so > that it can't be made to reconnect anywhere else. After all, the socket > disassociation with connect(2) with AF_UNSPEC seems to be a somewhat obscure > and seldomly used feature - if we could just disallow that operation, we > could ensure that this socket gets reused for such a nefarious purpose. We could also add a new "scope" for socket reconfiguration of sockets created by a parent or sibling domain, similar to the ptrace restrictions. > > It would still require two nested Landlock rulesets to make the TCP server > example work, but they would be a bit simpler than in the alternative above. > > - There are probably more alternatives...? > > What do you think? I see other alternatives: - We could have a new ruleset's attribute to specify if network restrictions should apply on the caller or the socket. That might be confusing for users though. - We could just stick to the initial approach and add new access rights (denied by default, similar to FS_REFER) that will only apply to newly created sockets. This is close to the previous alternative but more explicit. Both use cases could then be used, with a default secure approach (i.e. the initial one). However, we need to have a clear rationale for the WIP scoping restrictions: should the caller or the socket be checked as the client? - We could extend the current approach and check both the caller's credential and the socket's credential. This could be confusing to users though. - We could have a new fcntl(2) command to (securely) transition a file descriptor's credential to the caller's one (e.g. approved by ptrace_may_access). That could be generic to all Linux access control systems. - According to a new ruleset's attribute, we could revalidate (at use time) file descriptors not opened by the caller's Landlock domain, but users would have to be explicit (e.g. stdio issue). And how to handle partially allowed accesses? > > —Günther > > [1] https://wiki.gnoack.org/LandlockTcpServerExample >