From 41688ad8ef88bc296f3bed30b171ec73e5876b88 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Wed, 21 Jan 2015 10:01:15 +0100 Subject: Fix the fix for CVE-2015-1196 * 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. --- NEWS | 2 +- src/pch.c | 16 +---------- src/util.c | 83 +++++++++++++++++++++++++--------------------------------- src/util.h | 1 + tests/symlinks | 32 ++++++++++++---------- 5 files changed, 57 insertions(+), 77 deletions(-) diff --git a/NEWS b/NEWS index d3f1c2d..d79cead 100644 --- a/NEWS +++ b/NEWS @@ -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: diff --git a/src/pch.c b/src/pch.c index bb39576..028d51f 100644 --- a/src/pch.c +++ b/src/pch.c @@ -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)) diff --git a/src/util.c b/src/util.c index 94c7582..ae05caa 100644 --- a/src/util.c +++ b/src/util.c @@ -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 diff --git a/src/util.h b/src/util.h index 579c5de..6b3308a 100644 --- a/src/util.h +++ b/src/util.h @@ -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 < symlink-target.diff < bad-symlink-target1.diff <