linux-kernel - Re: [PATCH] apply: refuse touching a file beyond symlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150130200731.GC30738@peff.net>
Date:	Fri, 30 Jan 2015 15:07:32 -0500
From:	Jeff King <peff@...f.net>
To:	Junio C Hamano <gitster@...ox.com>
Cc:	Git Mailing List <git@...r.kernel.org>,
	Josh Boyer <jwboyer@...oraproject.org>,
	"Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
	twaugh@...hat.com, Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] apply: refuse touching a file beyond symlink

On Fri, Jan 30, 2015 at 11:48:14AM -0800, Junio C Hamano wrote:

> Jeff King <peff@...f.net> writes:
> 
> >> +	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
> >> +		return error(_("affected file '%s' is beyond a symbolic link"),
> >> +			     patch->new_name);
> >
> > Why does this not kick in when deleting a file? If it is not OK to
> > add across a symlink, why is it OK to delete?
> 
> Hmph, adding
> 
> 	if (patch->is_delete &&	path_is_beyond_symlink(patch->old_name))
> 		return error(_("deleted file '%s' is beyond a symlink"),
> 				patch->old_name);
> 
> seems to break t4114.11, which wants to apply this patch to a tree
> that does not have a symbolic link but a directory at 'foo/'.
> 
> diff --git a/foo b/foo
> new file mode 120000
> index 0000000..ba0e162
> --- /dev/null
> +++ b/foo
> @@ -0,0 +1 @@
> +bar
> \ No newline at end of file
> diff --git a/foo/baz b/foo/baz
> deleted file mode 100644
> index 682c76b..0000000
> --- a/foo/baz
> +++ /dev/null
> @@ -1 +0,0 @@
> -if only I knew

Hrm. That only works in the current code because we apply the deletion
in the directory (and then clean up the now-empty directory) first. So I
think you would need to check the paths progressively as you apply them,
since those other parts of the diff "haven't happened yet".

If we take deletion as one phase and addition as another, I think you
could get away with:

diff --git a/builtin/apply.c b/builtin/apply.c
index f5491cd..12c9d8e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3549,7 +3549,7 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
-static int path_is_beyond_symlink(const char *name_)
+static int path_is_beyond_symlink(const char *name_, int check_table)
 {
 	struct strbuf name = STRBUF_INIT;
 
@@ -3562,7 +3562,8 @@ static int path_is_beyond_symlink(const char *name_)
 		if (!name.len)
 			break;
 		name.buf[name.len] = '\0';
-		previous = in_fn_table(name.buf);
+
+		previous = check_table ? in_fn_table(name.buf) : NULL;
 		if (previous) {
 			if (!was_deleted(previous) &&
 			    !to_be_deleted(previous) &&
@@ -3676,9 +3677,12 @@ static int check_patch(struct patch *patch)
 		}
 	}
 
-	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
+	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name, 1))
 		return error(_("affected file '%s' is beyond a symbolic link"),
 			     patch->new_name);
+	if (patch->is_delete && path_is_beyond_symlink(patch->old_name, 0))
+		return error(_("affected file '%s' is beyond a symbolic link"),
+			     patch->old_name);
 
 	if (apply_data(patch, &st, ce) < 0)
 		return error(_("%s: patch does not apply"), name);


but I suspect we could construct a case that depends more closely on the
order of application. E.g., a patch that does:

  1. add foo as a symlink
  2. add file foo/bar

is definitely wrong. But is:

  1. add file foo/bar
  2. add foo as a symlink

does not technically fall afoul of the symlink rules. It is a _bogus_
patch, of course, because the second part will get a D/F conflict. I am
not sure if there are any legitimate patches that could run into this
ordering problem, but even without it, it smells a bit funny to complain
for the wrong reason.

Looking at the code, though, it seems like we should be doing these
checks progressively as we add entries to the fn_table. So that is doing
the right thing. It is only the deletion re-ordering that trips us up.
Can we reorder all deletions before all additions before calling
check_patch on each?

-Peff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/