Last Comment Bug 419188 - [SECURITY] lets you set the changer as @reporter instead of only checking the "From" header
: [SECURITY] lets you set the changer as @reporter instead of only ...
[ready for 3.x]
Product: Bugzilla
Classification: Server Software
Component: Incoming Email (show other bugs)
: 2.23.4
: All All
: -- blocker (vote)
: Bugzilla 3.0
Assigned To: Frédéric Buclin
: default-qa
Depends on:
Blocks: 425110
  Show dependency treegraph
Reported: 2008-02-23 08:27 PST by Frédéric Buclin
Modified: 2008-05-05 14:47 PDT (History)
5 users (show)
LpSolit: approval+
LpSolit: blocking3.2+
LpSolit: approval3.0+
LpSolit: blocking3.0.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

patch, v1 (778 bytes, patch)
2008-02-23 09:07 PST, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v2 (1.07 KB, patch)
2008-02-23 09:13 PST, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Frédéric Buclin 2008-02-23 08:27:00 PST lets you do changes even without altering the email headers. This means that even if you use PGP to sign your emails, or use tokens, it's still trivial to alter bugs as if you were someone else. One useful case would be the ability to CC yourself to bugs you want to see. All you have to do is to add the following line to your email:

@reporter = someone.else@user.account

Where the email address is a valid user account with privs to access security bugs. This trick works with process_bug.cgi, i.e. with existing bugs, which is why I restricted this bug to the security group.

I know currently has no way to check the real identity of the sender, but for installations which added some checks to it and turned on incoming emails, they are still fully vulnerable (e.g. Mandriva Bugzilla).

Now technically, the reason this works is because we have:

    my $username = $mail_fields->{'reporter'};
    my $user = Bugzilla::User->new({ name => $username })
      || ThrowUserError('invalid_username', { name => $username });


But $mail_fields->{'reporter'} comes from:

    my ($reporter) = Email::Address->parse($input_email->header('From'));
    $fields{'reporter'} = $reporter->address;

The problem is that later in the code, we have:

    if ($line =~ /^@(\S+)\s*=\s*(.*)\s*/) {
        $current_field = lc($1);
        $fields{$current_field} = $2;

So if you have the @reporter line, it will override the one taken from the email header, and so all authenticated emails are vulnerable.

I just tried this trick to CC myself to a bug I'm not allowed to see on a test installation, and this works great. I could do the same on Mandriva!
Comment 1 Frédéric Buclin 2008-02-23 09:07:06 PST
Created attachment 305215 [details] [diff] [review]
patch, v1

Disallow to pass the 'reporter' field. This one is now only set based on the "From": field.
Comment 2 Frédéric Buclin 2008-02-23 09:13:13 PST
Created attachment 305220 [details] [diff] [review]
patch, v2

Even better fix: prevent the attacker to split the reporter field on several lines. To do that, I reset the $current_field variable.
Comment 3 Frédéric Buclin 2008-02-23 09:52:41 PST
CC'ing the Mandriva Bugzilla maintainer as he should be aware of the issue.
Comment 4 Vincent Danen 2008-02-23 10:27:12 PST
Thanks for the heads up on this.

I just applied the v2 patch and verified it works:

You are not authorized to access bug #38032.
From  Sat Feb 23 19:30:14 2008
 Subject: [Bug 38032]
  Folder: /var/www/html/bugzilla/bugzilla/			   2518

From  Sat Feb 23 19:32:57 2008
 Subject: [Bug 38032]
  Folder: /var/www/html/bugzilla/bugzilla/			   2509

That's the output from procmail with the patch applied; previously I was able to send a comment with my account which has no privs to the bug in question.

So I can verify that the fix works.

Thanks again for the heads up.
Comment 5 Max Kanat-Alexander 2008-02-23 13:38:58 PST
Comment on attachment 305220 [details] [diff] [review]
patch, v2

Cool, that looks good to me.
Comment 6 Max Kanat-Alexander 2008-02-23 13:40:39 PST
Now, just to be clear about all this, you could already add yourself to any bug you can't see, by changing the "From" address. The docs are pretty clear about the safety risks.

In that sense, I don't think this is a security bug in Bugzilla's code itself. It might be a security bug on some particular installation that has customized their code, but that's not our code.

However, I agree that you shouldn't be able to set the reporter field in any situation.

Unless you can show me some security risk that this creates in upstream Bugzilla itself, I think we should remove this from the security group. People using should be aware of its security risks.
Comment 7 Frédéric Buclin 2008-02-23 15:34:33 PST
(In reply to comment #6)
> I think we should remove this from the security group. People
> using should be aware of its security risks.

That's paradoxal. Should be aware of what? That using in 3.0 and 3.2 is like turning off user authentication completely? I don't see how any bug could have more security risk than that.
Comment 8 Max Kanat-Alexander 2008-02-23 16:08:17 PST
(In reply to comment #7)
> Should be aware of what? That using in 3.0 and
> 3.2 is like turning off user authentication completely?

  Yes, because the POD says so:
Comment 9 Frédéric Buclin 2008-02-29 03:24:29 PST
Pushing this bug in our radar for 3.x.4. But has to wait for the release to happen before being committed to CVS.
Comment 10 Frédéric Buclin 2008-05-04 14:50:16 PDT
This problem is present since exists, i.e. 2.23.4.
Comment 11 Frédéric Buclin 2008-05-04 16:57:01 PDT

Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.18; previous revision: 1.17


Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision:; previous revision:
Comment 12 Max Kanat-Alexander 2008-05-05 14:47:57 PDT
Security advisory sent, removing bugs from security group.

Note You need to log in before you can comment on or make changes to this bug.