Closed
Bug 621107
Opened 13 years ago
Closed 13 years ago
[SECURITY] Sanity checking lacks CSRF protection
Categories
(Bugzilla :: Administration, task)
Tracking
()
RESOLVED
FIXED
Bugzilla 4.0
People
(Reporter: reed, Assigned: LpSolit)
References
Details
(Whiteboard: [infrasec:csrf][ws:low])
Attachments
(2 files, 2 obsolete files)
6.07 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
6.13 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
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 2•13 years ago
|
||
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?
Reporter | ||
Comment 3•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
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 → ---
Reporter | ||
Comment 6•13 years ago
|
||
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 → ---
Reporter | ||
Updated•13 years ago
|
Status: REOPENED → NEW
Priority: P3 → --
Target Milestone: --- → Bugzilla 3.2
Comment 7•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Assignee: administration → LpSolit
Comment 8•13 years ago
|
||
(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
Reporter | ||
Comment 9•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 11•13 years ago
|
||
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)
Assignee | ||
Comment 12•13 years ago
|
||
I forgot the Voting extension.
Attachment #499862 -
Attachment is obsolete: true
Attachment #499890 -
Flags: review?(mkanat)
Attachment #499862 -
Flags: review?(mkanat)
Assignee | ||
Updated•13 years ago
|
Attachment #499890 -
Flags: review?(mkanat) → review?(dkl)
Reporter | ||
Updated•13 years ago
|
Flags: blocking4.0?
Flags: blocking3.6.4?
Flags: blocking3.4.10?
Flags: blocking3.2.10?
Comment 13•13 years ago
|
||
Comment on attachment 499890 [details] [diff] [review] patch, v1.1 Looks good and works as expected. r=dkl
Attachment #499890 -
Flags: review?(dkl) → review+
Updated•13 years ago
|
Flags: approval?
Flags: approval4.0?
Comment 14•13 years ago
|
||
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
Comment 15•13 years ago
|
||
I also think that we should unprotect this bug and remove the tags from the status whiteboard.
Comment 16•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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]
Comment 20•13 years ago
|
||
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-
Reporter | ||
Comment 21•13 years ago
|
||
(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]
Assignee | ||
Comment 22•13 years ago
|
||
I'm on it. It will be ready on time, so no worry. :)
Status: NEW → ASSIGNED
Flags: blocking4.0? → blocking4.0-
Comment 23•13 years ago
|
||
(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.
Assignee | ||
Comment 24•13 years ago
|
||
Attachment #499890 -
Attachment is obsolete: true
Attachment #506154 -
Flags: review?(dkl)
Comment 25•13 years ago
|
||
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-
Assignee | ||
Comment 26•13 years ago
|
||
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)
Assignee | ||
Comment 27•13 years ago
|
||
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)
Comment 28•13 years ago
|
||
(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•13 years ago
|
||
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 31•13 years ago
|
||
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+
Updated•13 years ago
|
Flags: approval?
Flags: approval4.0?
Assignee | ||
Updated•13 years ago
|
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval+
Assignee | ||
Comment 32•13 years ago
|
||
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 ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•