Closed
Bug 621105
Opened 13 years ago
Closed 13 years ago
[SECURITY] Voting lacks CSRF protection
Categories
(Bugzilla :: Extensions, defect, P3)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.2
People
(Reporter: reed, Assigned: dkl)
References
Details
(Whiteboard: [infrasec:csrf][ws:moderate])
Attachments
(2 files, 2 obsolete files)
1.67 KB,
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
1.86 KB,
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
Voting lacks CSRF protection.
Comment 1•13 years ago
|
||
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
Reporter | ||
Updated•13 years ago
|
Whiteboard: [infrasec:csrf][ws:critical] → [infrasec:csrf][ws:moderate]
Assignee | ||
Updated•13 years ago
|
Attachment #501796 -
Flags: review? → review?(mkanat)
Comment 4•13 years ago
|
||
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-
Assignee | ||
Comment 5•13 years ago
|
||
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 6•13 years ago
|
||
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?
Updated•13 years ago
|
Assignee: extensions → dkl
Reporter | ||
Updated•13 years ago
|
Assignee | ||
Comment 7•13 years ago
|
||
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)
Assignee | ||
Comment 8•13 years ago
|
||
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 9•13 years ago
|
||
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 10•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #501796 -
Attachment is obsolete: false
Updated•13 years ago
|
Attachment #502893 -
Flags: review?(mkanat) → review-
Comment 11•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #501780 -
Attachment is obsolete: false
Updated•13 years ago
|
Attachment #502881 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #502893 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Flags: approval?
Flags: approval4.0?
Flags: approval3.6?
Comment 12•13 years ago
|
||
I don't really see that this is important enough to backport past 3.6.
Reporter | ||
Updated•13 years ago
|
Flags: blocking3.4.10?
Flags: blocking3.2.10?
Updated•13 years ago
|
Target Milestone: Bugzilla 3.2 → Bugzilla 3.6
Updated•13 years ago
|
Flags: blocking4.0?
Flags: blocking4.0+
Flags: blocking3.6.4?
Flags: blocking3.6.4+
Comment 13•13 years ago
|
||
(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
Comment 14•13 years ago
|
||
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.
Comment 15•13 years ago
|
||
Also, whoever writes the patches is on the hook to help provide any newly-needed APIs for clients that break.
Comment 17•13 years ago
|
||
(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.
Comment 18•13 years ago
|
||
(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.
Comment 19•13 years ago
|
||
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.
Comment 20•13 years ago
|
||
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.
Comment 21•13 years ago
|
||
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+
Comment 22•13 years ago
|
||
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+
Updated•13 years ago
|
Target Milestone: Bugzilla 3.6 → Bugzilla 3.2
Comment 23•13 years ago
|
||
No backport needed. The patch applies cleanly and works correctly.
Updated•13 years ago
|
Attachment #501796 -
Attachment description: Patch to add CSRF voting protection for trunk (v1) → Patch to add CSRF voting protection for 4.0 + trunk (v1)
Updated•13 years ago
|
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)
Assignee | ||
Comment 24•13 years ago
|
||
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
Comment 25•13 years ago
|
||
Security advisory sent. Removing the security flag.
Group: bugzilla-security
Comment hidden (spam) |
You need to log in
before you can comment on or make changes to this bug.
Description
•