289365 - incorrect (?) group check in process_bug.cgi at lines 1464-1475

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

my $newproduct_id = $oldhash{'product_id'};
    if ((defined $::FORM{'product'})
        && ($::FORM{'product'} ne $::FORM{'dontchange'})) {
        my $newproduct_id = get_product_id($::FORM{'product'});
    }

Due to the second "my" in the IF block, $newproduct_id is not updated out of the
IF block when reassigning a bug to a new product. Later, we have

    SendSQL("SELECT id, membercontrol 
             FROM groups LEFT JOIN group_control_map
             ON id = group_id
             AND product_id = $newproduct_id WHERE isactive != 0");

Then, group IDs and member controls are the ones of the old group instead of the
new one.

Either should we remove the second "my" (the one in the IF block) to update
$newproduct_id to the new product ID or this IF block should be removed completely.

I don't think this is exploitable though, so I don't set the security flag.

Comment 1

[Frédéric Buclin]

(In reply to comment #0)
> Then, group IDs and member controls are the ones of the old group instead of 

err... I meant "old product".

Comment 2

[Dave Miller [:justdave]]

At first glance, this allows someone to move a bug into a product they don't
have access to.

1) They'd either have to know the product existed anyway or be brute forcing
names (which would make a lot of noise when they got it right)
2) They still only have access to that bug.

If my analysis is correct, then this is indeed not a security issue, however
it's definitely worthy of fixing.

I'll defer to joel since it's his code, but the correct answer to me appears to
be to remove the "my" from the one inside the if block.

Comment 3

[Frédéric Buclin]

Note that by removing the "my" from the IF block, we don't need line 1620 anymore:

my $newproduct_id = get_product_id($::FORM{'product'});

as $newproduct_id will already be updated earlier.

Comment 4

[Dave Miller [:justdave]]

So does that mean this isn't actually broken, but someone previously fixed it
and missed the obivious?

Comment 5

[Joel Peshkin]

Actually, this goes way back.  It appears that the first section (line 1464)
incorrectly but harmlessly re-applies the old product's group controls.  Then,
the later code (around line 1620) correctly applies the new group controls.

After that, it needs a real careful round of review and testing.

Comment 6

[Frédéric Buclin]

Per discussion with joel in IRC, it appears that lines 1495-1538 apply to the
product we are moving from and lines 1642-1750 to the product we are moving to.
Then the actual code seems correct and the IF block would be useless. But joel
prefers to delay any change here till we release 2.18.1 and 2.19.3. This sounds
reasonable and then I ask justdave to move the blocking flag from 2.18.1 to 2.18.2.

Reassigning to joel too.

Comment 7

[Dave Miller [:justdave]]

On the premise that this is not actually broken, but is just a code correctness
issue, this no longer qualifies for a branch checkin.  The code may be ugly and
unnecessary, but it's not broken.

Comment 8

[Frédéric Buclin]

We should avoid changing (working) group validation routines on the 2.20 branch.
Let's do it on the tip only. Retargetting to 2.22.

Comment 9

[victory <[e-mail]>]

this doesn't exist any more

Comment 10

[Frédéric Buclin]

This bug is still valid. We have to make sure everything is OK there. And only a part of the code has gone.

Comment 11

[Frédéric Buclin]

I fixed this problem as part of bug 338796. And bug 347213 changed this part of the code completely anyway, so there is no need to worry about it anymore.