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.

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.

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.

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?)

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

I'll test it.

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

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.

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().

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

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

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

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.

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.

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

Comment on attachment 199660 [details] [diff] [review] Patch with security fix wrong file :-)

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 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.

>> + [% title = "Not allowed" %] Probably that should be "Allowed", especially since the following title in the diff appears capitalized.

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.

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

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.

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

(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.

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 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.

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

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

The patch applies cleanly and works correctly on 2.20 too.

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

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

<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.

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

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