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 (gonzalo.aguilar@seglan.com), 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

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.

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?

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

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.

As a workaround, you can add the following right before the exec line in sendmail.pm: foreach (@$args) { my ($match) = $_ =~ /^(.*)$/s; $_ = $match; }

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.

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.

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

Untested patch.

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.

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 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 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 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.

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

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.

Taking a wider approach now. How's this?

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.

Addressing comments.

Unrotted.

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 on attachment 233634 [details] [diff] [review] Patch 2.1.1 for tip We still need back port to 2.22 branch.

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

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 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.

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.

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)

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

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

After trick_taint audit.

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

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.

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 on attachment 240516 [details] [diff] [review] Patch 2.2 for tip As far as I can test, this is working correctly. r=LpSolit

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

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>