Bug 667529 (CVE-2010-4651)

Summary: CVE-2010-4651 patch: directory traversal flaw allows for arbitrary file creation
Product: [Other] Security Response Reporter: Vincent Danen <vdanen>
Component: vulnerabilityAssignee: Red Hat Product Security <security-response-team>
Status: CLOSED WONTFIX QA Contact:
Severity: low Docs Contact:
Priority: low    
Version: unspecifiedCC: agruen, agruen, bressers, carnil, cochranb, jacob.oursland, ldv, segoon, twaugh, underling, wnefal+redhatbugzilla
Target Milestone: ---Keywords: Security
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard: impact=low,public=20101230,reported=20110104,source=internet,cvss2=2.1/AV:L/AC:L/Au:N/C:N/I:P/A:N,rhel-3/patch=wontfix,rhel-4/patch=wontfix,rhel-5/patch=wontfix,rhel-6/patch=wontfix,fedora-all/patch=affected
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-08-21 23:27:09 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On: 675379    
Bug Blocks:    
Attachments:
Description Flags
patch I posted to bug-patch@gnu.org
none
better patch, including length check
none
do not reject an absolute first-file-name in -p0 patch
none
here's a lightly-tested, cumulative patch addressing the CVE, relative to upstream git
none
do not validate target name when it is specified on the command line
none
fix interdiff regression none

Description Vincent Danen 2011-01-05 16:42:51 EST
It was discovered that the patch utility allowed '..' in path names which could allow an attacker to create arbitrary files using a specially-crafted patch file.  For instance, the following patch will arbitrarily create the file /tmp/allyourbase.txt:

--- /dev/null
+++ /../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/allyourbase.txt
@@ -0,0 +1 @@
+All your base are belong to us.
Comment 1 Vincent Danen 2011-01-05 16:47:37 EST
Forgot to note the original report is here:

http://osdir.com/ml/bug-patch-gnu/2010-12/msg00000.html
Comment 2 Kurt Seifried 2011-01-05 19:12:42 EST
What command line options/etc. are being used to trigger this? It only creates the file in the local directory for me on Fedora 14:

[kurt@fedora14 ~]$ patch < evil.patch
patching file allyourbase.txt
[kurt@fedora14 ~]$ ls -la allyourbase.txt
-rw-rw-r--. 1 kurt kurt 32 Jan  6 00:08 allyourbase.txt
Comment 3 Tim Waugh 2011-01-06 04:39:27 EST
I think quilt uses "-p1".
Comment 4 Tim Waugh 2011-01-06 04:50:40 EST
$ patch -p1 -i evil.diff 
patching file ../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/allyourbase.txt
$ ls /tmp/allyourbase.txt 
/tmp/allyourbase.txt
Comment 5 Vincent Danen 2011-01-06 16:18:08 EST
This has been assigned the name CVE-2010-4651:

http://article.gmane.org/gmane.comp.security.oss.general/4063
Comment 6 Dmitry V. Levin 2011-01-06 19:22:53 EST
(In reply to comment #1)
> Forgot to note the original report is here:
> 
> http://osdir.com/ml/bug-patch-gnu/2010-12/msg00000.html

Unfortunately, this link responds with an error.
The canonical link seems to work better:
http://lists.gnu.org/archive/html/bug-patch/2010-12/msg00000.html
Comment 7 Jim Meyering 2011-02-01 06:13:23 EST
Created attachment 476361 [details]
patch I posted to bug-patch@gnu.org

I haven't seen a patch for this yet, so posted the attached fix to bug-patch@gnu.org.
Comment 8 Jim Meyering 2011-02-01 06:27:04 EST
Created attachment 476365 [details]
better patch, including length check
Comment 9 Jim Meyering 2011-02-01 06:27:48 EST
Credit where due:  Bert Wesarg <bert.wesarg@googlemail.com>
spotted the error corrected above.
Comment 10 Andreas Gruenbacher 2011-02-01 15:32:21 EST
Thanks to Jim Meyering for notifying me about this bug; I dropped off the bug-patch@gnu.org mailing list by accident.
Comment 11 Josh Bressers 2011-02-04 15:00:53 EST
This appears to have been fixed upstream:
http://git.savannah.gnu.org/cgit/patch.git/commit/?id=685a78b6052f4df6eac6d625a545cfb54a6ac0e1
Comment 12 Huzaifa S. Sidhpurwala 2011-02-05 05:13:39 EST
Created patch tracking bugs for this issue

Affects: fedora-all [bug 675379]
Comment 14 Dmitry V. Levin 2011-02-06 19:38:18 EST
(In reply to comment #11)
> This appears to have been fixed upstream:
> http://git.savannah.gnu.org/cgit/patch.git/commit/?id=685a78b6052f4df6eac6d625a545cfb54a6ac0e1

As reported by Alexey Tourbin, there seems to be a regression introduced by this change.  Compare two samples:

1$ echo '--- some/path
+++ target
@@ -0,0 +1 @@
+x
' | patch -p0
patching file target

2$ echo '--- /some/path
+++ target
@@ -0,0 +1 @@
+x
' | patch -p0
patch: **** rejecting absolute file name: /some/path

There are no reason to reject those absolute or relative path names that are not going to be used in later computations.
Comment 15 Jim Meyering 2011-02-07 05:10:04 EST
I have a patch.  Will post once I've confirmed it covers all relevant code paths.
Comment 16 Jim Meyering 2011-02-07 05:14:16 EST
Thanks for letting us know, Dmitry!
And thanks to Alexey for reporting it.
Comment 17 Jim Meyering 2011-02-07 05:43:04 EST
Created attachment 477393 [details]
do not reject an absolute first-file-name in -p0 patch

Posted to bug-patch as well.
Comment 18 Tim Waugh 2011-02-07 11:44:04 EST
Jim: could you please post a cumulative patch containing all the changes?  Ideally it would be against 2.6.1 but I can try to back-port it.
Comment 19 Jim Meyering 2011-02-07 12:06:17 EST
Sure.  Attached.  For porting to any linux/gnu-only system, this should help:

#ifndef DIRECTORY_SEPARATOR
# define DIRECTORY_SEPARATOR '/'
#endif
#ifndef ISSLASH
# define ISSLASH(C) ((C) == DIRECTORY_SEPARATOR)
#endif
#define IS_ABSOLUTE_FILE_NAME(F) (ISSLASH ((F)[0]))
Comment 20 Jim Meyering 2011-02-07 12:12:33 EST
Created attachment 477460 [details]
here's a lightly-tested, cumulative patch addressing the CVE, relative to upstream git
Comment 21 Tim Waugh 2011-02-10 06:25:02 EST
This change is still too strict:

$ diff -u /dev/null <(echo b) | patch /tmp/foo 
patch: **** rejecting absolute target file name: /tmp/foo

When a filename has been explicitly passed on the command line we shouldn't be validating it.
Comment 22 Jim Meyering 2011-02-10 07:23:01 EST
Created attachment 478038 [details]
do not validate target name when it is specified on the command line

Thanks, Tim.
I've posted a patch for that.  Here's a copy:
Comment 23 Tim Waugh 2011-02-10 07:50:06 EST
Thanks.
Comment 24 Vasiliy Kulikov 2011-02-18 11:45:33 EST
Created attachment 479566 [details]
fix interdiff regression

The patch introduces interdiff regression:

$ interdiff -z john-1.7.6-jumbo-9.diff.gz john-1.7.6-jumbo-10.diff.gz
patch: **** rejecting absolute target file name: /tmp/.private/genie/interdiff-1.7yovIC
interdiff: Error applying patch1 to reconstructed file

interdiff creates a patch with absolute filenames, but doesn't pass the target filename as argument to patch.

It is fixed in the latest upstream version 0.3.2.  The fix itself is attached.
Comment 25 Bob Cochran 2011-03-13 11:35:10 EDT
I'm getting errors when attempting to apply patches containing '..' in the headers used by the OpenEmbedded (see http://www.openembedded.org/index.php/Main_Page) system:

[bob@deaf57 ~]$ ls -al patch*
-rw-rw-r-- 1 bob bob 422 Mar 13 09:34 patch_issue_db-native.txt
[bob@deaf57 ~]$ cat patch*
 10400K .......... .......... .......... .......... .......... 83%  517K 4s
 10450K .......... ........FATAL: Execution of 'quilt --quiltrc=/home/bob3a/overo-oe/tmp/sysroots/x86_64-linux/usr/bin/quiltrc push' failed with exit code 1:
Applying patch db5-arm-thumb-mutex.patch
patch: **** rejecting target file name with ".." component: ../dbinc/mutex_int.h
Patch db5-arm-thumb-mutex.patch does not apply (enforce with -f)
 
I want patch to apply these types of patches in some circumstances and this is one of them; patch will stop the build of an entire OpenEmbedded system for this issue. I was able to build OpenEmbedded successfully with patch versions prior to 2.6.1-7. I think there should be an option for patch to work as it did in earlier versions, perhaps by setting options to the utility in an options file. 

I do not think patch should break entire source code distributions in the manner it is.


Thanks!
Comment 26 Bob Cochran 2011-03-13 15:35:53 EDT
I was able to successfully complete an OpenEmbedded build by downgrading patch to version 2.6.1-5:

'yum downgrade patch'
Comment 27 Tim Waugh 2011-04-18 09:16:55 EDT
The OpenEmbedded project needs to adjust their build so that the filename of the file to patch is given on the command line, or so that the input file contains a subordinate relative path (i.e. no '..' references or leading '/').
Comment 28 Jacob Oursland 2011-06-13 22:26:34 EDT
There are valid uses for relative '..' paths without a known absolute paths.  This functionality should not be removed entirely.  I would like a command line option to accept relative '..' paths.
Comment 29 Jim Meyering 2011-06-15 08:20:18 EDT
Jacob, if you can describe such a use, please do.
There are certainly valid use cases with ".." in a *source* file name,
but that is fine: those are not rejected.  It's only when ".." appears
as a component of a destination file name that there is a security problem.


Bob,
I found a copy of the mutex_int.h patch you mentioned above:

   http://git.shr-project.org/git/?p=n900-oe.git;a=blob;f=recipes/db/files/db5-arm-thumb-mutex.patch;h=51b88823cf8ec1d959e1c73adb83abc4be4ae395;hb=9cd6a9dcdb323334186f0fe7c4537cd9dc8a58b1

It starts like this:

   1 Index: db-5.0.21/../dbinc/mutex_int.h
   2 ===================================================================
   3 --- db-5.0.21.orig/../dbinc/mutex_int.h 2010-03-30 10:36:09.000000000 -0700
   4 +++ db-5.0.21/../dbinc/mutex_int.h      2010-05-22 12:07:38.281286337 -0700

That is a fine example of a useless ".." in a destination file name component.
That patch will work just as well if you transform it to look like this:

   1 Index: dbinc/mutex_int.h
   2 ===================================================================
   3 --- dbinc/mutex_int.h 2010-03-30 10:36:09.000000000 -0700
   4 +++ dbinc/mutex_int.h      2010-05-22 12:07:38.281286337 -0700

Please understand that patch cannot (or at least will not) read options from a configuration file, and I would be strongly opposed to letting an environment variable control security sensitive behavior like this.

The above file is doubtless version-controlled somewhere.
I suggest you update it accordingly, along with any others that fit the pattern.
Even if patch were to add a --force (-like) option to make it accept such names,
the code that applies patches would have to be modified to invoke patch with that new option.

If you find a case in which the ".." component is somehow required, please report it here or on the upstream bug-patch@gnu.org mailing list.

Thanks,

Jim
Comment 31 Jacob Oursland 2011-08-07 04:41:23 EDT
Fixing this "bug" has caused problems in environments where patch is being used to add and remove text from text files.  Buildroot uses this methood to add functionality a target filesystem based upon selected components.  For example, you may wish to manipulate a config file in ../target/etc/ to use one DHCP client instead of another.

This "fix" ignores the real problem, but creates a headache for others.  The bug isn't that patch can create files or manipulate text files, as that is its purpose, but rather that patch is being used on untrusted patch files.  If a patch file can be successfully used to create a file, it can also be used to install a backdoor in source that the user is expecting to be manipulated.  It should be up to the user to not apply arbitrary patches, just as it is up to the user to not copy and paste arbitrary shell code into a terminal.
Comment 32 Tim Waugh 2011-08-07 13:30:25 EDT
The issue is that patch can modify/create files *outside* the current working directory without the invoking user's knowledge or consent.

If you really want to do this, simply specify that file on the patch command line.
Comment 33 Jacob Oursland 2011-08-07 14:58:46 EDT
I am well aware that this behavior can modify files outside of the working directory; in fact, that is the intended behavior.  The file I wish to modify is in a subdirectory of the parent directory to the current working directory, such as "../target/file".

Furthermore, if the user is unaware of this behavior, it is because they are not looking at the patch file.  Consent to this behavior is implied by the user running patch on the patch file.  It should be up to the user to verify that the patch file is valid and its modifications intended before applying.

The example case that was used in the initial report was that of a malicious patch file in unverified and/or untrustworthy source.  While the patch concern is being addressed, it ignores the fact that the source itself is untrustworthy.  Ignoring the patch concerns, there is no guarantee that the resulting binary is not malicious in nature.

Removing ".." from file names is a bad fix.  The real fix is "don't run code from untrustworthy or unverified sources!"
Comment 34 Jim Meyering 2011-08-07 15:38:18 EDT
(In reply to comment #33)
> I am well aware that this behavior can modify files outside of the working
> directory; in fact, that is the intended behavior.  The file I wish to modify
> is in a subdirectory of the parent directory to the current working directory,
> such as "../target/file".
>
> Furthermore, if the user is unaware of this behavior, it is because they are
> not looking at the patch file.  Consent to this behavior is implied by the user
> running patch on the patch file.  It should be up to the user to verify that
> the patch file is valid and its modifications intended before applying.

Sorry, but restoring the default behavior is not an option.
It would be too risky/surprising -- like telling people always to
verify that a tarball "looks ok" before unpacking it.
Comment 35 Andreas Gruenbacher 2011-08-08 04:12:01 EDT
I'm on the side of Jim and Tim on this issue; Jacob's use case doesn't seem very convincing.  Disallowing ".." in pathname components is a step towards the least surprise: it confines the area in which a patch can cause damage.  As the example of tar shows, patch isn't alone in taking this approach.
Comment 36 Vincent Danen 2011-08-10 16:44:09 EDT
I agree as well.  However, maybe it makes sense to add an --unsafe option to patch so that it _can_ do what it used to?  That would keep the default to what we've got here now (rejecting these relative paths), but allowing people in esoteric environments or with certain use cases, to continue using patch in a way they expect.

Just a thought.  I certainly agree that reverting these changes is not an option, but perhaps having an ungainly commandline option like --unsafe might be useful as well.
Comment 37 Jim Meyering 2011-08-11 07:09:45 EDT
I would not reject an upstream patch that adds that -- it'd have to update documentation and NEWS, and add a test case or two.
Comment 38 Vincent Danen 2015-08-21 23:26:48 EDT
Statement:

Red Hat Product Security has rated this issue as having Low security impact. This issue is not currently planned to be addressed in future updates. For additional information, refer to the Issue Severity Classification: https://access.redhat.com/security/updates/classification/.