140784 - edit*.cgi need a use lib "." on Win32/IIS w/ taint

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Jouni Heikniemi]

At least on W2k ActiveState default installation the edit*.cgi don't work,
because  they cannot find the required 'cgi.pl'. 

I'll attach a patch which adds 'use lib "."' to all the edit*.cgi files. This is
the solution used in the other cgis as well.

Comment 1

[Jouni Heikniemi]

Attachment: Patch proposal v1

Comment 2

[Bradley Baetz (:bbaetz)]

Why is this needed? Those templates don't run in taint mode, so '.' should be
part of the default lib path.

Comment 3

[Jouni Heikniemi]

Oh yeah, I must've been really tired when writing that bug report. It's a much
clearer one now that I've slept overnight. ;-)

The bug is caused by a particular Microsoft IIS web server configuration and
ActiveState Perl interpreter. In order to run CGIs in taint mode through IIS,
you have to hardcode -T switch into the association command line configured into
the IIS properties. This is needed for the user-wise cgi:s, but since the
edit*.cgis reside in the same directory, they get the -T tagged on them too, and
thus are run in taint mode. Shebang lines won't make much difference there. :-(

So, while technically this isn't a Bugzilla bug, my fix proposal seems harmless
security-wise and makes the Win32-IIS-combination much easier to use.
Resummarized for the specifics noted.

Comment 4

[Bradley Baetz (:bbaetz)]

Ah. mod_perl has the same restriction.

Does the edit* stuff run in taint mode, though? Hopefully there are no security
faults, but there may be stuff which we check, but don't explicitly detaint -
that was the problem in the majority of the user-facing scripts.

The patch won't hurt, but when I did the taint stuff we decided that 2.16 would
only do the user-facing pages, and that it would be done as part of rewriting
the edit* stuff for 2.18.

If they work, then we should add the -T option so that everyone gets the
benefit, although thats risky at this stage of the release process.

Comment 5

[Jouni Heikniemi]

I had no problems with the edit* code even after applying the patch I attached
and setting IIS to use a fixed -T parameter, but I did not check every possible
alternative way of breaking them. 

Your proposal of adding the -T to shebang lines sounds sensible, if the
edit*cgis really work 100%. But regardless of that, I think the use lib fix
should be checked in separately. It has no harmful effect but benefits IIS and
mod_perl users, and making admin cgis taint-checked is really a separate issue
(is there already a bug on this? couldn't find one). Besides, 2.16 doesn't
really need the potential unstability caused by taint issues. 

Nevertheless, unless you're going to do something about it, I can check out
whether there are any hiding taint problems in edit* files. Hopefully later today.

Comment 6

[Matthew Tuck [:CodeMachine]]

I would be suprised if all of edit*.cgi work with taint as is.  I suppose use
lib . could be done for 2.16 but I don't think we should be adding taint to the
shebang line without a fair bit of testing.

Comment 7

[Bradley Baetz (:bbaetz)]

Comment on attachment 81419 [details] [diff] [review]
Patch proposal v1

r=bbaetz. This patch can't hurt.

Theres very little validation to be done, though, so if everywhere does use
SendSQL (and for stuff like usernames, which can have spaces, we sould have
noticed if this wasn't done), I wouldn't be surprised if it mostly worked.

Maybe we'll just have to document that due to limitations in the way IIS and
Perl interacts, taint mode can't be enabled on windows?

Comment 8

[Jouni Heikniemi]

A more appropriate documentation note might be "Use taint mode under Windows and
IIS at your own risk; some of the administrative pages may function incorrectly."

The problem with leaving taint mode off is that you will have to edit all the
user-wise cgis and remove the -Ts from their shebang lines. So it's source
editing even that way. At least my current experience says that using taint is
easier than not, especially after this patch is checked in. And additionally
there's of course the safety benefit.

Comment 9

[Dave Miller [:justdave]]

Perl on Win32 *does* read the shebang line for interpreter flags *at runtime*. 
-T can't be turned on that way due to the way it works though.  That one has to
be done on the command-line or it won't work at all.  (Ever seen the "Too late
for -T" error when running "perl -cw" instead of "perl -cWT" on a script with a
-T on the shebang line?)  But it wouldn't surprise me if a -T on the
command-line was disabled by the lack of one on the shebang line on Win32.  I
will investigate later today when I have access to a Win32 machine to try it out on.

Comment 10

[Jouni Heikniemi]

>But it wouldn't surprise me if a -T on the command-line was disabled by 
>the lack of one on the shebang line on Win32.  

Now that would surprise _me_ ;-) 

I filed this bug because my commandline -T also affected the edit*.cgi:s which 
certainly do not have -T's on their shebang lines. But I'll be happy to be 
corrected if I'm wrong; hopefully someone can then point out (and document) 
what one has to do to make both tainted and untainted files work with the same 
command-line parameters.

Might there be difference between Perl implementations? My results were based 
on a couple-of-months-old ActiveState Perl binary.

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 81419 [details] [diff] [review]
Patch proposal v1

r=gerv.

Gerv

Comment 12

[Gervase Markham [:gerv]]

Fixed. Thanks again :-)

Checking in editattachstatuses.cgi;
/cvsroot/mozilla/webtools/bugzilla/editattachstatuses.cgi,v  <-- 
editattachstatuses.cgi
new revision: 1.7; previous revision: 1.6
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <-- 
editcomponents.cginew revision: 1.22; previous revision: 1.21
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.16; previous revision: 1.15
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.8; previous revision: 1.7
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <-- 
editmilestones.cginew revision: 1.8; previous revision: 1.7
done
Checking in editparams.cgi;
/cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v  <--  editparams.cgi
new revision: 1.14; previous revision: 1.13
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.24; previous revision: 1.23
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.34; previous revision: 1.33
done
Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.11; previous revision: 1.10
done

Gerv

Comment 13

[Jouni Heikniemi]

I'll still add that I quickly went through all the edit files and found no taint
problems. The rest of -T stuff _should_ thus be trivial...

I couldn't find an existing bug on tainting of admin cgis, so I filed a new one,
bug 141006.

Comment 14

[Dave Miller [:justdave]]

and just as a followup, no, a missing -T on the shebang line does not disable
taint on Win32.