| Message ID | 20230116085818.165539-13-konstantin.meskhidze@huawei.com |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Network support for Landlock | expand |
Hello! Thank you for sending these patches! I'll start poking a bit at the Go-Landlock library to see how we can support it there when this lands. On Mon, Jan 16, 2023 at 04:58:18PM +0800, Konstantin Meskhidze wrote: > Describe network access rules for TCP sockets. Add network access > example in the tutorial. Add kernel configuration support for network. > > Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > --- > > Changes since v8: > * Minor refactoring. > > Changes since v7: > * Fixes documentaion logic errors and typos as Mickaёl suggested: > https://lore.kernel.org/netdev/9f354862-2bc3-39ea-92fd-53803d9bbc21@digikod.net/ > > Changes since v6: > * Adds network support documentaion. > > --- > Documentation/userspace-api/landlock.rst | 72 ++++++++++++++++++------ > 1 file changed, 56 insertions(+), 16 deletions(-) > > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst > index d8cd8cd9ce25..980558b879d6 100644 > --- a/Documentation/userspace-api/landlock.rst > +++ b/Documentation/userspace-api/landlock.rst > @@ -11,10 +11,10 @@ Landlock: unprivileged access control > :Date: October 2022 > > The goal of Landlock is to enable to restrict ambient rights (e.g. global > -filesystem access) for a set of processes. Because Landlock is a stackable > -LSM, it makes possible to create safe security sandboxes as new security layers > -in addition to the existing system-wide access-controls. This kind of sandbox > -is expected to help mitigate the security impact of bugs or > +filesystem or network access) for a set of processes. Because Landlock > +is a stackable LSM, it makes possible to create safe security sandboxes as new > +security layers in addition to the existing system-wide access-controls. This > +kind of sandbox is expected to help mitigate the security impact of bugs or > unexpected/malicious behaviors in user space applications. Landlock empowers > any process, including unprivileged ones, to securely restrict themselves. > > @@ -30,18 +30,20 @@ Landlock rules > > A Landlock rule describes an action on an object. An object is currently a > file hierarchy, and the related filesystem actions are defined with `access > -rights`_. A set of rules is aggregated in a ruleset, which can then restrict > -the thread enforcing it, and its future children. > +rights`_. Since ABI version 4 a port data appears with related network actions > +for TCP socket families. A set of rules is aggregated in a ruleset, which > +can then restrict the thread enforcing it, and its future children. > > Defining and enforcing a security policy > ---------------------------------------- > > We first need to define the ruleset that will contain our rules. For this > example, the ruleset will contain rules that only allow read actions, but write > -actions will be denied. The ruleset then needs to handle both of these kind of > +actions will be denied. The ruleset then needs to handle both of these kind of (This one looks like an unintentional whitespace change; the double-space ending is used in this file, so we should probably stay consistent.) > actions. This is required for backward and forward compatibility (i.e. the > kernel and user space may not know each other's supported restrictions), hence > -the need to be explicit about the denied-by-default access rights. > +the need to be explicit about the denied-by-default access rights. Also ruleset Wording nit: "Also, the ruleset"...? > +will have network rules for specific ports, so it should handle network actions. > > .. code-block:: c > > @@ -62,6 +64,9 @@ the need to be explicit about the denied-by-default access rights. > LANDLOCK_ACCESS_FS_MAKE_SYM | > LANDLOCK_ACCESS_FS_REFER | > LANDLOCK_ACCESS_FS_TRUNCATE, > + .handled_access_net = > + LANDLOCK_ACCESS_NET_BIND_TCP | > + LANDLOCK_ACCESS_NET_CONNECT_TCP, > }; > > Because we may not know on which kernel version an application will be > @@ -70,14 +75,18 @@ should try to protect users as much as possible whatever the kernel they are > using. To avoid binary enforcement (i.e. either all security features or > none), we can leverage a dedicated Landlock command to get the current version > of the Landlock ABI and adapt the handled accesses. Let's check if we should > -remove the ``LANDLOCK_ACCESS_FS_REFER`` or ``LANDLOCK_ACCESS_FS_TRUNCATE`` > -access rights, which are only supported starting with the second and third > -version of the ABI. > +remove the ``LANDLOCK_ACCESS_FS_REFER`` or ``LANDLOCK_ACCESS_FS_TRUNCATE`` or > +network access rights, which are only supported starting with the second, > +third and fourth version of the ABI. > > .. code-block:: c > > int abi; > > + #define ACCESS_NET_BIND_CONNECT ( \ > + LANDLOCK_ACCESS_NET_BIND_TCP | \ > + LANDLOCK_ACCESS_NET_CONNECT_TCP) > + > abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); > if (abi < 0) { > /* Degrades gracefully if Landlock is not handled. */ > @@ -92,6 +101,11 @@ version of the ABI. > case 2: > /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */ > ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE; > + case 3: > + /* Removes network support for ABI < 4 */ > + ruleset_attr.handled_access_net &= ^^^^^ Nit: Indentation differs from "case 2" > + ~(LANDLOCK_ACCESS_NET_BIND_TCP | > + LANDLOCK_ACCESS_NET_CONNECT_TCP); > } > > This enables to create an inclusive ruleset that will contain our rules. > @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock > ABI version. In this example, this is not required because all of the requested > ``allowed_access`` rights are already available in ABI 1. > > -We now have a ruleset with one rule allowing read access to ``/usr`` while > -denying all other handled accesses for the filesystem. The next step is to > -restrict the current thread from gaining more privileges (e.g. thanks to a SUID > -binary). > +For network access-control, we can add a set of rules that allow to use a port > +number for a specific action. All ports values must be defined in network byte > +order. What is the point of asking user space to convert this to network byte order? It seems to me that the kernel would be able to convert it to network byte order very easily internally and in a single place -- why ask all of the users to deal with that complexity? Am I overlooking something? > + > +.. code-block:: c > + > + struct landlock_net_service_attr net_service = { > + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, > + .port = htons(8080), > + }; This is a more high-level comment: The notion of a 16-bit "port" seems to be specific to TCP and UDP -- how do you envision this struct to evolve if other protocols need to be supported in the future? Should this struct and the associated constants have "TCP" in its name, and other protocols use a separate struct in the future? > + > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, > + &net_service, 0); > + > +The next step is to restrict the current thread from gaining more privileges > +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing ^^^^^^ "through" a SUID binary? "thanks to" sounds like it's desired to do that, while we're actually trying to prevent it here? > +read access to ``/usr`` while denying all other handled accesses for the filesystem, > +and a second rule allowing TCP binding on port 8080. > > .. code-block:: c > > @@ -355,7 +383,7 @@ Access rights > ------------- > > .. kernel-doc:: include/uapi/linux/landlock.h > - :identifiers: fs_access > + :identifiers: fs_access net_access > > Creating a new ruleset > ---------------------- > @@ -374,6 +402,7 @@ Extending a ruleset > > .. kernel-doc:: include/uapi/linux/landlock.h > :identifiers: landlock_rule_type landlock_path_beneath_attr > + landlock_net_service_attr > > Enforcing a ruleset > ------------------- > @@ -451,6 +480,12 @@ always allowed when using a kernel that only supports the first or second ABI. > Starting with the Landlock ABI version 3, it is now possible to securely control > truncation thanks to the new ``LANDLOCK_ACCESS_FS_TRUNCATE`` access right. > > +Network support (ABI < 4) > +------------------------- > + > +Starting with the Landlock ABI version 4, it is now possible to restrict TCP > +bind and connect actions to only a set of allowed ports. > + > .. _kernel_support: > > Kernel support > @@ -469,6 +504,11 @@ still enable it by adding ``lsm=landlock,[...]`` to > Documentation/admin-guide/kernel-parameters.rst thanks to the bootloader > configuration. > > +To be able to explicitly allow TCP operations (e.g., adding a network rule with > +``LANDLOCK_ACCESS_NET_TCP_BIND``), the kernel must support TCP (``CONFIG_INET=y``). > +Otherwise, sys_landlock_add_rule() returns an ``EAFNOSUPPORT`` error, which can > +safely be ignored because this kind of TCP operation is already not possible. > + > Questions and answers > ===================== > > -- > 2.25.1 >
1/22/2023 2:07 AM, Günther Noack пишет: > Hello! > > Thank you for sending these patches! I'll start poking a bit at the > Go-Landlock library to see how we can support it there when this lands. > > On Mon, Jan 16, 2023 at 04:58:18PM +0800, Konstantin Meskhidze wrote: >> Describe network access rules for TCP sockets. Add network access >> example in the tutorial. Add kernel configuration support for network. >> >> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> >> --- >> >> Changes since v8: >> * Minor refactoring. >> >> Changes since v7: >> * Fixes documentaion logic errors and typos as Mickaёl suggested: >> https://lore.kernel.org/netdev/9f354862-2bc3-39ea-92fd-53803d9bbc21@digikod.net/ >> >> Changes since v6: >> * Adds network support documentaion. >> >> --- >> Documentation/userspace-api/landlock.rst | 72 ++++++++++++++++++------ >> 1 file changed, 56 insertions(+), 16 deletions(-) >> >> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst >> index d8cd8cd9ce25..980558b879d6 100644 >> --- a/Documentation/userspace-api/landlock.rst >> +++ b/Documentation/userspace-api/landlock.rst >> @@ -11,10 +11,10 @@ Landlock: unprivileged access control >> :Date: October 2022 >> >> The goal of Landlock is to enable to restrict ambient rights (e.g. global >> -filesystem access) for a set of processes. Because Landlock is a stackable >> -LSM, it makes possible to create safe security sandboxes as new security layers >> -in addition to the existing system-wide access-controls. This kind of sandbox >> -is expected to help mitigate the security impact of bugs or >> +filesystem or network access) for a set of processes. Because Landlock >> +is a stackable LSM, it makes possible to create safe security sandboxes as new >> +security layers in addition to the existing system-wide access-controls. This >> +kind of sandbox is expected to help mitigate the security impact of bugs or >> unexpected/malicious behaviors in user space applications. Landlock empowers >> any process, including unprivileged ones, to securely restrict themselves. >> >> @@ -30,18 +30,20 @@ Landlock rules >> >> A Landlock rule describes an action on an object. An object is currently a >> file hierarchy, and the related filesystem actions are defined with `access >> -rights`_. A set of rules is aggregated in a ruleset, which can then restrict >> -the thread enforcing it, and its future children. >> +rights`_. Since ABI version 4 a port data appears with related network actions >> +for TCP socket families. A set of rules is aggregated in a ruleset, which >> +can then restrict the thread enforcing it, and its future children. >> >> Defining and enforcing a security policy >> ---------------------------------------- >> >> We first need to define the ruleset that will contain our rules. For this >> example, the ruleset will contain rules that only allow read actions, but write >> -actions will be denied. The ruleset then needs to handle both of these kind of >> +actions will be denied. The ruleset then needs to handle both of these kind of > > (This one looks like an unintentional whitespace change; the > double-space ending is used in this file, so we should probably stay > consistent.) Got it. Thanks. > >> actions. This is required for backward and forward compatibility (i.e. the >> kernel and user space may not know each other's supported restrictions), hence >> -the need to be explicit about the denied-by-default access rights. >> +the need to be explicit about the denied-by-default access rights. Also ruleset > > Wording nit: "Also, the ruleset"...? Right. Thanks for the nit. > >> +will have network rules for specific ports, so it should handle network actions. >> >> .. code-block:: c >> >> @@ -62,6 +64,9 @@ the need to be explicit about the denied-by-default access rights. >> LANDLOCK_ACCESS_FS_MAKE_SYM | >> LANDLOCK_ACCESS_FS_REFER | >> LANDLOCK_ACCESS_FS_TRUNCATE, >> + .handled_access_net = >> + LANDLOCK_ACCESS_NET_BIND_TCP | >> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >> }; >> >> Because we may not know on which kernel version an application will be >> @@ -70,14 +75,18 @@ should try to protect users as much as possible whatever the kernel they are >> using. To avoid binary enforcement (i.e. either all security features or >> none), we can leverage a dedicated Landlock command to get the current version >> of the Landlock ABI and adapt the handled accesses. Let's check if we should >> -remove the ``LANDLOCK_ACCESS_FS_REFER`` or ``LANDLOCK_ACCESS_FS_TRUNCATE`` >> -access rights, which are only supported starting with the second and third >> -version of the ABI. >> +remove the ``LANDLOCK_ACCESS_FS_REFER`` or ``LANDLOCK_ACCESS_FS_TRUNCATE`` or >> +network access rights, which are only supported starting with the second, >> +third and fourth version of the ABI. >> >> .. code-block:: c >> >> int abi; >> >> + #define ACCESS_NET_BIND_CONNECT ( \ >> + LANDLOCK_ACCESS_NET_BIND_TCP | \ >> + LANDLOCK_ACCESS_NET_CONNECT_TCP) >> + >> abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); >> if (abi < 0) { >> /* Degrades gracefully if Landlock is not handled. */ >> @@ -92,6 +101,11 @@ version of the ABI. >> case 2: >> /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */ >> ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE; >> + case 3: >> + /* Removes network support for ABI < 4 */ >> + ruleset_attr.handled_access_net &= > ^^^^^ > Nit: Indentation differs from "case 2" Got it. Will be fixed. Thanks. > >> + ~(LANDLOCK_ACCESS_NET_BIND_TCP | >> + LANDLOCK_ACCESS_NET_CONNECT_TCP); >> } >> >> This enables to create an inclusive ruleset that will contain our rules. >> @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock >> ABI version. In this example, this is not required because all of the requested >> ``allowed_access`` rights are already available in ABI 1. >> >> -We now have a ruleset with one rule allowing read access to ``/usr`` while >> -denying all other handled accesses for the filesystem. The next step is to >> -restrict the current thread from gaining more privileges (e.g. thanks to a SUID >> -binary). >> +For network access-control, we can add a set of rules that allow to use a port >> +number for a specific action. All ports values must be defined in network byte >> +order. > > What is the point of asking user space to convert this to network byte > order? It seems to me that the kernel would be able to convert it to > network byte order very easily internally and in a single place -- why > ask all of the users to deal with that complexity? Am I overlooking > something? I had a discussion about this issue with Mickaёl. Please check these threads: 1. https://lore.kernel.org/netdev/49391484-7401-e7c7-d909-3bd6bd024731@digikod.net/ 2. https://lore.kernel.org/netdev/1ed20e34-c252-b849-ab92-78c82901c979@huawei.com/ > >> + >> +.. code-block:: c >> + >> + struct landlock_net_service_attr net_service = { >> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >> + .port = htons(8080), >> + }; > > This is a more high-level comment: > > The notion of a 16-bit "port" seems to be specific to TCP and UDP -- > how do you envision this struct to evolve if other protocols need to > be supported in the future? When TCP restrictions land into Linux, we need to think about UDP support. Then other protocols will be on the road. Anyway you are right this struct will be evolving in long term, but I don't have a particular envision now. Thanks for the question - we need to think about it. > > Should this struct and the associated constants have "TCP" in its > name, and other protocols use a separate struct in the future? > >> + >> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, >> + &net_service, 0); >> + >> +The next step is to restrict the current thread from gaining more privileges >> +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing > ^^^^^^ > "through" a SUID binary? "thanks to" sounds like it's desired > to do that, while we're actually trying to prevent it here? This is Mickaёl's part. Let's ask his opinion here. Mickaёl, any thoughts? > >> +read access to ``/usr`` while denying all other handled accesses for the filesystem, >> +and a second rule allowing TCP binding on port 8080. >> >> .. code-block:: c >> >> @@ -355,7 +383,7 @@ Access rights >> ------------- >> >> .. kernel-doc:: include/uapi/linux/landlock.h >> - :identifiers: fs_access >> + :identifiers: fs_access net_access >> >> Creating a new ruleset >> ---------------------- >> @@ -374,6 +402,7 @@ Extending a ruleset >> >> .. kernel-doc:: include/uapi/linux/landlock.h >> :identifiers: landlock_rule_type landlock_path_beneath_attr >> + landlock_net_service_attr >> >> Enforcing a ruleset >> ------------------- >> @@ -451,6 +480,12 @@ always allowed when using a kernel that only supports the first or second ABI. >> Starting with the Landlock ABI version 3, it is now possible to securely control >> truncation thanks to the new ``LANDLOCK_ACCESS_FS_TRUNCATE`` access right. >> >> +Network support (ABI < 4) >> +------------------------- >> + >> +Starting with the Landlock ABI version 4, it is now possible to restrict TCP >> +bind and connect actions to only a set of allowed ports. >> + >> .. _kernel_support: >> >> Kernel support >> @@ -469,6 +504,11 @@ still enable it by adding ``lsm=landlock,[...]`` to >> Documentation/admin-guide/kernel-parameters.rst thanks to the bootloader >> configuration. >> >> +To be able to explicitly allow TCP operations (e.g., adding a network rule with >> +``LANDLOCK_ACCESS_NET_TCP_BIND``), the kernel must support TCP (``CONFIG_INET=y``). >> +Otherwise, sys_landlock_add_rule() returns an ``EAFNOSUPPORT`` error, which can >> +safely be ignored because this kind of TCP operation is already not possible. >> + >> Questions and answers >> ===================== >> >> -- >> 2.25.1 >> > .
On 23/01/2023 10:38, Konstantin Meskhidze (A) wrote: > > > 1/22/2023 2:07 AM, Günther Noack пишет: [...] >>> @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock >>> ABI version. In this example, this is not required because all of the requested >>> ``allowed_access`` rights are already available in ABI 1. >>> >>> -We now have a ruleset with one rule allowing read access to ``/usr`` while >>> -denying all other handled accesses for the filesystem. The next step is to >>> -restrict the current thread from gaining more privileges (e.g. thanks to a SUID >>> -binary). >>> +For network access-control, we can add a set of rules that allow to use a port >>> +number for a specific action. All ports values must be defined in network byte >>> +order. >> >> What is the point of asking user space to convert this to network byte >> order? It seems to me that the kernel would be able to convert it to >> network byte order very easily internally and in a single place -- why >> ask all of the users to deal with that complexity? Am I overlooking >> something? > > I had a discussion about this issue with Mickaёl. > Please check these threads: > 1. > https://lore.kernel.org/netdev/49391484-7401-e7c7-d909-3bd6bd024731@digikod.net/ > 2. > https://lore.kernel.org/netdev/1ed20e34-c252-b849-ab92-78c82901c979@huawei.com/ I'm definitely not sure if this is the right solution, or if there is one. The rationale is to make it close to the current (POSIX) API. We didn't get many opinion about that but I'd really like to have a discussion about port endianness for this Landlock API. I looked at some code (e.g. see [1]) and it seems that using htons() might make application patching more complex after all. What do you think? Is there some network (syscall) API that don't use this convention? [1] https://github.com/landlock-lsm/tuto-lighttpd >> >>> + >>> +.. code-block:: c >>> + >>> + struct landlock_net_service_attr net_service = { >>> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >>> + .port = htons(8080), >>> + }; >> >> This is a more high-level comment: >> >> The notion of a 16-bit "port" seems to be specific to TCP and UDP -- >> how do you envision this struct to evolve if other protocols need to >> be supported in the future? > > When TCP restrictions land into Linux, we need to think about UDP > support. Then other protocols will be on the road. Anyway you are right > this struct will be evolving in long term, but I don't have a particular > envision now. Thanks for the question - we need to think about it. >> >> Should this struct and the associated constants have "TCP" in its >> name, and other protocols use a separate struct in the future? Other protocols such as AF_VSOCK uses a 32-bit port. We could use a 32-bits port field or ever a 64-bit one. The later could make more sense because each field would eventually be aligned on 64-bit. Picking a 16-bit value was to help developers (and compilers/linters) with the "correct" type (for TCP). If we think about protocols other than TCP and UDP (e.g. AF_VSOCK), it could make sense to have a dedicated attr struct specifying other properties (e.g. CID). Anyway, the API is flexible but it would be nice to not mess with it too much. What do you think? >> >>> + >>> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, >>> + &net_service, 0); >>> + >>> +The next step is to restrict the current thread from gaining more privileges >>> +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing >> ^^^^^^ >> "through" a SUID binary? "thanks to" sounds like it's desired >> to do that, while we're actually trying to prevent it here? > > This is Mickaёl's part. Let's ask his opinion here. > > Mickaёl, any thoughts? Yep, "through" looks better.
1/27/2023 9:22 PM, Mickaël Salaün пишет: > > On 23/01/2023 10:38, Konstantin Meskhidze (A) wrote: >> >> >> 1/22/2023 2:07 AM, Günther Noack пишет: > > [...] > >>>> @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock >>>> ABI version. In this example, this is not required because all of the requested >>>> ``allowed_access`` rights are already available in ABI 1. >>>> >>>> -We now have a ruleset with one rule allowing read access to ``/usr`` while >>>> -denying all other handled accesses for the filesystem. The next step is to >>>> -restrict the current thread from gaining more privileges (e.g. thanks to a SUID >>>> -binary). >>>> +For network access-control, we can add a set of rules that allow to use a port >>>> +number for a specific action. All ports values must be defined in network byte >>>> +order. >>> >>> What is the point of asking user space to convert this to network byte >>> order? It seems to me that the kernel would be able to convert it to >>> network byte order very easily internally and in a single place -- why >>> ask all of the users to deal with that complexity? Am I overlooking >>> something? >> >> I had a discussion about this issue with Mickaёl. >> Please check these threads: >> 1. >> https://lore.kernel.org/netdev/49391484-7401-e7c7-d909-3bd6bd024731@digikod.net/ >> 2. >> https://lore.kernel.org/netdev/1ed20e34-c252-b849-ab92-78c82901c979@huawei.com/ > > I'm definitely not sure if this is the right solution, or if there is > one. The rationale is to make it close to the current (POSIX) API. We > didn't get many opinion about that but I'd really like to have a > discussion about port endianness for this Landlock API. As for me, the kernel should take care about port converting. This work should be done under the hood. Any thoughts? > > I looked at some code (e.g. see [1]) and it seems that using htons() > might make application patching more complex after all. What do you > think? Is there some network (syscall) API that don't use this convention? > > [1] https://github.com/landlock-lsm/tuto-lighttpd > >>> >>>> + >>>> +.. code-block:: c >>>> + >>>> + struct landlock_net_service_attr net_service = { >>>> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >>>> + .port = htons(8080), >>>> + }; >>> >>> This is a more high-level comment: >>> >>> The notion of a 16-bit "port" seems to be specific to TCP and UDP -- >>> how do you envision this struct to evolve if other protocols need to >>> be supported in the future? >> >> When TCP restrictions land into Linux, we need to think about UDP >> support. Then other protocols will be on the road. Anyway you are right >> this struct will be evolving in long term, but I don't have a particular >> envision now. Thanks for the question - we need to think about it. >>> >>> Should this struct and the associated constants have "TCP" in its >>> name, and other protocols use a separate struct in the future? > > Other protocols such as AF_VSOCK uses a 32-bit port. We could use a > 32-bits port field or ever a 64-bit one. The later could make more sense > because each field would eventually be aligned on 64-bit. Picking a > 16-bit value was to help developers (and compilers/linters) with the > "correct" type (for TCP). > > If we think about protocols other than TCP and UDP (e.g. AF_VSOCK), it > could make sense to have a dedicated attr struct specifying other > properties (e.g. CID). Anyway, the API is flexible but it would be nice > to not mess with it too much. What do you think? > > >>> >>>> + >>>> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, >>>> + &net_service, 0); >>>> + >>>> +The next step is to restrict the current thread from gaining more privileges >>>> +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing >>> ^^^^^^ >>> "through" a SUID binary? "thanks to" sounds like it's desired >>> to do that, while we're actually trying to prevent it here? >> >> This is Mickaёl's part. Let's ask his opinion here. >> >> Mickaёl, any thoughts? > > Yep, "through" looks better. > .
1/27/2023 9:22 PM, Mickaël Salaün пишет: > > On 23/01/2023 10:38, Konstantin Meskhidze (A) wrote: >> >> >> 1/22/2023 2:07 AM, Günther Noack пишет: > > [...] > >>>> @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock >>>> ABI version. In this example, this is not required because all of the requested >>>> ``allowed_access`` rights are already available in ABI 1. >>>> >>>> -We now have a ruleset with one rule allowing read access to ``/usr`` while >>>> -denying all other handled accesses for the filesystem. The next step is to >>>> -restrict the current thread from gaining more privileges (e.g. thanks to a SUID >>>> -binary). >>>> +For network access-control, we can add a set of rules that allow to use a port >>>> +number for a specific action. All ports values must be defined in network byte >>>> +order. >>> >>> What is the point of asking user space to convert this to network byte >>> order? It seems to me that the kernel would be able to convert it to >>> network byte order very easily internally and in a single place -- why >>> ask all of the users to deal with that complexity? Am I overlooking >>> something? >> >> I had a discussion about this issue with Mickaёl. >> Please check these threads: >> 1. >> https://lore.kernel.org/netdev/49391484-7401-e7c7-d909-3bd6bd024731@digikod.net/ >> 2. >> https://lore.kernel.org/netdev/1ed20e34-c252-b849-ab92-78c82901c979@huawei.com/ > > I'm definitely not sure if this is the right solution, or if there is > one. The rationale is to make it close to the current (POSIX) API. We > didn't get many opinion about that but I'd really like to have a > discussion about port endianness for this Landlock API. > > I looked at some code (e.g. see [1]) and it seems that using htons() > might make application patching more complex after all. What do you > think? Is there some network (syscall) API that don't use this convention? > > [1] https://github.com/landlock-lsm/tuto-lighttpd > >>> >>>> + >>>> +.. code-block:: c >>>> + >>>> + struct landlock_net_service_attr net_service = { >>>> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >>>> + .port = htons(8080), >>>> + }; >>> >>> This is a more high-level comment: >>> >>> The notion of a 16-bit "port" seems to be specific to TCP and UDP -- >>> how do you envision this struct to evolve if other protocols need to >>> be supported in the future? >> >> When TCP restrictions land into Linux, we need to think about UDP >> support. Then other protocols will be on the road. Anyway you are right >> this struct will be evolving in long term, but I don't have a particular >> envision now. Thanks for the question - we need to think about it. >>> >>> Should this struct and the associated constants have "TCP" in its >>> name, and other protocols use a separate struct in the future? > > Other protocols such as AF_VSOCK uses a 32-bit port. We could use a > 32-bits port field or ever a 64-bit one. The later could make more sense > because each field would eventually be aligned on 64-bit. Picking a > 16-bit value was to help developers (and compilers/linters) with the > "correct" type (for TCP). > > If we think about protocols other than TCP and UDP (e.g. AF_VSOCK), it > could make sense to have a dedicated attr struct specifying other > properties (e.g. CID). Anyway, the API is flexible but it would be nice > to not mess with it too much. What do you think? > I think it would not be hard to add new protocols in future. You are right - the current API is more or less flexible. The main question is what other protocols are worth for landlocking. > >>> >>>> + >>>> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, >>>> + &net_service, 0); >>>> + >>>> +The next step is to restrict the current thread from gaining more privileges >>>> +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing >>> ^^^^^^ >>> "through" a SUID binary? "thanks to" sounds like it's desired >>> to do that, while we're actually trying to prevent it here? >> >> This is Mickaёl's part. Let's ask his opinion here. >> >> Mickaёl, any thoughts? > > Yep, "through" looks better. > .
On 30/01/2023 11:03, Konstantin Meskhidze (A) wrote: > > > 1/27/2023 9:22 PM, Mickaël Salaün пишет: >> >> On 23/01/2023 10:38, Konstantin Meskhidze (A) wrote: >>> >>> >>> 1/22/2023 2:07 AM, Günther Noack пишет: >> >> [...] >> >>>>> @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock >>>>> ABI version. In this example, this is not required because all of the requested >>>>> ``allowed_access`` rights are already available in ABI 1. >>>>> >>>>> -We now have a ruleset with one rule allowing read access to ``/usr`` while >>>>> -denying all other handled accesses for the filesystem. The next step is to >>>>> -restrict the current thread from gaining more privileges (e.g. thanks to a SUID >>>>> -binary). >>>>> +For network access-control, we can add a set of rules that allow to use a port >>>>> +number for a specific action. All ports values must be defined in network byte >>>>> +order. >>>> >>>> What is the point of asking user space to convert this to network byte >>>> order? It seems to me that the kernel would be able to convert it to >>>> network byte order very easily internally and in a single place -- why >>>> ask all of the users to deal with that complexity? Am I overlooking >>>> something? >>> >>> I had a discussion about this issue with Mickaёl. >>> Please check these threads: >>> 1. >>> https://lore.kernel.org/netdev/49391484-7401-e7c7-d909-3bd6bd024731@digikod.net/ >>> 2. >>> https://lore.kernel.org/netdev/1ed20e34-c252-b849-ab92-78c82901c979@huawei.com/ >> >> I'm definitely not sure if this is the right solution, or if there is >> one. The rationale is to make it close to the current (POSIX) API. We >> didn't get many opinion about that but I'd really like to have a >> discussion about port endianness for this Landlock API. > > As for me, the kernel should take care about port converting. This > work should be done under the hood. > > Any thoughts? > >> >> I looked at some code (e.g. see [1]) and it seems that using htons() >> might make application patching more complex after all. What do you >> think? Is there some network (syscall) API that don't use this convention? >> >> [1] https://github.com/landlock-lsm/tuto-lighttpd >> >>>> >>>>> + >>>>> +.. code-block:: c >>>>> + >>>>> + struct landlock_net_service_attr net_service = { >>>>> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >>>>> + .port = htons(8080), >>>>> + }; >>>> >>>> This is a more high-level comment: >>>> >>>> The notion of a 16-bit "port" seems to be specific to TCP and UDP -- >>>> how do you envision this struct to evolve if other protocols need to >>>> be supported in the future? >>> >>> When TCP restrictions land into Linux, we need to think about UDP >>> support. Then other protocols will be on the road. Anyway you are right >>> this struct will be evolving in long term, but I don't have a particular >>> envision now. Thanks for the question - we need to think about it. >>>> >>>> Should this struct and the associated constants have "TCP" in its >>>> name, and other protocols use a separate struct in the future? >> >> Other protocols such as AF_VSOCK uses a 32-bit port. We could use a >> 32-bits port field or ever a 64-bit one. The later could make more sense >> because each field would eventually be aligned on 64-bit. Picking a >> 16-bit value was to help developers (and compilers/linters) with the >> "correct" type (for TCP). Thinking more about this, let's use a __u64 port (and remove the explicit packing). The landlock_append_net_rule() function should use a __u16 port argument, but the add_rule_net_service() function should check that there is no overflow with the port attribute (not higher than U16_MAX) before passing it to landlock_append_net_rule(). We should prioritize flexibility for the kernel UAPI over stricter types. User space libraries can improve this kind of types with a more complex API. Big endian can make sense for a pure network API because the port value (and the IP address) is passed to other machines through the network, as-is. However, with Landlock, the port value is only used by the kernel. Moreover, in practice, port values are mostly converted when filling the sockaddr*_in structs. It would then make it more risky to ask developers another explicit htons() conversion for Landlock syscalls. Let's stick to the host endianess and let the kernel do the conversion. Please include these rationales in code comments. We also need to update the tests for endianess, but still check big and little endian consistency as it is currently done in these tests. A new test should be added to check port boundaries with: - port = 0 - port = U16_MAX - port = U16_MAX + 1 (which should get an EINVAL) - port = U16_MAX + 2 (to check u16 casting != 0) - port = U32_MAX + 1 - port = U32_MAX + 2 >> >> If we think about protocols other than TCP and UDP (e.g. AF_VSOCK), it >> could make sense to have a dedicated attr struct specifying other >> properties (e.g. CID). Anyway, the API is flexible but it would be nice >> to not mess with it too much. What do you think? >> >> >>>> >>>>> + >>>>> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, >>>>> + &net_service, 0); >>>>> + >>>>> +The next step is to restrict the current thread from gaining more privileges >>>>> +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing >>>> ^^^^^^ >>>> "through" a SUID binary? "thanks to" sounds like it's desired >>>> to do that, while we're actually trying to prevent it here? >>> >>> This is Mickaёl's part. Let's ask his opinion here. >>> >>> Mickaёl, any thoughts? >> >> Yep, "through" looks better. >> .
2/21/2023 7:16 PM, Mickaël Salaün пишет: > > On 30/01/2023 11:03, Konstantin Meskhidze (A) wrote: >> >> >> 1/27/2023 9:22 PM, Mickaël Salaün пишет: >>> >>> On 23/01/2023 10:38, Konstantin Meskhidze (A) wrote: >>>> >>>> >>>> 1/22/2023 2:07 AM, Günther Noack пишет: >>> >>> [...] >>> >>>>>> @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock >>>>>> ABI version. In this example, this is not required because all of the requested >>>>>> ``allowed_access`` rights are already available in ABI 1. >>>>>> >>>>>> -We now have a ruleset with one rule allowing read access to ``/usr`` while >>>>>> -denying all other handled accesses for the filesystem. The next step is to >>>>>> -restrict the current thread from gaining more privileges (e.g. thanks to a SUID >>>>>> -binary). >>>>>> +For network access-control, we can add a set of rules that allow to use a port >>>>>> +number for a specific action. All ports values must be defined in network byte >>>>>> +order. >>>>> >>>>> What is the point of asking user space to convert this to network byte >>>>> order? It seems to me that the kernel would be able to convert it to >>>>> network byte order very easily internally and in a single place -- why >>>>> ask all of the users to deal with that complexity? Am I overlooking >>>>> something? >>>> >>>> I had a discussion about this issue with Mickaёl. >>>> Please check these threads: >>>> 1. >>>> https://lore.kernel.org/netdev/49391484-7401-e7c7-d909-3bd6bd024731@digikod.net/ >>>> 2. >>>> https://lore.kernel.org/netdev/1ed20e34-c252-b849-ab92-78c82901c979@huawei.com/ >>> >>> I'm definitely not sure if this is the right solution, or if there is >>> one. The rationale is to make it close to the current (POSIX) API. We >>> didn't get many opinion about that but I'd really like to have a >>> discussion about port endianness for this Landlock API. >> >> As for me, the kernel should take care about port converting. This >> work should be done under the hood. >> >> Any thoughts? >> >>> >>> I looked at some code (e.g. see [1]) and it seems that using htons() >>> might make application patching more complex after all. What do you >>> think? Is there some network (syscall) API that don't use this convention? >>> >>> [1] https://github.com/landlock-lsm/tuto-lighttpd >>> >>>>> >>>>>> + >>>>>> +.. code-block:: c >>>>>> + >>>>>> + struct landlock_net_service_attr net_service = { >>>>>> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >>>>>> + .port = htons(8080), >>>>>> + }; >>>>> >>>>> This is a more high-level comment: >>>>> >>>>> The notion of a 16-bit "port" seems to be specific to TCP and UDP -- >>>>> how do you envision this struct to evolve if other protocols need to >>>>> be supported in the future? >>>> >>>> When TCP restrictions land into Linux, we need to think about UDP >>>> support. Then other protocols will be on the road. Anyway you are right >>>> this struct will be evolving in long term, but I don't have a particular >>>> envision now. Thanks for the question - we need to think about it. >>>>> >>>>> Should this struct and the associated constants have "TCP" in its >>>>> name, and other protocols use a separate struct in the future? >>> >>> Other protocols such as AF_VSOCK uses a 32-bit port. We could use a >>> 32-bits port field or ever a 64-bit one. The later could make more sense >>> because each field would eventually be aligned on 64-bit. Picking a >>> 16-bit value was to help developers (and compilers/linters) with the >>> "correct" type (for TCP). > > Thinking more about this, let's use a __u64 port (and remove the > explicit packing). The landlock_append_net_rule() function should use a > __u16 port argument, but the add_rule_net_service() function should > check that there is no overflow with the port attribute (not higher than > U16_MAX) before passing it to landlock_append_net_rule(). We should > prioritize flexibility for the kernel UAPI over stricter types. User > space libraries can improve this kind of types with a more complex API. > > Big endian can make sense for a pure network API because the port value > (and the IP address) is passed to other machines through the network, > as-is. However, with Landlock, the port value is only used by the > kernel. Moreover, in practice, port values are mostly converted when > filling the sockaddr*_in structs. It would then make it more risky to > ask developers another explicit htons() conversion for Landlock > syscalls. Let's stick to the host endianess and let the kernel do the > conversion. > > Please include these rationales in code comments. We also need to update > the tests for endianess, but still check big and little endian > consistency as it is currently done in these tests. A new test should be > added to check port boundaries with: > - port = 0 > - port = U16_MAX port = U16_MAX value passes. > - port = U16_MAX + 1 (which should get an EINVAL) port = U16_MAX + 1 after casting is 0, EINVAL is returned. > - port = U16_MAX + 2 (to check u16 casting != 0) port = U16_MAX + 2 after casting is 1, is it passes? > - port = U32_MAX + 1 > - port = U32_MAX + 2 Don't you think that all port values >= U16_MAX + 1, EINVAL should be returned? > > >>> >>> If we think about protocols other than TCP and UDP (e.g. AF_VSOCK), it >>> could make sense to have a dedicated attr struct specifying other >>> properties (e.g. CID). Anyway, the API is flexible but it would be nice >>> to not mess with it too much. What do you think? >>> >>> >>>>> >>>>>> + >>>>>> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, >>>>>> + &net_service, 0); >>>>>> + >>>>>> +The next step is to restrict the current thread from gaining more privileges >>>>>> +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing >>>>> ^^^^^^ >>>>> "through" a SUID binary? "thanks to" sounds like it's desired >>>>> to do that, while we're actually trying to prevent it here? >>>> >>>> This is Mickaёl's part. Let's ask his opinion here. >>>> >>>> Mickaёl, any thoughts? >>> >>> Yep, "through" looks better. >>> . > .
On 06/03/2023 14:43, Konstantin Meskhidze (A) wrote: > > > 2/21/2023 7:16 PM, Mickaël Salaün пишет: >> >> On 30/01/2023 11:03, Konstantin Meskhidze (A) wrote: >>> >>> >>> 1/27/2023 9:22 PM, Mickaël Salaün пишет: >>>> >>>> On 23/01/2023 10:38, Konstantin Meskhidze (A) wrote: >>>>> >>>>> >>>>> 1/22/2023 2:07 AM, Günther Noack пишет: >>>> >>>> [...] >>>> >>>>>>> @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock >>>>>>> ABI version. In this example, this is not required because all of the requested >>>>>>> ``allowed_access`` rights are already available in ABI 1. >>>>>>> >>>>>>> -We now have a ruleset with one rule allowing read access to ``/usr`` while >>>>>>> -denying all other handled accesses for the filesystem. The next step is to >>>>>>> -restrict the current thread from gaining more privileges (e.g. thanks to a SUID >>>>>>> -binary). >>>>>>> +For network access-control, we can add a set of rules that allow to use a port >>>>>>> +number for a specific action. All ports values must be defined in network byte >>>>>>> +order. >>>>>> >>>>>> What is the point of asking user space to convert this to network byte >>>>>> order? It seems to me that the kernel would be able to convert it to >>>>>> network byte order very easily internally and in a single place -- why >>>>>> ask all of the users to deal with that complexity? Am I overlooking >>>>>> something? >>>>> >>>>> I had a discussion about this issue with Mickaёl. >>>>> Please check these threads: >>>>> 1. >>>>> https://lore.kernel.org/netdev/49391484-7401-e7c7-d909-3bd6bd024731@digikod.net/ >>>>> 2. >>>>> https://lore.kernel.org/netdev/1ed20e34-c252-b849-ab92-78c82901c979@huawei.com/ >>>> >>>> I'm definitely not sure if this is the right solution, or if there is >>>> one. The rationale is to make it close to the current (POSIX) API. We >>>> didn't get many opinion about that but I'd really like to have a >>>> discussion about port endianness for this Landlock API. >>> >>> As for me, the kernel should take care about port converting. This >>> work should be done under the hood. >>> >>> Any thoughts? >>> >>>> >>>> I looked at some code (e.g. see [1]) and it seems that using htons() >>>> might make application patching more complex after all. What do you >>>> think? Is there some network (syscall) API that don't use this convention? >>>> >>>> [1] https://github.com/landlock-lsm/tuto-lighttpd >>>> >>>>>> >>>>>>> + >>>>>>> +.. code-block:: c >>>>>>> + >>>>>>> + struct landlock_net_service_attr net_service = { >>>>>>> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >>>>>>> + .port = htons(8080), >>>>>>> + }; >>>>>> >>>>>> This is a more high-level comment: >>>>>> >>>>>> The notion of a 16-bit "port" seems to be specific to TCP and UDP -- >>>>>> how do you envision this struct to evolve if other protocols need to >>>>>> be supported in the future? >>>>> >>>>> When TCP restrictions land into Linux, we need to think about UDP >>>>> support. Then other protocols will be on the road. Anyway you are right >>>>> this struct will be evolving in long term, but I don't have a particular >>>>> envision now. Thanks for the question - we need to think about it. >>>>>> >>>>>> Should this struct and the associated constants have "TCP" in its >>>>>> name, and other protocols use a separate struct in the future? >>>> >>>> Other protocols such as AF_VSOCK uses a 32-bit port. We could use a >>>> 32-bits port field or ever a 64-bit one. The later could make more sense >>>> because each field would eventually be aligned on 64-bit. Picking a >>>> 16-bit value was to help developers (and compilers/linters) with the >>>> "correct" type (for TCP). >> >> Thinking more about this, let's use a __u64 port (and remove the >> explicit packing). The landlock_append_net_rule() function should use a >> __u16 port argument, but the add_rule_net_service() function should >> check that there is no overflow with the port attribute (not higher than >> U16_MAX) before passing it to landlock_append_net_rule(). We should >> prioritize flexibility for the kernel UAPI over stricter types. User >> space libraries can improve this kind of types with a more complex API. >> >> Big endian can make sense for a pure network API because the port value >> (and the IP address) is passed to other machines through the network, >> as-is. However, with Landlock, the port value is only used by the >> kernel. Moreover, in practice, port values are mostly converted when >> filling the sockaddr*_in structs. It would then make it more risky to >> ask developers another explicit htons() conversion for Landlock >> syscalls. Let's stick to the host endianess and let the kernel do the >> conversion. >> >> Please include these rationales in code comments. We also need to update >> the tests for endianess, but still check big and little endian >> consistency as it is currently done in these tests. A new test should be >> added to check port boundaries with: >> - port = 0 >> - port = U16_MAX > port = U16_MAX value passes. correct > >> - port = U16_MAX + 1 (which should get an EINVAL) > port = U16_MAX + 1 after casting is 0, EINVAL is returned. In the tests, we want the casting to be be done by the kernel. The test should then pass 0x10000 to the struct and the kernel should return EINVAL because it is greater than U16_MAX, not because it is zero. > >> - port = U16_MAX + 2 (to check u16 casting != 0) > port = U16_MAX + 2 after casting is 1, is it passes? In this case, 0x10001 should be rejected by the kernel (and return EINVAL) because it is greater than U16_MAX. > >> - port = U32_MAX + 1 >> - port = U32_MAX + 2 > > Don't you think that all port values >= U16_MAX + 1, EINVAL should > be returned? All port values > U16_MAX should indeed return EINVAL, and tests should check kernel casting (i.e. the kernel must check the 64-bit value before casting it to a 16-bit value and only check the casted zero). I didn't mean that these cases should pass, only that they should be tested, but I think you got it. ;) >> >> >>>> >>>> If we think about protocols other than TCP and UDP (e.g. AF_VSOCK), it >>>> could make sense to have a dedicated attr struct specifying other >>>> properties (e.g. CID). Anyway, the API is flexible but it would be nice >>>> to not mess with it too much. What do you think? >>>> >>>> >>>>>> >>>>>>> + >>>>>>> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, >>>>>>> + &net_service, 0); >>>>>>> + >>>>>>> +The next step is to restrict the current thread from gaining more privileges >>>>>>> +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing >>>>>> ^^^^^^ >>>>>> "through" a SUID binary? "thanks to" sounds like it's desired >>>>>> to do that, while we're actually trying to prevent it here? >>>>> >>>>> This is Mickaёl's part. Let's ask his opinion here. >>>>> >>>>> Mickaёl, any thoughts? >>>> >>>> Yep, "through" looks better. >>>> . >> .
3/6/2023 7:09 PM, Mickaël Salaün пишет: > > On 06/03/2023 14:43, Konstantin Meskhidze (A) wrote: >> >> >> 2/21/2023 7:16 PM, Mickaël Salaün пишет: >>> >>> On 30/01/2023 11:03, Konstantin Meskhidze (A) wrote: >>>> >>>> >>>> 1/27/2023 9:22 PM, Mickaël Salaün пишет: >>>>> >>>>> On 23/01/2023 10:38, Konstantin Meskhidze (A) wrote: >>>>>> >>>>>> >>>>>> 1/22/2023 2:07 AM, Günther Noack пишет: >>>>> >>>>> [...] >>>>> >>>>>>>> @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock >>>>>>>> ABI version. In this example, this is not required because all of the requested >>>>>>>> ``allowed_access`` rights are already available in ABI 1. >>>>>>>> >>>>>>>> -We now have a ruleset with one rule allowing read access to ``/usr`` while >>>>>>>> -denying all other handled accesses for the filesystem. The next step is to >>>>>>>> -restrict the current thread from gaining more privileges (e.g. thanks to a SUID >>>>>>>> -binary). >>>>>>>> +For network access-control, we can add a set of rules that allow to use a port >>>>>>>> +number for a specific action. All ports values must be defined in network byte >>>>>>>> +order. >>>>>>> >>>>>>> What is the point of asking user space to convert this to network byte >>>>>>> order? It seems to me that the kernel would be able to convert it to >>>>>>> network byte order very easily internally and in a single place -- why >>>>>>> ask all of the users to deal with that complexity? Am I overlooking >>>>>>> something? >>>>>> >>>>>> I had a discussion about this issue with Mickaёl. >>>>>> Please check these threads: >>>>>> 1. >>>>>> https://lore.kernel.org/netdev/49391484-7401-e7c7-d909-3bd6bd024731@digikod.net/ >>>>>> 2. >>>>>> https://lore.kernel.org/netdev/1ed20e34-c252-b849-ab92-78c82901c979@huawei.com/ >>>>> >>>>> I'm definitely not sure if this is the right solution, or if there is >>>>> one. The rationale is to make it close to the current (POSIX) API. We >>>>> didn't get many opinion about that but I'd really like to have a >>>>> discussion about port endianness for this Landlock API. >>>> >>>> As for me, the kernel should take care about port converting. This >>>> work should be done under the hood. >>>> >>>> Any thoughts? >>>> >>>>> >>>>> I looked at some code (e.g. see [1]) and it seems that using htons() >>>>> might make application patching more complex after all. What do you >>>>> think? Is there some network (syscall) API that don't use this convention? >>>>> >>>>> [1] https://github.com/landlock-lsm/tuto-lighttpd >>>>> >>>>>>> >>>>>>>> + >>>>>>>> +.. code-block:: c >>>>>>>> + >>>>>>>> + struct landlock_net_service_attr net_service = { >>>>>>>> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >>>>>>>> + .port = htons(8080), >>>>>>>> + }; >>>>>>> >>>>>>> This is a more high-level comment: >>>>>>> >>>>>>> The notion of a 16-bit "port" seems to be specific to TCP and UDP -- >>>>>>> how do you envision this struct to evolve if other protocols need to >>>>>>> be supported in the future? >>>>>> >>>>>> When TCP restrictions land into Linux, we need to think about UDP >>>>>> support. Then other protocols will be on the road. Anyway you are right >>>>>> this struct will be evolving in long term, but I don't have a particular >>>>>> envision now. Thanks for the question - we need to think about it. >>>>>>> >>>>>>> Should this struct and the associated constants have "TCP" in its >>>>>>> name, and other protocols use a separate struct in the future? >>>>> >>>>> Other protocols such as AF_VSOCK uses a 32-bit port. We could use a >>>>> 32-bits port field or ever a 64-bit one. The later could make more sense >>>>> because each field would eventually be aligned on 64-bit. Picking a >>>>> 16-bit value was to help developers (and compilers/linters) with the >>>>> "correct" type (for TCP). >>> >>> Thinking more about this, let's use a __u64 port (and remove the >>> explicit packing). The landlock_append_net_rule() function should use a >>> __u16 port argument, but the add_rule_net_service() function should >>> check that there is no overflow with the port attribute (not higher than >>> U16_MAX) before passing it to landlock_append_net_rule(). We should >>> prioritize flexibility for the kernel UAPI over stricter types. User >>> space libraries can improve this kind of types with a more complex API. >>> >>> Big endian can make sense for a pure network API because the port value >>> (and the IP address) is passed to other machines through the network, >>> as-is. However, with Landlock, the port value is only used by the >>> kernel. Moreover, in practice, port values are mostly converted when >>> filling the sockaddr*_in structs. It would then make it more risky to >>> ask developers another explicit htons() conversion for Landlock >>> syscalls. Let's stick to the host endianess and let the kernel do the >>> conversion. >>> >>> Please include these rationales in code comments. We also need to update >>> the tests for endianess, but still check big and little endian >>> consistency as it is currently done in these tests. A new test should be >>> added to check port boundaries with: >>> - port = 0 >>> - port = U16_MAX >> port = U16_MAX value passes. > > correct > >> >>> - port = U16_MAX + 1 (which should get an EINVAL) >> port = U16_MAX + 1 after casting is 0, EINVAL is returned. > > In the tests, we want the casting to be be done by the kernel. The test > should then pass 0x10000 to the struct and the kernel should return > EINVAL because it is greater than U16_MAX, not because it is zero. > >> >>> - port = U16_MAX + 2 (to check u16 casting != 0) >> port = U16_MAX + 2 after casting is 1, is it passes? > > In this case, 0x10001 should be rejected by the kernel (and return > EINVAL) because it is greater than U16_MAX. > >> >>> - port = U32_MAX + 1 >>> - port = U32_MAX + 2 >> >> Don't you think that all port values >= U16_MAX + 1, EINVAL should >> be returned? > > All port values > U16_MAX should indeed return EINVAL, and tests should > check kernel casting (i.e. the kernel must check the 64-bit value before > casting it to a 16-bit value and only check the casted zero). I didn't > mean that these cases should pass, only that they should be tested, but > I think you got it. ;) Yep. I got the point. Thanks. > >>> >>> >>>>> >>>>> If we think about protocols other than TCP and UDP (e.g. AF_VSOCK), it >>>>> could make sense to have a dedicated attr struct specifying other >>>>> properties (e.g. CID). Anyway, the API is flexible but it would be nice >>>>> to not mess with it too much. What do you think? >>>>> >>>>> >>>>>>> >>>>>>>> + >>>>>>>> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, >>>>>>>> + &net_service, 0); >>>>>>>> + >>>>>>>> +The next step is to restrict the current thread from gaining more privileges >>>>>>>> +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing >>>>>>> ^^^^^^ >>>>>>> "through" a SUID binary? "thanks to" sounds like it's desired >>>>>>> to do that, while we're actually trying to prevent it here? >>>>>> >>>>>> This is Mickaёl's part. Let's ask his opinion here. >>>>>> >>>>>> Mickaёl, any thoughts? >>>>> >>>>> Yep, "through" looks better. >>>>> . >>> . > .
diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst index d8cd8cd9ce25..980558b879d6 100644 --- a/Documentation/userspace-api/landlock.rst +++ b/Documentation/userspace-api/landlock.rst @@ -11,10 +11,10 @@ Landlock: unprivileged access control :Date: October 2022 The goal of Landlock is to enable to restrict ambient rights (e.g. global -filesystem access) for a set of processes. Because Landlock is a stackable -LSM, it makes possible to create safe security sandboxes as new security layers -in addition to the existing system-wide access-controls. This kind of sandbox -is expected to help mitigate the security impact of bugs or +filesystem or network access) for a set of processes. Because Landlock +is a stackable LSM, it makes possible to create safe security sandboxes as new +security layers in addition to the existing system-wide access-controls. This +kind of sandbox is expected to help mitigate the security impact of bugs or unexpected/malicious behaviors in user space applications. Landlock empowers any process, including unprivileged ones, to securely restrict themselves. @@ -30,18 +30,20 @@ Landlock rules A Landlock rule describes an action on an object. An object is currently a file hierarchy, and the related filesystem actions are defined with `access -rights`_. A set of rules is aggregated in a ruleset, which can then restrict -the thread enforcing it, and its future children. +rights`_. Since ABI version 4 a port data appears with related network actions +for TCP socket families. A set of rules is aggregated in a ruleset, which +can then restrict the thread enforcing it, and its future children. Defining and enforcing a security policy ---------------------------------------- We first need to define the ruleset that will contain our rules. For this example, the ruleset will contain rules that only allow read actions, but write -actions will be denied. The ruleset then needs to handle both of these kind of +actions will be denied. The ruleset then needs to handle both of these kind of actions. This is required for backward and forward compatibility (i.e. the kernel and user space may not know each other's supported restrictions), hence -the need to be explicit about the denied-by-default access rights. +the need to be explicit about the denied-by-default access rights. Also ruleset +will have network rules for specific ports, so it should handle network actions. .. code-block:: c @@ -62,6 +64,9 @@ the need to be explicit about the denied-by-default access rights. LANDLOCK_ACCESS_FS_MAKE_SYM | LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_TRUNCATE, + .handled_access_net = + LANDLOCK_ACCESS_NET_BIND_TCP | + LANDLOCK_ACCESS_NET_CONNECT_TCP, }; Because we may not know on which kernel version an application will be @@ -70,14 +75,18 @@ should try to protect users as much as possible whatever the kernel they are using. To avoid binary enforcement (i.e. either all security features or none), we can leverage a dedicated Landlock command to get the current version of the Landlock ABI and adapt the handled accesses. Let's check if we should -remove the ``LANDLOCK_ACCESS_FS_REFER`` or ``LANDLOCK_ACCESS_FS_TRUNCATE`` -access rights, which are only supported starting with the second and third -version of the ABI. +remove the ``LANDLOCK_ACCESS_FS_REFER`` or ``LANDLOCK_ACCESS_FS_TRUNCATE`` or +network access rights, which are only supported starting with the second, +third and fourth version of the ABI. .. code-block:: c int abi; + #define ACCESS_NET_BIND_CONNECT ( \ + LANDLOCK_ACCESS_NET_BIND_TCP | \ + LANDLOCK_ACCESS_NET_CONNECT_TCP) + abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); if (abi < 0) { /* Degrades gracefully if Landlock is not handled. */ @@ -92,6 +101,11 @@ version of the ABI. case 2: /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */ ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE; + case 3: + /* Removes network support for ABI < 4 */ + ruleset_attr.handled_access_net &= + ~(LANDLOCK_ACCESS_NET_BIND_TCP | + LANDLOCK_ACCESS_NET_CONNECT_TCP); } This enables to create an inclusive ruleset that will contain our rules. @@ -143,10 +157,24 @@ for the ruleset creation, by filtering access rights according to the Landlock ABI version. In this example, this is not required because all of the requested ``allowed_access`` rights are already available in ABI 1. -We now have a ruleset with one rule allowing read access to ``/usr`` while -denying all other handled accesses for the filesystem. The next step is to -restrict the current thread from gaining more privileges (e.g. thanks to a SUID -binary). +For network access-control, we can add a set of rules that allow to use a port +number for a specific action. All ports values must be defined in network byte +order. + +.. code-block:: c + + struct landlock_net_service_attr net_service = { + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, + .port = htons(8080), + }; + + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, + &net_service, 0); + +The next step is to restrict the current thread from gaining more privileges +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing +read access to ``/usr`` while denying all other handled accesses for the filesystem, +and a second rule allowing TCP binding on port 8080. .. code-block:: c @@ -355,7 +383,7 @@ Access rights ------------- .. kernel-doc:: include/uapi/linux/landlock.h - :identifiers: fs_access + :identifiers: fs_access net_access Creating a new ruleset ---------------------- @@ -374,6 +402,7 @@ Extending a ruleset .. kernel-doc:: include/uapi/linux/landlock.h :identifiers: landlock_rule_type landlock_path_beneath_attr + landlock_net_service_attr Enforcing a ruleset ------------------- @@ -451,6 +480,12 @@ always allowed when using a kernel that only supports the first or second ABI. Starting with the Landlock ABI version 3, it is now possible to securely control truncation thanks to the new ``LANDLOCK_ACCESS_FS_TRUNCATE`` access right. +Network support (ABI < 4) +------------------------- + +Starting with the Landlock ABI version 4, it is now possible to restrict TCP +bind and connect actions to only a set of allowed ports. + .. _kernel_support: Kernel support @@ -469,6 +504,11 @@ still enable it by adding ``lsm=landlock,[...]`` to Documentation/admin-guide/kernel-parameters.rst thanks to the bootloader configuration. +To be able to explicitly allow TCP operations (e.g., adding a network rule with +``LANDLOCK_ACCESS_NET_TCP_BIND``), the kernel must support TCP (``CONFIG_INET=y``). +Otherwise, sys_landlock_add_rule() returns an ``EAFNOSUPPORT`` error, which can +safely be ignored because this kind of TCP operation is already not possible. + Questions and answers =====================
Describe network access rules for TCP sockets. Add network access example in the tutorial. Add kernel configuration support for network. Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> --- Changes since v8: * Minor refactoring. Changes since v7: * Fixes documentaion logic errors and typos as Mickaёl suggested: https://lore.kernel.org/netdev/9f354862-2bc3-39ea-92fd-53803d9bbc21@digikod.net/ Changes since v6: * Adds network support documentaion. --- Documentation/userspace-api/landlock.rst | 72 ++++++++++++++++++------ 1 file changed, 56 insertions(+), 16 deletions(-)