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

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Stephen Lee]

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

Comment 1

[Frédéric Buclin]

Did you check the code recently? *All* administrative pages use tokens to prevent such actions.

Comment 2

[Stephen Lee]

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

Comment 3

[Frédéric Buclin]

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

Comment 4

[Stephen Lee]

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.

Comment 5

[Stephen Lee]

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.

Comment 6

[Frédéric Buclin]

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.

Comment 7

[Frédéric Buclin]

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.

Comment 8

[Stephen Lee]

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

Comment 9

[Frédéric Buclin]

Attachment: patch for 3.x, v1

This patch applies to all 3.x branches + trunk.

Comment 10

[Daniel Veditz [:dveditz]]

> 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

Comment 11

[Max Kanat-Alexander]

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

Looks good and works. :-)

Comment 12

[Frédéric Buclin]

Attachment: patch for 2.22.7, v1

Here is the backport for 2.22.7.

Comment 13

[Frédéric Buclin]

Attachment: 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+.

Comment 14

[Max Kanat-Alexander]

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

Yeah, looks good to me. :-)

Comment 15

[Frédéric Buclin]

OK, this one has all the required backports and is now ready for checkin.

Comment 16

[Max Kanat-Alexander]

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.

Comment 17

[Frédéric Buclin]

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

Comment 18

[Max Kanat-Alexander]

Removing this bug from the security group, as the Security Advisory was sent (bug 468249)