Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 Junio C Hamano gits...@pobox.com writes:

 If the user wants to apply a patch that touches ../etc/shadow, is
 the tool in the place to complain?

 Let me take this part back.

 I think git apply should behave closely to git apply --index
 (which is used by git am unless there is a very good reason not to
 (and 'git apply --index' behaves differently from GNU patch, and we
 should match what the latter does is not a very good reason).  When
 the index guards the working tree, we do not follow any symlink,
 whether the destination is inside the current directory or not.

 I however do not think the current git apply notices that it will
 overwrite a path beyond a symlink---we may need to fix that if that
 is the case.  I'll see what I can find (but I'll be doing 2.3-rc2
 today so it may be later this week).

Yikes.  It turns out that the index is what protects us from going
outside the working tree.  apply --index (hence am) is immune
against the CVE-2015-1196, but that is not because we do not follow
symbolic links.

Also the solution is not just a simple has_symlink_leading_path().
Here is tonight's snapshot of what I've found out (not tested beyond
passing the test suite including the new test added by the patch).

-- 8 --
Subject: [PATCH] apply: refuse touching a file beyond symlink

Because Git tracks symbolic links as symbolic links, a path that has
a symbolic link in its leading part (e.g. path/to/dir being a
symbolic link to somewhere else, be it inside or outside the working
tree) can never appear in a patch that validly apply, unless the
same patch first removes the symbolic link.

Detect and reject such a patch.  Things to note:

 - Unfortunately, we cannot reuse the has_symlink_leading_path()
   from dir.c, as that is only about the working tree, but git
   apply can be told to apply the patch only to the index.

 - We cannot directly use has_symlink_leading_path() even when we
   are applying to the working tree, as an early patch of a valid
   input may remove a symbolic link path/to/dir and then a later
   patch of the input may create a path path/to/dir/file.  The
   leading symbolic link check must be done on the interim result we
   compute in core (i.e. after the first patch, there is no
   path/to/dir symbolic link and it is perfectly valid to create
   path/to/dir/file).  Similarly, when an input creates a symbolic
   link path/to/dir and then creates a file path/to/dir/file, we
   need to flag it as an error without actually creating path/to/dir
   symbolic link in the filesystem.

 - Instead, for any patch in the input that leaves a path (i.e. a
   non deletion) in the result, we check all leading paths against
   interim result and then either the index or the working tree.
   The interim result of applying patch is already kept track of
   by fn_table logic for us.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 builtin/apply.c | 44 +
 t/t4122-apply-symlink-inside.sh | 37 ++
 2 files changed, 81 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index ef32e4f..da088c5 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3483,6 +3483,46 @@ static int check_to_create(const char *new_name, int 
ok_if_exists)
return 0;
 }
 
+static int path_is_beyond_symlink(const char *name_)
+{
+   struct strbuf name = STRBUF_INIT;
+
+   strbuf_addstr(name, name_);
+   do {
+   struct patch *previous;
+
+   while (--name.len  name.buf[name.len] != '/')
+   ; /* scan backwards */
+   if (!name.len)
+   break;
+   name.buf[name.len] = '\0';
+   previous = in_fn_table(name.buf);
+   if (previous) {
+   if (!was_deleted(previous) 
+   !to_be_deleted(previous) 
+   previous-new_mode 
+   S_ISLNK(previous-new_mode))
+   goto symlink_found;
+   } else if (check_index) {
+   int pos = cache_name_pos(name.buf, name.len);
+   if (0 = pos 
+   S_ISLNK(active_cache[pos]-ce_mode))
+   goto symlink_found;
+   } else {
+   struct stat st;
+   if (!lstat(name.buf, st)  S_ISLNK(st.st_mode))
+   goto symlink_found;
+   }
+   } while (1);
+
+   strbuf_release(name);
+   return 0;
+symlink_found:
+   strbuf_release(name);
+   return -1;
+
+}
+
 /*
  * Check and apply the patch in-core; leave the result in patch-result
  * for the caller to write it out to the final destination.
@@ -3570,6 +3610,10 @@ static int check_patch(struct patch *patch)
}
}
 
+   if (!patch-is_delete  

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 Subject: [PATCH] apply: refuse touching a file beyond symlink

 Because Git tracks symbolic links as symbolic links, a path that has
 a symbolic link in its leading part (e.g. path/to/dir being a
 symbolic link to somewhere else, be it inside or outside the working
 tree) can never appear in a patch that validly apply, unless the
 same patch first removes the symbolic link.

I should rephrase the above to make it more readable.

... its leading part (e.g. path/to/dir/file, where path/to/dir is a
symbolic link to somewhere else, ...

is what I meant to say.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 Linus Torvalds torva...@linux-foundation.org writes:

 Ugh. I don't see anything we can do about this on the git side, and I
 do kind of understand why 'patch' would be worried about '..' files.
 In a perfect world, patch would parse the filename and see that it
 stays within the directory structure of the project, but that is a
 rather harder thing to do than just say no dot-dot files.

 It is unclear to me why limit to the current directory and below
 is such a big deal in the first place.

 If the user wants to apply a patch that touches ../etc/shadow, is
 the tool in the place to complain?

Let me take this part back.

I think git apply should behave closely to git apply --index
(which is used by git am unless there is a very good reason not to
(and 'git apply --index' behaves differently from GNU patch, and we
should match what the latter does is not a very good reason).  When
the index guards the working tree, we do not follow any symlink,
whether the destination is inside the current directory or not.

I however do not think the current git apply notices that it will
overwrite a path beyond a symlink---we may need to fix that if that
is the case.  I'll see what I can find (but I'll be doing 2.3-rc2
today so it may be later this week).

Thanks.



--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Andreas Gruenbacher]

On Mon, 26 Jan 2015 12:44:33 -0800, Linus Torvalds wrote:
 I've considered that for a while already, because patch _does_ kind of
 understand them these days, although I think it gets the cross-rename
 case wrong because it fundamentally works on a file-by-file basis.

Patch handles cross-renames correctly nowadays; if not, it's a bug.

That's not enough to solve the symlink problem unfortunately.

Andreas

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Josh Boyer]

[Adding Junio's correct email address.  Sigh.]

On Mon, Jan 26, 2015 at 11:29 AM, Josh Boyer jwbo...@fedoraproject.org wrote:
 Hi,

 I went to do the Fedora 3.19-rc6 build this morning and it failed in
 our buildsystem with:

 + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
 + case $patch in
 + unxz
 + patch -p1 -F1 -s
 symbolic link target '../../../../../include/dt-bindings' is invalid
 error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)

 That is coming from the hunk in patch-3.19-rc6.xz that creates the
 symbolic link from arch/arm64/boot/dts/include/dt-bindings to
 include/dt-bindings.  Oddly enough, patch-3.19-rc5.xz contains the
 same hunk and it built fine last week.

 Digging in, it seems that upstream patch has decided that relative
 symlinks are forbidden now as part of a fix for CVE-2015-1196.  You
 can find the relevant bugs here:

 https://bugzilla.redhat.com/show_bug.cgi?id=1185928
 https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775901#13

 Aside from locally modifying patch-3.19-rc6.xz, I'm not sure what else
 to do.  I thought I would send a heads up since anyone that is using
 patch-2.7.3 is probably going to run into this issue.

 josh
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

patch-2.7.3 no longer applies relative symbolic link patches

[Josh Boyer]

Hi,

I went to do the Fedora 3.19-rc6 build this morning and it failed in
our buildsystem with:

+ '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
+ case $patch in
+ unxz
+ patch -p1 -F1 -s
symbolic link target '../../../../../include/dt-bindings' is invalid
error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)

That is coming from the hunk in patch-3.19-rc6.xz that creates the
symbolic link from arch/arm64/boot/dts/include/dt-bindings to
include/dt-bindings.  Oddly enough, patch-3.19-rc5.xz contains the
same hunk and it built fine last week.

Digging in, it seems that upstream patch has decided that relative
symlinks are forbidden now as part of a fix for CVE-2015-1196.  You
can find the relevant bugs here:

https://bugzilla.redhat.com/show_bug.cgi?id=1185928
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775901#13

Aside from locally modifying patch-3.19-rc6.xz, I'm not sure what else
to do.  I thought I would send a heads up since anyone that is using
patch-2.7.3 is probably going to run into this issue.

josh
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Josh Boyer]

On Mon, Jan 26, 2015 at 4:30 PM, Linus Torvalds
torva...@linux-foundation.org wrote:
 On Mon, Jan 26, 2015 at 1:07 PM, Josh Boyer jwbo...@fedoraproject.org wrote:

 Or did I miss a way that git-apply can take a git patch and apply it
 to a tree that isn't a git repo?

 Exactly. git apply works as a straight patch replacement outside
 of a git repository. It doesn't actually need a git tree to work.

Ah.  I had somehow missed that entirely.  Good to know for future reference.

 (Of course, git apply is _not_ a patch replacement in the general
 sense. It only applies context diffs - preferentially git style ones -
  so no old-style patches etc need apply. And it's not
 replacement-compatible in a syntax sense either, in that while many of
 the options are the same, not all are etc etc).

Sure.  Though for the Fedora kernel builds, we tend to use git
formatted patches only anyway.  I might play around with this and see
how it works as the normal way to apply things.

josh
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[David Kastrup]

Linus Torvalds torva...@linux-foundation.org writes:

 On Mon, Jan 26, 2015 at 8:32 AM, Josh Boyer jwbo...@fedoraproject.org wrote:

 I went to do the Fedora 3.19-rc6 build this morning and it failed in
 our buildsystem with:

 + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
 + case $patch in
 + unxz
 + patch -p1 -F1 -s
 symbolic link target '../../../../../include/dt-bindings' is invalid
 error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)

 Ugh. I don't see anything we can do about this on the git side, and I
 do kind of understand why 'patch' would be worried about '..' files.
 In a perfect world, patch would parse the filename and see that it
 stays within the directory structure of the project, but that is a
 rather harder thing to do than just say no dot-dot files.

 The short-term fix is likely to just use git apply instead of patch.

 The long-term fix? I dunno. I don't see us not using symlinks, and a
 quick check says that every *single* symlink we have in the kernel
 source tree is one that points to a different directory using ..
 format. And while I could imagine that patch ends up counting the
 dot-dot entries and checking that it's all inside the same tree it is
 patching, I could also easily see patch *not* doing that.

I consider it rather hard and error-prone and/or an attack vector to
choose a course of action for ../ in connection with the -p option.

-- 
David Kastrup
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Linus Torvalds]

On Mon, Jan 26, 2015 at 1:35 PM, Junio C Hamano gits...@pobox.com wrote:

 What is your take on CVE-2015-1196, which brought this /regression/ to
 GNU patch?
 If git apply get /fixed/ for that same CVE, would that /break/ your fix?

I _think_ we allow arbitrary symlinks to be created, but then we
should be careful about actually _following_ them.

At least I _thought_ we were already quite careful not to do that,
even if it's been a long time since I looked at the code. So even if
we create a symlink to outside the repository, it normally shouldn't
matter. We have that whole lstat_cache() thing that exists exactly
to make it efficient to do pathname lookups while at the same time
being aware of symlinks in the middle.

Of course, our lstat cache is racy if somebody else modifies the tree
concurrently and changes things, but that's a non-issue, because if
somebody can just directly create random symlinks in the middle of the
tree, I don't think we care about any symlinks _git_ might be creating
concurrently ;)

But it is entirely possible that git apply - especially when used
outside of a real git directory - ends up doing that. And it's not
like we necessarily always use the whole lstat-cache mechanism to
begin with, so the fact that we have the infrastructure to be careful
in no way means that we necessarily always _are_ careful...

 Linus
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Linus Torvalds]

On Mon, Jan 26, 2015 at 1:07 PM, Josh Boyer jwbo...@fedoraproject.org wrote:

 Or did I miss a way that git-apply can take a git patch and apply it
 to a tree that isn't a git repo?

Exactly. git apply works as a straight patch replacement outside
of a git repository. It doesn't actually need a git tree to work.

(Of course, git apply is _not_ a patch replacement in the general
sense. It only applies context diffs - preferentially git style ones -
 so no old-style patches etc need apply. And it's not
replacement-compatible in a syntax sense either, in that while many of
the options are the same, not all are etc etc).

   Linus
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Josh Boyer]

On Mon, Jan 26, 2015 at 3:44 PM, Linus Torvalds
torva...@linux-foundation.org wrote:
 On Mon, Jan 26, 2015 at 8:32 AM, Josh Boyer jwbo...@fedoraproject.org wrote:

 I went to do the Fedora 3.19-rc6 build this morning and it failed in
 our buildsystem with:

 + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
 + case $patch in
 + unxz
 + patch -p1 -F1 -s
 symbolic link target '../../../../../include/dt-bindings' is invalid
 error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)

 Ugh. I don't see anything we can do about this on the git side, and I
 do kind of understand why 'patch' would be worried about '..' files.
 In a perfect world, patch would parse the filename and see that it
 stays within the directory structure of the project, but that is a
 rather harder thing to do than just say no dot-dot files.

 The short-term fix is likely to just use git apply instead of patch.

Well, that's one fix anyway.  I just removed the hunk from the local
copy of patch-3.19-rc6.xz and added the symlink manually.  See why
below.

 The long-term fix? I dunno. I don't see us not using symlinks, and a
 quick check says that every *single* symlink we have in the kernel
 source tree is one that points to a different directory using ..
 format. And while I could imagine that patch ends up counting the
 dot-dot entries and checking that it's all inside the same tree it is
 patching, I could also easily see patch *not* doing that. So using
 git apply _might_ end up being the long-term fix too.

It could, but from a distro perspective that requires either doing
'untar linux-3.N.tar.xz; cd linux-3.N; git add .; git apply
patch-3.N+1-rcX' , or just using a git tree to begin with, which then
makes all of this unnecessary anyway.  Creating a git repo from a
tarball for each build is kind of silly.  Some might say not just
using a git tree to build from to begin with in 2015 is also kind of
silly.

Or did I miss a way that git-apply can take a git patch and apply it
to a tree that isn't a git repo?

 I suspect that if patch cannot apply even old-style kernel patches
 due to the symlinks we have in the tree, and people end up having to
 use git apply for them, I might end up starting to just use
 rename-patches (ie using git diff -M) for the kernel.

I'm kind of wondering why we'd generate patches at all if you have to
apply them to a git repo, but maybe people like doing things the
old-fashioned way just for the hell of it.

 I've considered that for a while already, because patch _does_ kind
 of understand them these days, although I think it gets the
 cross-rename case wrong because it fundamentally works on a
 file-by-file basis. But if patch just ends up not working at all,
 the argument for trying to maintain backwards compatibility gets
 really weak.

Yeah.  I mostly wanted to give people a heads up on the issue.  I'm
sure it's going to impact more than just the kernel.  I think for us
it's mostly limited to the -rcX patches, because once the tarball for
the final release is out the symlink should be created by tar just
fine.

josh
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

On Mon, Jan 26, 2015 at 1:30 PM, Linus Torvalds
torva...@linux-foundation.org wrote:
 On Mon, Jan 26, 2015 at 1:07 PM, Josh Boyer jwbo...@fedoraproject.org wrote:

 Or did I miss a way that git-apply can take a git patch and apply it
 to a tree that isn't a git repo?

 Exactly. git apply works as a straight patch replacement outside
 of a git repository. It doesn't actually need a git tree to work.

What is your take on CVE-2015-1196, which brought this /regression/ to
GNU patch?
If git apply get /fixed/ for that same CVE, would that /break/ your fix?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

Linus Torvalds torva...@linux-foundation.org writes:

 Ugh. I don't see anything we can do about this on the git side, and I
 do kind of understand why 'patch' would be worried about '..' files.
 In a perfect world, patch would parse the filename and see that it
 stays within the directory structure of the project, but that is a
 rather harder thing to do than just say no dot-dot files.

It is unclear to me why limit to the current directory and below
is such a big deal in the first place.

If the user wants to apply a patch that touches ../etc/shadow, is
the tool in the place to complain?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Linus Torvalds]

On Mon, Jan 26, 2015 at 8:32 AM, Josh Boyer jwbo...@fedoraproject.org wrote:

 I went to do the Fedora 3.19-rc6 build this morning and it failed in
 our buildsystem with:

 + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
 + case $patch in
 + unxz
 + patch -p1 -F1 -s
 symbolic link target '../../../../../include/dt-bindings' is invalid
 error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)

Ugh. I don't see anything we can do about this on the git side, and I
do kind of understand why 'patch' would be worried about '..' files.
In a perfect world, patch would parse the filename and see that it
stays within the directory structure of the project, but that is a
rather harder thing to do than just say no dot-dot files.

The short-term fix is likely to just use git apply instead of patch.

The long-term fix? I dunno. I don't see us not using symlinks, and a
quick check says that every *single* symlink we have in the kernel
source tree is one that points to a different directory using ..
format. And while I could imagine that patch ends up counting the
dot-dot entries and checking that it's all inside the same tree it is
patching, I could also easily see patch *not* doing that. So using
git apply _might_ end up being the long-term fix too.

I suspect that if patch cannot apply even old-style kernel patches
due to the symlinks we have in the tree, and people end up having to
use git apply for them, I might end up starting to just use
rename-patches (ie using git diff -M) for the kernel.

I've considered that for a while already, because patch _does_ kind
of understand them these days, although I think it gets the
cross-rename case wrong because it fundamentally works on a
file-by-file basis. But if patch just ends up not working at all,
the argument for trying to maintain backwards compatibility gets
really weak.

   Linus
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html