Closed Bug 204042 Opened 17 years ago Closed 16 years ago

internal error after adding an attachment

Categories

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

2.17.4

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: spier, Assigned: justdave)

References

Details

Attachments

(4 files, 2 obsolete files)

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:
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
Severity: normal → major
OS: Windows XP → MacOS X
Version: unspecified → 2.17.4
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?
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.
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
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?
Here's the log for $str.
Attachment #122408 - Attachment mime type: application/octet-stream → text/plain
I don't see the dependency error in that log...   which one of those does the
dependency error show up after?
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.
I couldn't find the trace.log. I got permission error after adding the line
below.

    DBI->trace(2, 'trace.log');
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...
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?
*** Bug 218750 has been marked as a duplicate of this bug. ***
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?
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
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
Max: cvs diff -u please :)
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");
Attachment #139472 - Flags: review-
Attached patch detaint_natural, v2 (obsolete) — Splinter Review
Okay, let's try this again. :-)

-M
Attachment #139472 - Attachment is obsolete: true
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.
Attachment #139479 - Flags: review-
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.18
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking2.18+
(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.
Depends on: 238870
OS: MacOS X → All
Hardware: PC → All
Assignee: myk → justdave
Attachment #139479 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #146333 - Flags: review?(bbaetz)
Can we require just perl5.6.1?
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 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.
Attachment #146333 - Flags: review+
Flags: approval?
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: approval? → approval+
Resolution: --- → FIXED
No longer depends on: 238870
Comment on attachment 146333 [details] [diff] [review]
detaint closer to the source

Removing r? from FIXED bug.
Attachment #146333 - Flags: review?(bbaetz)
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.