Closed Bug 621105 Opened 13 years ago Closed 13 years ago

[SECURITY] Voting lacks CSRF protection

Categories

(Bugzilla :: Extensions, defect, P3)

3.6.3

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: reed, Assigned: dkl)

References

Details

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

Attachments

(2 files, 2 obsolete files)

Voting lacks CSRF protection.
And is unlikely to get it any time soon unless some brave soul wishes to implement this.

I don't think this is ws:critical--there's nothing particularly dangerous about voting.
Severity: normal → minor
Priority: -- → P3
Whiteboard: [infrasec:csrf][ws:critical] → [infrasec:csrf][ws:moderate]
Patch 3.6, v1
Attachment #501780 - Flags: review?(mkanat)
Patch trunk, v1
Attachment #501796 - Flags: review?
Attachment #501796 - Flags: review? → review?(mkanat)
Comment on attachment 501796 [details] [diff] [review]
Patch to add CSRF voting protection for 4.0 + trunk (v1)

Ah, you're mis-using issue_hash_token. The data in the array needs to be something that prevents replaying the token, not just a token name. See how issue_hash_token is used elsewhere.
Attachment #501796 - Flags: review?(mkanat) → review-
Comment on attachment 501796 [details] [diff] [review]
Patch to add CSRF voting protection for 4.0 + trunk (v1)

Re-requesting review based on outcome of bug 621090.
Attachment #501796 - Flags: review- → review?(mkanat)
Comment on attachment 501796 [details] [diff] [review]
Patch to add CSRF voting protection for 4.0 + trunk (v1)

Ah, this one still needs at least some bug ids or something, doesn't it?
Assignee: extensions → dkl
Blocks: 620540
Flags: blocking4.0?
Flags: blocking3.6.4?
Flags: blocking3.4.10?
Flags: blocking3.2.10?
New patch for trunk that uses the list of bug ids when calling issue_hash_token.

Dave
Attachment #501796 - Attachment is obsolete: true
Attachment #502881 - Flags: review?(mkanat)
Attachment #501796 - Flags: review?(mkanat)
New patch that uses the bug ids in the issue_hash_token and check_hash_token calls for 3.6.

Dave
Attachment #501780 - Attachment is obsolete: true
Attachment #502893 - Flags: review?(mkanat)
Attachment #501780 - Flags: review?(mkanat)
Comment on attachment 502881 [details] [diff] [review]
Patch to add CSRF voting protection for trunk (v2)

Ah, you know, this does add a lot of complexity and voting is not all that important to keep secure. Let's just go with your original, simpler patch.
Attachment #502881 - Flags: review?(mkanat) → review-
Comment on attachment 501796 [details] [diff] [review]
Patch to add CSRF voting protection for 4.0 + trunk (v1)

This looks good.
Attachment #501796 - Flags: review+
Attachment #501796 - Attachment is obsolete: false
Attachment #502893 - Flags: review?(mkanat) → review-
Comment on attachment 501780 [details] [diff] [review]
Patch to add CSRF voting protection for 3.2 - 3.6 (v1)

As does this.
Attachment #501780 - Flags: review+
Attachment #501780 - Attachment is obsolete: false
Attachment #502881 - Attachment is obsolete: true
Attachment #502893 - Attachment is obsolete: true
Flags: approval?
Flags: approval4.0?
Flags: approval3.6?
I don't really see that this is important enough to backport past 3.6.
Flags: blocking3.4.10?
Flags: blocking3.2.10?
Target Milestone: Bugzilla 3.2 → Bugzilla 3.6
Flags: blocking4.0?
Flags: blocking4.0+
Flags: blocking3.6.4?
Flags: blocking3.6.4+
(In reply to comment #12)
> I don't really see that this is important enough to backport past 3.6.

Actually, for the reason I gave on IRC, it would still make sense to take this bug for 3.2 and 3.4.

<LpSolit> I use votes to keep an eye when I don't want to CC myself (when I don't care about comments)
<LpSolit> if someone manages to remove all my votes, I would be unable to retrieve the list as it's not in the bug history
Status: NEW → ASSIGNED
  Okay. Whether or not we take this for those branches will depend on whether or not there are patches that have been tested and reviewed for them by Monday.
Also, whoever writes the patches is on the hook to help provide any newly-needed APIs for clients that break.
(In reply to comment #15)
> Also, whoever writes the patches is on the hook to help provide any
> newly-needed APIs for clients that break.

No, that's definitely not their job. You are just trying to discourage someone from writing these backports.
(In reply to comment #17)
> No, that's definitely not their job. You are just trying to discourage someone
> from writing these backports.

  No, I'm encouraging people to take responsibility for what they are causing. I'm saying, "If you are going to this against my wishes, then I really don't want you to also be creating extra work for me that I am going to HAVE to do." I don't see why somebody else should have the right to cause me to have to do more work that *I* will ultimately be on the hook for--if somebody writes a patch that is going to cause other problems, then they are the person responsible for handling those problems, not me.
I'm going to approve this for branches for the same reasons I gave in bug 621090 comment 36 (too lengthly to repeat here, and you can see that one publicly).

On the stability of compatibility with released branches I can probably be talked out of 3.6 and earlier, though I personally see no reason not to take it for those anyway, and am of the opinion that we should still take it on those, but this should definitely go into 4.0.
  Sure, I had already approved this for 4.0 and 3.6 both. And this bug is indeed locked, so I agree it's a security issue.

  Does the existing patch apply to 3.4 and 3.2? If this bug is retargeted to 3.2 and the existing patch does not apply, then this bug will miss the boat for the release unless it those patches are supplied. If this bug is not retargeted to 3.2, then I will assume we're only checking it back to 3.6 and release it accordingly.
justdave, so what do you decide for 3.2 and 3.4?
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval3.4?
Flags: approval3.2?
Flags: approval+
a=justdave on IRC for 3.2 and 3.4. dkl is checking right now if a backport is needed or not.
Flags: approval3.4?
Flags: approval3.4+
Flags: approval3.2?
Flags: approval3.2+
Target Milestone: Bugzilla 3.6 → Bugzilla 3.2
No backport needed. The patch applies cleanly and works correctly.
Attachment #501796 - Attachment description: Patch to add CSRF voting protection for trunk (v1) → Patch to add CSRF voting protection for 4.0 + trunk (v1)
Attachment #501780 - Attachment description: Patch to add CSRF voting protection for 3.6 (v1) → Patch to add CSRF voting protection for 3.2 - 3.6 (v1)
3.2:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/3.2/   modified votes.cgi
modified template/en/default/bug/votes/delete-all.html.tmpl
modified template/en/default/bug/votes/list-for-user.html.tmpl              Committed revision 6413.

3.4:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/3.4/ modified votes.cgi
modified template/en/default/bug/votes/delete-all.html.tmpl
modified template/en/default/bug/votes/list-for-user.html.tmpl
Committed revision 6792.

3.6:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/3.6/ modified votes.cgi
modified template/en/default/bug/votes/delete-all.html.tmpl
modified template/en/default/bug/votes/list-for-user.html.tmpl            Committed revision 7225.

4.0:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.0/
modified extensions/Voting/Extension.pm
modified extensions/Voting/template/en/default/pages/voting/user.html.tmpl
modified extensions/Voting/template/en/default/voting/delete-all.html.tmpl  Committed revision 7532.

trunk:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified extensions/Voting/Extension.pm
modified extensions/Voting/template/en/default/pages/voting/user.html.tmpl
modified extensions/Voting/template/en/default/voting/delete-all.html.tmpl    Committed revision 7675.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Security advisory sent. Removing the security flag.
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.