Closed Bug 495257 Opened 11 years ago Closed 11 years ago

[SECURITY] Reporters without canconfirm privileges can confirm their own bugs

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

3.1.1
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: bbaetz, Assigned: mkanat)

References

Details

Attachments

(1 file, 2 obsolete files)

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.)
Flags: blocking3.4?
Flags: blocking3.2.4?
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.
Assignee: create-and-change → LpSolit
Status: NEW → ASSIGNED
Flags: blocking3.4?
Flags: blocking3.4+
Flags: blocking3.2.4?
Flags: blocking3.2.4+
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → Bugzilla 3.2
(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.
Attached patch patch, v1 (obsolete) — Splinter Review
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.
Attachment #380561 - Flags: review?(wicked)
Summary: User without canconfirm can create confirmed bug → [SECURITY] Reporters without canconfirm privileges can confirm their own bugs
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.
Attachment #380561 - Flags: review?(wicked) → review-
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 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.
Attachment #380561 - Flags: review- → review?(wicked)
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 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.
This will probably fix bug 482654, too.
(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.
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. :-)
(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.
Attached patch v2, mkanat (obsolete) — Splinter Review
As far as I can tell, this fixes the bug reported and described in comment 0 without any regressions.
Attachment #387006 - Flags: review?(LpSolit)
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.
Attachment #387006 - Flags: review?(LpSolit) → review-
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?
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.
Ahhh. Hmm, seems like a different bug, though, really.
(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)
(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.
Attached patch v3, mkanatSplinter Review
Okay, I think this handles all cases.
Assignee: LpSolit → mkanat
Attachment #380561 - Attachment is obsolete: true
Attachment #387006 - Attachment is obsolete: true
Attachment #387245 - Flags: review?(LpSolit)
Attachment #380561 - Flags: review?(wicked)
Attachment #387245 - Flags: review?(LpSolit) → review+
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)
Tested on 3.5 and 3.2.3+. No problem found.
Flags: approval?
Flags: approval3.4?
Flags: approval3.2?
Blocks: 503046
Okay, we're ready to release. I'm going to check this in, now.
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval3.2?
Flags: approval3.2+
Flags: approval+
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Security advisory sent, removing from security group.
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.