509794 - Crash if setting a flag with Unicode characters in the name

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Pedro Zorzenon Neto]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; pt-BR; rv:1.9.0.13) Gecko/2009080315 Ubuntu/9.04 (jaunty) Firefox/3.0.5, Ant.com Toolbar 1.1
Build Identifier: Bugzilla 3.4.1

bugzilla crashes with the following message:

Software error: Insecure dependency in parameter 8 of DBI::db=HASH(0x92d3624)->do method call while running with -T switch at Bugzilla/Flag.pm line 636, <fh00001163_r5.diff> line 1.   For help, please send mail to the webmaster

the conditions to trigger the bug are: in the "new attachment window"
 - chose a file
 - turn on "is a patch"
 - turn on "obsoletes" of some previous file
 - set the flag "revision" to "?" and select an username to review the patch
 - write a comment
if you do ALL this at the same time, it triggers the bug.

if I do in some steps (add the attach, then enter it to edit the revision, obsolete, ...) it does not trigger the bug.


Reproducible: Always

Comment 1

[Max Kanat-Alexander]

Does your flag's name have any Unicode characters or a Unicode space/hyphen in it?

Comment 2

[Max Kanat-Alexander]

Also, can you reproduce this at http://landfill.bugzilla.org/bugzilla-3.4-branch/ ?

Comment 3

[Pedro Zorzenon Neto]

could not reproduce at landfill.
my 3.4.1 was downloaded and I applied patches to translation.
do you think that could be triggered by the template patches?

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> could not reproduce at landfill.
> my 3.4.1 was downloaded and I applied patches to translation.
> do you think that could be triggered by the template patches?

  Yeah, that could be. Do you mean that you actually modified the templates, or that you just installed a publicly-available localization pack?

Comment 5

[Pedro Zorzenon Neto]

there is no public-available template for my language. I did it by myself.

yes, my flag has unicode chars in its name.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> yes, my flag has unicode chars in its name.

  Okay, I suspect that that is the problem, having seen this in other places and having resolved it by removing the Unicode characters.

  We should investigate why trick_taint isn't detainting utf8 flag names, or why in the world DBI would think that they were tainted to begin with.

Comment 7

[Pedro Zorzenon Neto]

what is weird, is that I can set the flag in other conditions.

just setting the flag is OK.

setting the flag + add comments at the same time, triggers the bug.

----

I entered in the definition of the flag and removed all unicode chars from its name and from its description.

Now bugzilla works well in the same conditions. no errors anymore.

Comment 8

[Frédéric Buclin]

(In reply to comment #0)
> Software error: Insecure dependency in parameter 8 of
> DBI::db=HASH(0x92d3624)->do method call while running with -T switch at
> Bugzilla/Flag.pm line 636

This is:

 $dbh->do('INSERT INTO bugs_activity
           (bug_id, attach_id, who, bug_when, fieldid, removed, added)
           VALUES (?, ?, ?, ?, ?, ?, ?)',
           undef, ($bug_id, $attach_id, Bugzilla->user->id,
           $timestamp, $field_id, $removed, $added));

So I guess param 8 is $added, which is considered tainted.

Comment 9

[Frédéric Buclin]

OK, I can reproduce the bug locally in 3.4.1 under the following conditions:

1. The flag must have Unicode in it;
2. You must set the flag to "?" + have a requestee;
3. Add a comment (nothing special in it, pure ASCII is enough).

You can also trigger the same error if you replace 2. by:

2'. The flag was set to "?" with a requestee, and you set the flag to "+", "-" or cancel it.


The easiest workaround is probably to trick_taint() $removed and $added before passing them to DBI. I don't like it very much as it shouldn't be tainted, but it's safe as we use placeholders.

Comment 10

[Frédéric Buclin]

I found what taints data:

foreach (@$removed, @$added) { s/^[^:]+:// } in Flag.pm at line 633.

Right before this line, nothing is tainted (as expected). Right after this line, data having Unicode + requestee is tainted.

Comment 11

[Frédéric Buclin]

I tried to write a reduced testcase, but the string isn't tainted after the regexp.

Comment 12

[Max Kanat-Alexander]

That sounds like a bug in Perl, to me.

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
> That sounds like a bug in Perl, to me.

No doubt about that. But without a reduced testcase, there is no chance to report the issue upstream. :(

For now, I will simply call trick_taint().

Comment 14

[Bradley Baetz (:bbaetz)]

I've tried reducing this (on landfill). (Below is a braindump)

* The comment has to be there.
* Whats triggering this is the creation of the notification mail to the requestee (note - not sending the mail) (Bugzilla::Flag::notify)
* The minimal template to trigger this is:
  [% USE Bugzilla %]
[% IF Bugzilla.cgi.param("comment") && Bugzilla.cgi.param("comment").length > 0 %]
[% END %]
  * This must be an if statement, and both parts of the conditional must be there - removing the IF, or having only one part has no problems.
* This occurs even if I don't pass any params into the inner template processing
* Using the pure-perl XS stash avoids the problem
* Can't reproduce from the command line
* Can't reproduce on my F11 perl 5.10 machine

<much time passes>

Minmimal non-bugzilla test case attached

Comment 15

[Frédéric Buclin]

(In reply to comment #14)
> * The minimal template to trigger this is:
>   [% USE Bugzilla %]
> [% IF Bugzilla.cgi.param("comment") && Bugzilla.cgi.param("comment").length > 0
> %]
> [% END %]

Yes, I found that too. And you can work around this problem if you write:

[% comment = Bugzilla.cgi.param("comment") %]
[% IF comment.length %]

which is what I did locally. I think I will go with this solution, so that we don't detaint originally tainted data in Flag.pm.

Comment 16

[Bradley Baetz (:bbaetz)]

Attachment: cgi that shows bug, without using Bugzilla code

I can't explain why CGI affects this test case. You need to pass |comment=test| as a param to the script for it to fail.

Comment 17

[Frédéric Buclin]

Attachment: patch, v1

This fixes the problem and is safe.

Comment 18

[Bradley Baetz (:bbaetz)]

https://rt.cpan.org/Ticket/Display.html?id=48654

Can we wait for a reply - I'm worried that we're just pushing the problem to show up somewhere else.

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
> Can we wait for a reply - I'm worried that we're just pushing the problem to
> show up somewhere else.

It's probably a bug in Perl itself. I can reproduce the taint issue with Perl 5.10.0, but I cannot reproduce with Perl 5.10.1 RC1. In both cases do I use TT 2.22. So it doesn't look like a TT bug.

Comment 20

[Frédéric Buclin]

bbaetz: ping?

Comment 21

[Bradley Baetz (:bbaetz)]

Comment on attachment 394038 [details] [diff] [review]
patch, v1

No update on the TT RT issue, so r=bbaetz if you add a (TT) comment above the changed line pointing to this bug. (Make sure that that doesn't add an extra output line, though)

Comment 22

[Frédéric Buclin]

tip:

Checking in template/en/default/request/email.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/email.txt.tmpl,v  <--  email.txt.tmpl
new revision: 1.23; previous revision: 1.22
done

3.4.1:

Checking in template/en/default/request/email.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/email.txt.tmpl,v  <--  email.txt.tmpl
new revision: 1.21.2.1; previous revision: 1.21
done