| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <02806c8e-e255-232b-1722-65ea1dba2948@huawei.com>
Date: Thu, 30 Dec 2021 04:43:14 +0300
From: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: <linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
<netfilter@...r.kernel.org>, <yusongping@...wei.com>,
<artem.kuzin@...wei.com>
Subject: Re: [RFC PATCH 0/1] Landlock network PoC
Hi Mickaël,
Thanks for the quick reply.
I apologise that I did not follow some rules here.
I'm a newbie in the Linux community communication and probably missed
some important points.
Thank you for noticing about them - I will fix my misses.
12/29/2021 1:09 AM, Mickaël Salaün wrote:
> Hi Konstantin,
>
> Please read the full how-to about sending patches:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
> There are at least these issues:
> - no link to the previous version:
> https://lore.kernel.org/linux-security-module/20211210072123.386713-1-konstantin.meskhidze@huawei.com/
>
> - no version: [RFC PATCH v2 0/1]
> - even if there is only one patch, please make the cover letter a
> separate email (i.e. git format-patch --cover-letter).
I got it. My mistake.
Anyway I can resend the patch ,if you would like, with all corrections.
>
> It seems that you missed some of my previous (inlined) comments, you
> didn't reply to most of them:
> https://lore.kernel.org/linux-security-module/b50ed53a-683e-77cf-9dc2-f4ae1b5fa0fd@digikod.net/
>
Sorry about that. I will take it into account.
I will reply your previous comments.
>
> Did you test your changes before submitting this patch series?
I tested it into QEMU environment. I wrote net_sandboxer.c
file just for test insering network rules and bind() and connect() syscalls.
Also I launched your sandboxer.c and it worked well.
I'm going to provide seltests in another patch series.
>
> This series can only forbids TCP connects and binds to processes other
> than the one creating the ruleset, which doesn't make sense here (as I
> already explained). TCP ports are not checked.
Yes. I provided just inserting network access rules into a prosess
ruleset.For the port checking, I wanted to ask your opinion about how it
would possible to insert port rule. Cause I have my view, that differs
from your one.
>
> If you have any doubts about any of the comments, please rephrase,
> challenge them and ask questions.
>
>
> On 28/12/2021 12:52, Konstantin Meskhidze wrote:
>> Hi, all!
>> Here is another PoC patch for Landlock network confinement.
>> Now 2 hooks are supported for TCP sockets:
>> - hook_socket_bind()
>> - hook_socket_connect()
>>
>> After architectuire has been more clear, there will be a patch with
>> selftests.
>>
>> Please welcome with any comments and suggestions.
>>
>>
>> Implementation related issues
>> =============================
>>
>> 1. It was suggested by Mickaёl using new network rules
>> attributes, like:
>>
>> struct landlock_net_service_attr {
>> __u64 allowed_access; // LANDLOCK_NET_*_TCP
>> __u16 port;
>> } __attribute__((packed));
>>
>> I found that, if we want to support inserting port attributes,
>> it's needed to add port member into struct landlock_rule:
>>
>> struct landlock_rule {
>> ...
>> struct landlock_object *object;
>> /**
>> * @num_layers: Number of entries in @layers.
>> */
>> u32 num_layers;
>>
>> u16 port;
>
> In this case the "object" is indeed defined by a port. You can then
> create an union containing either a struct landlock_object pointer or a
> raw value (here a u16 port). Of course, every code that use the object
> field must be audited and updated accordingly. I think the following
> update should be a good approach (with updated documentation):
>
> struct landlock_rule {
> [...]
> union {
> struct landlock_object *ptr;
> uintptr_t data;
> } object;
> [...]
> };
>
> …and then a function helper to convert raw data to/from port.
>
> It would be a good idea to use a dedicated tree for objects identified
> by (typed) data vs. pointer:
>
> struct landlock_ruleset {
> struct rb_root root_inode; // i.e. the current "root" field.
> struct rb_root root_net_port;
> [...]
> };
>
This approach makes sense. I did not think in this way. I was following
the concept that every rule must be tied to a real kernel object. Using
pseudo "object" defined by a port is a good idea. Thanks.
>
>> ...
>> };
>>
>> In this case 2 functions landlock_insert_rule() and insert_rule()
>> must be refactored;
>>
>> But, if struct landlock_layer be modified -
>>
>> struct landlock_layer {
>> /**
>> * @level: Position of this layer in the layer stack.
>> */
>> u16 level;
>> /**
>> * @access: Bitfield of allowed actions on the kernel object.
>> They are
>> * relative to the object type (e.g. %LANDLOCK_ACTION_FS_READ).
>> */
>> u16 access;
>>
>> u16 port;
>
> No, struct landlock_layer doesn't need to be modified. This struct is
> independent from the type of object. I talked about that in the previous
> series.
I got it. Thanks for the comment.
>
>> };
>> so, just one landlock_insert_rule() must be slightly refactored.
>> Also many new attributes could be easily supported in future versions.
>>
>> 2. access_masks[] member of struct landlock_ruleset was modified
>> to support multiple rule type masks.
>> I suggest using 2D array semantic for convenient usage:
>> access_masks[rule_type][layer_level]
>>
>> But its also possible to use 1D array with modulo arithmetic:
>> access_masks[rule_type % layer_level]
>
> What about my previous suggestion to use a (well defined) upper bit to
> identify the type of access?
>
Maybe I missed your point here. But I can't see how to identify
different rule types here. If there just one ruleset created, so there
is just one access_mask[0] in the ruleset. Its either can be used for
filesystem mask or for network one. To support both rule type we need to
use at least access_mask[] array with size of 2.
Also using upper bit to identify access rule type will narrow down
possible access ammount in future version.
Anyway please corrent me if I'm wrong here.
>>
>> 3. Kernel objects.
>> As was disscussed earlier, base Landlock version supports some file
>> access
>> rules, and inodes objects (files' and directories' inodes) are used to
>> tie
>> filesystem landlock rules.
>> For socket operations it makes sense tagging underlying socket inode by
>> landlock rules and it's perfectly fits to subject-object concept where
>> proccess is a subject and object is one of kernel objects: inodes,
>> sockets,
>> ect. But here is one issue that should be also discussed and solved.
>> Now there is an undergoing work for Landlock support in RUNC and other
>> container environments:
>> https://github.com/opencontainers/runtime-spec/issues/1110
>> https://github.com/opencontainers/runc/issues/2859
>>
>> When RUNC wants to launch a container it first reads a spec.json file
>> with container specification. Here is an example landlock rules
>> specification in spec.json:
>>
>> "landlock": {
>> "rules": [
>> {
>> "type": "path_beneath",
>> "restrictPaths": {
>> "allowedAccess": [
>> "LANDLOCK_ACCESS_FS_EXECUTE",
>> "LANDLOCK_ACCESS_FS_READ_FILE",
>> "LANDLOCK_ACCESS_FS_READ_DIR"
>> ],
>> "paths": [
>> "/usr",
>> "/bin"
>> ]
>> }
>> },
>> {
>> "type": "net_service",
>> "tcp_net_service":
>> "allowedAccess": [
>> "LANDLOCK_ACCESS_NET_CONNECT_TCP",
>> "LANDLOCK_ACCESS_NET_BIND_TCP",
>> ],
>> "port": [
>> "3920"
>> ],
>> }
>> },
>> ]
>>
>> For fs part RUNC can easily create rules and tie them to underlying
>> inodes of filesystem. Then, when it starts a container all fs rules will
>> be inherited by the container proccess.
>> But for network rules it's impossible, because there no any socket object
>> created during RUNC phase. All socket objects are created after an
>> application has started in the container.
>> Possible solution:
>> - All network rules from spec.json are tied to task_struct object of
>> the
>> container process. In this case, RUNC creates network rules, attach
>> them
>> to itself and, then rules are inherited by the launched container
>> proccess. These rules are global for all sockets in a container and,
>> then user could add additional restrictions to any socket connection,
>> imposing some kind of granularity.
>
> The proposed net_service mechanism doesn't tie anything to socket. I
> don't see any issue here.
Yes. With the approach you proposed there are no questions here. I
totally agree.
>
>>
>> Konstantin Meskhidze (1):
>> landlock: TCP network hooks implementation
>>
>> include/uapi/linux/landlock.h | 52 +++++++++
>> security/landlock/Makefile | 2 +-
>> security/landlock/fs.c | 8 +-
>> security/landlock/limits.h | 5 +
>> security/landlock/net.c | 213 ++++++++++++++++++++++++++++++++++
>> security/landlock/net.h | 40 +++++++
>> security/landlock/ruleset.c | 70 +++++++++--
>> security/landlock/ruleset.h | 19 +--
>> security/landlock/setup.c | 3 +
>> security/landlock/syscalls.c | 141 +++++++++++++++-------
>> 10 files changed, 487 insertions(+), 66 deletions(-)
>> create mode 100644 security/landlock/net.c
>> create mode 100644 security/landlock/net.h
>>
>> --
>> 2.25.1
>>
>
> The following should be an independent email.
>
>>> From fee4f0dbec1e4a8fa8d34cec57a8cdbf351e9c12 Mon Sep 17 00:00:00 2001
>> From: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>> Date: Tue, 28 Dec 2021 14:47:39 +0800
>> Subject: [RFC PATCH 1/1] landlock: TCP network hooks implementation
>>
>> Support of socket_bind() and socket_connect() hooks.
>> Current prototype can restrict binding and connecting of TCP
>> types of sockets. Its just basic idea how Landlock could support
>> network confinement.
>>
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>> ---
>> include/uapi/linux/landlock.h | 52 +++++++++
>> security/landlock/Makefile | 2 +-
>> security/landlock/fs.c | 8 +-
>> security/landlock/limits.h | 5 +
>> security/landlock/net.c | 213 ++++++++++++++++++++++++++++++++++
>> security/landlock/net.h | 40 +++++++
>> security/landlock/ruleset.c | 70 +++++++++--
>> security/landlock/ruleset.h | 19 +--
>> security/landlock/setup.c | 3 +
>> security/landlock/syscalls.c | 141 +++++++++++++++-------
>> 10 files changed, 487 insertions(+), 66 deletions(-)
>> create mode 100644 security/landlock/net.c
>> create mode 100644 security/landlock/net.h
>>
>> diff --git a/include/uapi/linux/landlock.h
>> b/include/uapi/linux/landlock.h
>> index b3d952067f59..1745a3a2f7a9 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -25,6 +25,15 @@ struct landlock_ruleset_attr {
>> * compatibility reasons.
>> */
>> __u64 handled_access_fs;
>> +
>> + /**
>> + * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
>> + * that is handled by this ruleset and should then be forbidden
>> if no
>> + * rule explicitly allow them. This is needed for backward
>> + * compatibility reasons.
>> + */
>> + __u64 handled_access_net;
>> +
>> };
>>
>> /*
>> @@ -46,6 +55,12 @@ enum landlock_rule_type {
>> * landlock_path_beneath_attr .
>> */
>> LANDLOCK_RULE_PATH_BENEATH = 1,
>> +
>> + /**
>> + * @LANDLOCK_RULE_NET_SERVICE: Type of a &struct
>> + * landlock_net_service_attr .
>> + */
>> + LANDLOCK_RULE_NET_SERVICE = 2,
>> };
>>
>> /**
>> @@ -70,6 +85,24 @@ struct landlock_path_beneath_attr {
>> */
>> } __attribute__((packed));
>>
>> +/**
>> + * struct landlock_net_service_attr - TCP subnet definition
>
> No subnet here.
I will fix it with your net_service mechanism.
>
>> + *
>> + * Argument of sys_landlock_add_rule().
>> + */
>> +struct landlock_net_service_attr {
>> + /**
>> + * @allowed_access: Bitmask of allowed access network for services
>> + * (cf. `Network flags`_).
>> + */
>> + __u64 allowed_access;
>> + /**
>> + * @port: Network port
>> + */
>> + __u16 port;
>> +
>> +} __attribute__((packed));
>> +
>> /**
>> * DOC: fs_access
>> *
>> @@ -134,4 +167,23 @@ struct landlock_path_beneath_attr {
>> #define LANDLOCK_ACCESS_FS_MAKE_BLOCK (1ULL << 11)
>> #define LANDLOCK_ACCESS_FS_MAKE_SYM (1ULL << 12)
>>
>> +/**
>> + * DOC: net_access
>> + *
>> + * Network flags
>> + * ~~~~~~~~~~~~~~~~
>> + *
>> + * These flags enable to restrict a sandboxed process to a set of
>> network
>> + * actions.
>> + *
>> + * TCP sockets with allowed actions:
>> + *
>> + * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a IP address.
>
> Bind a TCP socket to a local port.
Got it. Will be fixed.
>
>> + * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
>> + * a listening one.
>
> Connect a TCP socket to a remote port.
Got it. Will be fixed.
>
>> + */
>> +#define LANDLOCK_ACCESS_NET_BIND_TCP (1ULL << 0)
>> +#define LANDLOCK_ACCESS_NET_CONNECT_TCP (1ULL << 1)
>> +
>> +
>> #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>> index 7bbd2f413b3e..afa44baaa83a 100644
>> --- a/security/landlock/Makefile
>> +++ b/security/landlock/Makefile
>> @@ -1,4 +1,4 @@
>> obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>
>> landlock-y := setup.o syscalls.o object.o ruleset.o \
>> - cred.o ptrace.o fs.o
>> + cred.o ptrace.o fs.o net.o
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index 97b8e421f617..a27ac76ea1ee 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -154,6 +154,7 @@ int landlock_append_fs_rule(struct
>> landlock_ruleset *const ruleset,
>> {
>> int err;
>> struct landlock_object *object;
>> + u8 rule_fs_type;
>>
>> /* Files only get access rights that make sense. */
>> if (!d_is_dir(path->dentry) && (access_rights | ACCESS_FILE) !=
>> @@ -162,8 +163,9 @@ int landlock_append_fs_rule(struct
>> landlock_ruleset *const ruleset,
>> if (WARN_ON_ONCE(ruleset->num_layers != 1))
>> return -EINVAL;
>>
>> + rule_fs_type = LANDLOCK_RULE_PATH_BENEATH - 1;
>> /* Transforms relative access rights to absolute ones. */
>> - access_rights |= LANDLOCK_MASK_ACCESS_FS &
>> ~ruleset->fs_access_masks[0];
>> + access_rights |= LANDLOCK_MASK_ACCESS_FS &
>> ~ruleset->access_masks[rule_fs_type][0];
>> object = get_inode_object(d_backing_inode(path->dentry));
>> if (IS_ERR(object))
>> return PTR_ERR(object);
>> @@ -229,6 +231,7 @@ static int check_access_path(const struct
>> landlock_ruleset *const domain,
>> struct path walker_path;
>> u64 layer_mask;
>> size_t i;
>> + u8 rule_fs_type;
>>
>> /* Make sure all layers can be checked. */
>> BUILD_BUG_ON(BITS_PER_TYPE(layer_mask) < LANDLOCK_MAX_NUM_LAYERS);
>> @@ -249,10 +252,11 @@ static int check_access_path(const struct
>> landlock_ruleset *const domain,
>> if (WARN_ON_ONCE(domain->num_layers < 1))
>> return -EACCES;
>>
>> + rule_fs_type = LANDLOCK_RULE_PATH_BENEATH - 1;
>> /* Saves all layers handling a subset of requested accesses. */
>> layer_mask = 0;
>> for (i = 0; i < domain->num_layers; i++) {
>> - if (domain->fs_access_masks[i] & access_request)
>> + if (domain->access_masks[rule_fs_type][i] & access_request)
>> layer_mask |= BIT_ULL(i);
>> }
>> /* An access request not handled by the domain is allowed. */
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index 2a0a1095ee27..6a1e5ea4c7d2 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -18,4 +18,9 @@
>> #define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_MAKE_SYM
>> #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS <<
>> 1) - 1)
>>
>> +#define LANDLOCK_LAST_ACCESS_NET LANDLOCK_ACCESS_NET_CONNECT_TCP
>> +#define LANDLOCK_MASK_ACCESS_NET ((LANDLOCK_LAST_ACCESS_NET << 1)
>> - 1)
>> +
>> +#define LANDLOCK_RULE_TYPE_NUM LANDLOCK_RULE_NET_SERVICE
>> +
>> #endif /* _SECURITY_LANDLOCK_LIMITS_H */
>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>> new file mode 100644
>> index 000000000000..443933db3e23
>> --- /dev/null
>> +++ b/security/landlock/net.c
>> @@ -0,0 +1,213 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Landlock LSM - Filesystem management and hooks
>> + *
>> + * Copyright © 2016-2020 Mickaël Salaün <mic@...ikod.net>
>> + * Copyright © 2018-2020 ANSSI
>> + */
>> +
>> +#include <linux/socket.h>
>> +#include <linux/net.h>
>> +
>> +#include "cred.h"
>> +#include "limits.h"
>> +#include "net.h"
>> +
>> +/* Underlying object management */
>> +static void release_task_object(struct landlock_object *const object)
>> +{
>> + /* Untie landlock object from a process task_struct */
>> + object->underobj = NULL;
>> +}
>> +
>> +static const struct landlock_object_underops landlock_net_underops = {
>> + .release = release_task_object
>> +};
>> +
>> +static struct landlock_object *get_task_object(struct task_struct
>> *const task)
>> +{
>> + struct landlock_object *object, *new_object;
>> + struct landlock_task_security *task_sec = landlock_task(task);
>> +
>> + object = task_sec->object;
>> + /* There is no race condition here, cause a process
>> + * creating a landlock object (for network rules) ties
>> + * one to itself, but it better to save landlock object
>> + * refcounter usage logic here.
>> + */
>> + if (object)
>> + if (likely(refcount_inc_not_zero(&object->usage)))
>> + return object;
>> +
>> + /*
>> + * If there is no object tied to task, then create a new one.
>> + */
>> + new_object = landlock_create_object(&landlock_net_underops, task);
>> + if (IS_ERR(new_object))
>> + return new_object;
>> +
>> + task_sec->object = new_object;
>> + return new_object;
>> +}
>> +
>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>> + u32 access_rights)
>> +{
>> + int err;
>> + struct landlock_object *object;
>> + u8 rule_net_type;
>> +
>> + rule_net_type = LANDLOCK_RULE_NET_SERVICE - 1;
>> + /* Transforms relative access rights to absolute ones. */
>> + access_rights |= LANDLOCK_MASK_ACCESS_NET &
>> ~ruleset->access_masks[rule_net_type][0];
>> +
>> + /* Get the current proccess's landlock object */
>> + object = get_task_object(current);
>> + if (IS_ERR(object))
>> + return PTR_ERR(object);
>> + mutex_lock(&ruleset->lock);
>> + err = landlock_insert_rule(ruleset, object, access_rights);
>> + mutex_unlock(&ruleset->lock);
>> + /*
>> + * No need to check for an error because landlock_insert_rule()
>> + * increments the refcount for the new object if needed.
>> + */
>> + landlock_put_object(object);
>> + return err;
>> +}
>> +
>> +/* Access-control management */
>> +static inline bool unmask_layers(
>> + const struct landlock_ruleset *const domain,
>> + const u32 access_request, u64 layer_mask)
>> +{
>> + const struct landlock_rule *rule;
>> + const struct task_struct *task;
>> + size_t i;
>> + bool allowed = false;
>> +
>> + /* Get current procces task_struct */
>> + task = current;
>> +
>> + rule = landlock_find_rule(domain, landlock_task(task)->object);
>
> Again, it doesn't make sense to check tasks in this use case. The
> subject is already the current task. The object should be a TCP port.
>
> Please take a look at my previous review.
Got it. I will fix it with your net_service mechanism.
>
>> +
>> + /* Grant access if there is no rule for an oject */
>> + if (!rule)
>> + return allowed = true;
>> +
>> + /*
>> + * An access is granted if, for each policy layer, at least one rule
>> + * encountered on network actions requested,
>> + * regardless of their position in the layer stack. We must then
>> check
>> + * the remaining layers, from the first added layer to
>> + * the last one.
>> + */
>> + for (i = 0; i < rule->num_layers; i++) {
>> + const struct landlock_layer *const layer = &rule->layers[i];
>> + const u64 layer_level = BIT_ULL(layer->level - 1);
>> +
>> + /* Checks that the layer grants access to the request. */
>> + if ((layer->access & access_request) == access_request) {
>> + layer_mask &= ~layer_level;
>> + allowed = true;
>> +
>> + if (layer_mask == 0)
>> + return allowed;
>> + } else {
>> + layer_mask &= ~layer_level;
>> +
>> + if (layer_mask == 0)
>> + return allowed;
>> + }
>> + }
>> + return allowed;
>> +}
>> +
>> +static int check_socket_access(const struct landlock_ruleset *const
>> domain,
>> + u32 access_request)
>> +{
>> + bool allowed = false;
>> + u64 layer_mask;
>> + size_t i;
>> + u8 rule_net_type;
>> +
>> + /* Make sure all layers can be checked. */
>> + BUILD_BUG_ON(BITS_PER_TYPE(layer_mask) < LANDLOCK_MAX_NUM_LAYERS);
>> +
>> + if (WARN_ON_ONCE(!domain))
>> + return 0;
>> + if (WARN_ON_ONCE(domain->num_layers < 1))
>> + return -EACCES;
>> +
>> + /* Saves all layers handling a subset of requested
>> + * socket access rules.
>> + */
>> + rule_net_type = LANDLOCK_RULE_NET_SERVICE - 1;
>> + layer_mask = 0;
>> + for (i = 0; i < domain->num_layers; i++) {
>> + if (domain->access_masks[rule_net_type][i] & access_request)
>> + layer_mask |= BIT_ULL(i);
>> + }
>> + /* An access request not handled by the domain is allowed. */
>> + if (layer_mask == 0)
>> + return 0;
>> +
>> + /*
>> + * We need to walk through all the hierarchy to not miss any
>> relevant
>> + * restriction.
>> + */
>> + allowed = unmask_layers(domain, access_request, layer_mask);
>> +
>> + return allowed ? 0 : -EACCES;
>> +}
>> +
>> +static int hook_socket_bind(struct socket *sock, struct sockaddr
>> *address, int addrlen)
>> +{
>> + short socket_type;
>> + const struct landlock_ruleset *const dom =
>> landlock_get_current_domain();
>> +
>> + /* Check if the hook catches AF_INET* socket's action */
>> + if ((address->sa_family != AF_INET) && (address->sa_family !=
>> AF_INET6))
>> + return 0;
>> +
>> + socket_type = sock->type;
>> + /* Check if it's a TCP socket */
>> + if (socket_type != SOCK_STREAM)
>> + return 0;
>> +
>> + if (!dom)
>> + return 0;
>> +
>> + return check_socket_access(dom, LANDLOCK_ACCESS_NET_BIND_TCP);
>
> There is no port check.
Got it. I will refactor the code.
>
>> +}
>> +
>> +static int hook_socket_connect(struct socket *sock, struct sockaddr
>> *address, int addrlen)
>> +{
>> + short socket_type;
>> + const struct landlock_ruleset *const dom =
>> landlock_get_current_domain();
>> +
>> + /* Check if the hook catches AF_INET* socket's action */
>> + if ((address->sa_family != AF_INET) && (address->sa_family !=
>> AF_INET6))
>> + return 0;
>> +
>> + socket_type = sock->type;
>> + /* Check if it's a TCP socket */
>> + if (socket_type != SOCK_STREAM)
>> + return 0;
>> +
>> + if (!dom)
>> + return 0;
>> +
>> + return check_socket_access(dom, LANDLOCK_ACCESS_NET_CONNECT_TCP);
>
> There is no port check.
Got it. I will refactor the code.
>
>> +}
>> +
>> +static struct security_hook_list landlock_hooks[] __lsm_ro_after_init
>> = {
>> + LSM_HOOK_INIT(socket_bind, hook_socket_bind),
>> + LSM_HOOK_INIT(socket_connect, hook_socket_connect),
>> +};
>> +
>> +__init void landlock_add_net_hooks(void)
>> +{
>> + security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
>> + LANDLOCK_NAME);
>> +}
>> diff --git a/security/landlock/net.h b/security/landlock/net.h
>> new file mode 100644
>> index 000000000000..27ef59a42a1e
>> --- /dev/null
>> +++ b/security/landlock/net.h
>> @@ -0,0 +1,40 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Landlock LSM - Network management and hooks
>> + *
>> + * Copyright © 2017-2020 Mickaël Salaün <mic@...ikod.net>
>> + * Copyright © 2018-2020 ANSSI
>> + */
>> +
>> +#ifndef _SECURITY_LANDLOCK_NET_H
>> +#define _SECURITY_LANDLOCK_NET_H
>> +
>> +#include "common.h"
>> +#include "ruleset.h"
>> +#include "setup.h"
>> +
>> +/**
>> + * struct landlock_task_security - Process task security blob
>> + *
>> + * Enable to reference a &struct landlock_object tied to a task (i.e.
>> + * underlying object).
>> + */
>> +struct landlock_task_security {
>> + /**
>> + * @object: Pointer to an allocated object.
>> + */
>> + struct landlock_object *object;
>> +};
>> +
>> +static inline struct landlock_task_security *landlock_task(
>> + const struct task_struct *const task)
>> +{
>> + return task->security + landlock_blob_sizes.lbs_task;
>> +}
>> +
>> +__init void landlock_add_net_hooks(void);
>> +
>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>> + u32 access_hierarchy);
>> +
>> +#endif /* _SECURITY_LANDLOCK_NET_H */
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index ec72b9262bf3..a335c475965c 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -27,9 +27,24 @@
>> static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>> {
>> struct landlock_ruleset *new_ruleset;
>> + u16 row, col, rules_types_num;
>> +
>> + new_ruleset = kzalloc(sizeof *new_ruleset +
>> + sizeof *(new_ruleset->access_masks),
>
> sizeof(access_masks) is 0.
Actually sizeof *(new_ruleset->access_masks) is 8.
It's a 64 bit pointer to u16 array[]. I checked this
2D FAM array implementation in a standalone test.
>
>> + GFP_KERNEL_ACCOUNT);
>> +
>> + rules_types_num = LANDLOCK_RULE_TYPE_NUM;
>> + /* Initializes access_mask array for multiple rule types.
>> + * Double array semantic is used convenience:
>> access_mask[rule_type][num_layer].
>> + */
>> + for (row = 0; row < rules_types_num; row++) {
>> + new_ruleset->access_masks[row] = kzalloc(sizeof
>> + *(new_ruleset->access_masks[row]),
>> + GFP_KERNEL_ACCOUNT);
>> + for (col = 0; col < num_layers; col++)
>> + new_ruleset->access_masks[row][col] = 0;
>> + }
>
> This code may segfault. I guess it wasn't tested. Please enable most
> test/check features as I suggested in the previous series.
I compiled the kernel 5.13 with this patch and tested it in QEMU. No
crashes. I tested your sandboxer.c and it works without segfaults.
But I will provide seltests in future patch.
>
>>
>> - new_ruleset = kzalloc(struct_size(new_ruleset, fs_access_masks,
>> - num_layers), GFP_KERNEL_ACCOUNT);
>
> What about my comment in the previous series?
As I replied above.
Maybe I missed your point here. But I can't see how to identify
different rule types here. If there just one ruleset created, so there
is just one access_mask[0] in the ruleset. Its either can be used for
filesystem mask or for network one. To support both rule type we need to
use at least access_mask[] array with size of 2.
Also using upper bit to identify access rule type will narrow down
possible access ammount in future version.
I suggest using 2D array or 1D array with module arithmetic to
support different rule type masks.
Anyway please corrent me if I'm wrong here.
>
>> if (!new_ruleset)
>> return ERR_PTR(-ENOMEM);
>> refcount_set(&new_ruleset->usage, 1);
>> @@ -39,21 +54,30 @@ static struct landlock_ruleset
>> *create_ruleset(const u32 num_layers)
>> /*
>> * hierarchy = NULL
>> * num_rules = 0
>> - * fs_access_masks[] = 0
>> + * access_masks[][] = 0
>> */
>> return new_ruleset;
>> }
>>
>> -struct landlock_ruleset *landlock_create_ruleset(const u32
>> fs_access_mask)
>> +struct landlock_ruleset *landlock_create_ruleset(const u32
>> fs_access_mask,
>> + const u32 net_access_mask)
>> {
>> struct landlock_ruleset *new_ruleset;
>> + u8 rule_fs_type, rule_net_type;
>>
>> /* Informs about useless ruleset. */
>> - if (!fs_access_mask)
>> + if (!fs_access_mask && !net_access_mask)
>> return ERR_PTR(-ENOMSG);
>> new_ruleset = create_ruleset(1);
>> - if (!IS_ERR(new_ruleset))
>> - new_ruleset->fs_access_masks[0] = fs_access_mask;
>> +
>> + rule_fs_type = LANDLOCK_RULE_PATH_BENEATH - 1;
>> + if (!IS_ERR(new_ruleset) && fs_access_mask)
>> + new_ruleset->access_masks[rule_fs_type][0] = fs_access_mask;
>> +
>> + rule_net_type = LANDLOCK_RULE_NET_SERVICE - 1;
>> + if (!IS_ERR(new_ruleset) && net_access_mask)
>> + new_ruleset->access_masks[rule_net_type][0] = net_access_mask;
>> +
>> return new_ruleset;
>> }
>>
>> @@ -112,15 +136,23 @@ static void free_rule(struct landlock_rule
>> *const rule)
>>
>> static void build_check_ruleset(void)
>> {
>> + u8 rule_fs_type, rule_net_type;
>> +
>> const struct landlock_ruleset ruleset = {
>> .num_rules = ~0,
>> .num_layers = ~0,
>> };
>> - typeof(ruleset.fs_access_masks[0]) fs_access_mask = ~0;
>> +
>> + rule_fs_type = LANDLOCK_RULE_PATH_BENEATH - 1;
>> + rule_net_type = LANDLOCK_RULE_NET_SERVICE - 1;
>> +
>> + typeof(ruleset.access_masks[rule_fs_type][0]) fs_access_mask = ~0;
>> + typeof(ruleset.access_masks[rule_net_type][0]) net_access_mask = ~0;
>>
>> BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
>> BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
>> BUILD_BUG_ON(fs_access_mask < LANDLOCK_MASK_ACCESS_FS);
>> + BUILD_BUG_ON(net_access_mask < LANDLOCK_MASK_ACCESS_NET);
>> }
>>
>> /**
>> @@ -260,8 +292,12 @@ static int merge_ruleset(struct landlock_ruleset
>> *const dst,
>> struct landlock_ruleset *const src)
>> {
>> struct landlock_rule *walker_rule, *next_rule;
>> + u8 rule_fs_type, rule_net_type;
>> int err = 0;
>>
>> + rule_fs_type = LANDLOCK_RULE_PATH_BENEATH - 1;
>> + rule_net_type = LANDLOCK_RULE_NET_SERVICE - 1;
>> +
>> might_sleep();
>> /* Should already be checked by landlock_merge_ruleset() */
>> if (WARN_ON_ONCE(!src))
>> @@ -279,7 +315,14 @@ static int merge_ruleset(struct landlock_ruleset
>> *const dst,
>> err = -EINVAL;
>> goto out_unlock;
>> }
>> - dst->fs_access_masks[dst->num_layers - 1] = src->fs_access_masks[0];
>> +
>> + /* Copy fs access masks. */
>> + dst->access_masks[rule_fs_type][dst->num_layers - 1] =
>> + src->access_masks[rule_fs_type][0];
>> + /* Copy network access masks. */
>> + dst->access_masks[rule_net_type][dst->num_layers - 1] =
>> + src->access_masks[rule_net_type][0];
>> +
>>
>> /* Merges the @src tree. */
>> rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
>> @@ -336,9 +379,12 @@ static int inherit_ruleset(struct
>> landlock_ruleset *const parent,
>> err = -EINVAL;
>> goto out_unlock;
>> }
>> - /* Copies the parent layer stack and leaves a space for the new
>> layer. */
>> - memcpy(child->fs_access_masks, parent->fs_access_masks,
>> - flex_array_size(parent, fs_access_masks,
>> parent->num_layers));
>> + /* Copies the parent layer stack and leaves a space for the new
>> layer.
>> + * Remember to copy num_layers*num_tule_types size.
>> + */
>> + memcpy(child->access_masks, parent->access_masks,
>> + flex_array_size(parent, access_masks,
>> LANDLOCK_RULE_TYPE_NUM *
>> + parent->num_layers));
>>
>> if (WARN_ON_ONCE(!parent->hierarchy)) {
>> err = -EINVAL;
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index 2d3ed7ec5a0a..965a69a108e9 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -124,23 +124,24 @@ struct landlock_ruleset {
>> */
>> u32 num_layers;
>> /**
>> - * @fs_access_masks: Contains the subset of filesystem
>> - * actions that are restricted by a ruleset. A domain
>> - * saves all layers of merged rulesets in a stack
>> - * (FAM), starting from the first layer to the last
>> - * one. These layers are used when merging rulesets,
>> - * for user space backward compatibility (i.e.
>> - * future-proof), and to properly handle merged
>> + * @access_masks: Contains the subset of filesystem
>> + * or network actions that are restricted by a ruleset.
>> + * A domain saves all layers of merged rulesets in a
>> + * stack(FAM), starting from the first layer to the
>> + * last one. These layers are used when merging
>> + * rulesets, for user space backward compatibility
>> + * (i.e. future-proof), and to properly handle merged
>> * rulesets without overlapping access rights. These
>> * layers are set once and never changed for the
>> * lifetime of the ruleset.
>> */
>> - u16 fs_access_masks[];
>> + u16 *access_masks[];
>> };
>> };
>> };
>>
>> -struct landlock_ruleset *landlock_create_ruleset(const u32
>> fs_access_mask);
>> +struct landlock_ruleset *landlock_create_ruleset(const u32
>> fs_access_mask,
>> + const u32 net_access_mask);
>>
>> void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>> void landlock_put_ruleset_deferred(struct landlock_ruleset *const
>> ruleset);
>> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
>> index f8e8e980454c..91ab06ec8ce0 100644
>> --- a/security/landlock/setup.c
>> +++ b/security/landlock/setup.c
>> @@ -14,6 +14,7 @@
>> #include "fs.h"
>> #include "ptrace.h"
>> #include "setup.h"
>> +#include "net.h"
>>
>> bool landlock_initialized __lsm_ro_after_init = false;
>>
>> @@ -21,6 +22,7 @@ struct lsm_blob_sizes landlock_blob_sizes
>> __lsm_ro_after_init = {
>> .lbs_cred = sizeof(struct landlock_cred_security),
>> .lbs_inode = sizeof(struct landlock_inode_security),
>> .lbs_superblock = sizeof(struct landlock_superblock_security),
>> + .lbs_task = sizeof(struct landlock_task_security),
>> };
>>
>> static int __init landlock_init(void)
>> @@ -28,6 +30,7 @@ static int __init landlock_init(void)
>> landlock_add_cred_hooks();
>> landlock_add_ptrace_hooks();
>> landlock_add_fs_hooks();
>> + landlock_add_net_hooks();
>> landlock_initialized = true;
>> pr_info("Up and running.\n");
>> return 0;
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index 32396962f04d..0dad22e99500 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -31,6 +31,7 @@
>> #include "limits.h"
>> #include "ruleset.h"
>> #include "setup.h"
>> +#include "net.h"
>>
>> /**
>> * copy_min_struct_from_user - Safe future-proof argument copying
>> @@ -73,7 +74,8 @@ static void build_check_abi(void)
>> {
>> struct landlock_ruleset_attr ruleset_attr;
>> struct landlock_path_beneath_attr path_beneath_attr;
>> - size_t ruleset_size, path_beneath_size;
>> + struct landlock_net_service_attr net_service_attr;
>> + size_t ruleset_size, path_beneath_size, net_service_size;
>>
>> /*
>> * For each user space ABI structures, first checks that there
>> is no
>> @@ -81,17 +83,22 @@ static void build_check_abi(void)
>> * struct size.
>> */
>> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
>> + ruleset_size += sizeof(ruleset_attr.handled_access_net);
>> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
>> - BUILD_BUG_ON(sizeof(ruleset_attr) != 8);
>> + BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
>>
>> path_beneath_size = sizeof(path_beneath_attr.allowed_access);
>> path_beneath_size += sizeof(path_beneath_attr.parent_fd);
>> BUILD_BUG_ON(sizeof(path_beneath_attr) != path_beneath_size);
>> BUILD_BUG_ON(sizeof(path_beneath_attr) != 12);
>> +
>> + net_service_size = sizeof(net_service_attr.allowed_access);
>> + net_service_size += sizeof(net_service_attr.port);
>> + BUILD_BUG_ON(sizeof(net_service_attr) != net_service_size);
>> + BUILD_BUG_ON(sizeof(net_service_attr) != 10);
>> }
>>
>> /* Ruleset handling */
>> -
>> static int fop_ruleset_release(struct inode *const inode,
>> struct file *const filp)
>> {
>> @@ -176,18 +183,24 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>>
>> /* Copies raw user space buffer. */
>> err = copy_min_struct_from_user(&ruleset_attr,
>> sizeof(ruleset_attr),
>> - offsetofend(typeof(ruleset_attr), handled_access_fs),
>> + offsetofend(typeof(ruleset_attr), handled_access_net),
>> attr, size);
>> if (err)
>> return err;
>>
>> - /* Checks content (and 32-bits cast). */
>> + /* Checks fs content (and 32-bits cast). */
>> if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_ACCESS_FS) !=
>> LANDLOCK_MASK_ACCESS_FS)
>> return -EINVAL;
>>
>> + /* Checks network content (and 32-bits cast). */
>> + if ((ruleset_attr.handled_access_net | LANDLOCK_MASK_ACCESS_NET) !=
>> + LANDLOCK_MASK_ACCESS_NET)
>> + return -EINVAL;
>> +
>> /* Checks arguments and transforms to kernel struct. */
>> - ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs);
>> + ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
>> + ruleset_attr.handled_access_net);
>> if (IS_ERR(ruleset))
>> return PTR_ERR(ruleset);
>>
>> @@ -306,6 +319,7 @@ SYSCALL_DEFINE4(landlock_add_rule,
>> const void __user *const, rule_attr, const __u32, flags)
>> {
>> struct landlock_path_beneath_attr path_beneath_attr;
>> + struct landlock_net_service_attr net_service_attr;
>> struct path path;
>> struct landlock_ruleset *ruleset;
>> int res, err;
>> @@ -317,47 +331,90 @@ SYSCALL_DEFINE4(landlock_add_rule,
>> if (flags)
>> return -EINVAL;
>>
>> - if (rule_type != LANDLOCK_RULE_PATH_BENEATH)
>> + if ((rule_type != LANDLOCK_RULE_PATH_BENEATH) &&
>> + (rule_type != LANDLOCK_RULE_NET_SERVICE))
>> return -EINVAL;
>>
>> - /* Copies raw user space buffer, only one type for now. */
>> - res = copy_from_user(&path_beneath_attr, rule_attr,
>> - sizeof(path_beneath_attr));
>> - if (res)
>> - return -EFAULT;
>> -
>> - /* Gets and checks the ruleset. */
>> - ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
>> - if (IS_ERR(ruleset))
>> - return PTR_ERR(ruleset);
>> -
>> - /*
>> - * Informs about useless rule: empty allowed_access (i.e. deny
>> rules)
>> - * are ignored in path walks.
>> - */
>> - if (!path_beneath_attr.allowed_access) {
>> - err = -ENOMSG;
>> - goto out_put_ruleset;
>> - }
>> - /*
>> - * Checks that allowed_access matches the @ruleset constraints
>> - * (ruleset->fs_access_masks[0] is automatically upgraded to
>> 64-bits).
>> - */
>> - if ((path_beneath_attr.allowed_access |
>> ruleset->fs_access_masks[0]) !=
>> - ruleset->fs_access_masks[0]) {
>> - err = -EINVAL;
>> - goto out_put_ruleset;
>> + switch (rule_type) {
>> + case LANDLOCK_RULE_PATH_BENEATH:
>> + /* Copies raw user space buffer, for fs rule type. */
>> + res = copy_from_user(&path_beneath_attr, rule_attr,
>> + sizeof(path_beneath_attr));
>> + if (res)
>> + return -EFAULT;
>> + break;
>> +
>> + case LANDLOCK_RULE_NET_SERVICE:
>> + /* Copies raw user space buffer, for net rule type. */
>> + res = copy_from_user(&net_service_attr, rule_attr,
>> + sizeof(net_service_attr));
>> + if (res)
>> + return -EFAULT;
>> + break;
>> }
>>
>> - /* Gets and checks the new rule. */
>> - err = get_path_from_fd(path_beneath_attr.parent_fd, &path);
>> - if (err)
>> - goto out_put_ruleset;
>> + if (rule_type == LANDLOCK_RULE_PATH_BENEATH) {
>> + /* Gets and checks the ruleset. */
>> + ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
>> + if (IS_ERR(ruleset))
>> + return PTR_ERR(ruleset);
>> +
>> + /*
>> + * Informs about useless rule: empty allowed_access (i.e.
>> deny rules)
>> + * are ignored in path walks.
>> + */
>> + if (!path_beneath_attr.allowed_access) {
>> + err = -ENOMSG;
>> + goto out_put_ruleset;
>> + }
>> + /*
>> + * Checks that allowed_access matches the @ruleset constraints
>> + * (ruleset->access_masks[fs_type][0] is automatically
>> upgraded to 64-bits).
>> + */
>> + if ((path_beneath_attr.allowed_access |
>> ruleset->access_masks[rule_type-1][0]) !=
>> + ruleset->access_masks[rule_type-1][0]) {
>> + err = -EINVAL;
>> + goto out_put_ruleset;
>> + }
>> +
>> + /* Gets and checks the new rule. */
>> + err = get_path_from_fd(path_beneath_attr.parent_fd, &path);
>> + if (err)
>> + goto out_put_ruleset;
>> +
>> + /* Imports the new rule. */
>> + err = landlock_append_fs_rule(ruleset, &path,
>> + path_beneath_attr.allowed_access);
>> + path_put(&path);
>> + }
>>
>> - /* Imports the new rule. */
>> - err = landlock_append_fs_rule(ruleset, &path,
>> - path_beneath_attr.allowed_access);
>> - path_put(&path);
>> + if (rule_type == LANDLOCK_RULE_NET_SERVICE) {
>> + /* Gets and checks the ruleset. */
>> + ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
>> + if (IS_ERR(ruleset))
>> + return PTR_ERR(ruleset);
>> +
>> + /*
>> + * Informs about useless rule: empty allowed_access (i.e.
>> deny rules)
>> + * are ignored in network actions
>> + */
>> + if (!net_service_attr.allowed_access) {
>> + err = -ENOMSG;
>> + goto out_put_ruleset;
>> + }
>> + /*
>> + * Checks that allowed_access matches the @ruleset constraints
>> + * (ruleset->access_masks[net_type][0] is automatically
>> upgraded to 64-bits).
>> + */
>> + if ((net_service_attr.allowed_access |
>> ruleset->access_masks[rule_type - 1][0]) !=
>> + ruleset->access_masks[rule_type - 1][0]) {
>> + err = -EINVAL;
>> + goto out_put_ruleset;
>> + }
>> +
>> + /* Imports the new rule. */
>> + err = landlock_append_net_rule(ruleset,
>> net_service_attr.allowed_access);
>> + }
>>
>> out_put_ruleset:
>> landlock_put_ruleset(ruleset);
>> --
>> 2.25.1
>>
>
> .
Powered by blists - more mailing lists