[iproute PATCH] ip netns: Make sure netns name is sane

[iproute PATCH] ip netns: Make sure netns name is sane

[Phil Sutter]

In order to keep track of created netns, 'ip netns' creates a mount
point inside NETNS_RUN_DIR. By not checking the user-specified name, it
allowed to create that mount point outside of NETNS_RUN_DIR and hence
lose track of it afterwards:

| # ip netns add ../../tmp/foobar
| # ip netns list
| # mount | grep foobar
| nsfs on /tmp/foobar type nsfs (rw)

Prevent this by making sure basename() does not see a need to alter the
given netns name.

Fixes: 0dc34c7713bb7 ("iproute2: Add processless network namespace support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipnetns.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 0b0378ab6560c..4eee85e146b3d 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -595,6 +595,21 @@ static int create_netns_dir(void)
 	return 0;
 }
 
+static bool is_basename(const char *name)
+{
+	char *name_dup = strdup(name);
+	bool rc = true;
+
+	if (!name_dup)
+		return false;
+
+	if (strcmp(basename(name_dup), name))
+		rc = false;
+
+	free(name_dup);
+	return rc;
+}
+
 static int netns_add(int argc, char **argv)
 {
 	/* This function creates a new network namespace and
@@ -616,6 +631,11 @@ static int netns_add(int argc, char **argv)
 	}
 	name = argv[0];
 
+	if (!is_basename(name)) {
+		fprintf(stderr, "Invalid netns name: contains path components\n");
+		return -1;
+	}
+
 	snprintf(netns_path, sizeof(netns_path), "%s/%s", NETNS_RUN_DIR, name);
 
 	if (create_netns_dir())
-- 
2.13.1

Re: [iproute PATCH] ip netns: Make sure netns name is sane

[Stephen Hemminger]

On Mon, 10 Jul 2017 13:19:12 +0200
Phil Sutter <phil@nwl.cc> wrote:

> +static bool is_basename(const char *name)
> +{
> +	char *name_dup = strdup(name);
> +	bool rc = true;
> +
> +	if (!name_dup)
> +		return false;
> +
> +	if (strcmp(basename(name_dup), name))
> +		rc = false;
> +
> +	free(name_dup);
> +	return rc;
> +}

Why not just:

static bool is_basename(const char *name)
{
	return strchr(name '/') == NULL;
}

Re: [iproute PATCH] ip netns: Make sure netns name is sane

[Phil Sutter]

On Mon, Jul 10, 2017 at 08:17:02AM -0700, Stephen Hemminger wrote:
> On Mon, 10 Jul 2017 13:19:12 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > +static bool is_basename(const char *name)
> > +{
> > +	char *name_dup = strdup(name);
> > +	bool rc = true;
> > +
> > +	if (!name_dup)
> > +		return false;
> > +
> > +	if (strcmp(basename(name_dup), name))
> > +		rc = false;
> > +
> > +	free(name_dup);
> > +	return rc;
> > +}
> 
> Why not just:
> 
> static bool is_basename(const char *name)
> {
> 	return strchr(name '/') == NULL;
> }

This is not sufficient since it doesn't cover netns names of '..' and
'.', as Matteo correctly pointed out.

Cheers, Phil

Re: [iproute PATCH] ip netns: Make sure netns name is sane

[Stephen Hemminger]

On Mon, 10 Jul 2017 13:19:12 +0200
Phil Sutter <phil@nwl.cc> wrote:

> +static bool is_basename(const char *name)
> +{
> +	char *name_dup = strdup(name);
> +	bool rc = true;
> +
> +	if (!name_dup)
> +		return false;
> +
> +	if (strcmp(basename(name_dup), name))
> +		rc = false;
> +
> +	free(name_dup);
> +	return rc;
> +}

Looks like natural place to use strdupa.

static bool is_basename(const char *name)
{
	return strcmp(basename(strdupa(name), name) == 0;
}

Re: [iproute PATCH] ip netns: Make sure netns name is sane

[Phil Sutter]

On Wed, Jul 12, 2017 at 09:38:34AM -0700, Stephen Hemminger wrote:
> On Mon, 10 Jul 2017 13:19:12 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > +static bool is_basename(const char *name)
> > +{
> > +	char *name_dup = strdup(name);
> > +	bool rc = true;
> > +
> > +	if (!name_dup)
> > +		return false;
> > +
> > +	if (strcmp(basename(name_dup), name))
> > +		rc = false;
> > +
> > +	free(name_dup);
> > +	return rc;
> > +}
> 
> Looks like natural place to use strdupa.
> 
> static bool is_basename(const char *name)
> {
> 	return strcmp(basename(strdupa(name), name) == 0;
> }

Good point! Anyway, my patch fails to cover 'ip netns del' command
(apart from the '..' issue), so I'd suggest to instead apply Matteo's
version (Message-ID 20170710120831.9355-1-mcroce@redhat.com).

Thanks, Phil