[PATCH] netns: avoid directory traversal (was: ip netns: Make sure netns name is sane)

[PATCH] netns: avoid directory traversal (was: ip netns: Make sure netns name is sane)

[Matteo Croce]

Hi Phil,

I noticed that your patch still leaves an uncovered scenario, the one where the
namespace name is "." or "..".
Calling 'ip netns del ..' will remove /var/run which is a symlink to /run on
most systems causing some daemons, eg. dbus, to fail.

ip netns doesn't validate input, allowing creation and deletion of files
relatives to /var/run/netns.
This patch denies creation or deletion of namespaces with names contaning
"/" or that matches exactly "." or "..".
---
 ip/ipnetns.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 0b0378a..4254994 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -766,6 +766,11 @@ static int netns_monitor(int argc, char **argv)
 	return 0;
 }
 
+static int invalid_name(const char *name)
+{
+	return strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, "..");
+}
+
 int do_netns(int argc, char **argv)
 {
 	netns_nsid_socket_init();
@@ -775,6 +780,11 @@ int do_netns(int argc, char **argv)
 		return netns_list(0, NULL);
 	}
 
+	if (argc > 1 && invalid_name(argv[1])) {
+		fprintf(stderr, "Invalid netns name \"%s\"\n", argv[1]);
+		exit(-1);
+	}
+
 	if ((matches(*argv, "list") == 0) || (matches(*argv, "show") == 0) ||
 	    (matches(*argv, "lst") == 0)) {
 		netns_map_init();
-- 
2.9.4

Re: [PATCH] netns: avoid directory traversal (was: ip netns: Make sure netns name is sane)

[Phil Sutter]

Hi Matteo,

On Mon, Jul 10, 2017 at 02:08:31PM +0200, Matteo Croce wrote:
> I noticed that your patch still leaves an uncovered scenario, the one where the
> namespace name is "." or "..".
> Calling 'ip netns del ..' will remove /var/run which is a symlink to /run on
> most systems causing some daemons, eg. dbus, to fail.

Oh, indeed. My patch even checks the name for 'ip netns add' only!

> ip netns doesn't validate input, allowing creation and deletion of files
> relatives to /var/run/netns.
> This patch denies creation or deletion of namespaces with names contaning
> "/" or that matches exactly "." or "..".

You might want to have a look at --scissors option to git-am for a more
elegant way of sending a reply with attached patch.

[...]
>  int do_netns(int argc, char **argv)
>  {
>  	netns_nsid_socket_init();
> @@ -775,6 +780,11 @@ int do_netns(int argc, char **argv)
>  		return netns_list(0, NULL);
>  	}
>  
> +	if (argc > 1 && invalid_name(argv[1])) {
> +		fprintf(stderr, "Invalid netns name \"%s\"\n", argv[1]);
> +		exit(-1);
> +	}

Maybe worth noting, this assumes argv[1] will be used for the netns name
which doesn't hold for 'ip netns identify' command. It doesn't matter
though, since netns_identify() enforces the parameter to be either
"self" or a decimal number. Yet, in future a new subcommand might be
added which requires this check to be refactored.

Thanks, Phil

Re: [PATCH] netns: avoid directory traversal (was: ip netns: Make sure netns name is sane)

[Stephen Hemminger]

On Mon, 10 Jul 2017 14:08:31 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> Hi Phil,
> 
> I noticed that your patch still leaves an uncovered scenario, the one where the
> namespace name is "." or "..".
> Calling 'ip netns del ..' will remove /var/run which is a symlink to /run on
> most systems causing some daemons, eg. dbus, to fail.
> 
> ip netns doesn't validate input, allowing creation and deletion of files
> relatives to /var/run/netns.
> This patch denies creation or deletion of namespaces with names contaning
> "/" or that matches exactly "." or "..".
> ---
>  ip/ipnetns.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

The patch itself is good, but the commit message needs fixing.
Please rewrite it to describe the problem, and add signed-off-by

[PATCH v2] netns: avoid directory traversal (was: ip netns: Make sure netns name is sane)

[Matteo Croce]

v2: reword commit message

ip netns keeps track of created namespaces with bind mounts named
/var/run/netns/<namespace>. No input sanitization is done, allowing creation and
deletion of files relatives to /var/run/netns or, if the path is non existent or
invalid, allows to create "untracked" namespaces (invisible to the tool).

This commit denies creation or deletion of namespaces with names contaning
"/" or matching exactly "." or "..".

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 ip/ipnetns.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 0b0378ab..42549944 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -766,6 +766,11 @@ static int netns_monitor(int argc, char **argv)
 	return 0;
 }
 
+static int invalid_name(const char *name)
+{
+	return strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, "..");
+}
+
 int do_netns(int argc, char **argv)
 {
 	netns_nsid_socket_init();
@@ -775,6 +780,11 @@ int do_netns(int argc, char **argv)
 		return netns_list(0, NULL);
 	}
 
+	if (argc > 1 && invalid_name(argv[1])) {
+		fprintf(stderr, "Invalid netns name \"%s\"\n", argv[1]);
+		exit(-1);
+	}
+
 	if ((matches(*argv, "list") == 0) || (matches(*argv, "show") == 0) ||
 	    (matches(*argv, "lst") == 0)) {
 		netns_map_init();
-- 
2.13.3

Re: [PATCH v2] netns: avoid directory traversal (was: ip netns: Make sure netns name is sane)

[Phil Sutter]

Hi Matteo,

I don't think git-am understands the subject change semantics you use
here, so please drop the '(was: ...)' part from it.

On Thu, Jul 20, 2017 at 12:36:32AM +0200, Matteo Croce wrote:
> v2: reword commit message

In my opinion, this belongs to patch meta info (below the three-dashes
line), not the commit message.

Please put the maintainer in Cc when submitting patches. It helps them
to keep track of it. This counts for all upstream projects, not just
iproute2.

Thanks, Phil

Re: [PATCH v2] netns: avoid directory traversal (was: ip netns: Make sure netns name is sane)

[Stephen Hemminger]

On Thu, 20 Jul 2017 00:36:32 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> v2: reword commit message
> 
> ip netns keeps track of created namespaces with bind mounts named
> /var/run/netns/<namespace>. No input sanitization is done, allowing creation and
> deletion of files relatives to /var/run/netns or, if the path is non existent or
> invalid, allows to create "untracked" namespaces (invisible to the tool).
> 
> This commit denies creation or deletion of namespaces with names contaning
> "/" or matching exactly "." or "..".
> 
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

Applied. I manually edited the commit description to remove the (was...)
thanks Matteo.