netdev - Re: [RFC PATCH v4 10/15] seltest/landlock: add tests for bind() hooks

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 4 Apr 2022 20:32:42 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc:     willemdebruijn.kernel@...il.com,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, yusongping@...wei.com,
        artem.kuzin@...wei.com, anton.sirazetdinov@...wei.com
Subject: Re: [RFC PATCH v4 10/15] seltest/landlock: add tests for bind() hooks


On 09/03/2022 14:44, Konstantin Meskhidze wrote:
> Adds two selftests for bind socket action.
> The one is with no landlock restrictions:
>      - bind_no_restrictions;
> The second one is with mixed landlock rules:
>      - bind_with_restrictions;
> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
> ---
> 
> Changes since v3:
> * Split commit.
> * Add helper create_socket.
> * Add FIXTURE_SETUP.
> 
> ---
>   .../testing/selftests/landlock/network_test.c | 153 ++++++++++++++++++
>   1 file changed, 153 insertions(+)
>   create mode 100644 tools/testing/selftests/landlock/network_test.c
> 
> diff --git a/tools/testing/selftests/landlock/network_test.c b/tools/testing/selftests/landlock/network_test.c
> new file mode 100644
> index 000000000000..4c60f6d973a8
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/network_test.c

[...]

> +
> +uint port[MAX_SOCKET_NUM];
> +struct sockaddr_in addr[MAX_SOCKET_NUM];

You should not change global variables, it is a source of issue. Instead 
use FIXTURE local variables accessible through self->X.

> +
> +const int one = 1;

This doesn't need to be global.

[...]

> +
> +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));
> +	}
> +}

You should move the same helper from fs_base.c to common.h (see caps 
helpers) and reuse it here.


> +
> +FIXTURE(socket) { };
> +
> +FIXTURE_SETUP(socket)
> +{
> +	int i;

Please add a new line between declaration and actual code (everywhere).

> +	/* Creates socket addresses */
> +	for (i = 0; i < MAX_SOCKET_NUM; i++) {

Use ARRAY_SIZE() instead of MAY_SOCKET_NUM.


> +		port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i;

Use self->port[i] and self->addr[i] instead.

> +		addr[i].sin_family = AF_INET;
> +		addr[i].sin_port = htons(port[i]);
> +		addr[i].sin_addr.s_addr = inet_addr(IP_ADDRESS);
> +		memset(&(addr[i].sin_zero), '\0', 8);
> +	}
> +}

[...]

> +	/* Allows connect and deny bind operations to the port[1] socket. */
> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
> +				&net_service_2, 0));
> +	/* Empty allowed_access (i.e. deny rules) are ignored in network actions

The kernel coding style says to start a multi-line comments with a "/*" 
and a new line.