340538 - Insecure dependency in exec while running with -T switch at /usr/lib/perl5/site_perl/5.8.6/Mail/Mailer/sendmail.pm line 16.

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Merlin]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.0.1) Gecko/20060215 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.0.1) Gecko/20060215 Firefox/1.5.0.1

Every time I try to submit a new password via "forgot your password" I get this message.

Also when I try to use the "forgot password" form and then I enter into my account with the current password.

Reproducible: Always

Steps to Reproduce:
1.try filling the "forgot password" text box at login.

Actual Results:  
Insecure dependency in exec while running with -T switch at /usr/lib/perl5/site_perl/5.8.6/Mail/Mailer/sendmail.pm line 16.

For help, please send mail to the webmaster ([e-mail]), giving this error message and the time and date of the error. 

Expected Results:  
Send an e-mail to the account

./checksetup.pl

Checking perl modules ...
Checking for       AppConfig (v1.52)   ok: found v1.56
Checking for             CGI (v2.93)   ok: found v3.10
Checking for    Data::Dumper (any)     ok: found v2.121_02
Checking for    Date::Format (v2.21)   ok: found v2.22
Checking for             DBI (v1.38)   ok: found v1.48
Checking for      File::Spec (v0.84)   ok: found v3.01
Checking for      File::Temp (any)     ok: found v0.14
Checking for        Template (v2.08)   ok: found v2.14
Checking for      Text::Wrap (v2001.0131) ok: found v2001.09292
Checking for    Mail::Mailer (v1.67)   ok: found v1.74
Checking for    MIME::Base64 (v3.01)   ok: found v3.05
Checking for    MIME::Parser (v5.406)  ok: found v5.420
Checking for        Storable (any)     ok: found v2.13

The following Perl modules are optional:
Checking for              GD (v1.20)   ok: found v2.32
Checking for     Chart::Base (v1.0)    ok: found v2.3
Checking for       XML::Twig (any)     ok: found v3.25
Checking for       GD::Graph (any)     ok: found v1.4308
Checking for GD::Text::Align (any)     ok: found v1.18
Checking for     PatchReader (v0.9.4)  ok: found v0.9.5
Checking for   Image::Magick (any)      not found

If you want to convert BMP image attachments to PNG to conserve
disk space, you will need to install the ImageMagick application
Available from http://www.imagemagick.org, and the Image::Magick
Perl module by running (as root):

   /usr/bin/perl -MCPAN -e 'install "Image::Magick"'

Checking user setup ...
Removing existing compiled templates ...
Precompiling templates ...
Checking for      DBD::mysql (v2.9003) ok: found v2.9007
Checking for           MySQL (v4.0.14) ok: found v4.1.16

Populating duplicates table...


------------------------------------------------------
 Bugzilla  Versión 2.22

Comment 1

[Vlad Dascalu]

Have you performed code/template changes to your Bugzilla installation?

This works, so it would probably end up marked WorksForMe. See http://www.bugzilla.org/support/ for support-related questions.

Comment 2

[Merlin]

Yep, I've done a iconv from iso-8859-1 (latin1) to UTF-8 in spanish templates. But I think that nothing else...

Can this broke my system configuration?

Comment 3

[Marc Schumann [:Wurblzap]]

Seen that, too. Might be related to a bug in Mail::Mailer, so I'm hesitating with confirmation as of now.

Comment 4

[Merlin]

I don't think like. Because sendmail.pm is really small and the bug has to come from another place...

I don't know how trace back the bug. If someone can point me where to look or how to enable any kind of function trace I will try.

Comment 5

[Marc Schumann [:Wurblzap]]

As a workaround, you can add the following right before the exec line in sendmail.pm:

    foreach (@$args) {
        my ($match) = $_ =~ /^(.*)$/s;
        $_ = $match;
    }

Comment 6

[Merlin]

I wanted to know why this is a good workaround.
It's better to know the causes than fixing it in my machine. I want to investigate if this is a iconv problem or what...

So why did you do that? I don't undestand this workaround...

Thank you.

Comment 7

[Marc Schumann [:Wurblzap]]

When running in taint mode (perl's -T parameter), exec() dies with an "Insecure dependency" message if its arguments contain tainted data. My additional code snippet flatly untaints all incoming arguments.

CGI variables are tainted. So my guess is that the core reason of this bug may be that the e-mail address somebody enters for a new password token stays tainted all the way through the mailing call. If so, then the same probably goes for new user accounts.

Comment 8

[Marc Schumann [:Wurblzap]]

Confirming. I can see this with 2.22/Linux/sendmail. I don't know whether 2.20 is affected or not.

Comment 9

[Marc Schumann [:Wurblzap]]

Attachment: Patch

Untested patch.

Comment 10

[Merlin]

I will review this patch ASAP...

But I found another issue...
=[Intro]=
The server is located inside an office LAN but you can reach it trough a NAT port of the firewall.

=[Issue]=
The thing is that, I tried to reproduce the bug inside the office but it worked well. And re-tried outside and did not worked.

=[Question]=
Can it be a problem not related directly with bugzilla?


I'll try the patch and see what happens.

Comment 11

[Frédéric Buclin]

Marc, this patch looks wrong to me. All Bugzilla routines using $login detaint it when needed. At least should we move trick_taint() right before calling MessageToMTA(), IMHO. Note that I couldn't reproduce the bug on landfill/qa222 using sendmail.

Comment 12

[Teemu Mannermaa (:wicked)]

Comment on attachment 224986 [details] [diff] [review]
Patch

>Index: createaccount.cgi
>===================================================================
>+    # We assume the validate_email_syntax checks to suffice to consider the
>+    # login untainted.
>+    trick_taint($login);

Nit: Hmm, all the other subs detaint this them self. I think this should be moved to MailPassword sub in BugMail.pm to make code consistent. Or, like LpSolit said, near the place we need to detaint here.

>Index: token.cgi
>===================================================================
>-    Bugzilla::Token::IssuePasswordToken($cgi->param('loginname'));
>+    my $loginname = $cgi->param('loginname');
>+
>+    # The login has been checked above by validate_email_syntax, which we
>+    # assume to be sufficient to consider it untainted.
>+    trick_taint($loginname);
>+    Bugzilla::Token::IssuePasswordToken($loginname);

Umm, I don't this change is needed. IssuePasswordToken already detains loginname.

Also, in token.cgi around line 102 the loginname seems to be used without detainting too. Probably should deal with that place also. Or at least verify it doesn't have problems.

Nit: Additionally, note that in insert_new_user sub in User.pm we have following comment regarding username:
    # XXX - These should be moved into is_available_username or validate_email_syntax
    #       At the least, they shouldn't be here. They're safe for now, though.
Maybe that should dealt with at the same time?

Comment 13

[Marc Schumann [:Wurblzap]]

Comment on attachment 224986 [details] [diff] [review]
Patch

Argh no, this thinking defeats all that taint mode is about!

Taint mode doesn't simply keep us safe from the background, so the deal is not to simply untaint when we need data.

The point of taint mode is to keep us using unsafe data. It is designed in a way so that it can bail out if we try to use unvalidated data. Taintedness is a flag to mark data unvalidated or derived from unvalidated data. That way, it can keep track for us whether we did our validation or not.

In order for all this to work, all we need to do is this:

   Validate    and then immediately
   Untaint.

That's all. If we do this stringently, we'll never need to untaint far away from validation.

When we see an "Insecure dependency" error message, our first thought must not be "I seem to have forgotten to untaint", but it must be "I seem to have forgotten to validate my data". Because this is what the error message indicates.

So what I'm saying is this: we always need to untaint as close as possible to validation. All other places are hacks or workarounds.

Wicked, the places where I'm untainting are better places that the current places of usage. Please take a second look.

Comment 14

[Teemu Mannermaa (:wicked)]

Comment on attachment 224986 [details] [diff] [review]
Patch

(In reply to comment #13)
> So what I'm saying is this: we always need to untaint as close as possible to
> validation. All other places are hacks or workarounds.

I agree.

> Wicked, the places where I'm untainting are better places that the current
> places of usage. Please take a second look.

Placement of your trick_taint() calls were only nits. Real reasons I deny review were because firstly there was a third place that uses loginname without validation/detaint and secondly token.cgi trick_taint() was duplication which I don't like.

Now that I look at second case, looks like token.cgi trick_taint() comment mentions validate_email_syntax which isn't called on loginname after it's retrieved from CGI env. OK, so it's called in the if block before but that's way too obscure for my taste.. Better is to pass the validated value to this sub from main block, IMHO.

If you can figure out correct places (probably inside validate_email_syntax sub) and change caller sites accordingly would be lovely. This is only a wish, though.

Comment 15

[Max Kanat-Alexander]

This is certainly a blocker. I've seen this on my local Pg installation, from time to time.

Comment 16

[Merlin]

Ooops! I'm sorry of hearing that. Because I installed this on my company to give a chance to the opensource comunity and I had very bad luck! Second version I installed to manage our company problems and... jejeje.
First one was flawlesly but never used!

Anyway, this is a great piece of software. I enjoy a lot filling bugs and never had a problem. This is called bad luck! 

Anyway this is a great project I really love it. Keep on going!

Thank you so much.

Comment 17

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2

Taking a wider approach now. How's this?

Comment 18

[Teemu Mannermaa (:wicked)]

Comment on attachment 227807 [details] [diff] [review]
Patch 2

Yes, much better, thanks.

In addition to following comments, we also need a backport to 2.22.

>Index: token.cgi
>===================================================================
>+my $login_name;
>+my $password;
...
> sub requestChangePassword {
>+    Bugzilla::Token::IssuePasswordToken($login_name);

Unfortunately using $password and $login_name variables that were defined outside of any sub inside subs won't work under mod_perl hence my r-. You need to either use "our" or "local our" instead of "my" or pass the variables to the subs. I'd prefer passing them in the if block starting around line 135.

Nit: Also place $password definition before the if block it's used in.

>Index: Bugzilla/Auth/Verify.pm
>===================================================================
>+            # Usually we'd call validate_password, but external authentication
>+            # systems might follow different standards than ours. So in this
>+            # place here, we call trick_taint without checks.
>+            trick_taint($password);
>             insert_new_user($username, $real_name, $password);

I'm concerned that $real_name will be tainted here now that insert_new_user() doesn't untaint it. Looking at env login this might happen because real_name is fetched directly from %ENV which I think is tainted. Couldn't find any place where these are detainted either.

Comment 19

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.1

Addressing comments.

Comment 20

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.1.1 for tip

Unrotted.

Comment 21

[Teemu Mannermaa (:wicked)]

Comment on attachment 233634 [details] [diff] [review]
Patch 2.1.1 for tip

>Index: token.cgi
>===================================================================
>+local our $login_name;
...
>+local our $password;

No need to use both "local our" and "pass as parameter" approaches to make this work on mod_perl. :)
So change these "local our" definitions back to "my" before commit. I tested the patch this way and it worked.

Comment 22

[Teemu Mannermaa (:wicked)]

Comment on attachment 233634 [details] [diff] [review]
Patch 2.1.1 for tip

We still need back port to 2.22 branch.

Comment 23

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.1.2 for tip

Unrotted.
I'll prepare a backport as soon as possible.

Comment 24

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.1.2 for 2.22

Comment 25

[Frédéric Buclin]

Comment on attachment 237449 [details] [diff] [review]
Patch 2.1.2 for 2.22

looks good. r=LpSolit. Note that the current code is now a bit redundant as trick_taint() calls haven't been removed after calling these validation routines, except in token.cgi. But let's not break things on branches, so I'm fine keeping these extra calls to trick_taint().

Comment 26

[Frédéric Buclin]

Comment on attachment 235709 [details] [diff] [review]
Patch 2.1.2 for tip

>Index: token.cgi

sub confirm_create_account {
    my (undef, undef, $login_name) = Bugzilla::Token::GetTokenData($::token);

    validate_password($cgi->param('passwd1') || '', 
                      $cgi->param('passwd2') || '');

Now validate_password() tries to detaint $_[0]. So you cannot use this syntax. Moreover, you should do an audit to remove all these redundant calls to trick_taint() as it's now done in validation routines. For instance:

userprefs.cgi

            validate_email_syntax($new_login_name)
              || ThrowUserError('illegal_email_address', {addr => $new_login_name});
            trick_taint($new_login_name);


editusers.cgi

            validate_email_syntax($login)
              || ThrowUserError('illegal_email_address', {addr => $login});
            is_available_username($login)
              || ThrowUserError('account_exists', {email => $login});

            trick_taint($login);

Token.pm

    sub issue_new_user_account_token {

        trick_taint($login_name);

this trick_taint() is useless as Token::issue_new_user_account_token() is only called from createaccount.cgi:

    $login = Bugzilla::User->check_login_name_for_creation($login);
    Bugzilla::Token::issue_new_user_account_token($login);

And check_login_name_for_creation() itself calls validate_email_syntax($name) so the login name is already detainted.

I didn't check Util::validate_password() though, so there may be some additional places to fix.

Comment 27

[Frédéric Buclin]

The patch for 2.22 is ready. Let's take it despite the one for tip is not ready yet, so that we can start QA tests on branches asap.

Comment 28

[Dave Miller [:justdave]]

ok.  Remember not to resolve this fixed when you commit on the branch then (you can do a fixed in 2.22.x on the whiteboard or something)

Comment 29

[Dave Miller [:justdave]]

err, I meant to approve that when I added that comment, sorry

Comment 30

[Frédéric Buclin]

Committed on 2.22 only. Leaving the bug open.

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/Attic/globals.pl,v  <--  globals.pl
new revision: 1.348.2.4; previous revision: 1.348.2.3
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.39.2.2; previous revision: 1.39.2.1
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.45.2.4; previous revision: 1.45.2.3
done

Comment 31

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.2 for tip

After trick_taint audit.

Comment 32

[Max Kanat-Alexander]

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

Comment 33

[Max Kanat-Alexander]

Just to let you know, Bug 353711 handles this for the tip. I just tested it locally on my own installation that was throwing this error.

Comment 34

[Jens Reimann]

this issue came up for me when adding two people at the same at a flag using "userA, userB" have a "exactly one" match for both on the next page. Works fine with only one user.

Using Bugzilla: 2.22.1

Comment 35

[Frédéric Buclin]

Comment on attachment 240516 [details] [diff] [review]
Patch 2.2 for tip

As far as I can test, this is working correctly. r=LpSolit

Comment 36

[Marc Schumann [:Wurblzap]]

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.139; previous revision: 1.138
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.48; previous revision: 1.47
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.107; previous revision: 1.106
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.50; previous revision: 1.49
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.135; previous revision: 1.134
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.56; previous revision: 1.55
done
Checking in Bugzilla/Auth/Verify.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify.pm,v  <--  Verify.pm
new revision: 1.6; previous revision: 1.5
done

Comment 37

[tran tran]

I upgraded Bugzilla from 2.22 to Bugzilla: 2.22.1 but this error still occured more often.  It occured after changing the flag from ? to +.  Does it need to have a special upgrade?  Please help.  Here is my checksetup.pl result:

D:\bugzilla-2.22.1>checksetup.pl

Checking perl modules ...
Checking for       AppConfig (v1.52)   ok: found v1.55
Checking for             CGI (v2.93)   ok: found v3.04
Checking for    Data::Dumper (any)     ok: found v2.121
Checking for    Date::Format (v2.21)   ok: found v2.22
Checking for             DBI (v1.38)   ok: found v1.48
Checking for      File::Spec (v0.84)   ok: found v0.87
Checking for      File::Temp (any)     ok: found v0.14
Checking for        Template (v2.10)   ok: found v2.13
Checking for      Text::Wrap (v2001.0131) ok: found v2001.09291
Checking for    Mail::Mailer (v1.67)   ok: found v1.67
Checking for    MIME::Base64 (v3.01)   ok: found v3.01
Checking for     MIME::Tools (v5.406)  ok: found v5.411
Checking for        Storable (any)     ok: found v2.12

The following Perl modules are optional:
Checking for              GD (v1.20)   ok: found v2.16
Checking for Template::Plugin::GD::Image (any)     ok: found v1.55
Checking for     Chart::Base (v1.0)    ok: found v2.3
Checking for       XML::Twig (any)      not found
Checking for       GD::Graph (any)     ok: found v1.43
Checking for GD::Text::Align (any)     ok: found v1.18
Checking for     PatchReader (v0.9.4)  ok: found v0.9.5
Checking for   Image::Magick (any)      not found
Checking for    HTML::Parser (v3.40)    found v3.36
Checking for  HTML::Scrubber (any)      not found

All the required modules are available at:
    http://landfill.bugzilla.org/ppm/
You can add the repository with the following command:
    ppm rep add bugzilla http://landfill.bugzilla.org/ppm/

If you want to use the bug import/export feature to move bugs to
or from other bugzilla installations, you will need to install
 the XML::Twig module by running (as Administrator):

   ppm install XML::Twig

If you want to convert BMP image attachments to PNG to conserve
disk space, you will need to install the ImageMagick application
Available from http://www.imagemagick.org, and the Image::Magick
Perl module by running (as Administrator):

   ppm install Image::Magick

If you want additional HTML tags within product and group descriptions,
you should install:
HTML::Scrubber: ppm install HTML::Scrubber
HTML::Parser: ppm install HTML::Parser

Checking user setup ...
Removing existing compiled templates ...
Precompiling templates ...
Checking for      DBD::mysql (v2.9003) ok: found v2.9006
Checking for           MySQL (v4.0.14) ok: found v4.1.11-nt


D:\bugzilla-2.22.1>