6/5/2024 8:04 PM, Günther Noack wrote:
Hello!
On Thu, May 30, 2024 at 03:05:56PM +0300, Mikhail Ivanov wrote:
5/27/2024 12:57 PM, Günther Noack wrote:
On Fri, May 24, 2024 at 05:30:04PM +0800, Mikhail Ivanov wrote:
+/**
+ * struct landlock_socket_attr - Socket definition
+ *
+ * Argument of sys_landlock_add_rule().
+ */
+struct landlock_socket_attr {
+ /**
+ * @allowed_access: Bitmask of allowed access for a socket
+ * (cf. `Socket flags`_).
+ */
+ __u64 allowed_access;
+ /**
+ * @family: Protocol family used for communication
+ * (same as domain in socket(2)).
+ */
+ int family;
+ /**
+ * @type: Socket type (see socket(2)).
+ */
+ int type;
+};
Regarding the naming of struct landlock_socket_attr and the associated
LANDLOCK_RULE_SOCKET enum:
For the two existing rule types LANDLOCK_RULE_PATH_BENEATH (struct
landlock_path_beneath_attr) and LANDLOCK_RULE_NET_PORT (struct
landlock_net_port_attr), the names of the rule types are describing the
*properties* by which we are filtering (path *beneath*, *network port*), rather
than just the kind of object that we are filtering on.
Should the new enum and struct maybe be called differently as well to match that
convention? Maybe LANDLOCK_RULE_SOCKET_FAMILY_TYPE and struct
landlock_socket_family_type_attr?
Are there *other* properties apart from family and type, by which you are
thinking of restricting the use of sockets in the future?
There was a thought about adding `protocol` (socket(2)) restriction,
but Mickaël noted that it would be useless [1]. Therefore, no other
properties are planned until someone has good use cases.
I agree that current naming can be associated with socket objects. But i
don't think using family-type words for naming of this rule would be
convenient for users. In comparison with net port and path beneath
family-type pair doesn't represent a single semantic unit, so it would
be a little harder to read the code.
Perhaps LANDLOCK_RULE_SOCKET_PROTO (struct landlock_socket_proto_attr)
would be more suitable here? Although socket(2) has `protocol` argument
to specify the socket protocol in some cases (e.g. RAW sockets), in most
cases family-type pair defines protocol itself. Since the purpose of
this patchlist is to restrict protocols used in a sandboxed process, I
think that in the presence of well-written documentation, such naming
may be appropriate here. WDYT?
[1]
https://lore.kernel.org/all/a6318388-e28a-e96f-b1ae-51948c13de4d@xxxxxxxxxxx/
It is difficult, I also can't come up with a much better name. In doubt, we
could stick with what you already have, I think.
LANDLOCK_RULE_SOCKET_PROTO alludes to "protocol" and even though that is the
general term, it can be confused with the third argument to socket(2), which is
also called "protocol" and is rarely used.
Mickaël, do you have any opinions on the naming of this?
(More about the content)
The Landlock documentation states the general approach up front:
A Landlock rule describes an *action* on an *object* which the process intends
to perform.
(In your case, the object is a socket, and the action is the socket's creation.
The Landlock rules describe predicates on objects to restrict the set of actions
through the access_mask_t.)
The implementation is perfectly in line with that, but it would help to phrase
the documentation also in terms of that framework. That means, what we are
restricting are *actions*, not protocols.
To make a more constructive suggestion:
"These flags restrict actions on sockets for a sandboxed process (e.g. socket
creation)."
I think this has too general meaning (e.g. bind(2) is also an action on
socket). Probably this one would be more suitable:
"These flags restrict actions of adding sockets in a sandboxed
process (e.g. socket creation, passing socket FDs to/from the
process)."
Sounds good. (Although I would not give "passing socket FDs to/from the
process" as an example, as long as it's not supported yet.)
Agreed, thanks
+ * - %LANDLOCK_ACCESS_SOCKET_CREATE: Create a socket.
Can we be more specific here what operations are affected by this? It is rather
obvious that this affects socket(2), but does this also affect accept(2) and
connect(2)?
A scenario that I could imagine being useful is to sandbox a TCP server like
this:
* create a socket, bind(2) and listen(2)
* sandbox yourself so that no new sockets can be created with socket(2)
* go into the main loop and start accept(2)ing new connections
Is this an approach that would work with this patch set?
Yes, such scenario is possible. This rule should apply to all socket
creation requests in the user space (socket(2), socketpair(2), io_uring
request). Perhaps it's necessary to clarify here that only user space
sockets are restricted?
Btw, current implementation doesn't check that the socket creation
request doesn't come from the kernel space. Will be fixed.
Two brief side side discussions:
* What are the scenarios where that creation request comes from kernel space?
If this is used under the hood for network-backed file systems like NFS, can
this result in surprising interactions when the program tries to access the
file system?
Yes, i've figured out that NFS can use __sock_create() kernel method for
allocating per-client socket. If kernel allocation of sockets would be
restricted by Landlock that NFS client allocation may fail and proccess
won't be able to connect to the NFS server.
Some of the socket protocols uses kernel space methods like
sock_create_lite() or sock_create_kern() for their own needs.
For example netlink fou family uses sock_create_kern() for UDP
tunneling.
Landlock shouldn't allow to indirectly change the behavior of such
mechanisms from user space.
* To be clear, I think it would be useful to support the scenario above, where
accept() continues to work. - It would make it easy to create sandboxed server
processes and they could still accept connections, but do no other networking.
Yes, accept() should work with any restrictions.
But to bring it back to my original remark, and to unblock progress:
I think for this patch set (focused on userspace-requested socket creation), it
would be enough to clarify in the documentation which operations are affected by
the LANDLOCK_ACCESS_SOCKET_CREATE right.
Ok, I'll do it.
(It might make a neat sample tool as well, if something like this works :))
Regarding the list of socket access rights with only one item in it:
I am still unsure what other socket actions are in scope in the future; it would
probably help to phrase the documentation in those terms. (listen(2), bind(2),
connect(2), shutdown(2)? On the other hand, bind(2) and connect(2) for TCP are
already restrictable differently.))
I think it would be useful to restrict sending and receiving socket
FDs via unix domain sockets (see SCM_RIGHTS in unix(7)).
That seems like a reasonable idea. Would you like to file an issue on the
Landlock bugtracker about it?
https://github.com/landlock-lsm/linux/issues
Ofc: https://github.com/landlock-lsm/linux/issues/33.
+ /* Checks that all supported socket families and types can be stored in socket_key. */
+ BUILD_BUG_ON(AF_MAX > (typeof(socket_key.data.family))~0);
+ BUILD_BUG_ON(SOCK_MAX > (typeof(socket_key.data.type))~0);
Off-by-one nit: AF_MAX and SOCK_MAX are one higher than the last permitted value,
so technically it would be ok if they are one higher than (unsigned short)~0.
(Did you see this remark?)
Yeah, sorry I've somehow lost my reply. This nit will be fixed.
I see that this function traces back to Mickaël's comment in
https://lore.kernel.org/all/20240412.phoh7laim7Th@xxxxxxxxxxx/
In my understanding, the motivation was to keep the key size in check.
But that does not mean that we need to turn it into a uintptr_t?
Would it not have been possible to extend the union landlock_key in ruleset.h
with a
struct {
unsigned short family, type;
}
and then do the AF_MAX, SOCK_MAX build-time checks on that?
It seems like that might be more in line with what we already have?
I don't think that complicating general entity with such a specific
representation would be a good solution here. `landlock_key` shouldn't
contain any semantic information about the key content.
Hm, OK. I think that is debatable, but these are all things that are
implementation details and can be changed later if needed. Sounds good to me if
we fix the undefined behaviour in the key calculation.
Ok, agreed! Mikael can probably give his opinion on this.
+ /* Denies inserting a rule with unsupported socket family and type. */
^^^^^^^^^^^^^^^^^^^^^^^^^
Is the wording "unsupported socket family" misleading here?
(a) It is technically a "protocol family" and a "socket type", according to
socket(2). (BTW, the exact delineation between a "protocol family" and an
"address family" is not clear to me.)
(b) "unsupported" in the context of protocol families may mean that the kernel
does not know how to speak that protocol, which is slightly different than
saying that it's outside of the [0, AF_MAX) range. If we wanted to check
for the protocol family being "supported", we should also probably return
-EAFNOSUPPORT, similar to what we already return when adding a "port" rule
with the wrong protocol [1]?
[1] https://docs.kernel.org/userspace-api/landlock.html#extending-a-ruleset
I suspect that -EINVAL is slightly more correct here, because this is not about
the protocols that the kernel supports, but only about the range. If we wanted
to return errors about the protocol that the kernel supports, I realized that
we'd probably also have to check whether the *combination* of family and type
makes sense. In my understanding, the equivalent errors for type and protocol,
ESOCKTNOSUPPORT and EPROTONOSUPPORT, only get returned based on whether they
make sense together with the other values.
Thanks, "unsupported" is indeed an incorrect naming in this case.
To check if the protocol is supported, we'll have to validate
family-type combination, which would be really ugly.
Taking into account the thoughts below regarding family-type checking,
the following description can be used.
/* Denies inserting a rule with family and type outside the range. */
+ if (family < 0 || family >= AF_MAX)
+ return -EINVAL;
+ if (type < 0 || type >= SOCK_MAX)
+ return -EINVAL;
enum sock_type (include/linux/net.h) has "holes": values 7, 8 and 9 are not
defined in the header. Should we check more specifically for the supported
values here? (Is there already a helper function for that?)
I think that a more detailed check of the family-type values may have a
good effect here, since the rules will contain real codes of families
and types.
I haven't found any helper to check the supported socket type value.
Performing a check inside landlock can lead to several minor problems,
which theoretically should not lead to any costs.
* There are would be a dependency with constants of enum sock_types. But
we are unlikely to see new types of sockets in the next few years, so
it wouldn't be a problem to maintain such check.
* enum sock_types can be redefined (see ARCH_HAS_SOCKET_TYPES in net.h),
but i haven't found anyone to actually change the constants of socket
types. It would be wrong to have a different landlock behavior for
arch that redefines sock_types for some purposes, so probably this
should also be maintained.
WDYT?
Thinking about it again, from a Landlock safety perspective, I believe it is
safe to keep the checks as they are and to check for the two values to be in the
ranges [0, AF_MAX) and [0, SOCK_MAX).
Even if we permit the rule to be added for an invalid socket type, there does
not seem to be any harm in that, as these sockets can't be created anyway.
Also, given the semantics of these errors in socket(2), where also the
*combinations* of the values are checked, it seems overly complicated to check
all these combinations. I think it would be fine to keep as is, I was mostly
wondering whether you had done any deeper analysis?
No, I think that checking only ranges is also a good solution. We won't
care about maintaining valid type constants, and that would simplify
behavior of this method for users. Let's take this approach.
It might be worth spelling out in the struct documentation that the values which
fulfil 0 <= family < AF_MAX and 0 <= type < SOCK_MAX are considered valid. Does
that sound reasonable?
Ofc, I'll add appropriate comments in the definition of struct
landlock_socket_attr and in the socket rule documentation. Thanks!
P.S., it seems that the security/apparmor/Makefile is turning the "#define"s
into C code with lookup tables, but it seems that this is only used for
human-readable audit-logging, not for validating the policies.
—Günther
