Closed Bug 621107 Opened 13 years ago Closed 13 years ago

[SECURITY] Sanity checking lacks CSRF protection

Categories

(Bugzilla :: Administration, task)

3.6.3
task
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: reed, Assigned: LpSolit)

References

Details

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

Attachments

(2 files, 2 obsolete files)

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.
Ah, my instinct here is WONTFIX.
Priority: -- → P3
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?
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.
(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.
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.
Group: bugzilla-security
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Target Milestone: Bugzilla 3.2 → ---
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → NEW
Priority: P3 → --
Target Milestone: --- → Bugzilla 3.2
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.
Group: bugzilla-security
Assignee: administration → LpSolit
(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.
Severity: normal → minor
(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?
(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.
Attached patch patch, v1 (obsolete) — Splinter Review
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.
Attachment #499862 - Flags: review?(mkanat)
Attached patch patch, v1.1 (obsolete) — Splinter Review
I forgot the Voting extension.
Attachment #499862 - Attachment is obsolete: true
Attachment #499890 - Flags: review?(mkanat)
Attachment #499862 - Flags: review?(mkanat)
Attachment #499890 - Flags: review?(mkanat) → review?(dkl)
Flags: blocking4.0?
Flags: blocking3.6.4?
Flags: blocking3.4.10?
Flags: blocking3.2.10?
Blocks: 620540
Comment on attachment 499890 [details] [diff] [review]
patch, v1.1

Looks good and works as expected. r=dkl
Attachment #499890 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval4.0?
I don't think that this qualifies as a security issue that needs to be checked in on the branches.
Flags: blocking3.6.4?
Flags: blocking3.6.4-
Flags: blocking3.4.10?
Flags: blocking3.4.10-
Flags: blocking3.2.10?
Flags: blocking3.2.10-
Target Milestone: Bugzilla 3.2 → Bugzilla 4.0
  I also think that we should unprotect this bug and remove the tags from the status whiteboard.
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?
(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.
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).
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.
Group: bugzilla-security
Flags: approval?
Flags: approval4.0?
Whiteboard: [infrasec:csrf][ws:critical] → [infrasec:csrf]
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.)
Flags: blocking4.0? → blocking4.0-
(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.
Flags: blocking4.0- → blocking4.0?
Whiteboard: [infrasec:csrf] → [infrasec:csrf][ws:low]
I'm on it. It will be ready on time, so no worry. :)
Status: NEW → ASSIGNED
Flags: blocking4.0? → blocking4.0-
(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.
Attached patch patch, v2Splinter Review
Attachment #499890 - Attachment is obsolete: true
Attachment #506154 - Flags: review?(dkl)
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
Attachment #506154 - Flags: review?(dkl) → review-
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.
Attachment #506154 - Flags: review- → review?(dkl)
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.
Attachment #506178 - Flags: review?(dkl)
(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 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
Attachment #506154 - Flags: review?(dkl) → review+
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
Attachment #506178 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval4.0?
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval+
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.