Closed Bug 160710 Opened 22 years ago Closed 22 years ago

Taint checking causes problem with rename function

Categories

(Bugzilla :: Bugzilla-General, defect)

Product:
Bugzilla
Component:
Bugzilla-General
Version:
2.16
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.18

People

(Reporter: jim.silverstein, Assigned: bbaetz)

References

Details

(Keywords: regression, Whiteboard: [fixed in 2.16.1])

Attachments

(1 file, 1 obsolete file)

take 2
1.99 KB, patch
bugreport
: review+
preed
: review+
Details | Diff | Splinter Review 
It seems that when userprefs.cgi has taint checking on, we get the following 
error, line 287 of Document.pm is:

    return rename($tmpfile, $file)
        || $class->error($!);


Bugzilla has suffered an internal error. Please save this page and send it to 
jim.silverstein@bear.com with details of what you were doing at the time this 
message appeared. 

URL: http://chicago1/bugzilla/userprefs.cgi

Template->process() failed twice.
First error: [Fri Aug 2 11:40:23 2002] userprefs.cgi: file error - [Fri Aug 2 
11:40:23 2002] userprefs.cgi: undef error - [Fri Aug 2 11:40:23 2002] 
userprefs.cgi: undef error - [Fri Aug 2 11:40:23 2002] userprefs.cgi: Insecure 
dependency in rename while running with -T switch 
at /usr/local/lib/perl5/site_perl/5.005/sun4-solaris/Template/Document.pm line 
287. 
Second error: [Fri Aug 2 11:40:24 2002] userprefs.cgi: Insecure dependency in 
rename while running with -T switch 
at /usr/local/lib/perl5/site_perl/5.005/sun4-solaris/Template/Document.pm line 
287.
What version of perl is this? Is this just from loading the page?
This is perl, version 5.005_03 built for sun4-solaris

Yes, just loading the page.
OK, this is because of
http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/userprefs.cgi#371

The FORM element is tainted, so the rename fails (I suspect that rename on 5.005
uses csh, but a builtin in 5.6)

I'll see if I can find time to hack this up tonight (and also display an error
page if an invalid tab is selected)
Keywords: regression
Whiteboard: want for 2.16.1
Target Milestone: --- → Bugzilla 2.18
*** Bug 161111 has been marked as a duplicate of this bug. ***
Attached patch v1 (obsolete) — — Splinter Review 
OK, this should work. I don't have 5.005 to test on, though.
I applied Bradley's patch but it doesn't work for me, 
I got the same error. (I reported 161111 (duplicate))
Sigh. Can you print the values of $file and $tmpfile just before it fails?
I got there twice:
1st time
TMPFILE (data/template/en/default/account/prefs/wQUOXoXCRZ)
FILE(data/template/en/default/account/prefs/account.html.tmpl)
TMPFILE (data/template/en/default/global/VOS64siUvw)
FILE(data/template/en/default/global/code-error.html.tmpl) 
Can you also try changing the PROCESS at the bottom of
template/en/default/account/prefs/prefs.html.tmpl to use ${current_tab_name}
instead of ${current_tab.name} ?
There are 3 places where current_tab.name is used
and one with current_tab_name - changing it to
current_tab.name makes it process ok but it's not
a solution because current_tab_name is set
(current_tab.name is not defined). 

So - if I change all of current_tab_name 
to something undefined (like current_tab.name)
it doesn't crash. 

btw, I can print like current_tab_name with
[% like current_tab.name %] (it's defined an 
contains proper value).

Right, but if you change *that specific* PROCESS (line 401) *TO*
${current_tab_name} (after applying the patch) does that fix the problem?
There's no line 401 in my template, I guess
you mean 101 - but PROCESS is in 103 
http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/template/en/default/account/prefs/prefs.html.tmpl#103

Anyway - I tried all possibilities without luck.

http://landfill.bugzilla.org/bbaetz/userprefs.cgi is running 5.005_03 (only
userprefs.cgi)

I don't see any problems.
Bradley, what version od Document.pm do you have?
I have
$Id: Document.pm,v 2.56 2002/07/30 12:44:56 abw Exp $

It seems to be other problem, following code causes the same problem:
[% current_tab_name_x = "account" %]
[% PROCESS "account/prefs/${current_tab_name_x}.html.tmpl" %] 
Hmm. 2.49 - this is running TT 2.07, and I assume you're running 2.08?

I have no problems with TT 2.08 on perl 5.6, but I don't have TT2.08 on the
machine which has perl 5.005. I'll see if I can get justdave to update
landfill's copy.

This is only userprefs which fails, right? Other files work without problems?
Only userprefs on my system.
I'm not sure if I checked all the functionality but 
everything else works ok.
OK, I can repro this now.
Hmm.

I'm not even sure why are trying to recreate the files in the first place (after
applying this patch, that is)
Bleh. OK, this is perl5.005's trick of considering the entire hash to be tainted
if any element in it is.

In this case, the user_agent is an ENV variable, so its tainted. This is only
used for the simple bugzilla entry thing; we could cheat, and localise that to
enter_bug.cgi (as well as apply this patch)

gerv: Would that work for you?
I don't understand how the issue reported relates to user_agent or to your last
comment.

Gerv
gerv:

1. %ENV is tainted
2. $ENV{HTTP_USER_AGENT} is added to the vars hash
3. under perl5.005, by the time that goes through TT, all of %vars is tainted
4. In userprefs.cgi, TT PROCESSes a template based on the contents of a template
variable
5. (4) means that TT tries to |require| that, which fails taint checks.
6. So it tries to recompile it, which used to work, but now fails because TT2.08
uses rename - ie it tries to effectivly overwrite a filename given in a tainted
string
7. TT dies

If we remove the user agent from the hash, then %vars isn't tainted, and it
works. Yes, this is ugly, but since it works in 5.6....

See ~bbaetz/taintTest/test.pl on landfill for the test I hacked up last time
this happened.
Right. Thanks :-)

Does this mean that in 2.16 TT always recompiles userprefs.cgi? That's a bit pants.

user_agent is used in several places, and is planned to be used in several more.
Also, it's a very good tool for admins who want to add extra function for only
certain agents. Reducing its availability is not the solution. We should detaint
it before we add it to $vars.

Gerv


Interestingly, it doesn't appear to be recompiled (at least, teh ctime doesn't
change) I'm not sure why - I need to look into this more.
Bleh - some of my landfill tests were done with 5.6.1 instead. Oops.

In any event, this patch + commenting out the user agent string does definately
work.

Since there is no user of the user_agent param in stock 2.16, and this doesm't
look easy (or, in fact, possible) to fix, I wonder if maybe we should just
require 5.6.0 for bugzilla 2.18, and remove the var from 2.16's hash. The
alternative is to try to trick_taint from within the template, but thats not
only ugly, it may not work if the then-detainted var is put back into the
tainted hash...

Upping the perl req is something I wanted us to consider, but I don't know if
this bug justifies it.

I would prefer not to use IF statements with separate PROCESSes for each tab,
but since there are only four of them, I think we can live with it.

If anyone has a better solution for this, then let me know. The main problem
with that is that no developers run 5.005, and so this sort of thing wouldn't be
picked up, and could easily happen again.
Ok, I commented out user_agent in globals.cgi and now
preferences page works ok.

One more question:

# grep -r user_agent *
enter_bug.cgi:$vars->{'user_agent'} = $ENV{'HTTP_USER_AGENT'};
globals.pl:    #'user_agent' => $ENV{'HTTP_USER_AGENT'} ,

what about the assignment in enter_bug.cgi?
Enter_bug doesn't load another template via a variable, so its ok.
Attached patch take 2 — — Splinter Review 
I can no longer reproduce this on landfill, even without any patches applied.
No idea why.

Anyway, see if this fixes it for you.
Attachment #94144 - Attachment is obsolete: true
Comment on attachment 94801 [details] [diff] [review]
take 2

This patch works fine for me! Great job, bbaetz :)
OK, taking
Assignee: justdave → bbaetz
Keywords: patch, review
Comment on attachment 94801 [details] [diff] [review]
take 2

This patch is using tick_taint and then using good care in what is subsequently
done with the data.

While I won't try to make policy in the middle of an r=, I am left a bit uneasy
about using trick_taint epecially when the value will then be passed to a
template which is potentially customized.

While userprefs is probably less prone to customization, we probably should add
a detaint_alphanum function for situations like this before someone goofs in
the future.
Attachment #94801 - Flags: review+
Comment on attachment 94801 [details] [diff] [review]
take 2

Hehe... good call Joel. I was also going to ask bbaetz about the use of
trick_taint here, which makes me a bit uneasy as well. But I guess, if it's
verified by something (the switch statement below), we're super-careful, and it
fixes the problem for those silly Win32 folks ;-), then r=preed.
Attachment #94801 - Flags: review+
Applied to trunk and 2.16 branch.

preed - its perl5.005, not win32, which has the problem with this one...
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: want for 2.16.1 → fixed in 2.16.1
Whiteboard: fixed in 2.16.1 → [fixed in 2.16.1]
*** Bug 167154 has been marked as a duplicate of this bug. ***
OS: Windows 2000 → All
Hardware: PC → All
bbaetz: can we revert this change now that we support only 5.6 and above?

Gerv
I guess we could. We should keep the ThrowUserError in userprefs.cgi, though.
I had the same trouble with our Bugzilla moved to a FreeBSD 4.8 System few days 
before. Per default there is a really old version of Perl installed (5.005).

After I installed Perl 5.6.1 from ports, the problem disappeared. 
It was necessary to create a symlink to the new version of interpreter:
ln -s /usr/local/bin/perl5.6.1 /usr/bin/perl

Maybe, this helps.

With best regards,
Juri
I had the same trouble with our Bugzilla moved to a FreeBSD 4.8 System few days 
before. Per default there is a really old version of Perl installed (5.005).

After I installed Perl 5.6.1 from ports, the problem disappeared. 
It was necessary to create a symlink to the new version of interpreter:
ln -s /usr/local/bin/perl5.6.1 /usr/bin/perl

Maybe, this helps.

With best regards,
Juri
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: