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: <xmqqmw50asan.fsf@gitster.dls.corp.google.com>
Date:	Fri, 30 Jan 2015 13:10:24 -0800
From:	Junio C Hamano <gitster@...ox.com>
To:	Jeff King <peff@...f.net>
Cc:	Git Mailing List <git@...r.kernel.org>,
	Josh Boyer <jwboyer@...oraproject.org>,
	"Linux-Kernel\@Vger. 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

Jeff King <peff@...f.net> writes:

> Ah, OK. Yeah, doing it progressively can only be accurate if our
> name-checks follow the same order as applying, because we are checking
> against a particular state.
>
> But could we instead pull this check to just before the write-out time?
> That is, to let any horrible thing happen in-core, as long as what we
> write out to the index and the filesystem is sane?

That would make me feel dirty.

I noticed one thing.  The PATH_TO_BE_DELETED/PATH_WAS_DELETED crud
kicks in -only- during the actual application phase, and all patches
that remove paths from the end result should have been appropriately
marked in fn_table[] by the call to prepare_fn_table() at the
beginning of check_patch_list() as PATH_TO_BE_DELETED.

But it was wrong to call previous_patch() in my fix.  The function
is the cause of evil I see in the "let's support concatenated patch,
making the later patch depend on the result of earlier ones" and
deliberately ignores PATH_TO_BE_DELETED patches.  We would need to
do the early part of previous_patch() without the filtering.

This is a preparatory step to clean-up the mess I have in mind.  It
does not mean to change the semantics (applied to the codebase with
or without the changes we have been discussing); it only makes it
always return the "previous" patch to the callers and makes them
responsible to see if the previous was to-be-deleted or was-deleted.

With that change, I think my symlink fix plus the "check the deleted
one with old_name, too" change has a better chance to do the moral
equivalent of your two-phase thing.  Essentially, "First see what
will be deleted in the input as a whole" has already been done by
the prepare_fn_table() thing.

 builtin/apply.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 41b7236..a064017 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3097,25 +3097,12 @@ static int checkout_target(struct cache_entry *ce, struct stat *st)
 	return 0;
 }
 
-static struct patch *previous_patch(struct patch *patch, int *gone)
+static struct patch *previous_patch(struct patch *patch)
 {
-	struct patch *previous;
-
-	*gone = 0;
 	if (patch->is_copy || patch->is_rename)
 		return NULL; /* "git" patches do not depend on the order */
 
-	previous = in_fn_table(patch->old_name);
-	if (!previous)
-		return NULL;
-
-	if (to_be_deleted(previous))
-		return NULL; /* the deletion hasn't happened yet */
-
-	if (was_deleted(previous))
-		*gone = 1;
-
-	return previous;
+	return in_fn_table(patch->old_name);
 }
 
 static int verify_index_match(const struct cache_entry *ce, struct stat *st)
@@ -3170,11 +3157,11 @@ static int load_preimage(struct image *image,
 	struct patch *previous;
 	int status;
 
-	previous = previous_patch(patch, &status);
-	if (status)
+	previous = previous_patch(patch);
+	if (was_deleted(previous))
 		return error(_("path %s has been renamed/deleted"),
 			     patch->old_name);
-	if (previous) {
+	if (previous && !to_be_deleted(previous)) {
 		/* We have a patched copy in memory; use that. */
 		strbuf_add(&buf, previous->result, previous->resultsize);
 	} else {
@@ -3384,18 +3371,18 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 {
 	const char *old_name = patch->old_name;
 	struct patch *previous = NULL;
-	int stat_ret = 0, status;
+	int stat_ret = 0;
 	unsigned st_mode = 0;
 
 	if (!old_name)
 		return 0;
 
 	assert(patch->is_new <= 0);
-	previous = previous_patch(patch, &status);
+	previous = previous_patch(patch);
 
-	if (status)
+	if (was_deleted(previous))
 		return error(_("path %s has been renamed/deleted"), old_name);
-	if (previous) {
+	if (previous && !to_be_deleted(previous)) {
 		st_mode = previous->new_mode;
 	} else if (!cached) {
 		stat_ret = lstat(old_name, st);
@@ -3403,6 +3390,9 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 			return error(_("%s: %s"), old_name, strerror(errno));
 	}
 
+	if (to_be_deleted(previous))
+		previous = NULL;
+
 	if (check_index && !previous) {
 		int pos = cache_name_pos(old_name, strlen(old_name));
 		if (pos < 0) {
--
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/