Landlock: Test, comment and doc improvements
Subsystems (4)
cover Landlock: Test, comment and doc improvements
Hi,
This series contains various unrelated improvements I stashed while working on pathname Unix socket scopes (I will send the series for that tomorrow). Only test/comment/doc changes.
I was planning to include this in the upcoming Unix socket scope series but I thought maybe it would be better to send them separately, as they would be unrelated to the main topic of that series.
Tingmao Wang (5):
selftests/landlock: Fix typo in fs_test selftests/landlock: Fix missing semicolon Documentation/landlock: Fix missing case for ABI 6 in downgrade
example
selftests/landlock: Use scoped_base_variants.h for ptrace_test
landlock: Improve the comment for domain_is_scoped
Documentation/userspace-api/landlock.rst | 6 + security/landlock/task.c | 9 +- tools/testing/selftests/landlock/fs_test.c | 4 +- .../testing/selftests/landlock/ptrace_test.c | 154 +----------------- .../landlock/scoped_abstract_unix_test.c | 2 +- .../selftests/landlock/scoped_base_variants.h | 9 +- 6 files changed, 26 insertions(+), 158 deletions(-)
base-commit: 161db1810f3625e97ab414908dbcf4b2ab73c309
Signature · 1 line
2.52.0
PATCH 1/5 selftests/landlock: Fix typo in fs_test
--- tools/testing/selftests/landlock/fs_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/landlock/fs_test.c +2 -2
@@ -7069,8 +7069,8 @@ static int matches_log_fs_extra(struct __test_metadata *const _metadata,return -E2BIG ;/* -* It is assume that absolute_path does not contain control characters nor -* spaces, see audit_string_contains_control(). +* It is assumed that absolute_path does not contain control +* characters nor spaces, see audit_string_contains_control(). */ absolute_path =realpath (path,NULL );if (!absolute_path)
Signature · 1 line
2.52.0
PATCH 2/5 selftests/landlock: Fix missing semicolon
In reply to Tingmao WangAdd missing semicolon after EXPECT_EQ(0, close(stream_server_child)) in the scoped_vs_unscoped test. I suspect currently it's just not executing the close statement after the line, but this causes no observable difference.
--- tools/testing/selftests/landlock/scoped_abstract_unix_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/landlock/scoped_abstract_unix_test.c +1 -1
@@ -543,7 +543,7 @@ TEST_F(scoped_vs_unscoped, unix_scoping)ASSERT_EQ (1 ,write (pipe_child[1 ],"." ,1 ));ASSERT_EQ (grand_child,waitpid (grand_child, &status,0 )); - EXPECT_EQ(0, close(stream_server_child)) +EXPECT_EQ (0 ,close (stream_server_child));EXPECT_EQ (0 ,close (dgram_server_child));return ; }
Signature · 1 line
2.52.0
PATCH 3/5 Documentation/landlock: Fix missing case for ABI 6 in downgrade example
In reply to Tingmao WangNote that this code is different from the one in sandboxer.c since sandboxer won't ever add LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF and LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF.
--- Documentation/userspace-api/landlock.rst | 6 ++++++ 1 file changed, 6 insertions(+)
Documentation/userspace-api/landlock.rst +6 -0
@@ -127,6 +127,12 @@ version, and only use the available subset of access rights: /* Removes LANDLOCK_SCOPE_* for ABI < 6 */ ruleset_attr.scoped &= ~(LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET | LANDLOCK_SCOPE_SIGNAL); + __attribute__((fallthrough)); + case 6: + /* Removes LANDLOCK_RESTRICT_SELF_LOG_* for ABI < 7 */ + supported_restrict_flags &= ~(LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON | + LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF | + LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF); } This enables the creation of an inclusive ruleset that will contain our rules.
Signature · 1 line
2.52.0
Mickaël Salaün
maintainerRe: [PATCH 3/5] Documentation/landlock: Fix missing case for ABI 6 in downgrade example
The subject for documentation should just start with "landlock: "
Quoted 3 lines · Tingmao Wang
Note that this code is different from the one in
sandboxer.csince sandboxer won't ever addLANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFFandLANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF.
A more complet patch was sent a few days ago:
https://lore.kernel.org/r/[email protected]
Quoted 2 lines
Fixes 12bfcda73ac2 ("landlock: Add LANDLOCK_RESTRICT_SELF_LOG_*_EXEC_* flags")
Unless required by other parts of the patch, there is no need to backport documentation changes, so there should not be any Fixes tag.
Quoted 10 lines
Signed-off-by Tingmao Wang <[email protected]>--- Documentation/userspace-api/landlock.rst | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/userspace-api/landlock.rst +0
-0
Documentation/userspace-api/landlock.rst version, and only use the available subset of access rights: · line 135
Quoted 9 lines
Documentation/userspace-api/landlock.rst +6
-0@@ -127,6 +127,12 @@ version, and only use the available subset of access rights: /* Removes LANDLOCK_SCOPE_* for ABI < 6 */ ruleset_attr.scoped &= ~(LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET | LANDLOCK_SCOPE_SIGNAL); + __attribute__((fallthrough)); + case 6: + /* Removes LANDLOCK_RESTRICT_SELF_LOG_* for ABI < 7 */ + supported_restrict_flags &= ~(LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON | + LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF | + LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF);
As Günther pointed out, this switch/case might not be needed for this part of the documentation.
Quoted 7 lines
Documentation/userspace-api/landlock.rst +0
-0} This enables the creation of an inclusive ruleset that will contain our rules.Signature · 1 line
2.52.0
PATCH 4/5 selftests/landlock: Use scoped_base_variants.h for ptrace_test
In reply to Tingmao Wangptrace_test.c currently contains a duplicated version of the scoped_domains fixture variants. This patch removes that and make it use the shared scoped_base_variants.h instead, like in scoped_abstract_unix_test and scoped_signal_test.
This required renaming the hierarchy fixture to scoped_domains, but the test is otherwise the same.
--- .../testing/selftests/landlock/ptrace_test.c | 154 +----------------- .../selftests/landlock/scoped_base_variants.h | 9 +- 2 files changed, 12 insertions(+), 151 deletions(-)
tools/testing/selftests/landlock/ptrace_test.c +5 -149
@@ -86,16 +86,9 @@ static int get_yama_ptrace_scope(void) }/* clang-format off */ -FIXTURE (hierarchy) {}; +FIXTURE (scoped_domains) {};/* clang-format on */ -FIXTURE_VARIANT (hierarchy) -{ -const bool domain_both; -const bool domain_parent; -const bool domain_child; -}; -/* * Test multiple tracing combinations between a parent process P1 and a child * process P2.
@@ -104,155 +97,18 @@ FIXTURE_VARIANT(hierarchy)* restriction is enforced in addition to any Landlock check, which means that * all P2 requests to trace P1 would be denied. */ +#include "scoped_base_variants.h" -/* -* No domain -* -* P1-. P1 -> P2 : allow -* \ P2 -> P1 : allow -* 'P2 -*/ -/* clang-format off */ -FIXTURE_VARIANT_ADD(hierarchy, allow_without_domain) { -/* clang-format on */ - .domain_both = false, - .domain_parent = false, - .domain_child = false, -}; - -/* -* Child domain -* -* P1--. P1 -> P2 : allow -* \ P2 -> P1 : deny -* .'-----. -* | P2 | -* '------' -*/ -/* clang-format off */ -FIXTURE_VARIANT_ADD(hierarchy, allow_with_one_domain) { -/* clang-format on */ - .domain_both = false, - .domain_parent = false, - .domain_child = true, -}; - -/* -* Parent domain -* .------. -* | P1 --. P1 -> P2 : deny -* '------' \ P2 -> P1 : allow -* ' -* P2 -*/ -/* clang-format off */ -FIXTURE_VARIANT_ADD(hierarchy, deny_with_parent_domain) { -/* clang-format on */ - .domain_both = false, - .domain_parent = true, - .domain_child = false, -}; - -/* -* Parent + child domain (siblings) -* .------. -* | P1 ---. P1 -> P2 : deny -* '------' \ P2 -> P1 : deny -* .---'--. -* | P2 | -* '------' -*/ -/* clang-format off */ -FIXTURE_VARIANT_ADD(hierarchy, deny_with_sibling_domain) { -/* clang-format on */ - .domain_both = false, - .domain_parent = true, - .domain_child = true, -}; - -/* -* Same domain (inherited) -* .-------------. -* | P1----. | P1 -> P2 : allow -* | \ | P2 -> P1 : allow -* | ' | -* | P2 | -* '-------------' -*/ -/* clang-format off */ -FIXTURE_VARIANT_ADD(hierarchy, allow_sibling_domain) { -/* clang-format on */ - .domain_both = true, - .domain_parent = false, - .domain_child = false, -}; - -/* -* Inherited + child domain -* .-----------------. -* | P1----. | P1 -> P2 : allow -* | \ | P2 -> P1 : deny -* | .-'----. | -* | | P2 | | -* | '------' | -* '-----------------' -*/ -/* clang-format off */ -FIXTURE_VARIANT_ADD(hierarchy, allow_with_nested_domain) { -/* clang-format on */ - .domain_both = true, - .domain_parent = false, - .domain_child = true, -}; - -/* -* Inherited + parent domain -* .-----------------. -* |.------. | P1 -> P2 : deny -* || P1 ----. | P2 -> P1 : allow -* |'------' \ | -* | ' | -* | P2 | -* '-----------------' -*/ -/* clang-format off */ -FIXTURE_VARIANT_ADD(hierarchy, deny_with_nested_and_parent_domain) { -/* clang-format on */ - .domain_both = true, - .domain_parent = true, - .domain_child = false, -}; - -/* -* Inherited + parent and child domain (siblings) -* .-----------------. -* | .------. | P1 -> P2 : deny -* | | P1 . | P2 -> P1 : deny -* | '------'\ | -* | \ | -* | .--'---. | -* | | P2 | | -* | '------' | -* '-----------------' -*/ -/* clang-format off */ -FIXTURE_VARIANT_ADD(hierarchy, deny_with_forked_domain) { -/* clang-format on */ - .domain_both = true, - .domain_parent = true, - .domain_child = true, -}; - -FIXTURE_SETUP (hierarchy) +FIXTURE_SETUP (scoped_domains) { } -FIXTURE_TEARDOWN (hierarchy) +FIXTURE_TEARDOWN (scoped_domains) { }/* Test PTRACE_TRACEME and PTRACE_ATTACH for parent and child. */ -TEST_F(hierarchy, trace) +TEST_F(scoped_domains, trace) {pid_t child, parent;int status, err_proc_read;
tools/testing/selftests/landlock/scoped_base_variants.h +7 -2
@@ -1,8 +1,13 @@/* SPDX-License-Identifier: GPL-2.0 */ /* -* Landlock scoped_domains variants +* Landlock scoped_domains test variant definition. * -* See the hierarchy variants from ptrace_test.c +* This file defines a fixture variant "scoped_domains" that has all +* permutations of parent/child process being in separate or shared +* Landlock domain, or not being in a Landlock domain at all. +* +* Scoped access tests can include this file to avoid repeating these +* combinations. * * Copyright © 2017-2020 Mickaël Salaün <[email protected]> * Copyright © 2019-2020 ANSSI
Signature · 1 line
2.52.0
RFC PATCH 5/5 landlock: Improve the comment for domain_is_scoped
In reply to Tingmao WangCurrently it is not obvious what "scoped" mean, and the fact that the function returns true when access should be denied is slightly surprising and in need of documentation.
---
Open to discussion on whether this actually explains it better.
security/landlock/task.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
security/landlock/task.c +5 -4
@@ -166,15 +166,16 @@ static int hook_ptrace_traceme(struct task_struct *const parent) }/** -* domain_is_scoped - Checks if the client domain is scoped in the same -* domain as the server. +* domain_is_scoped - Check if an interaction from a client/sender to a +* server/receiver should be restricted based on scope controls. * * @client: IPC sender domain. * @server: IPC receiver domain. * @scope: The scope restriction criteria. * -* Returns: True if the @client domain is scoped to access the @server, -* unless the @server is also scoped in the same domain as @client. +* Returns: True if the @server is in a different domain from @client, and +* the @client domain is scoped to access the @server (i.e. access +* should be denied). */ static bool domain_is_scoped(const struct landlock_ruleset *const client, const struct landlock_ruleset *const server,
Signature · 1 line
2.52.0
Mickaël Salaün
maintainerRe: [RFC PATCH 5/5] landlock: Improve the comment for domain_is_scoped
Quoted 25 lines · Tingmao Wang
Currently it is not obvious what "scoped" mean, and the fact that the function returns true when access should be denied is slightly surprising and in need of documentation.
Cc Tahera Fahimi <[email protected]>Signed-off-by Tingmao Wang <[email protected]>---
Open to discussion on whether this actually explains it better.
security/landlock/task.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)security/landlock/task.c +2
-2@@ -166,15 +166,16 @@ static int hook_ptrace_traceme(struct task_struct *const parent) }/** -* domain_is_scoped - Checks if the client domain is scoped in the same -* domain as the server. +* domain_is_scoped - Check if an interaction from a client/sender to a +* server/receiver should be restricted based on scope controls.
security/landlock/task.c static int hook_ptrace_traceme(struct task_struct *const parent) · line 178
Quoted 10 lines
security/landlock/task.c +3
-2* * @client: IPC sender domain. * @server: IPC receiver domain. * @scope: The scope restriction criteria. * -* Returns: True if the @client domain is scoped to access the @server, -* unless the @server is also scoped in the same domain as @client. +* Returns: True if the @server is in a different domain from @client, and +* the @client domain is scoped to access the @server (i.e. access +* should be denied).
I removed the "the" and the heading spaces:
*
- * Returns: True if the @client domain is scoped to access the @server,
- * unless the @server is also scoped in the same domain as @client.
+ * Returns: True if @server is in a different domain from @client, and @client
+ * is scoped to access @server (i.e. access should be denied).
*/
Quoted 6 lines
security/landlock/task.c +0
-0*/ static bool domain_is_scoped(const struct landlock_ruleset *const client, const struct landlock_ruleset *const server,Signature · 1 line
2.52.0
Quoted 5 lines · Tingmao Wang
Hi,
This series contains various unrelated improvements I stashed while working on pathname Unix socket scopes (I will send the series for that tomorrow). Only test/comment/doc changes.
Thanks!
Quoted 4 lines
I was planning to include this in the upcoming Unix socket scope series but I thought maybe it would be better to send them separately, as they would be unrelated to the main topic of that series.
Yes, that's better.
Quoted 3 lines
Tingmao Wang (5):
selftests/landlock: Fix typo infs_test
Applied
Quoted 1 line
selftests/landlock: Fix missing semicolon
Applied
Quoted 2 lines
Documentation/landlock: Fix missing case for ABI 6 in downgrade
example
Not applied, see review.
Quoted 1 line
selftests/landlock: Use
scoped_base_variants.hforptrace_test
Applied
Quoted 1 line
landlock: Improve the comment for
domain_is_scoped
Applied with cosmetic changes.
Quoted 15 lines
Documentation/userspace-api/landlock.rst | 6 + security/landlock/task.c | 9 +- tools/testing/selftests/landlock/fs_test.c | 4 +- .../testing/selftests/landlock/ptrace_test.c | 154 +----------------- .../landlock/scoped_abstract_unix_test.c | 2 +- .../selftests/landlock/scoped_base_variants.h | 9 +- 6 files changed, 26 insertions(+), 158 deletions(-)base-commit:
161db1810f3625e97ab414908dbcf4b2ab73c309Signature · 1 line
2.52.0