Last Comment Bug 495257 - [SECURITY] Reporters without canconfirm privileges can confirm their own bugs
: [SECURITY] Reporters without canconfirm privileges can confirm their own bugs
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 3.1.1
: All All
: -- normal (vote)
: Bugzilla 3.2
Assigned To: Max Kanat-Alexander
: default-qa
Mentors:
Depends on:
Blocks: 503046
  Show dependency treegraph
 
Reported: 2009-05-28 09:32 PDT by Bradley Baetz (:bbaetz)
Modified: 2009-07-08 08:16 PDT (History)
3 users (show)
mkanat: approval+
mkanat: approval3.4+
LpSolit: blocking3.4+
mkanat: approval3.2+
LpSolit: blocking3.2.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (5.42 KB, patch)
2009-05-29 18:30 PDT, Frédéric Buclin
no flags Details | Diff | Review
v2, mkanat (1014 bytes, patch)
2009-07-06 10:23 PDT, Max Kanat-Alexander
LpSolit: review-
Details | Diff | Review
v3, mkanat (1.77 KB, patch)
2009-07-07 12:55 PDT, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Review

Description Bradley Baetz (:bbaetz) 2009-05-28 09:32:58 PDT
The workflow allows RESOLVED to both UNCONFIRMED and REOPENED. If a bug has never been confirmed, then only UNCONFIRMED is allowed.

However, this is restricted by the UI in template/en/default/bug/knob.html.tmpl rather than any code. This means that by editing the HTML its possible for a user without canconfirm or canedit to confirm the bug:

 1. File bug as UNCONFIRMED
 2. Resolve it (any resolution)
 3. Load show_bug.cgi
 4. Using firebug, change the UNCONFIRMED <option> value to REOPENED
 5. Submit change
 6. Bug now has everconfirmed set (although its still in REOPENED).
 7. Repeat steps 4-5, using NEW instead.
 8. Bug is in NEW status

This should be enforced in the code, either by changing the statuses returned from Bugzilla::Status->can_change_to (which would then allow the UI hacks to be removed from the template), or in process_bug.

I've traced this back to bug 344965, which removed the |$status = $self->everconfirmed ? 'REOPENED' : 'UNCONFIRMED';| logic in favour of the UI stuff, as part of the custom workflow transition. I'm not sure of the best way to fix this.

The only code restriction on changes required canconfirm on changes from UNCONFIRMED to any other open state. (in check_can_change_field) If process_bug is changed then that should instead restrict setting any open state other than UNCONFIRMED if everconfirmed is false.

However, that doesn't stop other manipulation of the HTML (steps 7-8 above), so there would also need to be a requirement for editbugs on any open_state->open_state transition other than UNCONFIRMED->NEW (which only requires canconfirm.)
Comment 1 Frédéric Buclin 2009-05-28 10:13:07 PDT
Taking. The right fix is to fix _check_bug_status() to filter valid statuses returned by Bugzilla::Status->can_change_to(). I don't want Bugzilla::Status to look at user privs, at least for now.
Comment 2 Frédéric Buclin 2009-05-28 10:38:44 PDT
(In reply to comment #1)
> Taking. The right fix is to fix _check_bug_status()

Hum... or check_can_change_field(). I will see what's best.
Comment 3 Frédéric Buclin 2009-05-29 18:30:42 PDT
Created attachment 380561 [details] [diff] [review]
patch, v1

I moved the logic from bug/edit.html.tmpl to check_can_change_field(). This checker is automatically called by _set_global_validator() which is triggered by set_status(), so we are sure this check is always done. I also added missing checks.
Comment 4 Max Kanat-Alexander 2009-06-02 22:37:24 PDT
Comment on attachment 380561 [details] [diff] [review]
patch, v1

  Instead of all of this, just check everconfirmed. If it's changing from 0 to 1, then you should be restricting based on canconfirm. There's already a _set_everconfirmed, and you could create a validator for everconfirmed that would be way simpler than all the check_can_change_field stuff, I think.
Comment 5 Max Kanat-Alexander 2009-06-02 22:37:59 PDT
Or you could keep it in check_can_change_field and just check if "everconfirmed" is changing and the user isn't in the canconfirm group, right?
Comment 6 Frédéric Buclin 2009-06-03 03:43:11 PDT
Comment on attachment 380561 [details] [diff] [review]
patch, v1

My patch does much more than just checking canconfirm/everconfirmed. It adds all the missing checks related to bug status changes. Also, set_status() is called before _set_everconfirmed(), and this function has no checker associated with it. I'm fine with how I did it.
Comment 7 Frédéric Buclin 2009-06-03 03:55:46 PDT
Another argument in favor of check_can_change_field() is that we can call it from templates to know if some specific bug status transition is available or not for the specific user.
Comment 8 Max Kanat-Alexander 2009-06-04 16:45:17 PDT
Comment on attachment 380561 [details] [diff] [review]
patch, v1

>Index: Bugzilla/Bug.pm
>+    if ($field eq 'bug_status') {
>+        my $old_open = is_open_state($oldvalue);
>+        my $new_open = is_open_state($newvalue);

  AFAIK, it doesn't matter if the statuses are open or closed--all that matters is if everconfirmed is becoming 1, and check_can_change_field *is* called for everconfirmed, I'm pretty sure (thanks to _set_global_validator).

>+        # XXX - If there are two or more confirmed open states available when
> [snip]

  I don't understand this comment, since you can't do two status transitions in one process_bug call.

>+        elsif (!$old_open && $new_open) {
>+            if ($newvalue ne 'UNCONFIRMED' && !$self->everconfirmed
>+                && !$user->in_group('canconfirm', $self->{'product_id'}))

  You could just check $field eq 'everconfirmed' becoming 1, here.

>+            # Only the assignee and QA contact can reset the everconfirmed bit.
>+            elsif ($newvalue eq 'UNCONFIRMED' && $self->everconfirmed && !$has_role) {
>+                $$PrivilegesRequired = 2;
>+                return 0;
>+            }

  And it becoming 0 here.

>Index: template/en/default/bug/knob.html.tmpl
>-    [% NEXT IF bug.isunconfirmed && bug_status.is_open && !bug.user.canconfirm %]
>-    [% NEXT IF bug.isopened && !bug.isunconfirmed && bug_status.is_open && !bug.user.canedit %]
>-    [% NEXT IF (!bug_status.is_open || !bug.isopened) && !bug.user.canedit && !bug.user.isreporter %]
>+    [% NEXT UNLESS bug.check_can_change_field("bug_status", bug.bug_status, bug_status.name) %]

  Yeah, that is a nice change. I still think the logic would be simpler in the backend if you were checking for a change in everconfirmed, though.
Comment 9 Bradley Baetz (:bbaetz) 2009-06-27 03:25:54 PDT
This will probably fix bug 482654, too.
Comment 10 Frédéric Buclin 2009-07-03 16:41:18 PDT
(In reply to comment #8)
>   AFAIK, it doesn't matter if the statuses are open or closed--all that matters
> is if everconfirmed is becoming 1, and check_can_change_field *is* called for
> everconfirmed, I'm pretty sure (thanks to _set_global_validator).

No, that's not enough. Allowed transitions are not the same if a bug status is open or closed.


>   I don't understand this comment, since you can't do two status transitions in
> one process_bug call.

This comment means that if the first change is NEW_1 -> CLOSED_1, and the 2nd change is CLOSED_1 -> NEW_2, check_can_change_field() cannot prevent that. So I don't mean to do this in a single process_bug.cgi call.


>   You could just check $field eq 'everconfirmed' becoming 1, here.

$field eq 'everconfirmed' doesn't exist. It never has this value.
Comment 11 Max Kanat-Alexander 2009-07-04 06:11:55 PDT
Oh, weird. If there's no validator for a field, then set_global_validator isn't called. We should probably change that, and then you can just check if everconfirmed is changing! Very easy, and fixes this bug. :-)
Comment 12 Frédéric Buclin 2009-07-05 11:35:21 PDT
(In reply to comment #11)
> Oh, weird. If there's no validator for a field, then set_global_validator isn't
> called. We should probably change that, and then you can just check if
> everconfirmed is changing! Very easy, and fixes this bug. :-)

This is too restrictive. My patch does much more than that.
Comment 13 Max Kanat-Alexander 2009-07-06 10:23:28 PDT
Created attachment 387006 [details] [diff] [review]
v2, mkanat

As far as I can tell, this fixes the bug reported and described in comment 0 without any regressions.
Comment 14 Frédéric Buclin 2009-07-06 13:57:56 PDT
Comment on attachment 387006 [details] [diff] [review]
v2, mkanat

This doesn't fix the 2nd part of comment 0 about open -> open transitions.

I will look if mixing your patch with mine can make things easier to write.
Comment 15 Max Kanat-Alexander 2009-07-06 13:59:53 PDT
I'm sorry, but I don't understand the second half of comment 0. Could you explain a series of steps necessary to reproduce the problem?
Comment 16 Frédéric Buclin 2009-07-06 14:08:08 PDT
For instance, as the reporter, try to change a bug from NEW to ASSIGNED by hacking the URL. The template explicitly forbirds this:

 [% NEXT IF bug.isopened && !bug.isunconfirmed && bug_status.is_open && !bug.user.canedit %]

But check_can_change_field() doesn't check this.
Comment 17 Max Kanat-Alexander 2009-07-06 21:32:52 PDT
Ahhh. Hmm, seems like a different bug, though, really.
Comment 18 Bradley Baetz (:bbaetz) 2009-07-07 04:57:48 PDT
(In reply to comment #17)
> Ahhh. Hmm, seems like a different bug, though, really.

No, thats the point of this bug - the restrictions need to be in the backend.

Otherwise someone without canedit can do NEW->ASSIGNED (and just checking for canedit doesn't help because it breaks the workflow)
Comment 19 Frédéric Buclin 2009-07-07 05:26:17 PDT
(In reply to comment #14)
> I will look if mixing your patch with mine can make things easier to write.

OK, I looked at the code again, and I still think my patch is the best and most robust way to fix the problem. This way, the whole check is in the backend rather than in the template. Including everconfirmed here doesn't help.
Comment 20 Max Kanat-Alexander 2009-07-07 12:55:32 PDT
Created attachment 387245 [details] [diff] [review]
v3, mkanat

Okay, I think this handles all cases.
Comment 21 Frédéric Buclin 2009-07-07 15:26:43 PDT
Comment on attachment 387245 [details] [diff] [review]
v3, mkanat

Wow, you managed to handle all possible transitions in a simpler way. Nice job! r=LpSolit for tip (I still have to test it on branches)
Comment 22 Frédéric Buclin 2009-07-07 16:45:03 PDT
Tested on 3.5 and 3.2.3+. No problem found.
Comment 23 Max Kanat-Alexander 2009-07-08 06:27:24 PDT
Okay, we're ready to release. I'm going to check this in, now.
Comment 24 Max Kanat-Alexander 2009-07-08 06:56:59 PDT
tip:

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

3.4:

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

3.2:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.241.2.22; previous revision: 1.241.2.21
done
Comment 25 Max Kanat-Alexander 2009-07-08 08:16:01 PDT
Security advisory sent, removing from security group.

Note You need to log in before you can comment on or make changes to this bug.