[PATCH v3 0/3] samples/landlock: Fix port parsing in sandboxer

[PATCH v3 0/3] samples/landlock: Fix port parsing in sandboxer

[Matthieu Buffet]

Hi Mickaël, Mikhail,

This v3 should have all your comments merged, thank your for your time
reviewing this.

Changes since v2:
- replaced help message with a static format string
- added possible LL_SCOPED values and their effect (also for easier
  understanding of expected environment variables, cc: Tahera)
- rebased on mic-linux/next

v2:
Link: https://lore.kernel.org/all/20241003005042.258991-1-matthieu@buffet.re/

Matthieu Buffet (3):
  samples/landlock: Fix port parsing in sandboxer
  samples/landlock: Refactor help message
  samples/landlock: Clarify option parsing behaviour

 samples/landlock/sandboxer.c | 112 +++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 43 deletions(-)


base-commit: fe76bd133024aaef12d12a7d58fa3e8d138d3bf3
-- 
2.39.5

[PATCH v3 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 strtoull(3) instead, which allows error checking. Check that the entire
string has been parsed correctly without overflows/underflows, but not
that the __u64 (the type of struct landlock_net_port_attr.port)
is a valid __u16 port: that is already done by the kernel.

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

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index f847e832ba14..4cbef9d2f15b 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -60,6 +60,25 @@ 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 = 0;
+	__u64 num;
+
+	errno = 0;
+	num = strtoull(numstr, &endptr, 10);
+	if (errno != 0)
+		err = errno;
+	/* Was the string empty, or not entirely parsed successfully? */
+	else if ((*numstr == '\0') || (*endptr != '\0'))
+		err = EINVAL;
+	else
+		*num_dst = num;
+
+	return err;
+}
+
 static int parse_path(char *env_path, const char ***const path_list)
 {
 	int i, num_paths = 0;
@@ -160,7 +179,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 +189,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.5

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

[Mickaël Salaün]

Thanks! I pushed the three patches in my -next branch with minor
changes.

On Sat, Oct 19, 2024 at 05:15:32PM +0200, Matthieu Buffet wrote:
> 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 strtoull(3) instead, which allows error checking. Check that the entire
> string has been parsed correctly without overflows/underflows, but not
> that the __u64 (the type of struct landlock_net_port_attr.port)
> is a valid __u16 port: that is already done by the kernel.
> 
> Fixes: 5e990dcef12e ("samples/landlock: Support TCP restrictions")
> Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
> ---
>  samples/landlock/sandboxer.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index f847e832ba14..4cbef9d2f15b 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -60,6 +60,25 @@ 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 = 0;
> +	__u64 num;
> +
> +	errno = 0;
> +	num = strtoull(numstr, &endptr, 10);
> +	if (errno != 0)
> +		err = errno;
> +	/* Was the string empty, or not entirely parsed successfully? */
> +	else if ((*numstr == '\0') || (*endptr != '\0'))

We cannot pass "0 " but we can still pass " 0".  I'm good with that
though.

> +		err = EINVAL;
> +	else
> +		*num_dst = num;
> +
> +	return err;
> +}
> +
>  static int parse_path(char *env_path, const char ***const path_list)
>  {
>  	int i, num_paths = 0;
> @@ -160,7 +179,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 +189,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.5
> 
> 

[PATCH v3 2/3] samples/landlock: Refactor help message

[Matthieu Buffet]

Help message is getting larger with each new supported feature (scopes,
and soon UDP). Also the large number of calls to fprintf with environment
variables make it hard to read. Refactor it away into a single simpler
constant format string.

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

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 4cbef9d2f15b..38fc6ebd7222 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -290,6 +290,43 @@ static bool check_ruleset_scope(const char *const env_var,
 
 #define LANDLOCK_ABI_LAST 6
 
+#define XSTR(s) #s
+#define STR(s) XSTR(s)
+
+/* clang-format off */
+
+static const char help[] =
+	"usage: "
+	ENV_FS_RO_NAME "=\"...\" "
+	ENV_FS_RW_NAME "=\"...\" "
+	ENV_TCP_BIND_NAME "=\"...\" "
+	ENV_TCP_CONNECT_NAME "=\"...\" "
+	ENV_SCOPED_NAME "=\"...\" %1$s <cmd> [args]...\n"
+	"\n"
+	"Execute a command in a restricted environment.\n"
+	"\n"
+	"Environment variables containing paths and ports each separated by a colon:\n"
+	"* " ENV_FS_RO_NAME ": list of paths allowed to be used in a read-only way\n"
+	"* " ENV_FS_RW_NAME ": list of paths allowed to be used in a read-write way\n"
+	"\n"
+	"Environment variables containing ports are optional and could be skipped.\n"
+	"* " ENV_TCP_BIND_NAME ": list of ports allowed to bind (server)\n"
+	"* " ENV_TCP_CONNECT_NAME ": list of ports allowed to connect (client)\n"
+	"* " ENV_SCOPED_NAME ": list of scoped IPCs\n"
+	"\n"
+	"Example:\n"
+	ENV_FS_RO_NAME "=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
+	ENV_FS_RW_NAME "=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
+	ENV_TCP_BIND_NAME "=\"9418\" "
+	ENV_TCP_CONNECT_NAME "=\"80:443\" "
+	ENV_SCOPED_NAME "=\"a:s\" "
+	"%1$s bash -i\n"
+	"\n"
+	"This sandboxer can use Landlock features up to ABI version "
+	STR(LANDLOCK_ABI_LAST) ".\n";
+
+/* clang-format on */
+
 int main(const int argc, char *const argv[], char *const *const envp)
 {
 	const char *cmd_path;
@@ -308,47 +345,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);
+		fprintf(stderr, help, argv[0]);
 		return 1;
 	}
 
-- 
2.39.5

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

[Matthieu Buffet]

Clarify the distinction between filesystem variables (mandatory)
and all others (optional).
For optional variables, explain the difference between unset variables
(no access check performed) and empty variables (nothing allowed for
lists of allowed paths/ports, or no effect for lists of scopes).
List LL_SCOPED values understood and their effect.

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

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 38fc6ebd7222..96b451cf0531 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -296,23 +296,24 @@ static bool check_ruleset_scope(const char *const env_var,
 /* clang-format off */
 
 static const char help[] =
-	"usage: "
-	ENV_FS_RO_NAME "=\"...\" "
-	ENV_FS_RW_NAME "=\"...\" "
-	ENV_TCP_BIND_NAME "=\"...\" "
-	ENV_TCP_CONNECT_NAME "=\"...\" "
-	ENV_SCOPED_NAME "=\"...\" %1$s <cmd> [args]...\n"
+	"usage: " ENV_FS_RO_NAME "=\"...\" " ENV_FS_RW_NAME "=\"...\" "
+	"[other environment variables] %1$s <cmd> [args]...\n"
 	"\n"
-	"Execute a command in a restricted environment.\n"
+	"Execute the given command in a restricted environment.\n"
+	"Multi-valued settings (lists of ports, paths, scopes) are colon-delimited.\n"
 	"\n"
-	"Environment variables containing paths and ports each separated by a colon:\n"
-	"* " ENV_FS_RO_NAME ": list of paths allowed to be used in a read-only way\n"
-	"* " ENV_FS_RW_NAME ": list of paths allowed to be used in a read-write way\n"
+	"Mandatory settings:\n"
+	"* " ENV_FS_RO_NAME ": paths allowed to be used in a read-only way\n"
+	"* " ENV_FS_RW_NAME ": paths allowed to be used in a read-write way\n"
 	"\n"
-	"Environment variables containing ports are optional and could be skipped.\n"
-	"* " ENV_TCP_BIND_NAME ": list of ports allowed to bind (server)\n"
-	"* " ENV_TCP_CONNECT_NAME ": list of ports allowed to connect (client)\n"
-	"* " ENV_SCOPED_NAME ": list of scoped IPCs\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"
+	"* " ENV_TCP_BIND_NAME ": ports allowed to bind (server)\n"
+	"* " ENV_TCP_CONNECT_NAME ": ports allowed to connect (client)\n"
+	"* " ENV_SCOPED_NAME ": actions denied on the outside of the landlock domain\n"
+	"  - \"a\" to restrict opening abstract unix sockets\n"
+	"  - \"s\" to restrict sending signals\n"
 	"\n"
 	"Example:\n"
 	ENV_FS_RO_NAME "=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
-- 
2.39.5

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

[Mickaël Salaün]

On Sat, Oct 19, 2024 at 05:15:34PM +0200, Matthieu Buffet wrote:
> Clarify the distinction between filesystem variables (mandatory)
> and all others (optional).
> For optional variables, explain the difference between unset variables
> (no access check performed) and empty variables (nothing allowed for
> lists of allowed paths/ports, or no effect for lists of scopes).
> List LL_SCOPED values understood and their effect.
> 
> Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
> ---
>  samples/landlock/sandboxer.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index 38fc6ebd7222..96b451cf0531 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -296,23 +296,24 @@ static bool check_ruleset_scope(const char *const env_var,
>  /* clang-format off */
>  
>  static const char help[] =
> -	"usage: "
> -	ENV_FS_RO_NAME "=\"...\" "
> -	ENV_FS_RW_NAME "=\"...\" "
> -	ENV_TCP_BIND_NAME "=\"...\" "
> -	ENV_TCP_CONNECT_NAME "=\"...\" "
> -	ENV_SCOPED_NAME "=\"...\" %1$s <cmd> [args]...\n"
> +	"usage: " ENV_FS_RO_NAME "=\"...\" " ENV_FS_RW_NAME "=\"...\" "
> +	"[other environment variables] %1$s <cmd> [args]...\n"
>  	"\n"
> -	"Execute a command in a restricted environment.\n"
> +	"Execute the given command in a restricted environment.\n"
> +	"Multi-valued settings (lists of ports, paths, scopes) are colon-delimited.\n"
>  	"\n"
> -	"Environment variables containing paths and ports each separated by a colon:\n"
> -	"* " ENV_FS_RO_NAME ": list of paths allowed to be used in a read-only way\n"
> -	"* " ENV_FS_RW_NAME ": list of paths allowed to be used in a read-write way\n"
> +	"Mandatory settings:\n"
> +	"* " ENV_FS_RO_NAME ": paths allowed to be used in a read-only way\n"
> +	"* " ENV_FS_RW_NAME ": paths allowed to be used in a read-write way\n"
>  	"\n"
> -	"Environment variables containing ports are optional and could be skipped.\n"
> -	"* " ENV_TCP_BIND_NAME ": list of ports allowed to bind (server)\n"
> -	"* " ENV_TCP_CONNECT_NAME ": list of ports allowed to connect (client)\n"
> -	"* " ENV_SCOPED_NAME ": list of scoped IPCs\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"

I would just add ":" at the end of the line.  No need to send another
patch for that.

> +	"* " ENV_TCP_BIND_NAME ": ports allowed to bind (server)\n"
> +	"* " ENV_TCP_CONNECT_NAME ": ports allowed to connect (client)\n"
> +	"* " ENV_SCOPED_NAME ": actions denied on the outside of the landlock domain\n"
> +	"  - \"a\" to restrict opening abstract unix sockets\n"
> +	"  - \"s\" to restrict sending signals\n"
>  	"\n"
>  	"Example:\n"
>  	ENV_FS_RO_NAME "=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
> -- 
> 2.39.5
> 
>