99608 - [security] Possible leak of bug summary in dependency changed e-mail

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect, P1)

Description

[Jacob Steenhagen]

During an IRC converstation today, it was mentioned that the dependency changed
messages may be leaking bug summaries.  I don't believe that the permissions are
checked before sending the message that begins "This bug depends on bug $bug,
which changed state."  This wasn't a big deal before bug 28736 was filed as a
bug's status is not conidered to be confidential, but its summary is.

Comment 1

[Matthew Tuck [:CodeMachine]]

My feeling is that status should be confidential, but if it isn't, I would
expect resolution to be.

And in any case, there's no point sending it out if they can't access it.

Comment 2

[Dave Miller [:justdave]]

This is going to be a blocker for the 2.16 release

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.1

How about this? Don't send changedmail if the depending bug is invisible to the
relevant user. This way the status, etc. all stays private.

Gerv

Comment 4

[Gervase Markham [:gerv]]

Taking.

Gerv

Comment 5

[Gervase Markham [:gerv]]

Accepting.

Gerv

Comment 6

[Christian Reis]

Comment on attachment 57547 [details] [diff] [review]
Patch v.1

>@@ -187,17 +187,19 @@
>     my $thisdiff = "";
>     my $lastbug = "";
>     my $interestingchange = 0;
>+    my $depbug = 0;
>     while (MoreSQLData()) {
>-        my ($bug, $summary, $what, $old, $new) = (FetchSQLData());
>-        if ($bug ne $lastbug) {
>+        my ($summary, $what, $old, $new);
>+        ($depbug, $summary, $what, $old, $new) = (FetchSQLData());
[...]

So basically this first part is just a namechage for depbug to avoid a
nameclash?

>@@ -665,6 +667,12 @@
>     #
>     return unless CanSeeBug($id, $userid, $groupset);
>     
>+    # We shouldn't send changedmail if this is a dependency mail, and the
>+    # depending bug is not visible to the user.
>+    if ($depbug) {
>+      return unless CanSeeBug($depbug, $userid, $groupset);
>+    }
>+

I'm not very familiar with NewProcessOnePerson, but I guess you've picked to
check for 
this here instead of ProcessOneBug to avoid an extra SQL hit, and because there
is a very
similar check close to it. Sounds okay; this API doesn't look very usable as it
is, so
it's no big deal making it worse. Still, this part of the code is scary: 12
parameters? 
Steve McConnell would have cried :)

If I've guessed properly, then r=kiko.

Comment 7

[Gervase Markham [:gerv]]

> So basically this first part is just a namechage for depbug to avoid a
> nameclash?

To make it more clear, yes - it's not to avoid a clash.

> because there is a very similar check close to it.

This was my logic. Yes, the API sucks - we'll have to wait until we spank that
file to fix it.

Gerv

Comment 8

[David D. Kilzer (ddk)]

In reviewing attachment 57547 [details] [diff] [review] ("Patch v.1"), I don't believe it will
properly handle the case where more than one bug depends on the bug
being changed.  Only the "last" bug id returned from the database
will be set to $depbug, and only that last bug id will be checked.

My recommendation is to change $depbug to @depbug or %depbug and
store *all* dependent bug ids, pass \@depbug or \%depbug to
NewProcessOnePerson(), then loop over @$depbug or keys %$depbug to
check all dependent bugs.

Comment 9

[David D. Kilzer (ddk)]

Comment on attachment 57547 [details] [diff] [review]
Patch v.1

See my previous comments.

Comment 10

[Gervase Markham [:gerv]]

OK, then I don't understand how this works. A single mail is about a single
change to a single bug, and a single dependency - isn't that right? I can see
why you think it should be a list, but I can't see how it could make any sense
as one.

Unless someone else can figure this out, this patch will have to wait, and
probably won't make 2.16.

Gerv

Comment 11

[Matthew Tuck [:CodeMachine]]

This is a release blocker.

Comment 12

[Dave Miller [:justdave]]

ddk: can you explain more what you're looking at?  As far as I know dependency
mail only goes out on one bug at a time.  Where is there a list of bugs being
processed that would be stale by the time the mail is sent?

Comment 13

[David D. Kilzer (ddk)]

When reviewing this patch to add more detailed comments (as per the
request by justdave), I ran into the code fragment below on line 172
(give or take a few lines).  Can anyone explain this???

    my $resid =

Okay, back to more detailed comments about my previous comment.  This
is the SendSQL statement that is run just before the while() loop
    SendSQL("SELECT " . join(',', @::log_columns) . ", lastdiffed, now() " .
            "FROM bugs WHERE bug_id = $id");
    my @row = FetchSQLData();
    foreach my $i (@::log_columns) {
        $values{$i} = shift(@row);
    }
    my ($start, $end) = (@row);
that appears in the patch.

    SendSQL("SELECT bugs_activity.bug_id, bugs.short_desc, fielddefs.name, " .
            "       removed, added " .
            "FROM bugs_activity, bugs, dependencies, fielddefs ".
            "WHERE bugs_activity.bug_id = dependencies.dependson " .
            "  AND bugs.bug_id = bugs_activity.bug_id ".
            "  AND dependencies.blocked = $id " .
            "  AND fielddefs.fieldid = bugs_activity.fieldid" .
            "  AND (fielddefs.name = 'bug_status' " .
            "    OR fielddefs.name = 'resolution') " .
            "  AND bug_when > '$start' " .
            "  AND bug_when <= '$end' " .
            "ORDER BY bug_when, bug_id");

This is the patched while() loop (with variable declarations) that
appears immediately after it:

    my $thisdiff = "";
    my $lastbug = "";
    my $interestingchange = 0;
    my $depbug = 0;
    while (MoreSQLData()) {
        my ($summary, $what, $old, $new);
        ($depbug, $summary, $what, $old, $new) = (FetchSQLData());
        if ($depbug ne $lastbug) {
            if ($interestingchange) {
                $deptext .= $thisdiff;
            }
            $lastbug = $depbug;
            $thisdiff =
                "\nThis bug depends on bug $depbug, which changed state.\n";
            $thisdiff .=
                "Bug $depbug Summary: $summary\n\n";
            $thisdiff .= FormatTriple("What    ", "Old Value", "New Value");
            $thisdiff .= ('-' x 76) . "\n";
            $interestingchange = 0;
        }
        $thisdiff .= FormatTriple($fielddescription{$what}, $old, $new);
        if ($what eq 'bug_status' && IsOpenedState($old) ne IsOpenedState($new)) {
            $interestingchange = 1;
        }
    }

My concern is that if the SQL statement returns more than one row
with different bug ids, then not every $depbug value will be checked.
In other words, I presume that if the current bug (represented by
$id) depends on more than one bug, the while() loop would go through
all such bugs, setting $depbug to each one in turn, and that if a
"confidential" bug id is assigned to $depbug before a non-confidential
bug id, then the safety check will be invalidated (since the check
will only be done on the non-confidential bug id).  Does that make
sense?

Note that it is very possible that I don't fully understand the
limits on what rows may be returned by the SQL statement at the time
the code runs.

You may also want to review this in light of bug 106377.  If the
lastdiffed column hasn't been updated, then more than one change will
be written out to the mail message, and I *believe* that the
situation I'm concerned about here would be much more likely.  See
this code in ProcessOneBug() (prior to the quoted code above) where
$start and $end are set.

    SendSQL("SELECT " . join(',', @::log_columns) . ", lastdiffed, now() " .
            "FROM bugs WHERE bug_id = $id");
    my @row = FetchSQLData();
    foreach my $i (@::log_columns) {
        $values{$i} = shift(@row);
    }
    my ($start, $end) = (@row);

If it's impossible to have such a condition, then I'll give the code
an r=.  Whew!

Comment 14

[David D. Kilzer (ddk)]

Ack!  Something's amiss with the Mozilla 0.9.5 textarea editor.  It
has some VERY strange behavior under RH 6.2.  Anyway, the second
paragraph in my previous comment should read like this (without all
the code in the middle of it):

"Okay, back to more detailed comments about my previous comment.  This
is the SendSQL statement that is run just before the while() loop
that appears in the patch."

Comment 15

[Dave Miller [:justdave]]

Hmm, it seems the bulk change thinks I'm not changing anything if all I do is
add names to the CC list, so I guess I have to make a comment.  Anyhow, adding
the representatives from the organizations we know of that support Bugzilla
distributions so they're aware of our upcoming security release

Comment 16

[Gervase Markham [:gerv]]

Attachment: Patch v.2

ddk is right. We need to create a list of all the values $depbug has been set
to, pass that around and then check them all. Here's another patch.

Gerv

Comment 17

[David D. Kilzer (ddk)]

Comment on attachment 63024 [details] [diff] [review]
Patch v.2

Looks great, except there are some typos:

- "push($depbug, @depbugs)" should be "push(@depbugs, $depbug)"

- Call to NewProcessOnePerson() uses "\@depbug" instead of "\@depbugs" as last
argument.

- Would like to see "$debpugRef" be called "$depbugsRef" for consistency,
but is not necessary.

Comment 18

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Try this for size. All comments addressed.

Gerv

Comment 19

[David D. Kilzer (ddk)]

Comment on attachment 63179 [details] [diff] [review]
Patch v.3

r=ddk  Looks good!

Comment 20

[Jacob Steenhagen]

Comment on attachment 63179 [details] [diff] [review]
Patch v.3

With this patch applied, I get an error when trying to	resolve or reopen a
secure bug:

 Insecure dependency in parameter 1 of DBI::db=HASH(0x83ac2d4)->prepare method
call while running with -T switch at globals.pl line 216.

I tracked this down to line 680 of the patched version of processmail.	If you
add the line:

      detaint_natural($_) || die 'Unexpected Error: @depbugs contains a
non-numeric value'; 

right before the return line in the foreach (@depbugs) loop it solves the
problem.

Comment 21

[Jacob Steenhagen]

Attachment: Patch v.4 [Jake]

This patch adds the detaint_natural() line I mentioned in my review (comment
#20)

Comment 22

[David D. Kilzer (ddk)]

Using die() here seems a bit drastic in this case in my opinion.

  detaint_natural($_) || die 'Unexpected Error: @depbugs contains a non-numeric
value';

Granted, we got an invalid bug number from the database, but a simple
'return' would prevent any email from being sent for the current
person/bug_id.

  detaint_natural($_) || return;

Another option would be to print a warning and then return:

  if (! detaint_natural($_)) {
      warn '@depbugs contains a non-numeric value';  
      return;
  }

It might be useful to save the original value in order to
complain about it:

  my $current_id = $_;
  if (! detaint_natural($_)) {
      warn "\@depbugs contains a non-numeric value: '$current_id'";
      return;
  }

Comment 23

[Jacob Steenhagen]

Attachment: Patch v.5 [Jake]

OK, this patch does warn && return instead of die.  Or course, if it has to do
anything, we've got real problems :)

Comment 24

[Gervase Markham [:gerv]]

Comment on attachment 64283 [details] [diff] [review]
Patch v.5 [Jake]

r=gerv.

Gerv

Comment 25

[David D. Kilzer (ddk)]

Comment on attachment 64283 [details] [diff] [review]
Patch v.5 [Jake]

Oh the pain!

1. Doesn't apply cleanly due to fix for bug 113383.

2. Arguments to warn() need to be quoted otherwise precedence rules
are equivalent to:

warn("Unexpected" && return);

at least in Perl v5.6.0 on my machine here.

3. If $dep_id is not a number, detaint_natural() makes it equivalent to
'undef',
which means the warn() generates yet another warning about using an undefined
value.

Another patch to follow.

Comment 26

[David D. Kilzer (ddk)]

Attachment: Patch v.6 [ddk]

Fixes from comment #25.

Comment 27

[David D. Kilzer (ddk)]

Example script for item two in comment #25.

#!/usr/bonsai/bin/perl -wT

use strict;

sub foo { warn "D'OH!" && return; }
sub bar { warn("D'OH!") && return; }

foo();
print "We should have seen D'OH! warning.\n";
bar();
print "There it is!\n";
exit;

Comment 28

[David D. Kilzer (ddk)]

Example script for item three in comment #25.  Must be run from
your Bugzilla directory, or have a local copy of globals.pl.


#!/usr/bonsai/bin/perl -wT

use lib '.';
use strict;

require 'globals.pl';

my $id = 'foobar';

detaint_natural($id) || warn "Unexpected Error: \$id contains a non-numeric
value: '$id'";

print "D'OH! Two warnings!\n";

exit;


Output:

[Thu Jan 10 18:44:14 2002] bar.pl: Use of uninitialized value in concatenation
(.) at ./bar.pl line 10.
[Thu Jan 10 18:44:14 2002] bar.pl: Unexpected Error: $id contains a non-numeric
value: '' at ./bar.pl line 10.
D'OH! Two warnings!

Comment 29

[David D. Kilzer (ddk)]

Attachment: Patch v.7 [ddk]

Duh.  Changed:
  my $save_id;
to:
  my $save_id = $dep_id;

Comment 30

[Gervase Markham [:gerv]]

Comment on attachment 64505 [details] [diff] [review]
Patch v.7 [ddk]

r=gerv.

Gerv

Comment 31

[Dave Miller [:justdave]]

Comment on attachment 64505 [details] [diff] [review]
Patch v.7 [ddk]

r= justdave

Comment 32

[Dave Miller [:justdave]]

Checked in.

/cvsroot/mozilla/webtools/bugzilla/processmail,v  <--  processmail
new revision: 1.75; previous revision: 1.74

Comment 33

[Matthew Tuck [:CodeMachine]]

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

Comment 34

[Bradley Baetz (:bbaetz)]

Do we want this for 2.14.2?

Comment 35

[Matthew Tuck [:CodeMachine]]

Bug summaries weren't included in these emails in 2.14.

Comment 36

[Bradley Baetz (:bbaetz)]

ah, ok

Comment 37

[Bradley Baetz (:bbaetz)]

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