[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [bug-patch] [PATCH] do not let a malicious patch create files above
From: |
Jim Meyering |
Subject: |
Re: [bug-patch] [PATCH] do not let a malicious patch create files above current directory |
Date: |
Tue, 01 Feb 2011 21:50:26 +0100 |
Jim Meyering wrote:
> Surprised that no one has posted a patch for this yet, I wrote my own:
>
>>From aeed44154b388210f0e70dbe802a78949224aa97 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> Date: Tue, 1 Feb 2011 11:21:15 +0100
> Subject: [PATCH] do not let a malicious patch create files above current
> directory
>
> This addresses CVE-2010-4651, reported by Jakub Wilk.
> https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2010-4651
> * src/pch.c (intuit_diff_type): Reject any file name containing a
> component of "..".
> * tests/dot-dot: New file. Test for this.
> * tests/Makefile.am (TESTS): Add it.
That patch did not go far enough in that it still permitted
absolute output file names, at least with -p0. I must have
tested only with -p1.
This improves on the preceding in a couple ways:
- now rejects *before* prompting about an invalid name at the expense
of also testing from "maybe_reverse". It looks like it would be
too invasive to try to defer the two calls to that function
until after inname is set.
- added a test case with an absolute name
- fixed the comment at the top of the test script
Also, it changes the diagnostic to be more general,
so that the same diagnostic applies to both cases.
>From ccf76efc15978399de01710502a1b153b87b73eb Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 1 Feb 2011 11:21:15 +0100
Subject: [PATCH] do not let a malicious patch create files above current
directory
This addresses CVE-2010-4651, reported by Jakub Wilk.
https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2010-4651
With this change, patch rejects any output file name that
is absolute or that contains a ".." component.
* src/pch.c (file_name_contains_dot_dot): New function.
(invalid_file_name): New function.
(maybe_reverse): Don't prompt about an invalid file name.
(intuit_diff_type): Reject any file name containing a
component of "..".
* tests/dot-dot: New file. Test for this.
* tests/Makefile.am (TESTS): Add it.
---
ChangeLog | 12 +++++++++++-
src/pch.c | 31 ++++++++++++++++++++++++++++++-
tests/Makefile.am | 3 ++-
tests/dot-dot | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 93 insertions(+), 3 deletions(-)
create mode 100644 tests/dot-dot
diff --git a/ChangeLog b/ChangeLog
index bbe5fe7..88db61c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2011-02-01 Jim Meyering <address@hidden>
+
+ do not let a malicious patch create files above current directory
+ This addresses CVE-2010-4651, reported by Jakub Wilk.
+ https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2010-4651
+ * src/pch.c (intuit_diff_type): Reject any file name containing a
+ component of "..".
+ * tests/dot-dot: New file. Test for this.
+ * tests/Makefile.am (TESTS): Add it.
+
2010-12-04 Andreas Gruenbacher <address@hidden>
* src/util.c (make_tempfile): Create missing directories when
@@ -3594,7 +3604,7 @@ Sun Dec 17 17:29:48 1989 Jim Kingdon (kingdon at
hobbes.ai.mit.edu)
Copyright (C) 1984, 1985, 1986, 1987, 1988 Larry Wall.
Copyright (C) 1989, 1990, 1991, 1992, 1993, 1997, 1998, 1999, 2000, 2001,
-2002, 2009, 2010 Free Software Foundation, Inc.
+2002, 2009, 2010, 2011 Free Software Foundation, Inc.
This file is part of GNU Patch.
diff --git a/src/pch.c b/src/pch.c
index 1653ee4..60a660c 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -3,7 +3,7 @@
/* Copyright (C) 1986, 1987, 1988 Larry Wall
Copyright (C) 1990, 1991, 1992, 1993, 1997, 1998, 1999, 2000, 2001,
- 2002, 2003, 2006, 2009, 2010 Free Software Foundation, Inc.
+ 2002, 2003, 2006, 2009, 2010, 2011 Free Software Foundation, Inc.
This program is free software: you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
@@ -193,11 +193,37 @@ grow_hunkmax (void)
return false;
}
+/* Return true if NAME contains a ".." component. */
+
+static bool
+file_name_contains_dot_dot (char const *name)
+{
+ size_t len = strlen (name);
+ return (len >= 3
+ && (strnEQ (name, "../", 3)
+ || strnEQ (name + len - 3, "/..", 3)
+ || strstr (name, "/../")));
+}
+
+/* A file name, NAME, is invalid if it is absolute or if
+ it contains a ".." component. */
+
+static bool
+invalid_file_name (char const *name)
+{
+ return (IS_ABSOLUTE_FILE_NAME (name)
+ || file_name_contains_dot_dot (name));
+}
+
static bool
maybe_reverse (char const *name, bool nonexistent, bool is_empty)
{
bool looks_reversed = (! is_empty) < p_says_nonexistent[reverse ^ is_empty];
+ /* If it's a name that we'll reject, don't bother prompting. */
+ if (invalid_file_name (name))
+ return false;
+
/* 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
@@ -935,6 +961,9 @@ intuit_diff_type (bool need_header, mode_t *p_file_type)
instat = st[i];
}
+ if (inname && invalid_file_name (inname))
+ fatal ("invalid output file name: %s", quotearg (inname));
+
return retval;
}
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ffe02af..69d8ea1 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,5 +1,5 @@
# Copyright (C) 1989, 1990, 1991, 1992, 1993, 1997, 1998, 1999, 2002,
-# 2003, 2006, 2009, 2010 Free Software Foundation, Inc.
+# 2003, 2006, 2009, 2010, 2011 Free Software Foundation, Inc.
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -24,6 +24,7 @@ TESTS = \
create-delete \
criss-cross \
crlf-handling \
+ dot-dot \
dash-o-append \
empty-files \
file-modes \
diff --git a/tests/dot-dot b/tests/dot-dot
new file mode 100644
index 0000000..b868a0c
--- /dev/null
+++ b/tests/dot-dot
@@ -0,0 +1,50 @@
+# Copyright (C) 2011 Free Software Foundation, Inc.
+#
+# Copying and distribution of this file, with or without modification,
+# in any medium, are permitted without royalty provided the copyright
+# notice and this notice are preserved.
+
+# Reject an output file name that is absolute or that contains a ".."
component.
+
+. $srcdir/test-lib.sh
+
+require_cat
+use_local_patch
+use_tmpdir
+
+# ==============================================================
+
+emit_patch()
+{
+cat <<EOF
+--- /dev/null
++++ $1
+@@ -0,0 +1 @@
++x
+EOF
+}
+
+check 'emit_patch /tmp/x | patch -p0; echo status: $?' <<EOF
+$PATCH: **** invalid output file name: /tmp/x
+status: 2
+EOF
+
+check 'emit_patch a/../z | patch -p0; echo status: $?' <<EOF
+$PATCH: **** invalid output file name: a/../z
+status: 2
+EOF
+
+check 'emit_patch a/../z | patch -p1; echo status: $?' <<EOF
+$PATCH: **** invalid output file name: ../z
+status: 2
+EOF
+
+check 'emit_patch a/.. | patch -p0; echo status: $?' <<EOF
+$PATCH: **** invalid output file name: a/..
+status: 2
+EOF
+
+check 'emit_patch ../z | patch -p0; echo status: $?' <<EOF
+$PATCH: **** invalid output file name: ../z
+status: 2
+EOF
--
1.7.3.5.44.g960a
Re: [bug-patch] [PATCH] do not let a malicious patch create files above current directory,
Jim Meyering <=