Closed Bug 621090 (CVE-2011-0046) Opened 13 years ago Closed 13 years ago

[SECURITY] Adding saved searches lacks CSRF protection

Categories

(Bugzilla :: Query/Bug List, defect)

3.6.3
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: javg0x83, Assigned: dkl)

References

Details

(Whiteboard: [infrasec:csrf][ws:moderate])

Attachments

(1 file)

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
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → 3.6.3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: CSRF on remembered search (bugzilla.mozilla.org) → [SECURITY] CSRF on remembered search
Summary: [SECURITY] CSRF on remembered search → [SECURITY] Adding saved searches lacks CSRF protection
Whiteboard: [infrasec:csrf][ws:moderate]
Target Milestone: --- → Bugzilla 3.2
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.
Severity: normal → minor
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?
(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.
I agree with LpSolit. I'd say that this would be a "security enhancement".
Group: bugzilla-security
Target Milestone: Bugzilla 3.2 → Bugzilla 4.0
Patch, v1
Attachment #501746 - Flags: review?(mkanat)
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.
Target Milestone: Bugzilla 4.0 → Bugzilla 4.2
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.
(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.
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.
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.
or 4.0.1 or whatever.
Assignee: query-and-buglist → dkl
Target Milestone: Bugzilla 4.2 → Bugzilla 4.0
Status: NEW → ASSIGNED
(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 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.
Attachment #501746 - Flags: review?(mkanat) → review-
(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.
Attachment #501746 - Flags: review- → review?
(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 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.
Attachment #501746 - Flags: review? → review-
(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.
(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.
(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 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?
(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
(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...
(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
(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.
(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.
(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.
(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 on attachment 501746 [details] [diff] [review]
Patch adding token check for saving searches (v1)

What LpSolit is saying makes sense.
Attachment #501746 - Flags: review- → review+
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.
Flags: approval+
Flags: approval4.0?
Flags: approval4.0? → approval4.0+
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.
Flags: approval4.0+ → approval4.0?
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.
Blocks: 620540
Flags: blocking4.0?
justdave: either retarget the bug to 4.2 and mark it as FIXED, or approve it for 4.0.
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: blocking4.0?
Flags: blocking4.0-
Flags: approval4.0?
Flags: approval4.0-
Resolution: --- → FIXED
Target Milestone: Bugzilla 4.0 → Bugzilla 4.2
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.
Status: RESOLVED → REOPENED
Flags: blocking4.0-
Flags: blocking4.0+
Flags: approval4.0-
Flags: approval4.0+
Resolution: FIXED → ---
Target Milestone: Bugzilla 4.2 → Bugzilla 4.0
  Okay, it is totally fine with me to take this for 4.0, because it is true, it hasn't been released yet.
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.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Assigning CVE-2011-0046 to this and related CSRF issues fixed while investigating this pattern.
Alias: CVE-2011-0046
Blocks: 768870
You need to log in before you can comment on or make changes to this bug.