diff options
author | Andreas Gruenbacher <agruen@gnu.org> | 2015-01-21 10:01:15 +0100 |
---|---|---|
committer | Andreas Gruenbacher <agruen@gnu.org> | 2015-01-22 21:51:51 +0100 |
commit | 41688ad8ef88bc296f3bed30b171ec73e5876b88 (patch) | |
tree | b2d4a9d3e31d2fec40e27f38dc85cd00bc6eaf1f | |
parent | 17953b5893f7c9835f0dd2a704ba04e0371d2cbd (diff) | |
download | patch-41688ad8ef88bc296f3bed30b171ec73e5876b88.tar.gz |
Fix the fix for CVE-2015-1196v2.7.3
* src/util.c (filename_is_safe): New function split off from name_is_valid().
(symlink_target_is_valid): Explain why we cannot have absolute symlinks or
symlinks with ".." components for now.
(move_file): Move absolute filename check here and explain.
* tests/symlinks: Put test case with ".." symlink in comments for now.
* NEWS: Add CVE number.
-rw-r--r-- | NEWS | 2 | ||||
-rw-r--r-- | src/pch.c | 16 | ||||
-rw-r--r-- | src/util.c | 83 | ||||
-rw-r--r-- | src/util.h | 1 | ||||
-rw-r--r-- | tests/symlinks | 32 |
5 files changed, 57 insertions, 77 deletions
@@ -4,7 +4,7 @@ deleting". * Function names in hunks (from diff -p) are now preserved in reject files. * With git-style patches, symlinks that point outside the working directory - will no longer be created. + will no longer be created (CVE-2015-1196). Changes in version 2.7.1: @@ -401,21 +401,7 @@ name_is_valid (char const *name) return false; } - if (IS_ABSOLUTE_FILE_NAME (name)) - is_valid = false; - else - for (n = name; *n; ) - { - if (*n == '.' && *++n == '.' && ( ! *++n || ISSLASH (*n))) - { - is_valid = false; - break; - } - while (*n && ! ISSLASH (*n)) - n++; - while (ISSLASH (*n)) - n++; - } + is_valid = filename_is_safe (name); /* Allow any filename if we are in the filesystem root. */ if (! is_valid && cwd_is_root (name)) @@ -423,55 +423,18 @@ create_backup (char const *to, const struct stat *to_st, bool leave_original) } } +/* Only allow symlink targets which are relative and free of ".." components: + * otherwise, the operating system may follow one of those symlinks in a + * pathname component, leading to a path traversal vulnerability. + * + * An alternative to disallowing many kinds of symlinks would be to implement + * path traversal in user space using openat() without following symlinks + * altogether. + */ static bool symlink_target_is_valid (char const *target, char const *to) { - bool is_valid; - - if (IS_ABSOLUTE_FILE_NAME (to)) - is_valid = true; - else if (IS_ABSOLUTE_FILE_NAME (target)) - is_valid = false; - else - { - unsigned int depth = 0; - char const *t; - - is_valid = true; - t = to; - while (*t) - { - while (*t && ! ISSLASH (*t)) - t++; - if (ISSLASH (*t)) - { - while (ISSLASH (*t)) - t++; - depth++; - } - } - - t = target; - while (*t) - { - if (*t == '.' && *++t == '.' && (! *++t || ISSLASH (*t))) - { - if (! depth--) - { - is_valid = false; - break; - } - } - else - { - while (*t && ! ISSLASH (*t)) - t++; - depth++; - } - while (ISSLASH (*t)) - t++; - } - } + bool is_valid = filename_is_safe (target); /* Allow any symlink target if we are in the filesystem root. */ return is_valid || cwd_is_root (to); @@ -520,7 +483,11 @@ move_file (char const *from, bool *from_needs_removal, read_fatal (); buffer[size] = 0; - if (! symlink_target_is_valid (buffer, to)) + /* If we are allowed to create a file with an absolute path name, + anywhere, we also don't need to worry about symlinks that can + leave the working directory. */ + if (! (IS_ABSOLUTE_FILE_NAME (to) + || symlink_target_is_valid (buffer, to))) { fprintf (stderr, "symbolic link target '%s' is invalid\n", buffer); @@ -1720,6 +1687,28 @@ int stat_file (char const *filename, struct stat *st) return xstat (filename, st) == 0 ? 0 : errno; } +/* Check if a filename is relative and free of ".." components. + Such a path cannot lead to files outside the working tree + as long as the working tree only contains symlinks that are + "filename_is_safe" when followed. */ +bool +filename_is_safe (char const *name) +{ + if (IS_ABSOLUTE_FILE_NAME (name)) + return false; + while (*name) + { + if (*name == '.' && *++name == '.' + && ( ! *++name || ISSLASH (*name))) + return false; + while (*name && ! ISSLASH (*name)) + name++; + while (ISSLASH (*name)) + name++; + } + return true; +} + /* Check if we are in the root of a particular filesystem namespace ("/" on UNIX or a particular drive's root on DOS-like systems). */ bool @@ -69,6 +69,7 @@ enum file_id_type lookup_file_id (struct stat const *); void set_queued_output (struct stat const *, bool); bool has_queued_output (struct stat const *); int stat_file (char const *, struct stat *); +bool filename_is_safe (char const *); bool cwd_is_root (char const *); enum file_attributes { diff --git a/tests/symlinks b/tests/symlinks index 6211026..04a9b73 100644 --- a/tests/symlinks +++ b/tests/symlinks @@ -148,20 +148,24 @@ ncheck 'test ! -L symlink' # Patch should not create symlinks which point outside the working directory. -cat > symlink-target.diff <<EOF -diff --git a/dir/foo b/dir/foo -new file mode 120000 -index 0000000..cad2309 ---- /dev/null -+++ b/dir/foo -@@ -0,0 +1 @@ -+../foo -\ No newline at end of file -EOF - -check 'patch -p1 < symlink-target.diff || echo "Status: $?"' <<EOF -patching symbolic link dir/foo -EOF +# We cannot even ensure that symlinks with ".." components are safe: we cannot +# guarantee that they won't end up higher up in the working tree than we think; +# the path to the symlink may follow symlinks itself. +# +#cat > symlink-target.diff <<EOF +#diff --git a/dir/foo b/dir/foo +#new file mode 120000 +#index 0000000..cad2309 +#--- /dev/null +#+++ b/dir/foo +#@@ -0,0 +1 @@ +#+../foo +#\ No newline at end of file +#EOF +# +#check 'patch -p1 < symlink-target.diff || echo "Status: $?"' <<EOF +#patching symbolic link dir/foo +#EOF cat > bad-symlink-target1.diff <<EOF diff --git a/bar b/bar |