[PATCH v2 1/3] samples/landlock: Fix port parsing in sandboxer

[PATCH v2 1/3] samples/landlock: Fix port parsing in sandboxer

[Matthieu Buffet]

If you want to specify that no port can be bind()ed, you would think
(looking quickly at both help message and code) that setting LL_TCP_BIND=""
would do it.

However the code splits on ":" then applies atoi(), which does not allow
checking for errors. Passing an empty string returns 0, which is
interpreted as "allow bind(0)", which means bind to any ephemeral port.
This bug occurs whenever passing an empty string or when leaving a
trailing/leading colon, making it impossible to completely deny bind().

To reproduce:
export LL_FS_RO="/" LL_FS_RW="" LL_TCP_BIND=""
./sandboxer strace -e bind nc -n -vvv -l -p 0
Executing the sandboxed command...
bind(3, {sa_family=AF_INET, sin_port=htons(0),
     sin_addr=inet_addr("0.0.0.0")}, 16) = 0
Listening on 0.0.0.0 37629

Use strtoul() instead, which allows error checking. Check that the entire
string has been parsed correctly without overflows/underflows.
Don't check that the __u64 (the type of struct landlock_net_port_attr.port)
is a valid __u16 port: that is already done by the kernel.
Two places check for an empty string, that is just to make the helper
function safer to use in the future.

Fixes: 5e990dcef12e ("samples/landlock: Support TCP restrictions")
Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
---
 samples/landlock/sandboxer.c | 37 ++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index f847e832ba14..aff5ef808e22 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -16,6 +16,7 @@
 #include <linux/prctl.h>
 #include <linux/socket.h>
 #include <stddef.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -60,6 +61,29 @@ static inline int landlock_restrict_self(const int ruleset_fd,
 #define ENV_SCOPED_NAME "LL_SCOPED"
 #define ENV_DELIMITER ":"
 
+static int str2num(const char *numstr, __u64 *num_dst)
+{
+	char *endptr = NULL;
+	int err = 1;
+	__u64 num;
+
+	if (*numstr == '\0')
+		goto out;
+
+	errno = 0;
+	num = strtoull(numstr, &endptr, 10);
+	if (errno != 0)
+		goto out;
+
+	if (*endptr != '\0')
+		goto out;
+
+	*num_dst = num;
+	err = 0;
+out:
+	return err;
+}
+
 static int parse_path(char *env_path, const char ***const path_list)
 {
 	int i, num_paths = 0;
@@ -160,7 +184,6 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
 	char *env_port_name, *env_port_name_next, *strport;
 	struct landlock_net_port_attr net_port = {
 		.allowed_access = allowed_access,
-		.port = 0,
 	};
 
 	env_port_name = getenv(env_var);
@@ -171,7 +194,17 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
 
 	env_port_name_next = env_port_name;
 	while ((strport = strsep(&env_port_name_next, ENV_DELIMITER))) {
-		net_port.port = atoi(strport);
+		__u64 port;
+
+		if (strcmp(strport, "") == 0)
+			continue;
+
+		if (str2num(strport, &port)) {
+			fprintf(stderr,
+				"Failed to parse port at \"%s\"\n", strport);
+			goto out_free_name;
+		}
+		net_port.port = port;
 		if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
 				      &net_port, 0)) {
 			fprintf(stderr,
-- 
2.39.2

[PATCH v2 2/3] samples/landlock: Refactor --help message in function

[Matthieu Buffet]

Help message is getting larger with each new supported feature (scopes,
and soon UDP). Refactor it away into a separate helper function.

Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
---
 samples/landlock/sandboxer.c | 87 +++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 41 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index aff5ef808e22..f16994d35d9e 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -295,6 +295,51 @@ static bool check_ruleset_scope(const char *const env_var,
 
 #define LANDLOCK_ABI_LAST 6
 
+static void print_help(const char *argv0)
+{
+	fprintf(stderr,
+		"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
+		"<cmd> [args]...\n\n",
+		ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
+		ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv0);
+	fprintf(stderr,
+		"Execute a command in a restricted environment.\n\n");
+	fprintf(stderr,
+		"Environment variables containing paths and ports "
+		"each separated by a colon:\n");
+	fprintf(stderr,
+		"* %s: list of paths allowed to be used in a read-only way.\n",
+		ENV_FS_RO_NAME);
+	fprintf(stderr,
+		"* %s: list of paths allowed to be used in a read-write way.\n\n",
+		ENV_FS_RW_NAME);
+	fprintf(stderr,
+		"Environment variables containing ports are optional "
+		"and could be skipped.\n");
+	fprintf(stderr,
+		"* %s: list of ports allowed to bind (server).\n",
+		ENV_TCP_BIND_NAME);
+	fprintf(stderr,
+		"* %s: list of ports allowed to connect (client).\n",
+		ENV_TCP_CONNECT_NAME);
+	fprintf(stderr, "* %s: list of scoped IPCs.\n",
+		ENV_SCOPED_NAME);
+	fprintf(stderr,
+		"\nexample:\n"
+		"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
+		"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
+		"%s=\"9418\" "
+		"%s=\"80:443\" "
+		"%s=\"a:s\" "
+		"%s bash -i\n\n",
+		ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
+		ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv0);
+	fprintf(stderr,
+		"This sandboxer can use Landlock features "
+		"up to ABI version %d.\n",
+		LANDLOCK_ABI_LAST);
+}
+
 int main(const int argc, char *const argv[], char *const *const envp)
 {
 	const char *cmd_path;
@@ -313,47 +358,7 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	};
 
 	if (argc < 2) {
-		fprintf(stderr,
-			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
-			"<cmd> [args]...\n\n",
-			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
-			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
-		fprintf(stderr,
-			"Execute a command in a restricted environment.\n\n");
-		fprintf(stderr,
-			"Environment variables containing paths and ports "
-			"each separated by a colon:\n");
-		fprintf(stderr,
-			"* %s: list of paths allowed to be used in a read-only way.\n",
-			ENV_FS_RO_NAME);
-		fprintf(stderr,
-			"* %s: list of paths allowed to be used in a read-write way.\n\n",
-			ENV_FS_RW_NAME);
-		fprintf(stderr,
-			"Environment variables containing ports are optional "
-			"and could be skipped.\n");
-		fprintf(stderr,
-			"* %s: list of ports allowed to bind (server).\n",
-			ENV_TCP_BIND_NAME);
-		fprintf(stderr,
-			"* %s: list of ports allowed to connect (client).\n",
-			ENV_TCP_CONNECT_NAME);
-		fprintf(stderr, "* %s: list of scoped IPCs.\n",
-			ENV_SCOPED_NAME);
-		fprintf(stderr,
-			"\nexample:\n"
-			"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
-			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
-			"%s=\"9418\" "
-			"%s=\"80:443\" "
-			"%s=\"a:s\" "
-			"%s bash -i\n\n",
-			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
-			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
-		fprintf(stderr,
-			"This sandboxer can use Landlock features "
-			"up to ABI version %d.\n",
-			LANDLOCK_ABI_LAST);
+		print_help(argv[0]);
 		return 1;
 	}
 
-- 
2.39.2

[PATCH v2 3/3] samples/landlock: Clarify option parsing behaviour

[Matthieu Buffet]

- Clarify which environment variables are optional, which ones are
  mandatory
- Clarify the difference between unset variables and empty ones

Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
---
 samples/landlock/sandboxer.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index f16994d35d9e..a28e4a9c5f87 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -298,24 +298,27 @@ static bool check_ruleset_scope(const char *const env_var,
 static void print_help(const char *argv0)
 {
 	fprintf(stderr,
-		"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
+		"usage: %s=\"...\" %s=\"...\" [other environment variables] %s "
 		"<cmd> [args]...\n\n",
-		ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
-		ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv0);
+		ENV_FS_RO_NAME, ENV_FS_RW_NAME, argv0);
 	fprintf(stderr,
 		"Execute a command in a restricted environment.\n\n");
 	fprintf(stderr,
-		"Environment variables containing paths and ports "
-		"each separated by a colon:\n");
+		"All environment variables can be multi-valued, with a "
+		"colon delimiter.\n"
+		"\n"
+		"Mandatory settings:\n");
 	fprintf(stderr,
 		"* %s: list of paths allowed to be used in a read-only way.\n",
 		ENV_FS_RO_NAME);
 	fprintf(stderr,
-		"* %s: list of paths allowed to be used in a read-write way.\n\n",
+		"* %s: list of paths allowed to be used in a read-write way.\n",
 		ENV_FS_RW_NAME);
 	fprintf(stderr,
-		"Environment variables containing ports are optional "
-		"and could be skipped.\n");
+		"\n"
+		"Optional settings (when not set, their associated access "
+		"check is always allowed, which is different from an empty "
+		"string which means an empty list)\n");
 	fprintf(stderr,
 		"* %s: list of ports allowed to bind (server).\n",
 		ENV_TCP_BIND_NAME);
@@ -325,7 +328,8 @@ static void print_help(const char *argv0)
 	fprintf(stderr, "* %s: list of scoped IPCs.\n",
 		ENV_SCOPED_NAME);
 	fprintf(stderr,
-		"\nexample:\n"
+		"\n"
+		"Example:\n"
 		"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
 		"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
 		"%s=\"9418\" "
-- 
2.39.2

Re: [PATCH v2 2/3] samples/landlock: Refactor --help message in function

[Mickaël Salaün]

This series looks good to me, but each patch still needs to pass
clang-format.

On Thu, Oct 03, 2024 at 02:50:41AM +0200, Matthieu Buffet wrote:
> Help message is getting larger with each new supported feature (scopes,
> and soon UDP). Refactor it away into a separate helper function.
> 
> Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
> ---
>  samples/landlock/sandboxer.c | 87 +++++++++++++++++++-----------------
>  1 file changed, 46 insertions(+), 41 deletions(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index aff5ef808e22..f16994d35d9e 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -295,6 +295,51 @@ static bool check_ruleset_scope(const char *const env_var,
>  
>  #define LANDLOCK_ABI_LAST 6
>  
> +static void print_help(const char *argv0)
> +{
> +	fprintf(stderr,
> +		"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
> +		"<cmd> [args]...\n\n",
> +		ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> +		ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv0);
> +	fprintf(stderr,
> +		"Execute a command in a restricted environment.\n\n");
> +	fprintf(stderr,
> +		"Environment variables containing paths and ports "
> +		"each separated by a colon:\n");
> +	fprintf(stderr,
> +		"* %s: list of paths allowed to be used in a read-only way.\n",
> +		ENV_FS_RO_NAME);
> +	fprintf(stderr,
> +		"* %s: list of paths allowed to be used in a read-write way.\n\n",
> +		ENV_FS_RW_NAME);
> +	fprintf(stderr,
> +		"Environment variables containing ports are optional "
> +		"and could be skipped.\n");
> +	fprintf(stderr,
> +		"* %s: list of ports allowed to bind (server).\n",
> +		ENV_TCP_BIND_NAME);
> +	fprintf(stderr,
> +		"* %s: list of ports allowed to connect (client).\n",
> +		ENV_TCP_CONNECT_NAME);
> +	fprintf(stderr, "* %s: list of scoped IPCs.\n",
> +		ENV_SCOPED_NAME);
> +	fprintf(stderr,
> +		"\nexample:\n"
> +		"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
> +		"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
> +		"%s=\"9418\" "
> +		"%s=\"80:443\" "
> +		"%s=\"a:s\" "
> +		"%s bash -i\n\n",
> +		ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> +		ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv0);
> +	fprintf(stderr,
> +		"This sandboxer can use Landlock features "
> +		"up to ABI version %d.\n",
> +		LANDLOCK_ABI_LAST);
> +}

While we are at it, could you please transform this set of fprintf()
calls to a static const char[] with "%1$s" where we need argv0, and use
the defined constants in place instead of %s (with string conversion for
the ABI).  This would make the whole help easier to read and maintain.

We'll need some help from the preprocessor as explained in
https://gcc.gnu.org/onlinedocs/cpp/Stringizing.html:

#define XSTR(s) STR(s)
#define STR(s) #s

static const char help[] =
"usage: " ENV_FS_RO_NAME "=\"...\" [...] %1$s "
[...]
"up to ABI version " XSTR(LANDLOCK_ABI_LAST) ".\n";

> +
>  int main(const int argc, char *const argv[], char *const *const envp)
>  {
>  	const char *cmd_path;
> @@ -313,47 +358,7 @@ int main(const int argc, char *const argv[], char *const *const envp)
>  	};
>  
>  	if (argc < 2) {
> -		fprintf(stderr,
> -			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
> -			"<cmd> [args]...\n\n",
> -			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> -			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> -		fprintf(stderr,
> -			"Execute a command in a restricted environment.\n\n");
> -		fprintf(stderr,
> -			"Environment variables containing paths and ports "
> -			"each separated by a colon:\n");
> -		fprintf(stderr,
> -			"* %s: list of paths allowed to be used in a read-only way.\n",
> -			ENV_FS_RO_NAME);
> -		fprintf(stderr,
> -			"* %s: list of paths allowed to be used in a read-write way.\n\n",
> -			ENV_FS_RW_NAME);
> -		fprintf(stderr,
> -			"Environment variables containing ports are optional "
> -			"and could be skipped.\n");
> -		fprintf(stderr,
> -			"* %s: list of ports allowed to bind (server).\n",
> -			ENV_TCP_BIND_NAME);
> -		fprintf(stderr,
> -			"* %s: list of ports allowed to connect (client).\n",
> -			ENV_TCP_CONNECT_NAME);
> -		fprintf(stderr, "* %s: list of scoped IPCs.\n",
> -			ENV_SCOPED_NAME);
> -		fprintf(stderr,
> -			"\nexample:\n"
> -			"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
> -			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
> -			"%s=\"9418\" "
> -			"%s=\"80:443\" "
> -			"%s=\"a:s\" "
> -			"%s bash -i\n\n",
> -			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> -			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> -		fprintf(stderr,
> -			"This sandboxer can use Landlock features "
> -			"up to ABI version %d.\n",
> -			LANDLOCK_ABI_LAST);
> +		print_help(argv[0]);
>  		return 1;
>  	}
>  
> -- 
> 2.39.2
> 
> 

Re: [PATCH v2 3/3] samples/landlock: Clarify option parsing behaviour

[Mickaël Salaün]

On Thu, Oct 03, 2024 at 02:50:42AM +0200, Matthieu Buffet wrote:
> - Clarify which environment variables are optional, which ones are
>   mandatory
> - Clarify the difference between unset variables and empty ones

You can remove the "-" and just go with two sentences.

> 
> Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
> ---
>  samples/landlock/sandboxer.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index f16994d35d9e..a28e4a9c5f87 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -298,24 +298,27 @@ static bool check_ruleset_scope(const char *const env_var,
>  static void print_help(const char *argv0)
>  {
>  	fprintf(stderr,
> -		"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
> +		"usage: %s=\"...\" %s=\"...\" [other environment variables] %s "
>  		"<cmd> [args]...\n\n",
> -		ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> -		ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv0);
> +		ENV_FS_RO_NAME, ENV_FS_RW_NAME, argv0);
>  	fprintf(stderr,
>  		"Execute a command in a restricted environment.\n\n");
>  	fprintf(stderr,
> -		"Environment variables containing paths and ports "
> -		"each separated by a colon:\n");
> +		"All environment variables can be multi-valued, with a "
> +		"colon delimiter.\n"
> +		"\n"
> +		"Mandatory settings:\n");
>  	fprintf(stderr,
>  		"* %s: list of paths allowed to be used in a read-only way.\n",
>  		ENV_FS_RO_NAME);
>  	fprintf(stderr,
> -		"* %s: list of paths allowed to be used in a read-write way.\n\n",
> +		"* %s: list of paths allowed to be used in a read-write way.\n",
>  		ENV_FS_RW_NAME);
>  	fprintf(stderr,
> -		"Environment variables containing ports are optional "
> -		"and could be skipped.\n");
> +		"\n"
> +		"Optional settings (when not set, their associated access "
> +		"check is always allowed, which is different from an empty "
> +		"string which means an empty list)\n");
>  	fprintf(stderr,
>  		"* %s: list of ports allowed to bind (server).\n",
>  		ENV_TCP_BIND_NAME);
> @@ -325,7 +328,8 @@ static void print_help(const char *argv0)
>  	fprintf(stderr, "* %s: list of scoped IPCs.\n",
>  		ENV_SCOPED_NAME);
>  	fprintf(stderr,
> -		"\nexample:\n"
> +		"\n"
> +		"Example:\n"
>  		"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
>  		"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
>  		"%s=\"9418\" "
> -- 
> 2.39.2
> 
>