621090 - (CVE-2011-0046) [SECURITY] Adding saved searches lacks CSRF protection

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Jose A. Vazquez]

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]

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]

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

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

I agree with LpSolit. I'd say that this would be a "security enhancement".

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch adding token check for saving searches (v1)

Patch, v1

Comment 7

[Max Kanat-Alexander]

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]

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]

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

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

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

or 4.0.1 or whatever.

Comment 13

[Max Kanat-Alexander]

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

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]

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

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

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]

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

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

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

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

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

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

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

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

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

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

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

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]

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

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

trunk:
Committing to: bzr+ssh://[e-mail]/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]

justdave: either retarget the bug to 4.2 and mark it as FIXED, or approve it for 4.0.

Comment 34

[Dave Miller [:justdave]]

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

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]

  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]

Committing to: bzr+ssh://[e-mail]/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]]

Assigning CVE-2011-0046 to this and related CSRF issues fixed while investigating this pattern.