From f663762bf0aa5089fee41d62a4e7528f436164d4 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Wed, 16 Feb 2011 11:57:57 +0100 Subject: Ignore dangerous filenames instead of failing immediately * src/pch.c (name_is_valid): New function. (intuit_diff_type, best_name): Use name_is_valid() here. (strip_leading_slashes): Remove name validation tests from here. * tests/bad-filenames: Add more tests for covering more of the file name guessing corner cases in intuit_diff_type(), update the existing tests. * NEWS: Update. --- ChangeLog | 10 ++++++ NEWS | 2 +- src/pch.c | 28 +++++++++++++++- src/util.c | 11 ------ tests/bad-filenames | 97 ++++++++++++++++++++++++++++++++++++++++++++--------- 5 files changed, 120 insertions(+), 28 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4fd8a9b..defc7be 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2011-02-16 Andreas Gruenbacher + + * src/pch.c (name_is_valid): New function. + (intuit_diff_type, best_name): Use name_is_valid() here. + (strip_leading_slashes): Remove name validation tests from here. + * tests/bad-filenames: Add more tests for covering more of the + file name guessing corner cases in intuit_diff_type(), update the + existing tests. + * NEWS: Update. + 2011-02-15 Andreas Gruenbacher * src/patch.c (main): Fix use of initialized outst and add an diff --git a/NEWS b/NEWS index 65d3796..f13002a 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,4 @@ -* patch now rejects a destination file name that is absolute or that contains +* Patch now ignores destination file names that are absolute or that contain a component of "..". This addresses CVE-2010-4651, * Support for most features of the "diff --git" format: renames and copies, permission changes, symlink diffs. Caveats: diff --git a/src/pch.c b/src/pch.c index 68f7bc8..41c15b6 100644 --- a/src/pch.c +++ b/src/pch.c @@ -376,6 +376,31 @@ skip_hex_digits (char const *str) return s == str ? NULL : s; } +static bool +name_is_valid (char const *name) +{ + const char *n = name; + + if (IS_ABSOLUTE_FILE_NAME (name)) + { + say ("Ignoring potentially dangerous file name %s\n", quotearg (name)); + return false; + } + for (n = name; *n; ) + { + if (*n == '.' && *++n == '.' && ( ! *++n || ISSLASH (*n))) + { + say ("Ignoring potentially dangerous file name %s\n", quotearg (name)); + return false; + } + while (*n && ! ISSLASH (*n)) + n++; + while (ISSLASH (*n)) + n++; + } + return true; +} + /* Determine what kind of diff is in the remaining part of the patch file. */ static enum diff @@ -829,7 +854,7 @@ intuit_diff_type (bool need_header, mode_t *p_file_type) else { stat_errno[i] = 0; - if (posixly_correct) + if (posixly_correct && name_is_valid (p_name[i])) break; } i0 = i; @@ -1002,6 +1027,7 @@ best_name (char *const *name, int const *ignore) /* Of those, take the first name. */ for (i = OLD; i <= INDEX; i++) if (name[i] && !ignore[i] + && name_is_valid (name[i]) && components[i] == components_min && basename_len[i] == basename_len_min && len[i] == len_min) diff --git a/src/util.c b/src/util.c index 553cfbd..f1187ff 100644 --- a/src/util.c +++ b/src/util.c @@ -1415,17 +1415,6 @@ strip_leading_slashes (char *name, int strip_leading) n = p+1; } } - if (IS_ABSOLUTE_FILE_NAME (n)) - fatal ("rejecting absolute file name: %s", quotearg (n)); - for (p = n; *p; ) - { - if (*p == '.' && *++p == '.' && ( ! *++p || ISSLASH (*p))) - fatal ("rejecting file name with \"..\" component: %s", quotearg (n)); - while (*p && ! ISSLASH (*p)) - p++; - while (ISSLASH (*p)) - p++; - } if ((strip_leading < 0 || s <= 0) && *n) { memmove (name, n, strlen (n) + 1); diff --git a/tests/bad-filenames b/tests/bad-filenames index f53a613..0bc23eb 100644 --- a/tests/bad-filenames +++ b/tests/bad-filenames @@ -7,6 +7,7 @@ . $srcdir/test-lib.sh use_local_patch +use_tmpdir # ================================================================ @@ -23,27 +24,93 @@ EOF # Ensure that patch rejects an output file name that is absolute # or that contains a ".." component. -check 'emit_patch /absolute/path | patch -p0; echo status: $?' < d.diff < f +check 'patch -f -p0 --dry-run < d.diff || echo status: $?' < g +check 'patch -f -p1 --dry-run < d.diff || echo status: $?' <