The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Query/Bug List
RESOLVED FIXED
8 years ago
8 years ago

People

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

Tracking

Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +
approval3.2 +
blocking3.2.1 +
approval3.0 +
blocking3.0.7 +
testcase ?

Details

Attachments

(2 attachments, 1 obsolete attachment)

5.69 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
13.68 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

8 years ago
Severity: normal → minor
Not sure why this is "minor"... any XSS, CSRF, or other issue could easily abuse this.
Severity: minor → normal
(Assignee)

Comment 2

8 years ago
Minor because you cannot do a lot of harm by abusing this page. It's still a security bug!
(Assignee)

Comment 3

8 years ago
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.
Flags: blocking3.2.1+
Flags: blocking3.0.7+
Target Milestone: --- → Bugzilla 3.0
(Assignee)

Updated

8 years ago
Blocks: 468249
(Assignee)

Comment 4

8 years ago
Won't be taken for coming releases.
No longer blocks: 468249
Flags: blocking3.2.1-
Flags: blocking3.2.1+
Flags: blocking3.0.7-
Flags: blocking3.0.7+

Updated

8 years ago
Whiteboard: Also make public bug 466692 when this bug is made public
(Assignee)

Comment 5

8 years ago
Created attachment 355622 [details] [diff] [review]
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.
Assignee: query-and-buglist → LpSolit
Status: NEW → ASSIGNED
Attachment #355622 - Flags: review?(mkanat)

Comment 6

8 years ago
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.
Attachment #355622 - Flags: review?(mkanat) → review-
(Assignee)

Comment 7

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

Comment 8

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

Please reconsider, per my previous comment.
Attachment #355622 - Flags: review- → review?(mkanat)

Comment 9

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

Comment 10

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

Comment 11

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

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

Comment 13

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

Comment 14

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

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

8 years ago
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.
Attachment #355622 - Flags: review?(mkanat) → review-
(Assignee)

Comment 17

8 years ago
Note: bug 26257 must be checked in first.
Depends on: 26257
(Assignee)

Comment 18

8 years ago
Created attachment 358758 [details] [diff] [review]
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.
Attachment #355622 - Attachment is obsolete: true
Attachment #358758 - Flags: review?(mkanat)

Updated

8 years ago
Attachment #358758 - Flags: review?(mkanat) → review+

Comment 19

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

Comment 20

8 years ago
I'll check later today if it needs backports or not, or if the same patch applies cleanly and works correctly on branches.
Flags: approval?
(Assignee)

Comment 21

8 years ago
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.
Flags: approval3.2?
Summary: Shared/saved searches can be deleted without user confirmation using predictable URL → [SECURITY] Shared/saved searches can be deleted without user confirmation using predictable URL
(Assignee)

Comment 22

8 years ago
Created attachment 358893 [details] [diff] [review]
patch for 3.0, v1

Same patch as for tip + required Token::* methods and new template.
Attachment #358893 - Flags: review?(mkanat)
(Assignee)

Updated

8 years ago
Attachment #358758 - Attachment description: patch, v2 → patch for 3.2 + tip, v2

Updated

8 years ago
Attachment #358893 - Flags: review?(mkanat) → review+

Comment 23

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

Comment 24

8 years ago
All backports are ready, and on time for the coming releases. Marking as blocker again.
Blocks: 468249
Flags: blocking3.2.1-
Flags: blocking3.2.1+
Flags: blocking3.0.7-
Flags: blocking3.0.7+
Flags: approval3.0?
Whiteboard: Also make public bug 466692 when this bug is made public
(Assignee)

Updated

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

Comment 25

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 26

8 years ago
Removing this bug from the security group, as the Security Advisory was sent (bug 468249)
Group: bugzilla-security

Updated

8 years ago
Blocks: 477513
(Assignee)

Updated

8 years ago
Flags: testcase?
You need to log in before you can comment on or make changes to this bug.