[PATCH] btrfs-progs: enforce chroot for btrfs receive

[PATCH] btrfs-progs: enforce chroot for btrfs receive

[Lauri Võsandi]

This patch forces btrfs receive to issue chroot before
parsing the btrfs stream to confine the process and
minimize damage that could be done via malicious
btrfs stream.

Signed-off-by: Lauri Võsandi <lauri.vosandi@gmail.com>
---
 cmds-receive.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/cmds-receive.c b/cmds-receive.c
index 44ef27e..8be92ea 100644
--- a/cmds-receive.c
+++ b/cmds-receive.c
@@ -867,15 +867,17 @@ static int do_receive(struct btrfs_receive *r, const char *tomnt, int r_fd,
 		goto out;
 	}
 
-	/*
-	 * find_mount_root returns a root_path that is a subpath of
-	 * dest_dir_full_path. Now get the other part of root_path,
-	 * which is the destination dir relative to root_path.
-	 */
-	r->dest_dir_path = dest_dir_full_path + strlen(r->root_path);
-	while (r->dest_dir_path[0] == '/')
-		r->dest_dir_path++;
+	if (chroot(dest_dir_full_path)) {
+		ret = -errno;
+		fprintf(stderr,
+			"ERROR: failed to chroot to %s, %s\n",
+			dest_dir_full_path,
+			strerror(-ret));
+		goto out;
+	}
 
+	r->root_path = r->dest_dir_path = strdup("/");
+	
 	ret = subvol_uuid_search_init(r->mnt_fd, &r->sus);
 	if (ret < 0)
 		goto out;
-- 
1.9.1

Re: [PATCH] btrfs-progs: enforce chroot for btrfs receive

[David Sterba]

On Tue, Apr 14, 2015 at 01:44:32PM +0300, Lauri Võsandi wrote:
> This patch forces btrfs receive to issue chroot before
> parsing the btrfs stream to confine the process and
> minimize damage that could be done via malicious
> btrfs stream.

Thanks.

As we've discussed, there are possibly some things to resolve:

* chdir("/") after chroot
* commandline options to enable/disable chroot, choose the default

Receive should work for a non-root user so chroot should be conditional,
but I'm not sure if this should be guessed from the UID or if this would
be better to specify only by the commandline options.

I'll put the patch into a separate branch for now.

Re: [PATCH] btrfs-progs: enforce chroot for btrfs receive

[Austin S Hemmelgarn]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

On 2015-04-14 08:28, David Sterba wrote:
> On Tue, Apr 14, 2015 at 01:44:32PM +0300, Lauri Võsandi wrote:
>> This patch forces btrfs receive to issue chroot before
>> parsing the btrfs stream to confine the process and
>> minimize damage that could be done via malicious
>> btrfs stream.
>
> Thanks.
>
> As we've discussed, there are possibly some things to resolve:
>
> * chdir("/") after chroot
> * commandline options to enable/disable chroot, choose the default
>
> Receive should work for a non-root user so chroot should be conditional,
> but I'm not sure if this should be guessed from the UID or if this would
> be better to specify only by the commandline options.
>
> I'll put the patch into a separate branch for now.

Personally, I would expect it to default to not using chroot(), provide 
a commandline option to tell it to do so, and then just catch the error 
from trying to chroot as a non-root user.



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 2967 bytes --]

Re: [PATCH] btrfs-progs: enforce chroot for btrfs receive

[David Sterba]

On Tue, Apr 14, 2015 at 09:19:12AM -0400, Austin S Hemmelgarn wrote:
> On 2015-04-14 08:28, David Sterba wrote:
> > On Tue, Apr 14, 2015 at 01:44:32PM +0300, Lauri Võsandi wrote:
> >> This patch forces btrfs receive to issue chroot before
> >> parsing the btrfs stream to confine the process and
> >> minimize damage that could be done via malicious
> >> btrfs stream.
> >
> > Thanks.
> >
> > As we've discussed, there are possibly some things to resolve:
> >
> > * chdir("/") after chroot
> > * commandline options to enable/disable chroot, choose the default
> >
> > Receive should work for a non-root user so chroot should be conditional,
> > but I'm not sure if this should be guessed from the UID or if this would
> > be better to specify only by the commandline options.
> >
> > I'll put the patch into a separate branch for now.
> 
> Personally, I would expect it to default to not using chroot(), provide 
> a commandline option to tell it to do so, and then just catch the error 
> from trying to chroot as a non-root user.

Thanks, I agree with that.

[PATCH] btrfs-progs: optionally enforce chroot for btrfs receive

[Lauri Võsandi]

This patch forces btrfs receive to issue chroot before
parsing the btrfs stream using command-line flag -C
to confine the process and minimize damage that could
be done via malicious btrfs stream.

Signed-off-by: Lauri Võsandi <lauri.vosandi@gmail.com>
---
 cmds-receive.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/cmds-receive.c b/cmds-receive.c
index 44ef27e..73bd88b 100644
--- a/cmds-receive.c
+++ b/cmds-receive.c
@@ -61,6 +61,7 @@ struct btrfs_receive
 	char *root_path;
 	char *dest_dir_path; /* relative to root_path */
 	char *full_subvol_path;
+	int dest_dir_chroot;
 
 	struct subvol_info *cur_subvol;
 
@@ -867,14 +868,27 @@ static int do_receive(struct btrfs_receive *r, const char *tomnt, int r_fd,
 		goto out;
 	}
 
-	/*
-	 * find_mount_root returns a root_path that is a subpath of
-	 * dest_dir_full_path. Now get the other part of root_path,
-	 * which is the destination dir relative to root_path.
-	 */
-	r->dest_dir_path = dest_dir_full_path + strlen(r->root_path);
-	while (r->dest_dir_path[0] == '/')
-		r->dest_dir_path++;
+	if (r->dest_dir_chroot) {
+		if (chroot(dest_dir_full_path)) {
+			ret = -errno;
+			fprintf(stderr,
+				"ERROR: failed to chroot to %s, %s\n",
+				dest_dir_full_path,
+				strerror(-ret));
+			goto out;
+		}
+		if(chdir("/")) {
+			ret = -errno;
+			fprintf(stderr,
+				"ERROR: failed to chdir to /, %s\n",
+				strerror(-ret));
+		}
+		if (g_verbose >= 1) {
+			fprintf(stderr, "chrooted to %s\n",
+				dest_dir_full_path);
+		}
+		r->root_path = r->dest_dir_path = strdup("/");
+	}
 
 	ret = subvol_uuid_search_init(r->mnt_fd, &r->sus);
 	if (ret < 0)
@@ -940,6 +954,7 @@ int cmd_receive(int argc, char **argv)
 	r.write_fd = -1;
 	r.dest_dir_fd = -1;
 	r.explicit_parent = NULL;
+	r.dest_dir_chroot = 0;
 
 	while (1) {
 		int c;
@@ -948,7 +963,7 @@ int cmd_receive(int argc, char **argv)
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "evf:p:", long_opts, NULL);
+		c = getopt_long(argc, argv, "Cevf:p:", long_opts, NULL);
 		if (c < 0)
 			break;
 
@@ -962,6 +977,9 @@ int cmd_receive(int argc, char **argv)
 		case 'e':
 			r.honor_end_cmd = 1;
 			break;
+		case 'C':
+			r.dest_dir_chroot = 1;
+			break;
 		case 'E':
 			max_errors = arg_strtou64(optarg);
 			break;
@@ -1014,6 +1032,7 @@ const char * const cmd_receive_usage[] = {
 	"                 in the data stream. Without this option,",
 	"                 the receiver terminates only if an error",
 	"                 is recognized or on EOF.",
+	"-C               Confine the process to <mount> using chroot",
 	"--max-errors <N> Terminate as soon as N errors happened while",
 	"                 processing commands from the send stream.",
 	"                 Default value is 1. A value of 0 means no limit.",
-- 
1.9.1

[PATCH] btrfs-progs: optionally enforce chroot for btrfs receive

[Lauri Võsandi]

This patch forces btrfs receive to issue chroot before
parsing the btrfs stream using command-line flag -C
to confine the process and minimize damage that could
be done via malicious btrfs stream.

Signed-off-by: Lauri Võsandi <lauri.vosandi@gmail.com>
---
 cmds-receive.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/cmds-receive.c b/cmds-receive.c
index a1c72f9..366a63a 100644
--- a/cmds-receive.c
+++ b/cmds-receive.c
@@ -61,6 +61,7 @@ struct btrfs_receive
 	char *root_path;
 	char *dest_dir_path; /* relative to root_path */
 	char *full_subvol_path;
+	int dest_dir_chroot;
 
 	struct subvol_info *cur_subvol;
 
@@ -858,14 +859,27 @@ static int do_receive(struct btrfs_receive *r, const char *tomnt, int r_fd,
 		goto out;
 	}
 
-	/*
-	 * find_mount_root returns a root_path that is a subpath of
-	 * dest_dir_full_path. Now get the other part of root_path,
-	 * which is the destination dir relative to root_path.
-	 */
-	r->dest_dir_path = dest_dir_full_path + strlen(r->root_path);
-	while (r->dest_dir_path[0] == '/')
-		r->dest_dir_path++;
+	if (r->dest_dir_chroot) {
+		if (chroot(dest_dir_full_path)) {
+			ret = -errno;
+			fprintf(stderr,
+				"ERROR: failed to chroot to %s, %s\n",
+				dest_dir_full_path,
+				strerror(-ret));
+			goto out;
+		}
+		if(chdir("/")) {
+			ret = -errno;
+			fprintf(stderr,
+				"ERROR: failed to chdir to /, %s\n",
+				strerror(-ret));
+		}
+		if (g_verbose >= 1) {
+			fprintf(stderr, "chrooted to %s\n",
+				dest_dir_full_path);
+		}
+		r->root_path = r->dest_dir_path = strdup("/");
+	}
 
 	ret = subvol_uuid_search_init(r->mnt_fd, &r->sus);
 	if (ret < 0)
@@ -930,6 +944,7 @@ int cmd_receive(int argc, char **argv)
 	r.mnt_fd = -1;
 	r.write_fd = -1;
 	r.dest_dir_fd = -1;
+	r.dest_dir_chroot = 0;
 
 	while (1) {
 		int c;
@@ -938,7 +953,7 @@ int cmd_receive(int argc, char **argv)
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "evf:", long_opts, NULL);
+		c = getopt_long(argc, argv, "Cevf:", long_opts, NULL);
 		if (c < 0)
 			break;
 
@@ -952,6 +967,9 @@ int cmd_receive(int argc, char **argv)
 		case 'e':
 			r.honor_end_cmd = 1;
 			break;
+		case 'C':
+			r.dest_dir_chroot = 1;
+			break;
 		case 'E':
 			max_errors = arg_strtou64(optarg);
 			break;
@@ -1001,6 +1019,7 @@ const char * const cmd_receive_usage[] = {
 	"                 in the data stream. Without this option,",
 	"                 the receiver terminates only if an error",
 	"                 is recognized or on EOF.",
+	"-C               Confine the process to <mount> using chroot",
 	"--max-errors <N> Terminate as soon as N errors happened while",
 	"                 processing commands from the send stream.",
 	"                 Default value is 1. A value of 0 means no limit.",
-- 
1.9.1

Re: [PATCH] btrfs-progs: optionally enforce chroot for btrfs receive

[Mike Fleetwood]

On 18 April 2015 at 14:59, Lauri Võsandi <lauri.vosandi@gmail.com> wrote:
> This patch forces btrfs receive to issue chroot before
> parsing the btrfs stream using command-line flag -C
> to confine the process and minimize damage that could
> be done via malicious btrfs stream.
>
> Signed-off-by: Lauri Võsandi <lauri.vosandi@gmail.com>
> ---
>  cmds-receive.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/cmds-receive.c b/cmds-receive.c
> index 44ef27e..73bd88b 100644
> --- a/cmds-receive.c
> +++ b/cmds-receive.c
> @@ -61,6 +61,7 @@ struct btrfs_receive
>         char *root_path;
>         char *dest_dir_path; /* relative to root_path */
>         char *full_subvol_path;
> +       int dest_dir_chroot;
>
>         struct subvol_info *cur_subvol;
>
> @@ -867,14 +868,27 @@ static int do_receive(struct btrfs_receive *r, const char *tomnt, int r_fd,
>                 goto out;
>         }
>
> -       /*
> -        * find_mount_root returns a root_path that is a subpath of
> -        * dest_dir_full_path. Now get the other part of root_path,
> -        * which is the destination dir relative to root_path.
> -        */
> -       r->dest_dir_path = dest_dir_full_path + strlen(r->root_path);
> -       while (r->dest_dir_path[0] == '/')
> -               r->dest_dir_path++;
> +       if (r->dest_dir_chroot) {
> +               if (chroot(dest_dir_full_path)) {
> +                       ret = -errno;
> +                       fprintf(stderr,
> +                               "ERROR: failed to chroot to %s, %s\n",
> +                               dest_dir_full_path,
> +                               strerror(-ret));
> +                       goto out;
> +               }
> +               if(chdir("/")) {
> +                       ret = -errno;
> +                       fprintf(stderr,
> +                               "ERROR: failed to chdir to /, %s\n",
> +                               strerror(-ret));

There appears to be a goto out missing here.

> +               }
> +               if (g_verbose >= 1) {
> +                       fprintf(stderr, "chrooted to %s\n",
> +                               dest_dir_full_path);
> +               }
> +               r->root_path = r->dest_dir_path = strdup("/");
> +       }
>
>         ret = subvol_uuid_search_init(r->mnt_fd, &r->sus);
>         if (ret < 0)
> @@ -940,6 +954,7 @@ int cmd_receive(int argc, char **argv)
>         r.write_fd = -1;
>         r.dest_dir_fd = -1;
>         r.explicit_parent = NULL;
> +       r.dest_dir_chroot = 0;
>
>         while (1) {
>                 int c;
> @@ -948,7 +963,7 @@ int cmd_receive(int argc, char **argv)
>                         { NULL, 0, NULL, 0 }
>                 };
>
> -               c = getopt_long(argc, argv, "evf:p:", long_opts, NULL);
> +               c = getopt_long(argc, argv, "Cevf:p:", long_opts, NULL);
>                 if (c < 0)
>                         break;
>
> @@ -962,6 +977,9 @@ int cmd_receive(int argc, char **argv)
>                 case 'e':
>                         r.honor_end_cmd = 1;
>                         break;
> +               case 'C':
> +                       r.dest_dir_chroot = 1;
> +                       break;
>                 case 'E':
>                         max_errors = arg_strtou64(optarg);
>                         break;
> @@ -1014,6 +1032,7 @@ const char * const cmd_receive_usage[] = {
>         "                 in the data stream. Without this option,",
>         "                 the receiver terminates only if an error",
>         "                 is recognized or on EOF.",
> +       "-C               Confine the process to <mount> using chroot",
>         "--max-errors <N> Terminate as soon as N errors happened while",
>         "                 processing commands from the send stream.",
>         "                 Default value is 1. A value of 0 means no limit.",
> --
> 1.9.1

Mike

[PATCH] btrfs-progs: optionally enforce chroot for btrfs receive

[Lauri Võsandi]

This patch forces btrfs receive to issue chroot before
parsing the btrfs stream using command-line flag -C
to confine the process and minimize damage that could
be done via malicious btrfs stream.

Signed-off-by: Lauri Võsandi <lauri.vosandi@gmail.com>
---
 cmds-receive.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/cmds-receive.c b/cmds-receive.c
index a1c72f9..d96eab6 100644
--- a/cmds-receive.c
+++ b/cmds-receive.c
@@ -61,6 +61,7 @@ struct btrfs_receive
 	char *root_path;
 	char *dest_dir_path; /* relative to root_path */
 	char *full_subvol_path;
+	int dest_dir_chroot;
 
 	struct subvol_info *cur_subvol;
 
@@ -858,14 +859,28 @@ static int do_receive(struct btrfs_receive *r, const char *tomnt, int r_fd,
 		goto out;
 	}
 
-	/*
-	 * find_mount_root returns a root_path that is a subpath of
-	 * dest_dir_full_path. Now get the other part of root_path,
-	 * which is the destination dir relative to root_path.
-	 */
-	r->dest_dir_path = dest_dir_full_path + strlen(r->root_path);
-	while (r->dest_dir_path[0] == '/')
-		r->dest_dir_path++;
+	if (r->dest_dir_chroot) {
+		if (chroot(dest_dir_full_path)) {
+			ret = -errno;
+			fprintf(stderr,
+				"ERROR: failed to chroot to %s, %s\n",
+				dest_dir_full_path,
+				strerror(-ret));
+			goto out;
+		}
+		if(chdir("/")) {
+			ret = -errno;
+			fprintf(stderr,
+				"ERROR: failed to chdir to /, %s\n",
+				strerror(-ret));
+			goto out;
+		}
+		if (g_verbose >= 1) {
+			fprintf(stderr, "chrooted to %s\n",
+				dest_dir_full_path);
+		}
+		r->root_path = r->dest_dir_path = strdup("/");
+	}
 
 	ret = subvol_uuid_search_init(r->mnt_fd, &r->sus);
 	if (ret < 0)
@@ -930,6 +945,7 @@ int cmd_receive(int argc, char **argv)
 	r.mnt_fd = -1;
 	r.write_fd = -1;
 	r.dest_dir_fd = -1;
+	r.dest_dir_chroot = 0;
 
 	while (1) {
 		int c;
@@ -938,7 +954,7 @@ int cmd_receive(int argc, char **argv)
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "evf:", long_opts, NULL);
+		c = getopt_long(argc, argv, "Cevf:", long_opts, NULL);
 		if (c < 0)
 			break;
 
@@ -952,6 +968,9 @@ int cmd_receive(int argc, char **argv)
 		case 'e':
 			r.honor_end_cmd = 1;
 			break;
+		case 'C':
+			r.dest_dir_chroot = 1;
+			break;
 		case 'E':
 			max_errors = arg_strtou64(optarg);
 			break;
@@ -1001,6 +1020,7 @@ const char * const cmd_receive_usage[] = {
 	"                 in the data stream. Without this option,",
 	"                 the receiver terminates only if an error",
 	"                 is recognized or on EOF.",
+	"-C               Confine the process to <mount> using chroot",
 	"--max-errors <N> Terminate as soon as N errors happened while",
 	"                 processing commands from the send stream.",
 	"                 Default value is 1. A value of 0 means no limit.",
-- 
1.9.1

Re: [PATCH] btrfs-progs: optionally enforce chroot for btrfs receive

[Roman Mamedov]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On Sun, 19 Apr 2015 14:46:28 +0300
Lauri Võsandi <lauri.vosandi@gmail.com> wrote:

> This patch forces btrfs receive to issue chroot before
> parsing the btrfs stream using command-line flag -C
> to confine the process and minimize damage that could
> be done via malicious btrfs stream.
> 
> Signed-off-by: Lauri Võsandi <lauri.vosandi@gmail.com>
> ---
>  cmds-receive.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)

No patch versions and changelogs between them?...

By now there are 4 versions of this patch with identical subject and vague
unclear differences in description and content.

Usually what you do is label each new one [PATCH v2] (v3, v4) and describe what
got changed on each iteration.

-- 
With respect,
Roman

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] btrfs-progs: optionally enforce chroot for btrfs receive

[lauri]

Hi,

the last one added missing goto. I'll try to make clear differences
next time :)

Re: [PATCH] btrfs-progs: optionally enforce chroot for btrfs receive

[David Sterba]

On Sun, Apr 19, 2015 at 02:46:28PM +0300, Lauri Võsandi wrote:
> This patch forces btrfs receive to issue chroot before
> parsing the btrfs stream using command-line flag -C
> to confine the process and minimize damage that could
> be done via malicious btrfs stream.
> 
> Signed-off-by: Lauri Võsandi <lauri.vosandi@gmail.com>

Applied with some updates, thanks.

> -	/*
> -	 * find_mount_root returns a root_path that is a subpath of
> -	 * dest_dir_full_path. Now get the other part of root_path,
> -	 * which is the destination dir relative to root_path.
> -	 */
> -	r->dest_dir_path = dest_dir_full_path + strlen(r->root_path);
> -	while (r->dest_dir_path[0] == '/')
> -		r->dest_dir_path++;

This goes to the 'else' branch of the 'if' below.

> +	if (r->dest_dir_chroot) {
> +		if (chroot(dest_dir_full_path)) {
> +			ret = -errno;
> +			fprintf(stderr,
> +				"ERROR: failed to chroot to %s, %s\n",
> +				dest_dir_full_path,
> +				strerror(-ret));
> +			goto out;
> +		}
> +		if(chdir("/")) {
> +			ret = -errno;
> +			fprintf(stderr,
> +				"ERROR: failed to chdir to /, %s\n",
> +				strerror(-ret));
> +			goto out;
> +		}
> +		if (g_verbose >= 1) {
> +			fprintf(stderr, "chrooted to %s\n",
> +				dest_dir_full_path);
> +		}
> +		r->root_path = r->dest_dir_path = strdup("/");
> +	}
> -		c = getopt_long(argc, argv, "evf:", long_opts, NULL);
> +		c = getopt_long(argc, argv, "Cevf:", long_opts, NULL);

added the long option --chroot