Last Comment Bug 621107 - [SECURITY] Sanity checking lacks CSRF protection
: [SECURITY] Sanity checking lacks CSRF protection
Status: RESOLVED FIXED
[infrasec:csrf][ws:low]
:
Product: Bugzilla
Classification: Server Software
Component: Administration (show other bugs)
: 3.6.3
: All All
: -- minor (vote)
: Bugzilla 4.0
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on:
Blocks: 835424 620540
  Show dependency treegraph
 
Reported: 2010-12-22 23:23 PST by Reed Loden [:reed] (use needinfo?)
Modified: 2013-01-28 10:07 PST (History)
5 users (show)
LpSolit: approval+
LpSolit: approval4.0+
LpSolit: blocking4.0-
mkanat: blocking3.6.4-
mkanat: blocking3.4.10-
mkanat: blocking3.2.10-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (6.03 KB, patch)
2010-12-27 11:49 PST, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v1.1 (6.98 KB, patch)
2010-12-27 13:36 PST, Frédéric Buclin
dkl: review+
Details | Diff | Splinter Review
patch, v2 (6.07 KB, patch)
2011-01-22 15:20 PST, Frédéric Buclin
dkl: review+
Details | Diff | Splinter Review
patch for 4.0, v1 (6.13 KB, patch)
2011-01-22 18:36 PST, Frédéric Buclin
dkl: review+
Details | Diff | Splinter Review

Description Reed Loden [:reed] (use needinfo?) 2010-12-22 23:23:43 PST
sanitycheck.cgi has no CSRF protection. Should display an intermediary page asking the user if he/she wants to sanity check (including a token). Command-line shouldn't require the token.
Comment 1 Max Kanat-Alexander 2010-12-22 23:28:33 PST
Ah, my instinct here is WONTFIX.
Comment 2 Max Kanat-Alexander 2010-12-22 23:30:30 PST
Or possibly INVALID--I'm not quite sure why we'd want to protect Sanity Check with CSRF tokens. Is there some actual danger here?
Comment 3 Reed Loden [:reed] (use needinfo?) 2010-12-22 23:35:54 PST
Sanity checking modifies the database and can do a wide number of things that can cause Bugzilla to slow way down while tables are locked. So, yes, there is at least DoS danger here if not actual unwanted modification of data.
Comment 4 Max Kanat-Alexander 2010-12-22 23:42:52 PST
(In reply to comment #3)
> Sanity checking modifies the database 

  No, just running a sanity check by itself doesn't modify the database (except perhaps in harmless ways that I'm forgetting about).

  There are links on the page that change the database, but I'm pretty sure you can't do anything dangerous with them.

> and can do a wide number of things that
> can cause Bugzilla to slow way down while tables are locked. So, yes, there is
> at least DoS danger here if not actual unwanted modification of data.

  Well, it's easy enough to just close the page, in most circumstances. (I do understand you're talking about opening it in multiple iframes here.)

  If we're concerned about DoS, there's probably some other solution for that that isn't related to CSRF. It's probably also not something we're going to fix any time soon.

  Also remember that Bugzilla doesn't lock tables anymore, there are only row locks from the transactions, in some circumstances.
Comment 5 Frédéric Buclin 2010-12-23 08:36:24 PST
I agree that this is not a bug. No harm to the DB can be done, even when triggering one of the links included in the page. [ws:critical] doesn't make any sense at all. About DoS, you can already do that with queries if you really want to. So the bug as is is invalid/wontfix.
Comment 6 Reed Loden [:reed] (use needinfo?) 2010-12-23 10:23:38 PST
It's still a bug, as I have no wish for random people to cause my client to invoke sanity check without my permission, ESPECIALLY when many sanitycheck options modify the DB itself. sanitycheck is obviously restricted to editcomponents and higher for a reason. If it was perfectly safe, you wouldn't be protecting it.

Also, if there's a problem with groups/products, I'm fairly sure sanitycheck will give away product names even if strict_isolation is on.
Comment 7 Dave Miller [:justdave] (justdave@bugzilla.org) 2010-12-23 10:57:54 PST
The links on that page that go do things are exactly the problem.  Some of them do dangerous things or resource-intensive things, and right now there's nothing stopping someone from tricking you to going to one of those links directly, without hitting sanitycheck itself on the way there.
Comment 8 Max Kanat-Alexander 2010-12-23 13:03:23 PST
(In reply to comment #6)
> Also, if there's a problem with groups/products, I'm fairly sure sanitycheck
> will give away product names even if strict_isolation is on.

  That's fine, that doesn't expose that data to any external entity, only to you, yourself.

(In reply to comment #7)
> Some of them do dangerous things or resource-intensive things

  I don't think that there are any that do dangerous things. If it's humanly possible for sanitycheck to be dangerous, we should just fix that.

  As far as being resource-intensive, I agree with LpSolit that you could just DoS Bugzilla in several other ways instead--by doing large queries, etc.


  My suggestion is that we remove the security flag from this bug and untarget it.
Comment 9 Reed Loden [:reed] (use needinfo?) 2010-12-23 13:10:52 PST
(In reply to comment #8)
>   My suggestion is that we remove the security flag from this bug and untarget
> it.

I disagree. Why not just fix the problem instead of sending this bug to a blackhole?
Comment 10 Max Kanat-Alexander 2010-12-23 13:13:16 PST
(In reply to comment #9)
> I disagree. Why not just fix the problem instead of sending this bug to a
> blackhole?

  Because it's very low-priority, there are very few of us, and we have a ton of work to do. Our efforts would be better spent on doing what is important instead of backporting unimportant changes to older branches.
Comment 11 Frédéric Buclin 2010-12-27 11:49:58 PST
Created attachment 499862 [details] [diff] [review]
patch, v1

Always require a token, even if no parameter is passed to the script. This means that you no longer can call http://bugzilla.foo.com/sanitycheck.cgi directly. If that's a problem, I can change my patch so that it only requires a token when a parameter is passed.
Comment 12 Frédéric Buclin 2010-12-27 13:36:52 PST
Created attachment 499890 [details] [diff] [review]
patch, v1.1

I forgot the Voting extension.
Comment 13 David Lawrence [:dkl] 2011-01-10 17:05:40 PST
Comment on attachment 499890 [details] [diff] [review]
patch, v1.1

Looks good and works as expected. r=dkl
Comment 14 Max Kanat-Alexander 2011-01-11 00:05:00 PST
I don't think that this qualifies as a security issue that needs to be checked in on the branches.
Comment 15 Max Kanat-Alexander 2011-01-11 00:12:06 PST
  I also think that we should unprotect this bug and remove the tags from the status whiteboard.
Comment 16 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-01-11 09:58:21 PST
This patch isn't that complicated.  Is there something that causes it to be more complicated on the branches, will something major be broken by it, or are we just looking for excuses to have fewer security bugs?
Comment 17 Max Kanat-Alexander 2011-01-11 23:07:36 PST
(In reply to comment #16)
> This patch isn't that complicated.  Is there something that causes it to be
> more complicated on the branches, will something major be broken by it, or are
> we just looking for excuses to have fewer security bugs?

  There are a lot of good reasons not to backport it:

  * More changes on stable security branches
  * Will very likely require backport work
  * I forget, but if we go back far enough we may need an entirely different patch.
  * Trying to break any existing links as little as possible on stable branches, with these CSRF patches.
Comment 18 Max Kanat-Alexander 2011-01-11 23:08:20 PST
Also, the only risk here that anybody has proven or demonstrated is a DoS, and we don't usually keep DoS bugs protected or backport them unless they have some sort of unusual severity (which this one does not).
Comment 19 Max Kanat-Alexander 2011-01-21 15:57:57 PST
LpSolit and I have discussed this, and we agree that:

* This is not a security issue that needs to be kept confidential.
* It would be best to just require tokens for the stuff that modifies the DB or does some action.
Comment 20 Max Kanat-Alexander 2011-01-21 15:58:26 PST
This is not a blocker for 4.0 but I will take it for 4.0 if it is ready in time. (I'll even take it for rc2 if it's ready before Monday.)
Comment 21 Reed Loden [:reed] (use needinfo?) 2011-01-21 16:00:17 PST
(In reply to comment #20)
> This is not a blocker for 4.0 but I will take it for 4.0 if it is ready in
> time. (I'll even take it for rc2 if it's ready before Monday.)

Yes, it is.
Comment 22 Frédéric Buclin 2011-01-21 16:07:25 PST
I'm on it. It will be ready on time, so no worry. :)
Comment 23 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-01-22 13:17:22 PST
(In reply to comment #19)
> LpSolit and I have discussed this, and we agree that:

Just to be clear (since a couple people asked me why mkanat and LpSolit were overriding me) I was in on this conversation as well, and he forgot to mention me. (gee, thanks)

But yes, after hashing through the various ways this could be exploited, we couldn't find a way to actually cause any damage.  The worst thing it can cause is a DoS, and there's easier ways to DoS bugzilla than this that don't even require exploiting a CSRF to get at.

I still think we should fix it, I've just agreed that it doesn't need the whole security firedrill thing for it.
Comment 24 Frédéric Buclin 2011-01-22 15:20:20 PST
Created attachment 506154 [details] [diff] [review]
patch, v2
Comment 25 David Lawrence [:dkl] 2011-01-22 15:48:13 PST
Comment on attachment 506154 [details] [diff] [review]
patch, v2

Will need a separate patch for 4.0:

[dkl@localhost 4.0]$ patch -p0 < ~/Downloads/bug2054-cvs.diff 
patching file sanitycheck.cgi
patching file template/en/default/admin/sanitycheck/messages.html.tmpl
Hunk #1 FAILED at 34.
1 out of 5 hunks FAILED -- saving rejects to file template/en/default/admin/sanitycheck/messages.html.tmpl.rej
patching file extensions/Example/template/en/default/hook/admin/sanitycheck/messages-statuses.html.tmpl
patching file extensions/Voting/template/en/default/hook/admin/sanitycheck/messages-statuses.html.tmpl
Comment 26 Frédéric Buclin 2011-01-22 15:50:08 PST
Comment on attachment 506154 [details] [diff] [review]
patch, v2

Because it doesn't apply cleanly to 4.0 doesn't mean you cannot review it for trunk.
Comment 27 Frédéric Buclin 2011-01-22 18:36:22 PST
Created attachment 506178 [details] [diff] [review]
patch for 4.0, v1

Bugzilla 4.0 still uses FILTER url_quote instead of FILTER uri. That was the reason why the patch didn't apply cleanly on this branch.
Comment 28 Max Kanat-Alexander 2011-01-22 23:43:07 PST
(In reply to comment #23)
> I was in on this conversation as well, and he forgot to mention
> me. (gee, thanks)

  Just FYI: As I recall, that was because you came into the conversation after I posted that comment.
Comment 30 David Lawrence [:dkl] 2011-01-23 20:50:59 PST
Comment on attachment 506154 [details] [diff] [review]
patch, v2

Looks good and as long as you have tested each of the different scenarios, then r=dkl
Comment 31 David Lawrence [:dkl] 2011-01-23 21:00:18 PST
Comment on attachment 506178 [details] [diff] [review]
patch for 4.0, v1

Looks good and as long as you have tested each of the different scenarios, then r=dkl
Comment 32 Frédéric Buclin 2011-01-24 09:08:35 PST
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified sanitycheck.cgi
modified extensions/Example/template/en/default/hook/admin/sanitycheck/messages-statuses.html.tmpl
modified extensions/Voting/template/en/default/hook/admin/sanitycheck/messages-statuses.html.tmpl
modified template/en/default/admin/sanitycheck/messages.html.tmpl
Committed revision 7668.


Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified sanitycheck.cgi
modified extensions/Example/template/en/default/hook/admin/sanitycheck/messages-statuses.html.tmpl
modified extensions/Voting/template/en/default/hook/admin/sanitycheck/messages-statuses.html.tmpl
modified template/en/default/admin/sanitycheck/messages.html.tmpl
Committed revision 7525.

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