141593 - You can add/remove dependencies on bugs you can't see

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P2)

Description

[Stephan Hohe]

To change the dependencies of a bug you don't have to have any rights. Anyone
with a bugzilla account can do this with any bug he can access. I think this is
insecure and a normal user without special rights should only be able to modify
the dependencies for his own bugs.

Comment 1

[Matthew Tuck [:CodeMachine]]

Are you sure you don't have 'editbugs' access?  The policy for Bugzilla has
always been fairly open in this way and will continue to be until the
administrator can define the security policy (bug #110947).

Moving to 2.18 on the proviso you can edit dependencies without editbugs.

Comment 2

[Joel Peshkin]

These unloved bugs have been sitting untouched since June 2002 or longer.  If
nobody does anything else to them, they certainly won't make 2.18
Retargetting to 2.20.  If you really plan to push them right now, you might pull
them back in.

Comment 3

[Max Kanat-Alexander]

Reporter never responded or confirmed. He obviously had *some* permissions,
since he filed the bug as NEW. (Or did we not have UNCONFIRMED back then?)

Comment 4

[Max Kanat-Alexander]

OK, OK... 

<justdave1> I'm pretty sure that still worked that way, but I'm not positive
<justdave1> we'd have to test it on landfill I suppose

Comment 5

[Max Kanat-Alexander]

I'll test it.

Comment 6

[Frédéric Buclin]

As far as I know, anyone is allowed to change dependencies, even without any privs.

Comment 7

[Max Kanat-Alexander]

Really, it should require editbugs, I think, to change a dep.

The problem is: What if bug A is in a "canedit" group, and bug B is not? I don't
have access to bug A, but I can make it block bug B and thus modify it.

I think that's OK, though. As long as the user can edit either this bug or the
one they're entering/removing, they should be able to mark it. That would be
better than how it works now.

Comment 8

[Frédéric Buclin]

I think ValidateBugID() does not let you add a bug # to the dependency list if
you cannot access that bug, see process_bug.cgi line 126. IMO, "dependson" and
"blocked" fields should be checked through CheckCanChangeField().

Comment 9

[Joel Peshkin]

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

Comment 10

[Joel Peshkin]

LpSolit	joel: in ValidateBugID:
	LpSolit	return if (defined $field && ($field eq "dependson" || $field eq
"blocked"));
	LpSolit	
	LpSolit	return if $user->can_see_bug($id);
	LpSolit	the function returns before checking whether the user can access the bug
	joel	LpSolit: Is that desired behavior for some reason?
	joel	(If so, we should only do it if strictisolation is not set)
	LpSolit	joel: yes, because if you, as a power user, add this bug to the list,
and then me, poor powerless user, add a second bug to the list, it will complain
that one of the bugs (the one you added) is not accessible by me
	LpSolit	joel: so ValidateBugID only makes sure you gave a valid bug ID or
alias, not whether you can access it or not
	joel	That part is OK, but it should know the diference between me adding one
and me leaving one on the list.
	LpSolit	joel: then the code should only consider newly added bugs
	LpSolit	not the existing ones
	joel	I agree....
	LpSolit	so we have to do two things here:
	joel	though it makes an interesting question for removing them.
	LpSolit	1) centralize the code about this check (actually duplicated in both
post_bug.cgi and process_bug.cgi) and only grep newly added bugs
	joel	I'm inclined not to let a user remove a dep on a bug he cannot see either.
	LpSolit	2) force ValidateBugID to do its check completely, as it would only
have to check newly added bugs
	LpSolit	joel: I agree
	LpSolit	joel: so we should take the diff of the two arrays, not only new ones
	joel	yes
	LpSolit	array = dep list

Comment 11

[Frédéric Buclin]

We could even update the existing Bug::ValidateDependencies() to do this check.

Comment 12

[André Batosti]

Attachment: batosti_v1

Comment 13

[Joel Peshkin]

Comment on attachment 199474 [details] [diff] [review]
batosti_v1

This works to prevent a user from adding a dependency, but it does not prevent
the user from removing a dependency.  Since removing the dependency is also an
edit to the other bug, I think those should be stopped as well.

So... I think this needs to locate any values in $bug->field that are no longer
listed and should do the validation on those as well.

Comment 14

[Joel Peshkin]

Attachment: v2 - compare lists

OK, this took a bit more of a rewrite...

Dependencies can only change when editing a single bug, so we enforce that.
Then, we compare the list of numeric ids in each field before and after the
change and validate that the user can see all of the differrences.

Comment 15

[Joel Peshkin]

Attachment: patch - v3

Comment 16

[Joel Peshkin]

Ewww.... 

lpsolit is correct...  Prior to this, ANY user (even without editbugs) can make
massive dependency changes independent of privilges.

Comment 17

[Joel Peshkin]

Attachment: Patch with security fix

Comment 18

[Joel Peshkin]

Attachment: Patch with security fix

Comment 19

[Joel Peshkin]

Attachment: Patch with security fix

Comment 20

[Joel Peshkin]

Comment on attachment 199660 [details] [diff] [review]
Patch with security fix

wrong file :-)

Comment 21

[Joel Peshkin]

Comment on attachment 199659 [details] [diff] [review]
Patch with security fix

ugh! and BMO ISE'd when I attached this and duplicate d it

Comment 22

[Joel Peshkin]

Attachment: The right file

Comment 23

[Joel Peshkin]

Comment on attachment 199661 [details] [diff] [review]
The right file

One more change is needed.

Now that this is aware of the distinction between dependencies REMAINING versus
dependecies CHANGINE, this needs to ensure that the user is permitted to edit
the other bug's product, not just see it.

The question is, must they be able to edit the bug itself?  I think a user
without editbugs should be permitted to add a dependency between a bug he
reported and a bug that someone else did as long as the other product is not
totally read-only to him.

This means that post_bug needs an update as well.

Comment 24

[Joel Peshkin]

Attachment: Patch for HEAD/2_20

Comment 25

[Vlad Dascalu]

>> +    [% title = "Not allowed" %]

Probably that should be "Allowed", especially since the following title in the
diff appears capitalized.

Comment 26

[Dave Miller [:justdave]]

I'm not sure how much this is really a security issue...  it's always been this
way, and this bug was filed in 2002, and the security flag was only added last
week.  It's not like people don't know about this.  It's generally been enforced
by social engineering.

Adding tighter controls on it is certainly a very good idea, but it's probably
not necessary to backport very far, and not worthy of an advisory (at least not
one specifically for that - it's certainly worth release notes and a mention on
the new features list).  We have a whole mess of fields which anyone used to be
able to change, which have only been locked down to editbugs in the last couple
versions of Bugzilla, and we never did advisories on those.  (The only exception
was when the UI *did* hide fields for people without privs, thus giving everyone
the impression they couldn't change it, but the back end didn't enforce it, so
someone could manually tweak the URL to change it - that we did an advisory for)

See also bug 219605, which is probably more of a security issue than this is.

Comment 27

[Frédéric Buclin]

I think we should fix this bug for the 2.20 branch and tip only, and remove the
security flag.

Comment 28

[Frédéric Buclin]

Comment on attachment 199774 [details] [diff] [review]
Patch for HEAD/2_20

>Index: post_bug.cgi

>+            my $changedbug = new Bugzilla::Bug($id, $user->id);
>+            if (!CanEditProductId(get_product_id($changedbug->product))) {
>+                $vars->{'field'} = $field;
>+                ThrowUserError("illegal_change_deps", $vars);
>+            }

I think this is too restrictive. If the user can see a bug, we should let him
add a dependency on it.


>-# Gather the dependency list, and make sure there are no circular refs
>+# Gather thedependency list, and make sure there are no circular refs

Nit: err... keep the whitespace.



>Index: process_bug.cgi

> foreach my $field ("dependson", "blocked") {

>         my @validvalues;

Nit: we don't use this variable anymore. Remove it.


>         foreach my $id (split(/[\s,]+/, $cgi->param($field))) {
>             ValidateBugID($id, $field);

Nit: here, we call ValidateBugID(), and then we call it again on the same bug
(but with no 2nd param). Is there no way to avoid calling it twice for each new
bug?


>+        my ($added, $removed) = Bugzilla::Util::diff_arrays(\@old, \@new);
>+        foreach my $id (@$added , @$removed) {

If the actual list is @old = "1, 2, 3", and now I write it as @new = "3, 1, 2",
what will be returned by Util::diff_arrays()? Shouldn't we sort the  @new list
first?


>+            my $changedbug = new Bugzilla::Bug($id, $user->id);
>+            if (!CheckCanChangeField($field, $id, '', $id)
>+                || !CanEditProductId(get_product_id($changedbug->product))) {

2 things:
- If you only want to test whether the field can be changed, I prefer to write
  CheckCanChangeField($field, $id, 0, 1) rather than (..., '', $id) which are
not the real new values for this field.

- Here again, I think the call to CanEditProductId() is too restrictive. After
all, I can edit any bug I can see by marking a bug as a dupe of this not
editable bug. As long as you can see a bug, I think blocking it or depending on
it is fine. Maybe should we keep this more restrictive condition for another
bug, till a consensus is found.


>+            if (!CheckCanChangeField($field, $bug->bug_id, '', $id)) {
>+                $vars->{'field'} = $field;
>+                ThrowUserError("illegal_change_deps", $vars);
>+            }

With my previous comment, this check is no longer required.


>+    } else {
>+        $cgi->delete($field);

Please add a comment why you delete this field when changing several bugs at
once.

Comment 29

[Joel Peshkin]

Attachment: Patch for HEAD and 2_20 with just the univesally releven portion

This has the strict_isolation portion removed so it can be done as another bug
on HEAD only.

Comment 30

[Joel Peshkin]

(In reply to comment #28)

> If the actual list is @old = "1, 2, 3", and now I write it as @new = "3, 1, 2",
> what will be returned by Util::diff_arrays()? Shouldn't we sort the  @new list
> first?
> 
> 

diff_arrays is smart enough for this.

Comment 31

[Dave Miller [:justdave]]

ok, nobody's voiced any objections yet, and I've had several people voice
approval to me in IRC, so going ahead and pulling the flag.

Comment 32

[Frédéric Buclin]

Comment on attachment 199886 [details] [diff] [review]
Patch for HEAD and 2_20 with just the univesally releven portion

>Index: process_bug.cgi

>+            my $changedbug = new Bugzilla::Bug($id, $user->id);
>+            if (!CheckCanChangeField($field, $id, 0, 1)) {
>+                $vars->{'field'} = $field;
>+                ThrowUserError("illegal_change_deps", $vars);
>+            }
>+            if (!CheckCanChangeField($field, $bug->bug_id, 0, 1)) {
>+                $vars->{'field'} = $field;
>+                ThrowUserError("illegal_change_deps", $vars);
>+            }

The error message is incorrect when you cannot edit the "other" bug (not the
one you are currently editing) as it mentions the wrong field (the depenson
field for this bug is the blocked field for the other bug). Moreover, when you
add several bugs at once in the dependency fields, saying that you need to have
permissions on both bugs is confusing as you may have more than 2 bugs
involved, and "both" isn't precise enough.

I suggest two ways to fix this problem:

1) Either only prevent the user from editing the dependency fields if he cannot
edit the *current* bug only (second test above). This is my preferred solution
(I consider this condition to be restrictive enough);

2) Or merge both tests in a single one and mention in the error message the
bugs you are talking about:

if (!CheckCanChangeField($field, $bug->bug_id, 0, 1)
    || !CheckCanChangeField($field, $id, 0, 1))
{
    $vars->{'field'} = $field;
    $vars->{'current_bug'} = $bug->bug_id;
    $vars->{'dep_bug'} = $id;
    ThrowUserError("illegal_change_deps", $vars);
}

Nit: "my $lastbugid = 0;" must be defined earlier in process_bug.cgi, else a
warning is reported in the apache error log saying that this variable is
undefined.



>Index: template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "illegal_change_deps" %]
>+    [% title = "Not allowed" %]
>+    You tried to change the 
>+    <strong>[% field_descs.$field FILTER html %]</strong> field 
>+    but only
>+    a user with permissions for both [% terms.bugs %] may change that field.

As said above, mention the two bugs you are talking about.

Comment 33

[Joel Peshkin]

Attachment: patch v++

OK,  this now only requires that the user can EDIT the current bug and be able
to SEE the other bug.

Comment 34

[Frédéric Buclin]

Comment on attachment 199934 [details] [diff] [review]
patch v++

>Index: process_bug.cgi

>         foreach my $id (split(/[\s,]+/, $cgi->param($field))) {
>-            next unless $id;

You have to keep this line. Writing ' 100000' (i.e. with a leading whitespace)
makes ValidateBugID() to complain.


>+            ValidateBugID($id);
>+            my $changedbug = new Bugzilla::Bug($id, $user->id);

Nit: $changedbug is not used anymore; remove it.


Please fix these comments on checkin. r=LpSolit

Comment 35

[Frédéric Buclin]

The patch applies cleanly and works correctly on 2.20 too.

Comment 36

[Dave Miller [:justdave]]

altering summary to reflect the final patch, in hopes of an accurate check-in
message.

Comment 37

[Dave Miller [:justdave]]

temporarily revoking approval for the 2.20 branch in light of the revelation
that this patch blocks people with canconfirm but without editbugs from changing
dependencies, pending discussion with Asa, since I have a feeling the triage
folks do this a lot on b.m.o

Comment 38

[Dave Miller [:justdave]]

<Asa_meeting> justdave: that'll be fine with me.
<Asa_meeting> we don't have people with canconfirm but not edit bugs doing any
dependency changes.
<justdave> ok, cool
<justdave> timeless said there were people doing their own tracking bugs :)
<Asa_meeting> if they don't have edit bugs and they're doing tracking bugs,
that's wrong.
* Asa_meeting wants to bind can confirm and edit bugs at bmo. I always hand them
out in pairs.
<Asa_meeting> for anyone that needs to do any triage at all, you need edit.

Comment 39

[Joel Peshkin]

    On HEAD

    Checking in post_bug.cgi;
    /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
    new revision: 1.128; previous revision: 1.127
    done
    Checking in process_bug.cgi;
    /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
    new revision: 1.289; previous revision: 1.288
    done
    Checking in template/en/default/global/user-error.html.tmpl;
    /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tm
    pl,v  <--  user-error.html.tmpl
    new revision: 1.135; previous revision: 1.134
    done


    And 2.20 Branch

    Checking in post_bug.cgi;
    /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
    new revision: 1.118.2.2; previous revision: 1.118.2.1
    done
    Checking in process_bug.cgi;
    /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
    new revision: 1.263.2.4; previous revision: 1.263.2.3
    done
    Checking in template/en/default/global/user-error.html.tmpl;
    /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tm
    pl,v  <--  user-error.html.tmpl
    new revision: 1.115.2.7; previous revision: 1.115.2.6
    done

Comment 40

[Frédéric Buclin]

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