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.
Forgot to note the original report is here: http://osdir.com/ml/bug-patch-gnu/2010-12/msg00000.html
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
I think quilt uses "-p1".
$ patch -p1 -i evil.diff patching file ../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/allyourbase.txt $ ls /tmp/allyourbase.txt /tmp/allyourbase.txt
This has been assigned the name CVE-2010-4651: http://article.gmane.org/gmane.comp.security.oss.general/4063
(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
Created attachment 476361 [details] patch I posted to bug-patch I haven't seen a patch for this yet, so posted the attached fix to bug-patch.
Created attachment 476365 [details] better patch, including length check
Credit where due: Bert Wesarg <bert.wesarg> spotted the error corrected above.
Thanks to Jim Meyering for notifying me about this bug; I dropped off the bug-patch mailing list by accident.
This appears to have been fixed upstream: http://git.savannah.gnu.org/cgit/patch.git/commit/?id=685a78b6052f4df6eac6d625a545cfb54a6ac0e1
Created patch tracking bugs for this issue Affects: fedora-all [bug 675379]
(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.
I have a patch. Will post once I've confirmed it covers all relevant code paths.
Thanks for letting us know, Dmitry! And thanks to Alexey for reporting it.
Created attachment 477393 [details] do not reject an absolute first-file-name in -p0 patch Posted to bug-patch as well.
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.
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]))
Created attachment 477460 [details] here's a lightly-tested, cumulative patch addressing the CVE, relative to upstream git
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.
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:
Thanks.
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.
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!
I was able to successfully complete an OpenEmbedded build by downgrading patch to version 2.6.1-5: 'yum downgrade patch'
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 '/').
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.
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 mailing list. Thanks, Jim
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.
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.
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!"
(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.
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.
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.
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.
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/.