Message-ID: <ZmCantjZlyxL8jzh@google.com>
Date: Wed, 5 Jun 2024 19:04:30 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
Cc: mic@...ikod.net, willemdebruijn.kernel@...il.com, gnoack3000@...il.com, 
	linux-security-module@...r.kernel.org, netdev@...r.kernel.org, 
	netfilter-devel@...r.kernel.org, yusongping@...wei.com, 
	artem.kuzin@...wei.com, konstantin.meskhidze@...wei.com
Subject: Re: [RFC PATCH v2 01/12] landlock: Support socket access-control

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@digikod.net/

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


> > > + * - %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 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?

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

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.


> > (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


> > > +	/* 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?)


> > I see that this function traces back to Mickaël's comment in
> > https://lore.kernel.org/all/20240412.phoh7laim7Th@digikod.net/
> > 
> > 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.


> > > +	/* 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.


> > > +	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?

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?

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

Powered by blists - more mailing lists