Closed
Bug 466748
Opened 16 years ago
Closed 16 years ago
[SECURITY] Shared/saved searches can be deleted without user confirmation using predictable URL
Categories
(Bugzilla :: Query/Bug List, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: bugzilla-mozilla, Assigned: LpSolit)
References
Details
Attachments
(2 files, 1 obsolete file)
|
5.69 KB,
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
|
13.68 KB,
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
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•16 years ago
|
Severity: normal → minor
Comment 1•16 years ago
|
||
Not sure why this is "minor"... any XSS, CSRF, or other issue could easily abuse this.
Severity: minor → normal
| Assignee | ||
Comment 2•16 years ago
|
||
Minor because you cannot do a lot of harm by abusing this page. It's still a security bug!
| Assignee | ||
Comment 3•16 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 | ||
Comment 4•16 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•16 years ago
|
Whiteboard: Also make public bug 466692 when this bug is made public
| Assignee | ||
Comment 5•16 years ago
|
||
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•16 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•16 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•16 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•16 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•16 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•16 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•16 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•16 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•16 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•16 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•16 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 18•16 years ago
|
||
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•16 years ago
|
Attachment #358758 -
Flags: review?(mkanat) → review+
Comment 19•16 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•16 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•16 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•16 years ago
|
||
Same patch as for tip + required Token::* methods and new template.
Attachment #358893 -
Flags: review?(mkanat)
| Assignee | ||
Updated•16 years ago
|
Attachment #358758 -
Attachment description: patch, v2 → patch for 3.2 + tip, v2
Updated•16 years ago
|
Attachment #358893 -
Flags: review?(mkanat) → review+
Comment 23•16 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•16 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•16 years ago
|
Flags: approval?
Flags: approval3.2?
Flags: approval3.2+
Flags: approval3.0?
Flags: approval3.0+
Flags: approval+
| Assignee | ||
Comment 25•16 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
Closed: 16 years ago
Resolution: --- → FIXED
Comment 26•16 years ago
|
||
Removing this bug from the security group, as the Security Advisory was sent (bug 468249)
Group: bugzilla-security
| Assignee | ||
Updated•16 years ago
|
Flags: testcase?
You need to log in
before you can comment on or make changes to this bug.
Description
•