Last Comment Bug 621090 - (CVE-2011-0046) [SECURITY] Adding saved searches lacks CSRF protection
(CVE-2011-0046)
: [SECURITY] Adding saved searches lacks CSRF protection
Status: RESOLVED FIXED
[infrasec:csrf][ws:moderate]
:
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 3.6.3
: All All
: -- minor (vote)
: Bugzilla 4.0
Assigned To: David Lawrence [:dkl]
: default-qa
Mentors:
Depends on:
Blocks: 835424 620540 768870
  Show dependency treegraph
 
Reported: 2010-12-22 19:46 PST by Jose A. Vazquez
Modified: 2013-01-28 10:07 PST (History)
6 users (show)
mkanat: approval+
justdave: approval4.0+
justdave: blocking4.0+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch adding token check for saving searches (v1) (1.94 KB, patch)
2011-01-06 11:41 PST, David Lawrence [:dkl]
mkanat: review+
Details | Diff | Review

Description Jose A. Vazquez 2010-12-22 19:46:41 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: 

Hi Mozilla people,

I found a new CSRF, this time on bugzilla.mozilla.org

It's easy, the remembered search is not protected and this could
be used to post "****" on panels of legitimate users.

I've made a simple PoC/Exploit to trigger the vulnerability (it's attached)

Solution: Protect this request with a token as others

Regards,
Jose.


Reproducible: Always

Steps to Reproduce:
See details
Actual Results:  
See details

Expected Results:  
See details

See details
Comment 2 Max Kanat-Alexander 2010-12-22 23:33:02 PST
I would say that this is very very minor--there's no irreversible damage that can be caused, and no important data that can be exposed, just a minor annoyance.

We could probably use issue_hash_token here. I don't want to generate a DB token every time somebody does a search.
Comment 3 Frédéric Buclin 2010-12-23 08:16:31 PST
I agree that this is ultra minor. Personally, I don't qualify this bug as security-sensitive. As mkanat said, it's more a minor annoyance (which is easy to revert) than a security issue (no dataloss, no leak, no permanent change).

mkanat, agree to remove the security flag and retarget it to 4.0?
Comment 4 Reed Loden [:reed] (use needinfo?) 2010-12-23 11:14:56 PST
(In reply to comment #3)
> mkanat, agree to remove the security flag and retarget it to 4.0?

Unwanted modification of a user's data is still a security issue, even if it's just saved searches. I won't disagree that this is minor as compared to other issues, but it's still a security issue nonetheless, and it should be fixed.
Comment 5 Max Kanat-Alexander 2010-12-23 13:00:06 PST
I agree with LpSolit. I'd say that this would be a "security enhancement".
Comment 6 David Lawrence [:dkl] 2011-01-06 11:41:37 PST
Created attachment 501746 [details] [diff] [review]
Patch adding token check for saving searches (v1)

Patch, v1
Comment 7 Max Kanat-Alexander 2011-01-06 11:43:21 PST
I actually think that this should go into 4.2, since it might have an effect on Bugzilla clients that currently screen-scrape (the only way to currently deal with saved searches in an automated fashion) and I don't want to break them with no solution, this close to the 4.0 release.
Comment 8 Frédéric Buclin 2011-01-06 12:06:57 PST
There are other changes which will have more effects than this one, IMO, so taking it for 4.0 won't hurt, especially because we are still in the RC step.
Comment 9 Max Kanat-Alexander 2011-01-06 12:30:57 PST
(In reply to comment #8)
> There are other changes which will have more effects than this one, IMO, so
> taking it for 4.0 won't hurt, especially because we are still in the RC step.

  I understand where you're coming from, but I don't think that you have enough experience writing clients or talking to the major writers of clients to really see how difficult you will be making their lives if you add this to 4.0 so close to its release time.

  I also plan to prevent other major changes which will break significant client functionality on the 4.0 branch, if possible.
Comment 10 Reed Loden [:reed] (use needinfo?) 2011-01-06 13:14:12 PST
We should hold 4.0 until these issues are fixed. Security fixes shouldn't have to wait another year to get fixed when we have a patch today.

justdave, requesting module owner approval for getting this on 4.0. Thanks.
Comment 11 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-01-06 13:19:26 PST
Security trumps compatibility, always.  Put it in 4.0.  Especially if you're going to break them anyway for 4.1.  Better to break them before the final release if you're going to break it, than after then final release when the API is supposedly stable on the branch.
Comment 12 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-01-06 13:19:50 PST
or 4.0.1 or whatever.
Comment 13 Max Kanat-Alexander 2011-01-06 14:30:20 PST
(In reply to comment #11)
> Security trumps compatibility, always.

  Dave, usually I don't disagree with you, but in this case, this is not a critical issue. This isn't about "security", it's about preventing petty vandalism that has never happened and wouldn't even be dangerous if it did happen. There's no information that could be exposed, there's nothing critical that could be permanently lost.

  Also, we would be adding a CSRF token to a part of Bugzilla that:

  (a) has no API
  (b) is used by major clients
  (c) has no method for clients to get the CSRF token

  In other words, there would no longer be any way automated way to create, update, or delete saved searches in clients, other than actually running the entire search and then pulling the token out from the HTML. In most clients, this would mean running a search twice in some cases--once for the HTML results and once for the XML/CSV results.

  The issue really is not critical--this bug isn't even hidden. I don't see any need for it to block 4.0, go into 4.0 at all, or to hinder clients for 4.0.
Comment 14 Max Kanat-Alexander 2011-01-06 14:31:29 PST
Comment on attachment 501746 [details] [diff] [review]
Patch adding token check for saving searches (v1)

The token generated here can be replayed by anybody for any search.
Comment 15 Frédéric Buclin 2011-01-06 14:36:43 PST
(In reply to comment #14)
> The token generated here can be replayed by anybody for any search.

Huh? Hash tokens automatically hash the user ID, you don't have to pass it. So you cannot use your own token to impersonate someone else.
Comment 16 Frédéric Buclin 2011-01-06 14:37:58 PST
(In reply to comment #13)
>   In other words, there would no longer be any way automated way to create,
> update, or delete saved searches in clients

You already need a token to delete a saved search. Nothing new here.
Comment 17 Max Kanat-Alexander 2011-01-06 14:38:48 PST
Comment on attachment 501746 [details] [diff] [review]
Patch adding token check for saving searches (v1)

Okay, in that case I could replay this token against any search done by the same user. There needs to be more data passed to issue_hash_token here.
Comment 18 Frédéric Buclin 2011-01-06 14:41:26 PST
(In reply to comment #17)
> Okay, in that case I could replay this token against any search done by the
> same user. There needs to be more data passed to issue_hash_token here.

They are time-bombed! And this is fine for a user to reuse the same token for himself for the next 3 days if he wants to, for non-critical actions. This would help the clients you are talking about to get a valid token, and reuse it for 3 days.
Comment 19 Frédéric Buclin 2011-01-06 14:42:26 PST
(In reply to comment #17)
> Okay, in that case I could replay this token against any search done by the
> same user.

Oh, and you have no way to know the token used by that user anyway. Unless the user pasted you the whole source code of the page.
Comment 20 Max Kanat-Alexander 2011-01-06 14:42:43 PST
(In reply to comment #16)
> You already need a token to delete a saved search. Nothing new here.

  Oh really? Then this is REALLY not critical, and REALLY does not need to go into 4.0.
Comment 21 Max Kanat-Alexander 2011-01-06 14:46:56 PST
Comment on attachment 501746 [details] [diff] [review]
Patch adding token check for saving searches (v1)

Ah, could you use the list_id as part of the token, if it exists?
Comment 22 David Lawrence [:dkl] 2011-01-06 14:49:54 PST
(In reply to comment #14)
> Comment on attachment 501746 [details] [diff] [review]
> Patch adding token check for saving searches (v1)
> 
> The token generated here can be replayed by anybody for any search.

Ok I see what you are saying now. I was using an example from LpSolit's sanitycheck CSRF patch doing issue_hash_token(['sanitycheck']). 

As LpSolit mentioned the user id and site wide secret are already added
to the hash. What if I used the actual query string as one of the args?

Dave
Comment 23 Frédéric Buclin 2011-01-06 14:53:39 PST
(In reply to comment #22)
> to the hash. What if I used the actual query string as one of the args?

This is IMO not needed. On one side, mkanat complains this issue is REALLY not critical, but on the other side, he wants stronger randomness to generate the token...
Comment 24 David Lawrence [:dkl] 2011-01-06 14:55:32 PST
(In reply to comment #21)
> Comment on attachment 501746 [details] [diff] [review]
> Patch adding token check for saving searches (v1)
> 
> Ah, could you use the list_id as part of the token, if it exists?

Doesn't exist in the remember search form block in buglist.cgi and also is not used in 3.6 which I am backporting this to.

Dave
Comment 25 Max Kanat-Alexander 2011-01-06 14:57:16 PST
(In reply to comment #23)
> This is IMO not needed. On one side, mkanat complains this issue is REALLY not
> critical, but on the other side, he wants stronger randomness to generate the
> token...

  Yeah, I know that seems weird. Here's the thing though:

  If it's not an issue, then we don't need to fix it.

  If it is an issue, then we need to fix it right.

  I don't want to do some in-between thing where we fix it but it's not really secure.
Comment 26 Max Kanat-Alexander 2011-01-06 15:00:35 PST
(In reply to comment #24)
> Doesn't exist in the remember search form block in buglist.cgi 

  Actually, yes it does. It's generated by redirect_search_url, which is at the very top of buglist.cgi.

> and also is not used in 3.6 which I am backporting this to.

  Okay. This bug isn't targeted there, though.
Comment 27 Frédéric Buclin 2011-01-06 15:02:02 PST
(In reply to comment #25)
>   I don't want to do some in-between thing where we fix it but it's not really
> secure.

Define secure? You cannot steal someone else's token. I think it's fine to see two levels of security: one high level where a token should be valid only once, in case it's disclosed, and a low lever where a token can be valid a short amount of time and wouldn't cause large damage if disclosed.
Comment 28 Max Kanat-Alexander 2011-01-06 15:28:07 PST
(In reply to comment #27)
> Define secure? You cannot steal someone else's token. I think it's fine to see
> two levels of security: one high level where a token should be valid only once,
> in case it's disclosed, and a low lever where a token can be valid a short
> amount of time and wouldn't cause large damage if disclosed.

  Hmm, okay, I think you're right.
Comment 29 Max Kanat-Alexander 2011-01-06 15:28:21 PST
Comment on attachment 501746 [details] [diff] [review]
Patch adding token check for saving searches (v1)

What LpSolit is saying makes sense.
Comment 30 Max Kanat-Alexander 2011-01-06 15:28:47 PST
I'm only approving this for trunk. One of the other two approvers can approve it for 4.0 if they feel that it is justified and important to do so.
Comment 31 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-01-06 16:51:04 PST
Too many other people replied to this after Max did before I saw the bug again and I missed his reply to me.  Hold up on this for 4.0 for a bit while I investigate some more.
Comment 32 David Lawrence [:dkl] 2011-01-06 20:08:13 PST
trunk:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/                                                                           modified buglist.cgi
modified template/en/default/global/per-bug-queries.html.tmpl
modified template/en/default/list/list.html.tmpl
Committed revision 7647.
Comment 33 Frédéric Buclin 2011-01-22 09:24:06 PST
justdave: either retarget the bug to 4.2 and mark it as FIXED, or approve it for 4.0.
Comment 34 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-01-22 11:08:08 PST
Per IRC discussion we agreed that we'd make the backported patches available for anyone that wants them, but not check them in on the branches on ones that you can't cause serious damage with, due to the potential of breaking third party apps that integrate with Bugzilla.
Comment 36 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-01-23 20:30:12 PST
OK, so I'm going to change my mind on this.  After some discussion with LpSolit (and a couple other people) it's evident that the only overriding reason NOT to take this on the 4.0 branch is the one given in comment 13 about third-party client integration.  The patch has been tested on 4.0 and is safe, and doesn't break anything other than the ability to submit without a token.  So the only thing we hurt if we DO take it on 4.0 is third-party client integration.

Well, Bugzilla 4.0 hasn't actually been released yet. Nothing with regards to API is set in stone until we do. Especially something like this which has no official API and already requires using a non-supported method to accomplish programatically from an external client anyway. Until we have a real API for it, they'll have to live with compensating for different ways it's done in different versions, just like they have in our previous releases.

As I said in comment 11, security (even only against petty vandalism) trumps over compatibility.  *ESPECIALLY* when we haven't actually released yet.  So the third-party clients need to learn a new trick in order to create a saved query for you.  I bet that's not the only new thing they're going to have to do for 4.0-compatibility, and others of these recent CSRF bugs have broken things far more critical to those apps already that they're already going to have to compensate for, so why not let them deal with this one at the same time while they're already dealing with other similar changes for 4.0 and avoid the aggravation of them having to do it all over again for 4.2.
Comment 37 Max Kanat-Alexander 2011-01-23 20:37:24 PST
  Okay, it is totally fine with me to take this for 4.0, because it is true, it hasn't been released yet.
Comment 38 Frédéric Buclin 2011-01-24 09:00:44 PST
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified buglist.cgi
modified template/en/default/global/per-bug-queries.html.tmpl
modified template/en/default/list/list.html.tmpl
Committed revision 7524.
Comment 39 Daniel Veditz [:dveditz] 2011-01-24 10:03:42 PST
Assigning CVE-2011-0046 to this and related CSRF issues fixed while investigating this pattern.

Note You need to log in before you can comment on or make changes to this bug.