288483 - Internal error on creating attachment. (Tainted)

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Jan Ruzicka]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: 2.18

Each time I was adding an attachment the result page was showing an error.


Reproducible: Always

Steps to Reproduce:
1. select a bug
2. click on "Create a New Attachment"
3. select a file
4. fill in Description
5. select Content Type
6. click submit

Actual Results:  
a page containing error message.

Expected Results:  
a page containing message about attachment successful submission.

This bug is similar to the bug 204042.
It is in the same subroutine, insert.

Also this bug behaves strange as the attachment is actually inserted into the
database.

Comment 1

[Jan Ruzicka]

Attachment: patch for attachment.cgi

using the same change as bug 204042.

Comment 2

[Marc Schumann [:Wurblzap]]

Comment on attachment 179190 [details] [diff] [review]
patch for attachment.cgi

Asking for review for you :)

Comment 3

[Jan Ruzicka]

Attachment: a stack trace

The stack trace was done by adding following change to the attachment.cgi
----------------
 # Include the Bugzilla CGI and general utility library.
 require "CGI.pl";
 
+use Carp qw/confess cluck/;
+$SIG{__WARN__} = \&cluck;
+$SIG{__DIE__} = \&confess;
+
 # Use these modules to handle flags.
 use Bugzilla::Constants;
 use Bugzilla::Flag;
----------------

Comment 4

[GavinS]

Comment on attachment 179190 [details] [diff] [review]
patch for attachment.cgi

A few issues: 

detaint_natural() is for numbers, trick_taint() would be used for a string, I
think.

Also, we only detaint_natural() the bugid because we have validated it earlier.
We have not (yet) validated the contenttypemethod, so it may not be safe to do
the trick_taint(). We can either add the trick_taint() in place of your
detaint_natural() anyway (the quick fix), or I would suggest maybe setting
$FORM{contenttypemethod} to a knownn, untainted value in the validateispatch()
and validatecontenttype() routines?

Comment 5

[Jan Ruzicka]

(In reply to comment #4)
> (From update of attachment 179190 [details] [diff] [review] [edit])
> A few issues: 
> 
> detaint_natural() is for numbers, trick_taint() would be used for a string, I
> think.
> 
> Also, we only detaint_natural() the bugid because we have validated it earlier.

> We have not (yet) validated the contenttypemethod, so it may not be safe to do
> the trick_taint(). We can either add the trick_taint() in place of your
> detaint_natural() anyway (the quick fix), or I would suggest maybe setting
> $FORM{contenttypemethod} to a knownn, untainted value in the validateispatch()
> and validatecontenttype() routines?

it was actually validated earlier validateContentType() is called before the
insert subroutine.

    118   Bugzilla->login(LOGIN_REQUIRED);
    119   ValidateBugID($::FORM{'bugid'});
    120   validateCanChangeBug($::FORM{'bugid'});
    121   ValidateComment($::FORM{'comment'});
    122   validateFilename();
    123   validateIsPatch();
    124   my $data = validateData();
    125   validateDescription();
    126   validateContentType() unless $::FORM{'ispatch'};
    127   validateObsolete() if $::FORM{'obsolete'};
    128   insert($data);

Comment 6

[Jan Ruzicka]

Attachment: patch for attachment.cgi #2

detaint_natural() changed to trick_taint() also added same treetment for
'contenttypemethod'.

Comment 7

[Joel Peshkin]

I don't see this problem on the tip.

Comment 8

[Jan Ruzicka]

My suspicion is that it is because the Perl 5.6.0 way of handling tainted data.
The installation in question can't be updated to the latest Perl.
See discussion for bug 204042.

Comment 9

[GavinS]

I also do not see this on the tip, or on 2.18. 

Jan, what version of OSX are you on? Is it running perl 5.6.0 which caused the
problems before?

I'm also confused that detainting the contenttypemethod fixes the problem,
because your trace points to something else. The trace looks like it has a
problem with a tainted bugid when sending mail after the attachment is created.
I cannot see anywhere where the contenttypemethod is used where it would need
detainting. Can anyone enlighten me?

Comment 10

[Jan Ruzicka]

I am runing on the OS X server 10.2.something.
I suspect that a tainted element in a hash makes the whole hash tainted. 

Comment 11

[Dave Miller [:justdave]]

(In reply to comment #10)
> I am runing on the OS X server 10.2.something.
> I suspect that a tainted element in a hash makes the whole hash tainted. 

Bingo.

This is a Perl 5.6.0-only problem, which makes this 2.18-branch only, because we
no longer support Perl 5.6.0 on the tip.

Comment 12

[Frédéric Buclin]

Comment on attachment 179223 [details] [diff] [review]
patch for attachment.cgi  #2

runtests.sh fails. Moreover, $contenttype is already defined as
SqlQuote($::FORM{'contenttype'}) so maybe the code could be simplified a bit.

Comment 13

[Jan Ruzicka]

Attachment: patch for attachment.cgi #3

Fixed the $contenttype redefinition as pointed out by Frédéric Buclin.

I am not sure if the if the SqlQuote($::FORM{'contenttype'}) is appropriate for

 $template->process(), so that part remains the same.

Comment 14

[Frédéric Buclin]

Comment on attachment 179841 [details] [diff] [review]
patch for attachment.cgi #3

I'm too busy right now to check this.

Comment 15

[GavinS]

(In reply to comment #13)
> I am not sure if the if the SqlQuote($::FORM{'contenttype'}) is appropriate for
> 
>  $template->process(), so that part remains the same.

Neither contenttype or contenttypemethod fields are used in the process() call,
so it it is fine as it is now.

Comment 16

[GavinS]

Comment on attachment 179841 [details] [diff] [review]
patch for attachment.cgi #3

Yes, this is better. runtests is OK now. (sorry for only
reviewing-by-inspecting before :(

Comment 17

[Frédéric Buclin]

(In reply to comment #15)
> Neither contenttype or contenttypemethod fields are used in the process() call,
> so it it is fine as it is now.

GavinS, so what do you mean? Does it hurt if:

$vars->{'contenttype'} = SqlQuote($::FORM{'contenttype'}); ?

If yes, then remove your r+ and read my comment 12. Else keep your r+. ;)

Comment 18

[GavinS]

(In reply to comment #17)
> (In reply to comment #15)
> > Neither contenttype or contenttypemethod fields are used in the process() call,
> > so it it is fine as it is now.
> 
> GavinS, so what do you mean? Does it hurt if:
> 
> $vars->{'contenttype'} = SqlQuote($::FORM{'contenttype'}); ?
> 
> If yes, then remove your r+ and read my comment 12. Else keep your r+. ;)
> 

Just to re-cap, I think that this patch (now the runtests.sh issue is fixed)
just trick_taint()s the 2 values we need it to, and does not add or remove any
SQLQuoting().

[When I said 'not used', I was not 100% accurate, sorry :( They _are_ used, but
only to display a value to the user, not for any other purpose.]

My understanding is that it would not break anything if $vars->{'contenttype'}
was SQLQuoted. This is because the contenttype value in %FORM is only used in
the template process() if we autodetected the attachment type, and then only
used to display the auto-detected value to the user.

Strictly speaking, I think it would probably be better to display the
autodetected value rather than the SQLQuoted() version. (Which is what we did
before, and still do now anyway)
 
I'm fine with asking someone else for a 2nd review, if you want...

Comment 19

[Dave Miller [:justdave]]

Comment on attachment 179841 [details] [diff] [review]
patch for attachment.cgi #3

Looks good to me.

I note some comments about "we can remove this is we require 5.6.1 or newer",
maybe we should do that on the tip (but it's probably another bug)

Comment 20

[Max Kanat-Alexander]

Comment on attachment 179841 [details] [diff] [review]
patch for attachment.cgi #3

>+  my $contenttypemethod = $::FORM{'contenttypemethod'};
>+  trick_taint($contenttypemethod); # Same Perl 5.6.0 hack as above
>+  $contenttype = $::FORM{'contenttype'};

  You know, I don't know why we're re-using the $contenttype variable here, at
all. It's used all over the place in this file, and I don't have any idea what
the consequences of modifying it here are. Couldn't we just make a
$form_content_type variable, to be safe? After all, it's just a hack for the
branch.

Comment 21

[Max Kanat-Alexander]

(In reply to comment #20)
>   You know, I don't know why we're re-using the $contenttype variable here 
> [snip]

  Oh, nevermind this comment. :-) Everything is fine, the block ends right after
that. :-)

Comment 22

[Max Kanat-Alexander]

2.18:

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