204042 - internal error after adding an attachment

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P1)

Description

[TaeYoung Oh]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; Avant Browser [avantbrowser.com])
Build Identifier: 

I got an error after adding an attachment. The error is as below.

undef error - Insecure dependency in parameter 1 of DBI::db=HASH(0x63af0c)-
>prepare method call while running with -T switch at Bugzilla/DB.pm line 64. 


Reproducible: Always

Steps to Reproduce:

Comment 1

[TaeYoung Oh]

I got this error after updating my existing installation of 2.17.3. (never seen 
this error in 2.17.3)
It still occurs in a fresh installation under osx server 10.2.5.

Perl version: 5.6.0
DBI version: 1.3.2

Comment 2

[Bradley Baetz (:bbaetz)]

dave has osx

I'm guessing a 5.6.0-ism somewhere, but I'm not sure where. What is $str at the
specified line when this happens?

Comment 3

[Dave Miller [:justdave]]

Can you give more complete steps to reproduce?  Do you need to upload a specific
type of file?  Patch, non-patch?  auto-select filetype or pick one?  did you
obsolete an old patch? is "take bug" checked?  Did you add a comment at the same
time?  Does any of the above matter, or can you reproduce with any combination?

I'm unable to reproduce this on my test setup with Mac OS X 10.2.5 and Perl
5.6.0, with a fresh Bugzilla setup from the 2.17.4 tarball.

Comment 4

[TaeYoung Oh]

Here're more on my configuration.

- MySQL 3.23.56-1 installed via Fink
- Bugzill 2.17-4  tarball

Steps to reproduce
1. installed bugzilla tarball with a fresh database
2. add a bug #1
3. add an attachment as a non-patch with auto-detect (commented/uncommented)
4. add an attachment as a non-patch with from-the-list (comment/uncommented)

Result
- Attachment was added thoush it showed an internal error message after 
uploading
- Not filetype-specific
- No error when used "Patch" option

Comment 5

[Dave Miller [:justdave]]

I still can't duplicate this.  The only difference I can tell in my config is
that I'm using the standard workstation version of OS X rather than OS X Server...

I also have MySQL 3.23.52-entropy.ch installed via .pkg, but that shouldn't
matter at all, since the DBI version is the same.

Can you do as Bradley suggests in comment 2 and put in something like this:
warn "str = $str";
just before the line where the error occurs, and see what shows up in the log
for it?

Comment 6

[TaeYoung Oh]

Attachment: log after adding warn "str = $str";

Here's the log for $str.

Comment 7

[Dave Miller [:justdave]]

I don't see the dependency error in that log...   which one of those does the
dependency error show up after?

Comment 8

[Bradley Baetz (:bbaetz)]

Rather than the warn, in Bugzila/DB.pm, in |sub _connect|, after the call to
DBI->connect, add the line:

DBI->trace(2, 'trace.log');

then load the page with the error, then attach the 'trace.log' file.

Comment 9

[TaeYoung Oh]

Attachment: log after adding DBI->trace(2, 'trace.log');

I couldn't find the trace.log. I got permission error after adding the line
below.

    DBI->trace(2, 'trace.log');

Comment 10

[Dave Miller [:justdave]]

Comment on attachment 122423 [details]
log after adding DBI->trace(2, 'trace.log');

ok, webserver just didn't have access to create a file there.  that's okay, it
falls back on stderr, which goes to the error_log anyway, so you got it.

Did you actually get the error that time?  I don't see any evidence of an error
in that log...

Comment 11

[TaeYoung Oh]

message for Attachment 122423 [details] is here;

undef error - Insecure dependency in parameter 1 of DBI::db=HASH(0x61f0ac)-
>prepare method call while running with -T switch at Bugzilla/DB.pm line 65. 

I was able to reproduce this bug in a separate OSX client (not server) machine with mysql installed 
using Fink.

Do you need logs when using "is patch" which showed no error after attaching?

Comment 12

[Dave Miller [:justdave]]

*** Bug 218750 has been marked as a duplicate of this bug. ***

Comment 13

[Curtis Donato]

Attachment: DBI trace captured while encountering the internal error described above.

Comment 14

[Simon Wilkinson]

We're seeing the same problem here, with MySQL and perl 5.6.0 under Linux.

I can reproduce it 100% of the time, and believe that I've tracked down the problem.

Doing warn $str if is_tainted($str) reveals the tainted SQL call to be:

SELECT alias,assigned_to,bug_file_loc,bug_severity,bug_status,
cclist_accessible,component_id,estimated_time,everconfirmed,keywords,
op_sys,priority,product_id,qa_contact,remaining_time,rep_platform,reporter,
reporter_accessible,resolution,short_desc,status_whiteboard,target_milestone,
version,votes, lastdiffed, now() FROM bugs WHERE bug_id = 49

This appears to be comming from the call to SendBugMail in 
bug/process/bugmail.html.tmpl, which is called from the attachment created template.

Checking in ProcessOneBug reveals that the $id variable is tainted, and 
adding a trick_taint($id) makes the problem go away.

However, as this code works fine when called from other locations, there must 
be something more going on. Is something in the template code tainting the variable?

Comment 15

[Simon Wilkinson]

We've now upgraded to RH9 here, and the problem seems to have resolved itself.
Looks like there's something changed in the way in which perl marks variables as
being tainted that's changed between 5.6 and 5.8

Comment 16

[Max Kanat-Alexander]

Attachment: detaint_natural $id in that function

If that's actually the issue, then this rather simple patch against today's
cvs-tip should fix this issue quite harmlessly, though this may not be the best
place to do this detaint_natural.

Does applying this patch fix the issue?

-M

Comment 17

[Dave Miller [:justdave]]

Max: cvs diff -u please :)

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 139472 [details] [diff] [review]
detaint_natural $id in that function

The return value of detaint_natural is a true/false whether it detainted
successfully or not.  (not the detainted value).  The value is detainted in
place.

You need to do something like this:

my ($id) = @_;
detaint_natural($id) || ThrowUserError("some_error_code");

Comment 19

[Max Kanat-Alexander]

Attachment: detaint_natural, v2

Okay, let's try this again. :-)

-M

Comment 20

[Dave Miller [:justdave]]

Comment on attachment 139479 [details] [diff] [review]
detaint_natural, v2

only one problem with this...  (one other behavior I forgot to mention...)

If detaint_natural fails, $id is going to be undefined.  If you want to pass
the untainted version to the error template, you'll need to copy it to another
variable first and then pass that to the template.

Comment 21

[Bradley Baetz (:bbaetz)]

Comment on attachment 139479 [details] [diff] [review]
detaint_natural, v2

I don't think we want this; instead we should work out where $id got tainted,
and why.

Comment 22

[Dave Miller [:justdave]]

(In reply to comment #21)
> I don't think we want this; instead we should work out where $id got tainted,
> and why.

It's passing in $::FORM{bug_id}.  However, it called ValidateBugID on it
previously, which detaints it.  Isn't this the "if anything in the hash is
tainted, then the entire hash is tainted" bug?  It's never removed from %::FORM
anywhere, and it detaints it in place, still in %::FORM.  There's probably
something else in %::FORM that's still tainted.

I have a hunch that bug 238870 is going to fix this as a side-effect.

Comment 23

[Dave Miller [:justdave]]

Attachment: detaint closer to the source

Comment 24

[Bradley Baetz (:bbaetz)]

Can we require just perl5.6.1?

Comment 25

[Dave Miller [:justdave]]

That's very tempting...  that's very very tempting...  Mac OS X was the last
major holdout among the "well known distributions" that still had 5.6.0 shipping
with the OS.  They've been shipping 5.8.0 since 10.3 came out.  There's a lot of
folks who haven't upgraded, but there are Perl 5.8.0 packages readily available
from third parties for older OS X's.

Comment 26

[Joel Peshkin]

Comment on attachment 146333 [details] [diff] [review]
detaint closer to the source

r=joel
Strange that the detaint within ValidateBugID doesn't cover this already,
though.

Comment 27

[Dave Miller [:justdave]]

attachment 146333 [details] [diff] [review] checked in.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.56; previous revision: 1.55
done

We *will* bump the minimum Perl requirement on the trunk shortly after 2.18
branches.

Comment 28

[Marc Schumann [:Wurblzap]]

Comment on attachment 146333 [details] [diff] [review]
detaint closer to the source

Removing r? from FIXED bug.