Closed Bug 99608 Opened 22 years ago Closed 21 years ago

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

Categories

(Bugzilla :: Email Notifications, defect, P1)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: jacob, Assigned: gerv)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
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.
Summary: Possible leak of bug summary in dependancy changed e-mail → Possible leak of bug summary in dependency changed e-mail
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
This is going to be a blocker for the 2.16 release
Severity: normal → blocker
Attached patch Patch v.1 (obsolete) — Splinter Review
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
Taking.

Gerv
Assignee: jake → gerv
Keywords: patch
Accepting.

Gerv
Status: NEW → ASSIGNED
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.
> 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

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.
Keywords: review
Comment on attachment 57547 [details] [diff] [review]
Patch v.1

See my previous comments.
Attachment #57547 - Flags: review-
Summary: Possible leak of bug summary in dependency changed e-mail → [security] Possible leak of bug summary in dependency changed e-mail
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
This is a release blocker.
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?
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!
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."
Keywords: patch, review
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
Attached patch Patch v.2 (obsolete) — Splinter Review
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
Attachment #57547 - Attachment is obsolete: true
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.
Attachment #63024 - Flags: review-
Attached patch Patch v.3 (obsolete) — Splinter Review
Try this for size. All comments addressed.

Gerv
Attachment #63024 - Attachment is obsolete: true
Comment on attachment 63179 [details] [diff] [review]
Patch v.3

r=ddk  Looks good!
Attachment #63179 - Flags: review+
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.
Attachment #63179 - Flags: review-
Attached patch Patch v.4 [Jake] (obsolete) — Splinter Review
This patch adds the detaint_natural() line I mentioned in my review (comment
#20)
Attachment #63179 - Attachment is obsolete: true
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;
  }
Attached patch Patch v.5 [Jake] (obsolete) — Splinter Review
OK, this patch does warn && return instead of die.  Or course, if it has to do
anything, we've got real problems :)
Attachment #64229 - Attachment is obsolete: true
Attachment #64283 - Flags: review+
Comment on attachment 64283 [details] [diff] [review]
Patch v.5 [Jake]

r=gerv.

Gerv
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.
Attachment #64283 - Flags: review-
Attached patch Patch v.6 [ddk] (obsolete) — Splinter Review
Fixes from comment #25.
Attachment #64283 - Attachment is obsolete: true
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;
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!
Attached patch Patch v.7 [ddk]Splinter Review
Duh.  Changed:
  my $save_id;
to:
  my $save_id = $dep_id;
Attachment #64410 - Attachment is obsolete: true
Comment on attachment 64505 [details] [diff] [review]
Patch v.7 [ddk]

r=gerv.

Gerv
Attachment #64505 - Flags: review+
OS: Windows 2000 → All
Hardware: PC → All
Comment on attachment 64505 [details] [diff] [review]
Patch v.7 [ddk]

r= justdave
Attachment #64505 - Flags: review+
Checked in.

/cvsroot/mozilla/webtools/bugzilla/processmail,v  <--  processmail
new revision: 1.75; previous revision: 1.74
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
*** Bug 146141 has been marked as a duplicate of this bug. ***
Do we want this for 2.14.2?
Whiteboard: wanted for 2.14.2 ?
Bug summaries weren't included in these emails in 2.14.
Whiteboard: wanted for 2.14.2 ?
ah, ok
*** Bug 149938 has been marked as a duplicate of this bug. ***
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.