209376 - Can access summary for secure bug if its been voted on.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Bradley Baetz (:bbaetz)]

If someone has voted on a bug which is restricted in some way, we can look at
their user's voting page and get teh summary of the bug.

This is because the CanSeeBug check uses $who, but $who is the user we're
looking at, not the current user.

I noticed this while looking into bug 193575. You need to know the email of
someone who has voted on the bug, though.

I think the fix is to just grab the Bugzilla->user->id, but I'm not sure. Theres
a lot of odd stuff in the code - the comment about

    # After DBNameToIdAndCheck is templatised and prints a Content-Type,
    # the above should revert to a call to that function, and this
    # special error handling should go away.

looks really old, too.

(I'm also not sure what all that LOCKing is about, but thats a separate bug)

Gerv?

Comment 1

[Bradley Baetz (:bbaetz)]

ping?

Comment 2

[Gervase Markham [:gerv]]

Good catch. Correct diagnosis, correct fix, and correct observation about
converting to DBNameToIDAndCheck.

Gerv

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Fixes the two issues raised.

Gerv

Comment 4

[Dave Miller [:justdave]]

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

so we're changing the way $who is defined, but then we're not using $who
anyway?

Does this problem exist in 2.16.3?

Comment 5

[Gervase Markham [:gerv]]

We aren't changing $who, and it continues to be used elsewhere in the code. The
two fixes are unrelated - one is a security fix, the other is a simple tidy-up.

Gerv

Comment 6

[Bradley Baetz (:bbaetz)]

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

Bugzilla->user will be undef if they're not logged in. You need to use a temp
var for 0 vs the id, unless CanSeeBug takes the user object directly

Comment 7

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Try this, then.

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

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

Err, no, you need:

my $userid = Bugzilla->user ? Bugzilla->user->id : 0;

We can access ->login diretly because theres an eralier check that FORM{'user'}
exists if we're not logged in. That doesn't work for this, though

Comment 9

[Gervase Markham [:gerv]]

Attachment: Patch v.3 (for tip)

Ok, how about this, then? :-)

Gerv

Comment 10

[Dave Miller [:justdave]]

Does this problem exist in 2.16.3?

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 127677 [details] [diff] [review]
Patch v.3 (for tip)

r=bbaetz

dave - looks like it from teh code, bnut I haven't tested. We can't use this
patch for 2.16 though, ebcause of of the Content-Type stuff.

Comment 12

[Dave Miller [:justdave]]

CCing Gavin since he reported this bug to me in email this weekend.

Comment 13

[Dave Miller [:justdave]]

we still need a patch for the 2.16 branch, or verification that branch isn't
affected.  We'll do same procedure as last time, once the patches are ready,
we'll flag approval? and let it sit until we have the advisory ready to go
before we check it in.

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.1a (for 2.16)

I've confirmed that this is a problem in 2.16. Here's a first attempt at a
patch.

Gerv

Comment 15

[Dave Miller [:justdave]]

Comment on attachment 130560 [details] [diff] [review]
Patch v.1a (for 2.16)

tested on landfill, it works.

Comment 16

[Bradley Baetz (:bbaetz)]

Comment on attachment 130560 [details] [diff] [review]
Patch v.1a (for 2.16)

Looks fine, although haven't tested

Comment 17

[Dave Miller [:justdave]]

Checked in on trunk:

Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.15; previous revision: 1.14
done

Comment 18

[Dave Miller [:justdave]]

Checked in on 2.16 branch:

Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.3.2.1; previous revision: 1.3
done

Comment 19

[Dave Miller [:justdave]]

Security advisory has been posted.