diff options
author | Andreas Gruenbacher <agruen@linbit.com> | 2011-02-16 11:57:57 +0100 |
---|---|---|
committer | Andreas Gruenbacher <agruen@linbit.com> | 2011-02-16 12:01:19 +0100 |
commit | f663762bf0aa5089fee41d62a4e7528f436164d4 (patch) | |
tree | 128b362e0aaecc50599f744e5dc8ce484cb437b3 | |
parent | dcfb493578174ad9ac19bb748a2b196bed2099ce (diff) | |
download | patch-f663762bf0aa5089fee41d62a4e7528f436164d4.tar.gz |
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.
-rw-r--r-- | ChangeLog | 10 | ||||
-rw-r--r-- | NEWS | 2 | ||||
-rw-r--r-- | src/pch.c | 28 | ||||
-rw-r--r-- | src/util.c | 11 | ||||
-rw-r--r-- | tests/bad-filenames | 97 |
5 files changed, 120 insertions, 28 deletions
@@ -1,3 +1,13 @@ +2011-02-16 Andreas Gruenbacher <agruen@linbit.com> + + * 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 <agruen@linbit.com> * src/patch.c (main): Fix use of initialized outst and add an @@ -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: @@ -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) @@ -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: $?' <<EOF -$PATCH: **** rejecting absolute file name: /absolute/path -status: 2 +check 'emit_patch ../z | patch -f -p1 --dry-run || echo status: $?' <<EOF +patching file z EOF -check 'emit_patch a/../z | patch -p0; echo status: $?' <<EOF -$PATCH: **** rejecting file name with ".." component: a/../z -status: 2 +check 'emit_patch /absolute/path | patch -f -p0 --dry-run || echo status: $?' <<EOF +Ignoring potentially dangerous file name /absolute/path +can't find file to patch at input line 3 +Perhaps you used the wrong -p or --strip option? +The text leading up to this was: +-------------------------- +|--- /dev/null +|+++ /absolute/path +-------------------------- +No file to patch. Skipping patch. +1 out of 1 hunk ignored +status: 1 EOF -check 'emit_patch a/../z | patch -p1; echo status: $?' <<EOF -$PATCH: **** rejecting file name with ".." component: ../z -status: 2 +check 'emit_patch a/../z | patch -f -p0 --dry-run || echo status: $?' <<EOF +Ignoring potentially dangerous file name a/../z +can't find file to patch at input line 3 +Perhaps you used the wrong -p or --strip option? +The text leading up to this was: +-------------------------- +|--- /dev/null +|+++ a/../z +-------------------------- +No file to patch. Skipping patch. +1 out of 1 hunk ignored +status: 1 EOF -check 'emit_patch a/.. | patch -p0; echo status: $?' <<EOF -$PATCH: **** rejecting file name with ".." component: a/.. -status: 2 +check 'emit_patch a/../z | patch -f -p1 --dry-run || echo status: $?' <<EOF +Ignoring potentially dangerous file name ../z +can't find file to patch at input line 3 +Perhaps you used the wrong -p or --strip option? +The text leading up to this was: +-------------------------- +|--- /dev/null +|+++ a/../z +-------------------------- +No file to patch. Skipping patch. +1 out of 1 hunk ignored +status: 1 EOF -check 'emit_patch ../z | patch -p0; echo status: $?' <<EOF -$PATCH: **** rejecting file name with ".." component: ../z -status: 2 +check 'emit_patch ../z | patch -f -p0 --dry-run || echo status: $?' <<EOF +Ignoring potentially dangerous file name ../z +can't find file to patch at input line 3 +Perhaps you used the wrong -p or --strip option? +The text leading up to this was: +-------------------------- +|--- /dev/null +|+++ ../z +-------------------------- +No file to patch. Skipping patch. +1 out of 1 hunk ignored +status: 1 +EOF + +cat > d.diff <<EOF +--- f ++++ ../g +@@ -1 +1 @@ +-1 ++1 +EOF + +check 'patch -f -p0 --dry-run < d.diff || echo status: $?' <<EOF +can't find file to patch at input line 3 +Perhaps you used the wrong -p or --strip option? +The text leading up to this was: +-------------------------- +|--- f +|+++ ../g +-------------------------- +No file to patch. Skipping patch. +1 out of 1 hunk ignored +status: 1 +EOF + +echo 1 > f +check 'patch -f -p0 --dry-run < d.diff || echo status: $?' <<EOF +patching file f +EOF + +echo 1 > g +check 'patch -f -p1 --dry-run < d.diff || echo status: $?' <<EOF +patching file g EOF |