Attempt. n°3 : followed the GNU coding standards and added a testsuite case
On Thu, Jun 15, 2017 at 9:37 PM, Cedric Buissart <cbuis...@redhat.com>
wrote:
> Attempt n.2 :
> Created a function that walks the whole path. If anything not-directory is
> found, return an error. If the path is not fully created, we consider that
> a success and let cpio decides when time has come.
> Files will be skipped if no-absolute-path is set and error is return.
>
> On Wed, Jun 7, 2017 at 10:46 AM, Pavel Raiskup <prais...@redhat.com>
> wrote:
>
>> On Wednesday, June 7, 2017 10:07:21 AM CEST Cedric Buissart wrote:
>> > > In other words and IMO, if we were about to fix this issue - we
>> should only
>> > > refuse to extract files through symlinks.
>> >
>> > Through any symlinks, or only those created by the archive itself ?
>>
>> Remembering the extracted links might be expensive, and with
>> --no-absolute-filenames we want to stay in CWD anyway - no matter how the
>> links
>> in CWD were created.
>>
>> > The latter might look less restrictive, but what happens if a local
>> > attacker is able to create a symlink. Is it something that should be
>> > considered ?
>>
>> Usually user should avoid races manually when running archiver:
>> https://www.gnu.org/software/tar/manual/html_node/Race-conditions.html
>
> based on the above, I did not try to avoid races.
>
>>
>>
>> Pavel
>>
>>
>>
>>
>
>
> --
> Cedric Buissart,
> Product Security
>
--
Cedric Buissart,
Product Security
commit f6632044c64423e1ec258be0be398b37c26459e8
Author: Cedric Buissart <cbuis...@redhat.com>
Date: Tue Jul 4 13:27:13 2017 +0200
no-absolute-filenames: prevent following symlinks
when --no-absolute-filenames is provided
diff --git a/src/copyin.c b/src/copyin.c
index ba887ae..064f07e 100644
--- a/src/copyin.c
+++ b/src/copyin.c
@@ -1408,6 +1408,18 @@ process_copy_in ()
}
}
+
+#ifdef CP_IFLNK
+ /* no_abs_paths_flag prevents us to follow symlink */
+ if (no_abs_paths_flag && existing_path_has_symlink (file_hdr.c_name) == -1)
+ {
+ error (0, 0, _("%s not created: symlink in its path"), file_hdr.c_name);
+ tape_toss_input (in_file_des, file_hdr.c_filesize);
+ tape_skip_padding (in_file_des, file_hdr.c_filesize);
+ continue;
+ }
+#endif
+
copyin_file(&file_hdr, in_file_des);
if (verbose_flag)
diff --git a/src/util.c b/src/util.c
index 10486dc..e2f44a5 100644
--- a/src/util.c
+++ b/src/util.c
@@ -1717,3 +1717,54 @@ arf_stores_inode_p (enum archive_format arf)
return 1;
}
+
+#ifdef CP_IFLNK
+/* existing_path_has_symlink will walk the path as far as possible, ensuring that no
+ * symlinks is crossed.
+ *
+ * returns :
+ * -1 if unexpected error or if any else than a directory is found on the way
+ * (with the exception of the basename)
+ * 0 otherwise
+ * Note: if part of the path is missing, it's ok, cpio might be allowed to create it
+ */
+int
+existing_path_has_symlink (char* path)
+{
+ int ret = 0;
+ struct stat stat;
+ char *next, *dup;
+ dup = malloc ((strlen(path)+1) * sizeof(char));
+ if (!dup)
+ return -1;
+
+ next = path;
+ while (1)
+ {
+ next = strchr (next, '/');
+ if (! next)
+ break;
+
+ strncpy(dup, path, next - path);
+ dup[next - path] = '\0';
+
+ if (lstat (dup, &stat) != 0)
+ {
+ /* only ENOENT is expected here (we let cpio create path if required) */
+ if (errno != ENOENT)
+ ret = -1;
+ break;
+ }
+
+ if (!S_ISDIR(stat.st_mode))
+ {
+ ret = -1;
+ break;
+ }
+ next++;
+ }
+
+ free (dup);
+ return ret;
+}
+#endif
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 65acb46..e71aee8 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -56,7 +56,8 @@ TESTSUITE_AT = \
symlink-long.at\
symlink-to-stdout.at\
version.at\
- big-block-size.at
+ big-block-size.at\
+ symlink-escape.at
TESTSUITE = $(srcdir)/testsuite
diff --git a/tests/symlink-escape.at b/tests/symlink-escape.at
new file mode 100644
index 0000000..92338d3
--- /dev/null
+++ b/tests/symlink-escape.at
@@ -0,0 +1,40 @@
+# Process this file with autom4te to create testsuite. -*- Autotest -*-
+# Copyright (C) 2004, 2006-2007, 2010, 2014-2015, 2017 Free Software
+# Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3, or (at your option)
+# any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301 USA.
+
+AT_SETUP([no-absolute-filenames escape])
+AT_KEYWORDS([security copyin symlink no-absolute-filenames])
+
+
+AT_CHECK([
+mkdir leak inside outside
+ln -s ../outside/ inside/leak
+touch leak/a
+cpio -o --quiet > leak.cpio <<<"leak/a"
+cd inside
+cpio -i --no-absolute-filenames --quiet < ../leak.cpio
+test -e ../outside/a && echo FAIL || echo SUCCESS
+],
+[0],
+[SUCCESS
+],
+[cpio: leak/a not created: symlink in its path
+])
+
+AT_CLEANUP
+
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 644ada0..f749716 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -43,3 +43,4 @@ m4_include([setstat03.at])
m4_include([setstat04.at])
m4_include([setstat05.at])
m4_include([big-block-size.at])
+m4_include([symlink-escape.at])
