[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: |
Thu, 03 Feb 2011 21:40:36 +0100 |
Jim Meyering wrote:
> Bert Wesarg wrote:
>>> + {
>>> + /* If inname starts with "../" ends with "/.." or contains
>>> + "/../", then issue a fatal error. */
>>> + size_t len = strlen (inname);
>>> + if (strnEQ (inname, "../", 3)
>>> + || strnEQ (inname + len - 3, "/..", 3)
>>
>> I miss a check, that len is at least 3.
>
> Good catch. That's why I stored the length in "len",
> but then I forgot to add it. Thanks!
> Here's the corrected patch (removed stray last line from the log, too):
>
>>From da345c4d84348c339a3c794b8427aaf0ce7ff805 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
Andreas noticed that invalid names were 'stat'ed before being
rejected and wanted to avoid that, so he proposed a revised patch
that moved the check into strip_leading_slashes and that avoided
using strstr (would have required gnulib support) and strnEQ.
He gave permission to resend this part of our most recent exchange:
----------------------------------------
I like your test script name change and the removal
of the unnecessary tmpdir and cat parts.
I like the way your rewrite removed the use of strstr
My first reaction to your moving the file name checks into
strip_leading_slashes was negative -- sounded like that added
functionality made a nominally library-like function do something
totally unrelated to its name (and possibly exit).
But once I understood what strip_leading_slashes does, and how
specialized it is, I see how this makes sense. And given the number
of global variables in patch, we can't be too picky on the style front ;-)
...
> + if (IS_ABSOLUTE_FILE_NAME (n))
> + fatal ("rejecting absolute file name: %s",
> + quotearg (n));
> + for (p = n; *p; )
> + {
> + if (*p != '.' || *++p != '.' || (*++p && !ISSLASH (*p)))
> + {
> + while (*p && ! ISSLASH (*p))
> + p++;
> + while (ISSLASH (*p))
> + p++;
> + continue;
> + }
> + fatal ("rejecting file name with \"..\" component: %s",
> + quotearg (n));
> + }
However, I prefer to negate the 3-term expression,
putting the 2nd fatal call right after it, and thus
decreasing nesting level of inner "while" loops.
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++;
}
>From cd5be4e8a3c9afe206c6d41243b9858745e96aeb 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/util.c (strip_leading_slashes): Reject absolute file names
and file names containing a component of "..".
* tests/bad-filenames: New file. Test for this.
* tests/Makefile.am (TESTS): Add it.
Improvements by Andreas Gruenbacher.
---
ChangeLog | 13 ++++++++++++-
src/pch.c | 2 +-
src/util.c | 13 ++++++++++++-
tests/Makefile.am | 3 ++-
tests/bad-filenames | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 76 insertions(+), 4 deletions(-)
create mode 100644 tests/bad-filenames
diff --git a/ChangeLog b/ChangeLog
index bbe5fe7..20810cb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2011-02-01 Jim Meyering <address@hidden>
+ and Andreas Gruenbacher <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/util.c (strip_leading_slashes): Reject absolute file names and
+ file names containing a component of "..".
+ * tests/bad-filenames: 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 +3605,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..8e64298 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
diff --git a/src/util.c b/src/util.c
index e03e48a..553cfbd 100644
--- a/src/util.c
+++ b/src/util.c
@@ -3,7 +3,7 @@
/* Copyright (C) 1986 Larry Wall
Copyright (C) 1992, 1993, 1997, 1998, 1999, 2001, 2002, 2003, 2006,
- 2009, 2010 Free Software Foundation, Inc.
+ 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
@@ -1415,6 +1415,17 @@ 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/Makefile.am b/tests/Makefile.am
index ffe02af..cca8a87 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
@@ -19,6 +19,7 @@
TESTS = \
asymmetric-hunks \
backup-prefix-suffix \
+ bad-filenames \
copy-rename \
corrupt-reject-files \
create-delete \
diff --git a/tests/bad-filenames b/tests/bad-filenames
new file mode 100644
index 0000000..489316f
--- /dev/null
+++ b/tests/bad-filenames
@@ -0,0 +1,49 @@
+# 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.
+
+. $srcdir/test-lib.sh
+
+use_local_patch
+
+# ==============================================================
+
+emit_patch()
+{
+cat <<EOF
+--- /dev/null
++++ $1
+@@ -0,0 +1 @@
++x
+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
+EOF
+
+check 'emit_patch a/../z | patch -p0; echo status: $?' <<EOF
+$PATCH: **** rejecting 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
+status: 2
+EOF
+
+check 'emit_patch a/.. | patch -p0; echo status: $?' <<EOF
+$PATCH: **** rejecting file name with ".." component: a/..
+status: 2
+EOF
+
+check 'emit_patch ../z | patch -p0; echo status: $?' <<EOF
+$PATCH: **** rejecting file name with ".." component: ../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, 2011/02/01