[PATCH v5 09/15] seltests/landlock: add tests for bind() hooks

[Mickaël Salaün]

On 19/05/2022 14:10, Konstantin Meskhidze wrote:
> 
> 
> 5/17/2022 12:11 AM, Mickaël Salaün пишет:
>>
>> On 16/05/2022 17:20, Konstantin Meskhidze wrote:
>>> Adds selftests for bind socket action.
>>> The first is with no landlock restrictions:
>>>      - bind_no_restrictions_ip4;
>>>      - bind_no_restrictions_ip6;
>>> The second ones is with mixed landlock rules:
>>>      - bind_with_restrictions_ip4;
>>>      - bind_with_restrictions_ip6;
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
>>> ---
>>>
>>> Changes since v3:
>>> * Split commit.
>>> * Add helper create_socket.
>>> * Add FIXTURE_SETUP.
>>>
>>> Changes since v4:
>>> * Adds port[MAX_SOCKET_NUM], struct sockaddr_in addr4
>>> and struct sockaddr_in addr6 in FIXTURE.
>>> * Refactoring FIXTURE_SETUP:
>>>      - initializing self->port, self->addr4 and self->addr6.
>>>      - adding network namespace.
>>> * Refactoring code with self->port, self->addr4 and
>>> self->addr6 variables.
>>> * Adds selftests for IP6 family:
>>>      - bind_no_restrictions_ip6.
>>>      - bind_with_restrictions_ip6.
>>> * Refactoring selftests/landlock/config
>>> * Moves enforce_ruleset() into common.h
>>>
>>> ---
>>>   tools/testing/selftests/landlock/common.h   |   9 +
>>>   tools/testing/selftests/landlock/config     |   5 +-
>>>   tools/testing/selftests/landlock/fs_test.c  |  10 -
>>>   tools/testing/selftests/landlock/net_test.c | 237 ++++++++++++++++++++
>>>   4 files changed, 250 insertions(+), 11 deletions(-)
>>>   create mode 100644 tools/testing/selftests/landlock/net_test.c
>>>
>>> diff --git a/tools/testing/selftests/landlock/common.h 
>>> b/tools/testing/selftests/landlock/common.h
>>> index 7ba18eb23783..c5381e641dfd 100644
>>> --- a/tools/testing/selftests/landlock/common.h
>>> +++ b/tools/testing/selftests/landlock/common.h
>>> @@ -102,6 +102,15 @@ static inline int landlock_restrict_self(const 
>>> int ruleset_fd,
>>>   }
>>>   #endif
>>>
>>> +static void enforce_ruleset(struct __test_metadata *const _metadata,
>>> +        const int ruleset_fd)
>>> +{
>>> +    ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
>>> +    ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) {
>>> +        TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
>>> +    }
>>> +}
>>> +
>>
>> Please create a commit which moves all the needed code for all network 
>> tests. I think there is only this helper though.
> 
>    Ok. I will create one additional commit for moving this helper.
>    But after I have moved the helper to common.h, I got warnings while 
> compiling seltests where I don't use the one (base_test and ptrace_test)

Move it after clear_cap() and use the same attributes.

[...]

 >>> diff --git a/tools/testing/selftests/landlock/config
 >>> b/tools/testing/selftests/landlock/config
 >>> index 0f0a65287bac..b56f3274d3f5 100644
 >>> --- a/tools/testing/selftests/landlock/config
 >>> +++ b/tools/testing/selftests/landlock/config
 >>> @@ -1,7 +1,10 @@
 >>> +CONFIG_INET=y
 >>> +CONFIG_IPV6=y
 >>> +CONFIG_NET=y
 >>>   CONFIG_OVERLAY_FS=y
 >>>   CONFIG_SECURITY_LANDLOCK=y
 >>>   CONFIG_SECURITY_PATH=y
 >>>   CONFIG_SECURITY=y
 >>>   CONFIG_SHMEM=y
 >>>   CONFIG_TMPFS_XATTR=y
 >>> -CONFIG_TMPFS=y
 >>> +CONFIG_TMPFS=y
 >>> \ No newline at end of file

You also need to add CONFIG_NET_NS.

[...]

>>
>>> +        self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i;
>>> +        self->addr4[i].sin_family = AF_INET;
>>> +        self->addr4[i].sin_port = htons(self->port[i]);
>>> +        self->addr4[i].sin_addr.s_addr = htonl(INADDR_ANY);
>>
>> Could you use the local addr (127.0.0.1) instead?
> 
>    Why cant I use INADDR_ANY here?

You can, but it is cleaner to bind to a specified address (i.e. you 
control where a connection come from), and I guess this variable/address 
could be used to establish connections as well.

>>
>>> +        memset(&(self->addr4[i].sin_zero), '\0', 8);
>>> +    }
>>> +
>>> +    /* Creates IP6 socket addresses */
>>> +    for (i = 0; i < MAX_SOCKET_NUM; i++) {
>>> +        self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i;
>>> +        self->addr6[i].sin6_family = AF_INET6;
>>> +        self->addr6[i].sin6_port = htons(self->port[i]);
>>> +        self->addr6[i].sin6_addr = in6addr_any;
>>
>> ditto
> 
>    Why cant I use in6addr_any here?

Same as for IPV4.

> 
>>
>>> +    }
>>> +
>>> +    set_cap(_metadata, CAP_SYS_ADMIN);
>>> +    ASSERT_EQ(0, unshare(CLONE_NEWNET));
>>> +    ASSERT_EQ(0, system("ip link set dev lo up"));
>>
>> If this is really required, could you avoid calling system() but set 
>> up the network in C? You can strace it to see what is going on 
>> underneath.
>>
>   I did check. It's a lot of code to be run under the hood (more than 
> one line) and it will just will complicate the test so I suggest to 
> leave just ONE line of code here.

OK


>>
>>> +    clear_cap(_metadata, CAP_SYS_ADMIN);
>>> +}
>>> +
>>> +FIXTURE_TEARDOWN(socket_test)
>>> +{ }
>>> +
>>> +TEST_F_FORK(socket_test, bind_no_restrictions_ip4) {
>>> +
>>> +    int sockfd;
>>> +
>>> +    sockfd = create_socket(_metadata, false, false);
>>> +    ASSERT_LE(0, sockfd);
>>> +
>>> +    /* Binds a socket to port[0] */
>>
>> This comment is not very useful in this context considering the below 
>> line. It will be even more clear with the bind_variant() call.
>>
>   Ok. I will fix it.
>>
>>> +    ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], 
>>> sizeof(self->addr4[0])));
>>> +
>>> +    ASSERT_EQ(0, close(sockfd));
>>> +}
>>> +
>>> +TEST_F_FORK(socket_test, bind_no_restrictions_ip6) {
>>> +
>>> +    int sockfd;
>>> +
>>> +    sockfd = create_socket(_metadata, true, false);
>>> +    ASSERT_LE(0, sockfd);
>>> +
>>> +    /* Binds a socket to port[0] */
>>> +    ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], 
>>> sizeof(self->addr6[0])));
>>> +
>>> +    ASSERT_EQ(0, close(sockfd));
>>> +}
>>> +
>>> +TEST_F_FORK(socket_test, bind_with_restrictions_ip4) {
>>> +
>>> +    int sockfd;
>>> +
>>> +    struct landlock_ruleset_attr ruleset_attr = {
>>> +        .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
>>> +                      LANDLOCK_ACCESS_NET_CONNECT_TCP,
>>> +    };
>>> +    struct landlock_net_service_attr net_service_1 = {
>>> +        .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
>>> +                  LANDLOCK_ACCESS_NET_CONNECT_TCP,
>>> +        .port = self->port[0],
>>> +    };
>>> +    struct landlock_net_service_attr net_service_2 = {
>>> +        .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
>>> +        .port = self->port[1],
>>> +    };
>>> +    struct landlock_net_service_attr net_service_3 = {
>>> +        .allowed_access = 0,
>>> +        .port = self->port[2],
>>> +    };
>>> +
>>> +    const int ruleset_fd = landlock_create_ruleset(&ruleset_attr,
>>> +            sizeof(ruleset_attr), 0);
>>> +    ASSERT_LE(0, ruleset_fd);
>>> +
>>> +    /* Allows connect and bind operations to the port[0] socket. */
>>
>> This comment is useful though because the below call is more complex.
>>
>    So I can leave it as it's, cant I?

Yes, keep it, I'd just like a fair amount of useful comments. ;)