287880 - Comments on secure bugs still available to templates... show_bug leaks

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Joel Peshkin]

In bug 287109, a strangeness was pointed out...

In show_bug.cgi, the history of comments IS AVAILABLE even if the bug
is not, so long as we are in the "long format" mode.  So, if you hack form data,
you CAN get the comments.  ValidateBugID() is only called for a single bug.

Try this...

Search for a bunch of bugs, some non-public.
Click on LONG FORMAT
see results
select one bug, view it,  and log out
back up to the LONG FORMAT listing and hit reload...

The tip has this problem.  It is not clear exactly which versions have or do not
have this problem.

Comment 1

[Dave Miller [:justdave]]

(In reply to comment #0)

> The tip has this problem.  It is not clear exactly which versions have or do not
> have this problem.

It is clear, we just hashed this out on IRC over the last hour.

The problem is inherent in the way the Bug object creates the comment lists for
bugs.  It doesn't bother to check if the initialization of the Bug object threw
an error, it just returns them anyway.

This was exposed to users when longlist.cgi was nuked and replaced with a format
for show_bug.cgi, and the longlist template started using the same API that the
XML ctype is using.  The XML template skips trying to load stuff if the bug
object is in an error state.  longlist doesn't.

This was all checked in on 2005 Jan 24 from bug 201818, which was AFTER 2.19.2
was released, so we've never had an actual release that contains this yet.  Even
so, it's been in CVS for 2 months.

1) The template needs to be fixed to skip bugs that are in an error state.  (We
can't remove said bugs from the list because the XML ctype needs to be able to
report the errors).

2) The underlying methods in Bug.pm which return data about a bug all need to be
fixed so that they return empty data if the Bug object is in an error state.

Comment 2

[Max Kanat-Alexander]

Attachment: Fix up Bug.pm

Here's a fix that includes the fixes only for Bug.pm. I can look into the
template stuff, also.

Comment 3

[Joel Peshkin]

Comment on attachment 178697 [details] [diff] [review]
Fix up Bug.pm

r=joel
Let's also get a 2xr

Comment 4

[Max Kanat-Alexander]

Attachment: Backport to 2.18

A simple backport to 2.18. I didn't test it, but it compiles and it does the
same thing we do on the tip.

Comment 5

[Max Kanat-Alexander]

Attachment: Fix actual_time condition (tip)

The condition I was using in actual_time was broken. Caught by justdave.

Comment 6

[Dave Miller [:justdave]]

Comment on attachment 178702 [details] [diff] [review]
Backport to 2.18


>@@ -288,7 +294,7 @@
> 
>     return $self->{'actual_time'} if exists $self->{'actual_time'};
> 
>-    return undef unless (Bugzilla->user && 
>+    return undef unless (!$self->{'error'} && Bugzilla->user && 
>                          Bugzilla->user->in_group(Param("timetrackinggroup")));
> 

This breaks it for people who *do* have timetrackinggroup.

Comment 7

[Joel Peshkin]

(In reply to comment #6)

> > 
> >-    return undef unless (Bugzilla->user && 
> >+    return undef unless (!$self->{'error'} && Bugzilla->user && 
> >                          Bugzilla->user->in_group(Param("timetrackinggroup")));
> > 
> 
> This breaks it for people who *do* have timetrackinggroup.
> 
Are you sure??

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 178697 [details] [diff] [review]
Fix up Bug.pm

Made that comment on the wrong attachment.  This is the one I meant to make
that comment on.

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 178702 [details] [diff] [review]
Backport to 2.18

And this one is okay :)

Comment 10

[Dave Miller [:justdave]]

For the record, for anyone peeking in on this bug, the backport to 2.18 is just
a precaution to prevent anyone who might customize 2.18.x from accidently
triggering this themselves.  There is no way for an end-user to exploit this in
2.18 unless you've made local modifications to any multi-bug templates used by
show_bug.cgi and didn't check the bug.error state in your template.

Comment 11

[Dave Miller [:justdave]]

We don't need to hold this for a release to check in because there's never been
a release containing this exploit.  I'll shoot an announcement to the mailing
lists advising people who have pulled from CVS during the time frame this
existed to update.

Comment 12

[Max Kanat-Alexander]

OK, we still need to send out that security announcement later today.

Tip: 

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.71; previous revision: 1.70
done

2.18: 

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.37.2.4; previous revision: 1.37.2.3
done