[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [bug-patch] [PATCH] do not reject an absolute first-file-name in -p0
From: |
Jim Meyering |
Subject: |
Re: [bug-patch] [PATCH] do not reject an absolute first-file-name in -p0 patch |
Date: |
Mon, 07 Feb 2011 17:27:56 +0100 |
Jim Meyering wrote:
> The latest version of patch introduced a regression.
> Thanks to Alexey and Dmitry for reporting it:
> https://bugzilla.redhat.com/show_bug.cgi?id=667529#c14
> Here's a patch to address it.
>
> Subject: [PATCH] do not reject an absolute first-file-name in -p0 patch
>
> The fix for CVE-2010-4651 was too aggressive; adjust it so
> that the name validation is applied only to target names.
> * src/util.c (strip_leading_slashes): Move target validation code...
> * src/pch.c (validate_target_name): ...to this new function.
> (maybe_reverse): Call it from here.
> (intuit_diff_type): Likewise.
> * tests/bad-filenames: Add more tests to exercise this case.
> Adjust expected output to reflect diagnostic wording changes.
> Reported by Alexey Tourbin via Dmitry V. Levin.
While seemingly correct, that patch provokes a minor "make distcheck"
failure due to a new test leaving behind the file, "tests/target".
With this incremental change, "make distcheck" now passes:
>From 356b65f3c6fc7e366a01c4dc8c389447d2fa6273 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 7 Feb 2011 16:52:30 +0100
Subject: [PATCH] perform bad-filenames tests in temporary dir
---
tests/bad-filenames | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/tests/bad-filenames b/tests/bad-filenames
index 315447c..00e6533 100644
--- a/tests/bad-filenames
+++ b/tests/bad-filenames
@@ -7,6 +7,7 @@
. $srcdir/test-lib.sh
use_local_patch
+use_tmpdir
# ================================================================
--
1.7.4.2.g597a6
Here's the merged change-set:
>From 8d2bb2690cf53a3770c342215b24a6e623c3377f Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 7 Feb 2011 11:17:49 +0100
Subject: [PATCH-v2] do not reject an absolute first-file-name in -p0 patch
The fix for CVE-2010-4651 was too aggressive; adjust it so
that the name validation is applied only to target names.
* src/util.c (strip_leading_slashes): Move target validation code...
* src/pch.c (validate_target_name): ...to this new function.
(maybe_reverse): Call it from here.
(intuit_diff_type): Likewise.
* tests/bad-filenames: Add more tests to exercise this case.
Adjust expected output to reflect diagnostic wording changes.
Now that we create a file, use "use_tmpdir".
Reported by Alexey Tourbin via Dmitry V. Levin.
---
ChangeLog | 13 +++++++++++++
src/pch.c | 21 +++++++++++++++++++++
src/util.c | 11 -----------
tests/bad-filenames | 30 ++++++++++++++++++++++--------
4 files changed, 56 insertions(+), 19 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index e72c82c..3062722 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2011-02-07 Jim Meyering <address@hidden>
+
+ do not reject an absolute first-file-name in -p0 patch
+ The fix for CVE-2010-4651 was too aggressive; adjust it so
+ that the name validation is applied only to target names.
+ * src/util.c (strip_leading_slashes): Move target validation code...
+ * src/pch.c (validate_target_name): ...to this new function.
+ (maybe_reverse): Call it from here.
+ (intuit_diff_type): Likewise.
+ * tests/bad-filenames: Add more tests to exercise this case.
+ Adjust expected output to reflect diagnostic wording changes.
+ Reported by Alexey Tourbin via Dmitry V. Levin.
+
2011-02-03 Ozan Çağlayan <address@hidden>
Create directory test case
diff --git a/src/pch.c b/src/pch.c
index 68f7bc8..f661b69 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -189,11 +189,31 @@ grow_hunkmax (void)
return false;
}
+static void
+validate_target_name (char const *n)
+{
+ char const *p = n;
+ if (IS_ABSOLUTE_FILE_NAME (p))
+ fatal ("rejecting absolute target file name: %s", quotearg (p));
+ while (*p)
+ {
+ if (*p == '.' && *++p == '.' && ( ! *++p || ISSLASH (*p)))
+ fatal ("rejecting target file name with \"..\" component: %s",
+ quotearg (n));
+ while (*p && ! ISSLASH (*p))
+ p++;
+ while (ISSLASH (*p))
+ p++;
+ }
+}
+
static bool
maybe_reverse (char const *name, bool nonexistent, bool is_empty)
{
bool looks_reversed = (! is_empty) < p_says_nonexistent[reverse ^ is_empty];
+ validate_target_name (name);
+
/* Allow to create and delete empty files when we know that they are empty:
in the "diff --git" format, we know that from the index header. */
if (is_empty
@@ -929,6 +949,7 @@ intuit_diff_type (bool need_header, mode_t *p_file_type)
inerrno = stat_errno[i];
invc = version_controlled[i];
instat = st[i];
+ validate_target_name (inname);
}
return retval;
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..00e6533 100644
--- a/tests/bad-filenames
+++ b/tests/bad-filenames
@@ -7,43 +7,57 @@
. $srcdir/test-lib.sh
use_local_patch
+use_tmpdir
# ================================================================
-emit_patch()
+emit_2()
{
cat <<EOF
---- /dev/null
-+++ $1
+--- $1
++++ $2
@@ -0,0 +1 @@
+x
EOF
}
+emit_patch() { emit_2 /dev/null "$1"; }
+
# 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
+$PATCH: **** rejecting absolute target file name: /absolute/path
status: 2
EOF
check 'emit_patch a/../z | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: a/../z
+$PATCH: **** rejecting target file name with ".." component: a/../z
status: 2
EOF
check 'emit_patch a/../z | patch -p1; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: ../z
+$PATCH: **** rejecting target file name with ".." component: ../z
status: 2
EOF
check 'emit_patch a/.. | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: a/..
+$PATCH: **** rejecting target file name with ".." component: a/..
status: 2
EOF
check 'emit_patch ../z | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: ../z
+$PATCH: **** rejecting target file name with ".." component: ../z
status: 2
EOF
+
+check 'emit_2 /abs/path target | patch -p0; echo status: $?' <<EOF
+patching file target
+status: 0
+EOF
+
+echo x > target
+check 'emit_2 /abs/path target | patch -R -p0; echo status: $?' <<EOF
+patching file target
+status: 0
+EOF
--
1.7.4.2.g597a6