User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040206 Firefox/0.8 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040206 Firefox/0.8 If I attach a patch that contains a change to a file with a space in its name, Patch Viewer ignores everything after the space. This truncation appears in the filenames in the diff view and in the filenames in the "Raw Unified" diff. Example: In the original patch: Index: My Application/Component.cpp =================================================================== RCS file: /home/hipsoft/root/Ware/Apps/My Application/Component.cpp,v <snip> Patch viewer treats this now as a file called: /home/hipsoft/root/Ware/Apps/My If I click on the "Raw Unified" link, it displays a patch containing: Index: Ware/Apps/My =================================================================== RCS file: /home/hipsoft/root/Ware/Apps/My Reproducible: Always Steps to Reproduce: 1. 2. 3.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
9 times out of 10, with programs that shell out to other programs (this uses cvs and interdiff and so forth), this means unquoted parameters to system calls, which would make this a security issue. Securing just in case, if it turns out to not be we can open it later.
Assignee: myk → john
Target Milestone: --- → Bugzilla 2.18
I'm not an expert, but this looks suspicious: open my $interdiff_fh, "$::interdiffbin $old_filename $new_filename|"; (attachment.cgi, line 510, current trunk.) Gerv
Ewwwww. Yep, that certainly looks like the culprit to me.
Whoever fixes this might want to think about how we can extend the test for dodgy system() and exec() calls to include this sort of thing as well. Gerv
Created attachment 145087 [details] [diff] [review] Patch v1 This replaces the piped open with a forked exec, matching the method we use in the dependencygraph stuff.
Hmm... I suspect the filename is already broken by that point though. I suspect there might be something wrong with the code that parses the attachment, perhaps in PatchReader::Raw.
I just audited all the external execution code within the PatchReader perl module, and it appears to be safe. The only place it's doing opens is on specific command lines without variables involved, and the system calls are all multi-parameter. So this one line in Bugzilla itself is the only thing I see that's obviously a problem...
(In reply to comment #7) > > So this one line in Bugzilla itself is the only thing I see that's obviously a > problem... From a security perspective I'm sure that's true, but the filenames in the patch are still truncated after the first space character in both the raw diff view and the HTML diff view. This greatly affects the usability of the tool. Please see the example in the original description.
Dave: are you sorting this out, or jkeiser, or someone else? Gerv
Comment on attachment 145087 [details] [diff] [review] Patch v1 This looks fine to me. Good catch.
Attachment #145087 - Flags: review+
Created attachment 145430 [details] [diff] [review] Patch for PatchReader This is a patch for the issue of spaces in the filename not showing up. It has been tested at http://landfill.bugzilla.org/bz176656/attachment.cgi?id=132&action=diff. I will upload a new version of PatchReader to the server as soon as I remember how, but this is small enough that you can edit it by hand to get going again.
Comment on attachment 145430 [details] [diff] [review] Patch for PatchReader The Raw Unified stuff isn't fixed yet. Will upload when done.
Attachment #145430 - Attachment is obsolete: true
John: let us know what version of PatchReader winds up including this fix, and we'll bump the minimum version requirement for Bugzilla to match.
Current status: waiting for PatchReader 0.9.3 or newer to hit CPAN. I just emailed jkeise directly to ask for status since he hasn't answered my plea on the bug :)
Whiteboard: [does not affect 2.16.x] [wanted for 2.18rc1]
OK, in addition to the existing patch here, we need to fix checksetup.pl to require version 0.9.4 of PatchReader, which should be available on CPAN later tonight. (per jkeiser on irc)
btw, the changes in PatchReader 0.9.4 are only sorta related to this, and are really just bugfixes. And as glob pointed out, there is not specifically a security issue here, because the filenames come from File::Temp. Still, there is a taint issue and there is a fix, so may as well take it.
OK, after a lengthly discussion on IRC about this, it's been decided that the original classification of this bug as a security issue was in error, and there is no exploitable issue here. The one and only problem with this was the spaces in filenames thing, and that was a header matching problem with PatchReader misreading the headers of the patch files, and nothing to do with filenames on disk. The issue with the piped open() command with variables in the command line part was a kneejerk reaction to usually disallowing that construct because it does potentially let security problems slip through. Examination of the code shows, however, that those two variables do contain trusted content (filenames returned from File::Temp::tempfile(), which guarantees well-formed filenames) and thus there is no security issue. In fact, the only thing we really need to do here is bump the minimum version requirement to pick up the new PatchReader that fixes this.
it's possible for File::Temp to return filenames with spaces in the path component (highly unlikely, bug possible). i'm addressing this issue in bug 250976.
The main problem here was fixed by the checkin from bug 250840. Bug 250976 is picking up the rest of the pieces left on this one.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Whiteboard: [does not affect 2.16.x] [wanted for 2.18rc1] → [does not affect 2.16.x] [fixed in 2.18rc2]
You need to log in before you can comment on or make changes to this bug.