[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241022.Xi2Wiemeehai@digikod.net>
Date: Tue, 22 Oct 2024 20:50:15 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Matthieu Buffet <matthieu@...fet.re>
Cc: Günther Noack <gnoack@...gle.com>,
Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>,
linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
Tahera Fahimi <fahimitahera@...il.com>
Subject: Re: [PATCH v3 1/3] samples/landlock: Fix port parsing in sandboxer
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@...fet.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
>
>
Powered by blists - more mailing lists