Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Creating/Changing Bugs
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bbaetz, Assigned: Max Kanat-Alexander)

Tracking

3.1.1
Bugzilla 3.2
Bug Flags:
approval +
approval3.4 +
blocking3.4 +
approval3.2 +
blocking3.2.4 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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?

Comment 1

8 years ago
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

Comment 2

8 years ago
(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

8 years ago
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.
Attachment #380561 - Flags: review?(wicked)

Updated

8 years ago
Summary: User without canconfirm can create confirmed bug → [SECURITY] Reporters without canconfirm privileges can confirm their own bugs
(Assignee)

Comment 4

8 years ago
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-
(Assignee)

Comment 5

8 years ago
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

8 years ago
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)

Comment 7

8 years ago
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.
(Assignee)

Comment 8

8 years ago
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.
(Reporter)

Comment 9

8 years ago
This will probably fix bug 482654, too.

Comment 10

8 years ago
(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.
(Assignee)

Comment 11

8 years ago
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

8 years ago
(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.
(Assignee)

Comment 13

8 years ago
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.
Attachment #387006 - Flags: review?(LpSolit)

Comment 14

8 years ago
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-
(Assignee)

Comment 15

8 years ago
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

8 years ago
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.
(Assignee)

Comment 17

8 years ago
Ahhh. Hmm, seems like a different bug, though, really.
(Reporter)

Comment 18

8 years ago
(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

8 years ago
(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.
(Assignee)

Comment 20

8 years ago
Created attachment 387245 [details] [diff] [review]
v3, mkanat

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)

Updated

8 years ago
Attachment #387245 - Flags: review?(LpSolit) → review+

Comment 21

8 years ago
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

8 years ago
Tested on 3.5 and 3.2.3+. No problem found.
Flags: approval?
Flags: approval3.4?
Flags: approval3.2?
(Assignee)

Updated

8 years ago
Blocks: 503046
(Assignee)

Comment 23

8 years ago
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+
(Assignee)

Comment 24

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 25

8 years ago
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.