Patch viewer mishandling patched filenames with spaces

RESOLVED FIXED in Bugzilla 2.18



Attachments & Requests
14 years ago
5 years ago


(Reporter: Cameron Smith, Assigned: John Keiser (jkeiser))


Bugzilla 2.18
Bug Flags:
blocking2.18 +


(Whiteboard: [does not affect 2.16.x] [fixed in 2.18rc2])


(1 attachment, 1 obsolete attachment)



14 years ago
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.


In the original patch:

Index: My Application/Component.cpp
RCS file: /home/hipsoft/root/Ware/Apps/My Application/Component.cpp,v

Patch viewer treats this now as a file called:


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:


14 years ago
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
Group: webtools-security
Flags: blocking2.18+
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.)

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.

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.

Comment 6

14 years ago
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

So this one line in Bugzilla itself is the only thing I see that's obviously a

Comment 8

14 years ago
(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?


Comment 10

14 years ago
Comment on attachment 145087 [details] [diff] [review]
Patch v1

This looks fine to me.	Good catch.
Attachment #145087 - Flags: review+

Comment 11

14 years ago
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  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 12

14 years ago
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 to
require version 0.9.4 of PatchReader, which should be available on CPAN later
tonight. (per jkeiser on irc)

Comment 17

14 years ago
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.
Group: webtools-security
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.
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]
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.