576064 - code/install-requirements.pl causes release-notes.html to fail

Status: RESOLVED WONTFIX

(Bugzilla :: Extensions, defect)

Description

[Michael Ching]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-us) AppleWebKit/533.16 (KHTML, like Gecko) Version/5.0 Safari/533.16
Build Identifier: 

In Bugzilla 3.4.x, the _get_extension_requirements method includes the following:
    my @extensions = glob(bz_locations()->{'extensionsdir'} . "/*");
    foreach my $extension (@extensions) {
        my $file = "$extension/code/install-requirements.pl";

The glob results in a tainted @extensions array.  If "code/install-requirements.pl" is present, the subsequent "rdo" will fail if called in taint mode.

"page.cgi?id=release-notes.html" runs in taint mode and calls utilizes this function to generate module requirement lists.  If code/install-requirements.pl is present it will die non-gracefully.

To fix, glob could be replaced with readdir or the resulting output can be untainted:

diff -u -r1.60.2.6 Requirements.pm
--- Requirements.pm	11 Sep 2009 16:57:28 -0000	1.60.2.6
+++ Requirements.pm	30 Jun 2010 19:47:51 -0000
@@ -295,6 +295,7 @@
     my @extensions = glob(bz_locations()->{'extensionsdir'} . "/*");
     foreach my $extension (@extensions) {
         my $file = "$extension/code/install-requirements.pl";
+        ( $file ) = $file =~ /(.*)/;
         if (-e $file) {
             my $safe = new Safe;
             # This is a very liberal Safe.


Reproducible: Always

Steps to Reproduce:
1.  Install Bugzilla 3.4.x
2.  echo "1;" > extensions/example/code/install-requirements.pl
3.  Go to page.cgi?id=release-notes.html
Actual Results:  
undef error - Can't locate object method "splitpath" via package "File::Spec" at /usr/lib/perl5/5.8.8/CGI/Carp.pm line 359.


The error message is misleading because CGI::Carp is failing and the original $@ is not being passed along.

Comment 1

[Max Kanat-Alexander]

  Oh, that sounds like a problem indeed. However, 3.4 is locked to security fixes, and its extension system is most definitely deprecated in favor of Bugzilla 3.6's Extension system. So it is unlikely that this will be fixed. However, if you want to supply a patch, it is indeed just a one-line change, so we could probably take it in case there is a 3.4.8 release in the future.

Comment 2

[Max Kanat-Alexander]

  Why isn't this failing on bugzilla.gnome.org, though? I wrote an extension there that has install-requirements, and they are running 3.4.

Comment 3

[Michael Ching]

Attachment: untaints filename before rdo

Comment 4

[Max Kanat-Alexander]

Thanks! You might want to ask me for review so that I don't forget. Here's info on our development process:

  http://wiki.mozilla.org/Bugzilla:Developers

Comment 5

[Michael Ching]

(In reply to comment #2)
>   Why isn't this failing on bugzilla.gnome.org, though? I wrote an extension
> there that has install-requirements, and they are running 3.4.

I am trying to see why it would not be failing on other systems.  I asked ghendricks at #testopia, and he says it is working for him, so perhaps it is something local to me.

From a theoretical standpoint, it seems like it should fail.  The File::Glob docs say that the output should be expected to be tainted, and Safe->rdo should be subject to taint checks.

Comment 6

[Greg Hendricks]

Interesting point to note that though it does not fail for me, it does not include the extension required or optional modules for Testopia, even though it appears to be getting them dynamically. It is arguable whether they should be included in the release-notes, but that is the only thing I see wrong here.

Comment 7

[Michael Ching]

Perhaps in the landfill install and on bugzilla.gnome.org, something is preventing the glob from seeing the extensions at all when running from the web server (and therefore not running the offending block at all).  We run suexec as the user which owns the Bugzilla directory.

After applying patch, our copy does show the additional Testopia requirements (JSON, Text::Diff, etc.):
http://wush.net/bugzilla/wdemotestopia/page.cgi?id=release-notes.html

It is not that we desire to see the additional requirements on this page so much as avoiding a fatal error if it does detect the extensions.

Comment 8

[Max Kanat-Alexander]

You know, this could somehow just be related to suexec, in some way that I don't understand. We never supported suexec until 3.6, and I know that there were problems surrounding extensions with suexec that will probably be there until 4.0.

Comment 9

[Max Kanat-Alexander]

Ultimately, I'm going to have to say that this is WONTFIX, since the 3.4 branch is locked to sec fixes only, and we can't reproduce the problem locally.