[SECURITY] Sanity checking lacks CSRF protection

RESOLVED FIXED in Bugzilla 4.0

Status

()

Bugzilla
Administration
--
minor
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: reed, Assigned: Frédéric Buclin)

Tracking

(Blocks: 1 bug)

3.6.3
Bugzilla 4.0
Dependency tree / graph
Bug Flags:
approval +
approval4.0 +
blocking4.0 -
blocking3.6.4 -
blocking3.4.10 -
blocking3.2.10 -

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
Ah, my instinct here is WONTFIX.
Priority: -- → P3

Comment 2

7 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

7 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

7 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

7 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
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
Target Milestone: Bugzilla 3.2 → ---
(Reporter)

Comment 6

7 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

7 years ago
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)

Updated

7 years ago
Assignee: administration → LpSolit

Comment 8

7 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

7 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

7 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

7 years ago
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.
Attachment #499862 - Flags: review?(mkanat)
(Assignee)

Comment 12

7 years ago
Created attachment 499890 [details] [diff] [review]
patch, v1.1

I forgot the Voting extension.
Attachment #499862 - Attachment is obsolete: true
Attachment #499890 - Flags: review?(mkanat)
Attachment #499862 - Flags: review?(mkanat)
(Assignee)

Updated

7 years ago
Attachment #499890 - Flags: review?(mkanat) → review?(dkl)
(Reporter)

Updated

7 years ago
Flags: blocking4.0?
Flags: blocking3.6.4?
Flags: blocking3.4.10?
Flags: blocking3.2.10?
(Reporter)

Updated

7 years ago
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+

Updated

7 years ago
Flags: approval?
Flags: approval4.0?

Comment 14

7 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

7 years ago
  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?

Comment 17

7 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

7 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

6 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

6 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

6 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

6 years ago
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.
(Assignee)

Comment 24

6 years ago
Created attachment 506154 [details] [diff] [review]
patch, v2
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-
(Assignee)

Comment 26

6 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

6 years ago
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.
Attachment #506178 - Flags: review?(dkl)

Comment 28

6 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 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+

Updated

6 years ago
Flags: approval?
Flags: approval4.0?
(Assignee)

Updated

6 years ago
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval+
(Assignee)

Comment 32

6 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
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED

Updated

4 years ago
Blocks: 835424
You need to log in before you can comment on or make changes to this bug.