466748 - [SECURITY] Shared/saved searches can be deleted without user confirmation using predictable URL

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Stephen Lee]

I added this to bug 466692, as at the time it seemed yet another instance of the same thing (missing token check / user confirmation for a privileged action), but it seems the cause is in fact rather different.

Steps to reproduce:
1. Find a shared query from the list on your user preferences.
2. Copy-paste the "Run" link
3. edit this to change remaction=run to remaction=forget, and to remove the sharer_id=....
4. Trick the user who created the query into loading this url

Steps 1 and 2 can be skipped, and this attack used on non-shared queries if you can predict the name of them. For example, users may have a legacy query named "My bugs" that can be deleted with:
https://[munged-to-prevent-accidents]/buglist.cgi?cmdtype=dorem&remaction=forget&namedcmd=My%20bugs

Comment 1

[Reed Loden [:reed]]

Not sure why this is "minor"... any XSS, CSRF, or other issue could easily abuse this.

Comment 2

[Frédéric Buclin]

Minor because you cannot do a lot of harm by abusing this page. It's still a security bug!

Comment 3

[Frédéric Buclin]

Now that some people mentioned this bug elsewhere, we cannot keep it secret for too long anymore. We have to include it in our set of security bugs we are going to fix in our next releases. So marking this bug as a blocker.

Comment 4

[Frédéric Buclin]

Won't be taken for coming releases.

Comment 5

[Frédéric Buclin]

Attachment: patch, v1

We reuse the functions in Token.pm implemented in bug 26257. To create the hash, I use both the query ID and name, because the query ID is unique and impossible to know. Also, if a user deletes and re-creates a saved search with the same name, the query ID will change, making the temptation to forge the correct hash to abuse the user even smaller.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 355622 [details] [diff] [review]
patch, v1

No, the query_id is predictable given enough time, since it's just a number. You'd have to also hash in something like the user's login token.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> (From update of attachment 355622 [details] [diff] [review])
> No, the query_id is predictable given enough time, since it's just a number.
> You'd have to also hash in something like the user's login token.

This is a weak reason to deny review. Its purpose is to randomize the hash a bit more. I even hesitated to include it at all. In case you forgot, the token generator already uses the site secret + the current time in the token. It's not like only the query ID is passed to build it.

Comment 8

[Frédéric Buclin]

Comment on attachment 355622 [details] [diff] [review]
patch, v1

Please reconsider, per my previous comment.

Comment 9

[Max Kanat-Alexander]

Sure, you still can't generate a token unless you have the site-wide secret, but the way you've done it, the token will always be the same for any given saved search, which I'm not very comfortable with. If the attacker gets the token in any way, then it's valid forever.

Comment 10

[Frédéric Buclin]

No, the token is valid only once, because it takes the search name + the search ID (which is an auto-increment, so unique).

Comment 11

[Frédéric Buclin]

To be clear: if the attacker manages to make you click the URL with the given token, and makes you delete your saved search, the token won't be usable anymore, because if the user recreates his saved search with the same name, the ID will be different and so the token would be invalid.

We are in the same situation about process_bug.cgi. As long as a bug didn't change, delta_ts will remain the same and the token would still be the same till the bug is edited.

And don't forget that the lifetime of a token is 3 days, not "forever".

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> To be clear: if the attacker manages to make you click the URL with the given
> token, and makes you delete your saved search,

  That's what we don't ever want to happen.

> We are in the same situation about process_bug.cgi. As long as a bug didn't
> change, delta_ts will remain the same and the token would still be the same
> till the bug is edited.

  True. I would have preferred we add the user's logincookie to the hash.

> And don't forget that the lifetime of a token is 3 days, not "forever".

  No, these tokens last forever. They don't have a lifetime--this isn't a session token.

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
>   No, these tokens last forever. They don't have a lifetime--this isn't a
> session token.

They have a lifetime (it's included in the token)! I wrote the code, see bug 26257.

Comment 14

[Frédéric Buclin]

(In reply to comment #12)
>   True. I would have preferred we add the user's logincookie to the hash.

If it's safer to have the user's logincookie in the token, then this should be part of issue_hash_token()'s internals, i.e. part of the patch being in bug 26257. Note that as long as a user doesn't log out, the cookie stored in logincookies remains unchanged, so the benefit of using it may not be that large (at least for users like me who don't log out very often, only once a day). Also, another reason to not use it is to keep things simple and fast. Having to query the DB for each page load to get the logincookie may slow down Bugzilla (and may be tricky on pages using the shadow DB if the shadow DB is a bit behind and the logincookie not yet replicated, as templates are generating the token on the fly).

Comment 15

[Max Kanat-Alexander]

Oh, right, I forgot that tokens had the current time hashed into them. Okay, I'll re-look at the patch, then, when I have some time.

Comment 16

[Max Kanat-Alexander]

Comment on attachment 355622 [details] [diff] [review]
patch, v1

>Index: buglist.cgi
>-    return $result;
>+    return wantarray ? ($result, $id) : $result;

  LookupNamedQuery is called at least once in boolean context, which I'm pretty sure is an array context. In that case, this will always be true, because an array of (undef, undef) is a scalar of 2, which is true.

  Otherwise this looks fine.

Comment 17

[Frédéric Buclin]

Note: bug 26257 must be checked in first.

Comment 18

[Frédéric Buclin]

Attachment: patch for 3.2 + tip, v2

I just found that the filtering was wrong. It must be FILTER url_quote, not FILTER html. I also fixed a link in user-error.html.tmpl if the query already exists. The token wasn't generated by the error message, and so you would trigger the confirmaton page if you were trying to delete the saved search from here.

Tested again, and still works fine.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 358758 [details] [diff] [review]
patch for 3.2 + tip, v2

Looks good to me. I assume you tested it thoroughly, as you always do. :-)

Comment 20

[Frédéric Buclin]

I'll check later today if it needs backports or not, or if the same patch applies cleanly and works correctly on branches.

Comment 21

[Frédéric Buclin]

This patch works fine as is on 3.2. But I need to backport it to 3.0, or more exactly, to add required Token::* methods as bug 26257 won't land on 3.0. Bugzilla 2.22.6 doesn't need a backport as shared searches exist only since Bugzilla 3.0, see bug 69000, and so attackers cannot guess user's query names.

Comment 22

[Frédéric Buclin]

Attachment: patch for 3.0, v1

Same patch as for tip + required Token::* methods and new template.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 358893 [details] [diff] [review]
patch for 3.0, v1

Looks good and works.

"Are you sure you want to commit these changes?" is a bit confusing when it's just a saved-search deletion, but I'm not too worried about that.

Comment 24

[Frédéric Buclin]

All backports are ready, and on time for the coming releases. Marking as blocker again.

Comment 25

[Frédéric Buclin]

tip:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.392; previous revision: 1.391
done
Checking in template/en/default/account/prefs/saved-searches.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/saved-searches.html.tmpl,v  <--  saved-searches.html.tmpl
new revision: 1.20; previous revision: 1.19
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <-- user-error.html.tmpl
new revision: 1.274; previous revision: 1.273
done
Checking in template/en/default/list/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.65; previous revision: 1.64
done


3.2:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.374.2.7; previous revision: 1.374.2.6
done
Checking in template/en/default/account/prefs/saved-searches.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/saved-searches.html.tmpl,v  <--  saved-searches.html.tmpl
new revision: 1.18.2.1; previous revision: 1.18
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <-- user-error.html.tmpl
new revision: 1.249.2.3; previous revision: 1.249.2.2
done
Checking in template/en/default/list/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.59.2.2; previous revision: 1.59.2.1
done


3.0.6:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.351.2.11; previous revision: 1.351.2.10
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.68.2.7; previous revision: 1.68.2.6
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.52.2.3; previous revision: 1.52.2.2
done
Checking in Bugzilla/Install/Localconfig.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Localconfig.pm,v  <--  Localconfig.pm
new revision: 1.8.2.1; previous revision: 1.8
done
Checking in template/en/default/account/prefs/saved-searches.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/saved-searches.html.tmpl,v  <--  saved-searches.html.tmpl
new revision: 1.11.2.4; previous revision: 1.11.2.3
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <-- user-error.html.tmpl
new revision: 1.204.2.16; previous revision: 1.204.2.15
done
Checking in template/en/default/list/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.52.2.1; previous revision: 1.52
done
Checking in template/en/default/global/confirm-action.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/confirm-action.html.tmpl,v<--  confirm-action.html.tmpl
new revision: 1.1.4.2; previous revision: 1.1.4.1
done

Comment 26

[Max Kanat-Alexander]

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