[SECURITY] keywords and unused flag types can be deleted by bypassing the token check

RESOLVED FIXED in Bugzilla 2.22

Status

()

Bugzilla
Administration
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Stephen Lee, Assigned: Frédéric Buclin)

Tracking

2.22.1
Bugzilla 2.22
Bug Flags:
approval +
approval3.2 +
blocking3.2.1 +
approval3.0 +
blocking3.0.7 +
approval2.22 +
blocking2.22.7 +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
+++ This bug was initially created as a clone of Bug #281181 +++

This may have been covered before a long time ago, and not been considered an issue, but if I could get an admin to click on the following URLs (which are intentionally broken to avoid any accidents, but you get the point), this could really mess things up, couldn't it? 

http://[webserver]/editkeywords.cgi?action=delete&id=[number]
http://[webserver]/editflagtypes.cgi?action=confirmdelete&id=[number]

... and especially if it is in fact a script or a "page" with embedded "images" or iframes enumerating all possible values of [number].

Would it make sense for an equivalent "token" to be embedded in the URL for keyword/flag/(etc.?) deletion? ... or for any other significant privileged action accessed via GET?

A malicious user would not know what to supply to make an admin click on (e.g.):

http://[webserver]/editkeywords.cgi?action=delete&id=[number]&token=[token]

... and unlike "all possible values of [number]", certainly couldn't enumerate "all possible values of [token]" too.
(Assignee)

Comment 1

9 years ago
Did you check the code recently? *All* administrative pages use tokens to prevent such actions.
Group: bugzilla-security
Status: NEW → RESOLVED
Last Resolved: 9 years ago
No longer depends on: 281181
Resolution: --- → INVALID
(Reporter)

Comment 2

9 years ago
editkeywords.cgi and editflagtypes.cgi do not appear to use any token in 3.2rc2 (or at least, there is no 'token' in the page source, nor in the target URL of the delete links)

http://[webserver]/buglist.cgi?cmdtype=dorem&remaction=forget&namedcmd=[name]
while we're at it too... force any user to delete a saved search (maybe a popular shared one?)
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> editkeywords.cgi and editflagtypes.cgi do not appear to use any token in 3.2rc2

They all check tokens using check_token_data(). The saved search deletion is not a security bug, first because it has no security implications, and secondly because you cannot know other users' saved searches.
(Reporter)

Comment 4

9 years ago
On a test installation of 3.2rc2, the link for deleting a flag was:
http://10.96.96.20:8000/editflagtypes.cgi?action=confirmdelete&id=3

Copy-pasting that link from this very bug report to another browser window did in fact delete the flag. I see no token in the URL.
(Reporter)

Comment 5

9 years ago
Re-opening - maybe someone with appropriate permissions may care to put the security flag back on until there is something more than mere assertion to suggest this bug does not exist.


Have now checked source... the apparent bug for flags seems to be here:
http://mxr.mozilla.org/bugzilla/source/editflagtypes.cgi#461

463     my $token = issue_session_token('delete_flagtype');
464     deleteType($flag_type, $token);

the token that is checked in the deleteType function appears to be issued in the very same call to the CGI... this seems to be mitigated by only being possible for flags which are not currently used on any bugs though.


For keywords, there is a similar apparent bug in editkeywords.cgi, 
http://mxr.mozilla.org/bugzilla/source/editkeywords.cgi#159
159     # We need this token even if there is no bug using this keyword.
160     $token = issue_session_token('delete_keyword');
161 
162     if (!$cgi->param('reallydelete') && $keyword->bug_count) {
...
169     }
170     # We cannot do this check earlier as we have to check 'reallydelete' first.
171     check_token_data($token, 'delete_keyword');

the token checked appears to be one that was generated just a couple of lines of code earlier. Given that an attacker can arrange for $cgi->param('reallydelete') to have a "desired" value, the if(...) block is irrelevant so there would not appear to be similar mitigation for keywords.


A quick check of other uses of issue_session_token suggests that these are the only two instances where the token is used immediately rather than passed to a template.

(In reply to comment #3)
> The saved search deletion is
> not a security bug, first because it has no security implications, and
> secondly because you cannot know other users' saved searches.

Shared searches are visible to all members of the relevant group. e.g.
https://bugzilla.mozilla.org/buglist.cgi?cmdtype=dorem&remaction=run&namedcmd=Bugzilla%20blockers&sharer_id=120380

seems trivially modifiable to:
https://[munged-to-prevent-accidents]/buglist.cgi?cmdtype=dorem&remaction=forget&namedcmd=Bugzilla%20blockers

... which if someone were to persuade sharer_id=120380 to click on it might have somewhat undesired consequences, albeit less severe than a widely used keyword unexpectedly disappearing.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 6

9 years ago
OK, your last comment is much clearer and tokens are indeed incorrectly validated when deleting keywords and unused flag types. The shared queries problem is another, separate bug as they current don't use the token check at all.
Assignee: administration → LpSolit
Group: bugzilla-security
Severity: major → normal
Status: REOPENED → ASSIGNED
Summary: [SECURITY] It's way too easy to delete keywords/flags (etc...?) → [SECURITY] keywords and unused flag types can be deleted by bypassing the token check
Target Milestone: --- → Bugzilla 2.22
Version: 3.2 → 2.22.1
(Assignee)

Comment 7

9 years ago
The only way to correctly fix this bug is to 1) display the deletion confirmation page in all cases, as we do with other field values, and 2) separate the code related to the confirmation page and the deletion itself. editflagtypes.cgi and editkeywords.cgi have these flaws since their initial checkin 6-8 years ago. But I set the version field to 2.22.1 which is when we first introduced token checks in admin pages.
(Reporter)

Comment 8

9 years ago
(In reply to comment #6)
> OK, your last comment is much clearer...

Apologies if initial report was unclear - it was based merely on observing the HTML output and the content of the link that resulted in the destructive action - these had given the impression that these CGIs didn't use tokens at all, hence mirroring the previous report that had resulted in use of tokens for other destructive admin actions.


Adding further report for the separate bug...


(In reply to comment #7)
> The only way to correctly fix this bug is to 1) display the deletion
> confirmation page in all cases, as we do with other field values

Another way might be to issue a token one page earlier, so that it is in the original link?
http://[webserver]/editkeywords.cgi?action=delete&id=[number]&token=[token]
(Assignee)

Updated

9 years ago
Flags: blocking3.2.1+
Flags: blocking3.0.7+
Flags: blocking2.22.7+
(Assignee)

Comment 9

9 years ago
Created attachment 350498 [details] [diff] [review]
patch for 3.x, v1

This patch applies to all 3.x branches + trunk.
Attachment #350498 - Flags: review?(mkanat)
Keywords: 4xp
> https://[munged-to-prevent-accidents]/buglist.cgi?cmdtype=dorem&remaction=forget&namedcmd=Bugzilla%20blockers
>
> ... which if someone were to persuade sharer_id=120380 to click on it might
> have somewhat undesired consequences

For those following along at home this bit has been filed separately as bug 466748
(Assignee)

Updated

9 years ago
Keywords: 4xp
(Assignee)

Updated

9 years ago
Blocks: 468249

Updated

9 years ago
Attachment #350498 - Flags: review?(mkanat) → review+

Comment 11

9 years ago
Comment on attachment 350498 [details] [diff] [review]
patch for 3.x, v1

Looks good and works. :-)

Updated

9 years ago
Flags: approval?
Flags: approval3.2?
Flags: approval3.0?
(Assignee)

Comment 12

9 years ago
Created attachment 354062 [details] [diff] [review]
patch for 2.22.7, v1

Here is the backport for 2.22.7.
Attachment #354062 - Flags: review?(mkanat)
(Assignee)

Comment 13

9 years ago
Created attachment 354063 [details] [diff] [review]
patch for 3.x, v1.1

This updated patch for 3.x simply removes a now unused parameter in the keyword confirm-delete template. Carrying forward mkanat's r+.
Attachment #350498 - Attachment is obsolete: true
Attachment #354063 - Flags: review+

Comment 14

9 years ago
Comment on attachment 354062 [details] [diff] [review]
patch for 2.22.7, v1

Yeah, looks good to me. :-)
Attachment #354062 - Flags: review?(mkanat) → review+
(Assignee)

Comment 15

9 years ago
OK, this one has all the required backports and is now ready for checkin.
Flags: approval2.22?

Comment 16

9 years ago
Note that because there is extensive discussion of another security issue in this bug, I can not make this bug public until that other bug is fixed. So this bug will remain closed even when the security advisory is sent.
(Assignee)

Updated

9 years ago
Flags: approval?
Flags: approval3.2?
Flags: approval3.2+
Flags: approval3.0?
Flags: approval3.0+
Flags: approval2.22?
Flags: approval2.22+
Flags: approval+
(Assignee)

Comment 17

9 years ago
tip:

Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.55; previous revision: 1.54
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.47; previous revision: 1.46
done
Checking in template/en/default/admin/flag-type/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/admin/keywords/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/admin/keywords/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.12; previous revision: 1.11
done


3.2:

Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.53.2.1; previous revision: 1.53
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.45.2.1; previous revision: 1.45
done
Checking in template/en/default/admin/flag-type/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.9.2.1; previous revision: 1.9
done
Checking in template/en/default/admin/keywords/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.7.2.1; previous revision: 1.7
done
Checking in template/en/default/admin/keywords/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.11.2.1; previous revision: 1.11
done


3.0.6:

Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.49.2.1; previous revision: 1.49
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.43.2.1; previous revision: 1.43
done
Checking in template/en/default/admin/flag-type/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.7.2.1; previous revision: 1.7
done
Checking in template/en/default/admin/keywords/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.6.2.1; previous revision: 1.6
done
Checking in template/en/default/admin/keywords/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.9.2.2; previous revision: 1.9.2.1
done


2.22.6:

Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.31.2.2; previous revision: 1.31.2.1
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.35.2.3; previous revision: 1.35.2.2
done
Checking in template/en/default/admin/flag-type/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.5.12.2; previous revision: 1.5.12.1
done
Checking in template/en/default/admin/keywords/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.3.12.2; previous revision: 1.3.12.1
done
Checking in template/en/default/admin/keywords/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.7.2.3; previous revision: 1.7.2.2
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 18

9 years ago
Removing this bug from the security group, as the Security Advisory was sent (bug 468249)
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.