Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[SECURITY] Voting lacks CSRF protection

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Extensions
P3
minor
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: reed, Assigned: dkl)

Tracking

(Blocks: 1 bug)

3.6.3
Bugzilla 3.2
Dependency tree / graph
Bug Flags:
approval +
approval4.0 +
blocking4.0 +
approval3.6 +
blocking3.6.4 +
approval3.4 +
approval3.2 +

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Voting lacks CSRF protection.

Comment 1

7 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

7 years ago
Whiteboard: [infrasec:csrf][ws:critical] → [infrasec:csrf][ws:moderate]
(Assignee)

Comment 2

7 years ago
Created attachment 501780 [details] [diff] [review]
Patch to add CSRF voting protection for 3.2 - 3.6 (v1)

Patch 3.6, v1
Attachment #501780 - Flags: review?(mkanat)
(Assignee)

Comment 3

7 years ago
Created attachment 501796 [details] [diff] [review]
Patch to add CSRF voting protection for 4.0 + trunk (v1)

Patch trunk, v1
Attachment #501796 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #501796 - Flags: review? → review?(mkanat)

Comment 4

7 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

7 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

7 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

7 years ago
Assignee: extensions → dkl
(Reporter)

Updated

7 years ago
Blocks: 620540
Flags: blocking4.0?
Flags: blocking3.6.4?
Flags: blocking3.4.10?
Flags: blocking3.2.10?
(Assignee)

Comment 7

7 years ago
Created attachment 502881 [details] [diff] [review]
Patch to add CSRF voting protection for trunk (v2)

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

7 years ago
Created attachment 502893 [details] [diff] [review]
Patch to add CSRF voting protection for 3.6 (v2)

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

7 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

7 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

7 years ago
Attachment #501796 - Attachment is obsolete: false

Updated

7 years ago
Attachment #502893 - Flags: review?(mkanat) → review-

Comment 11

7 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

7 years ago
Attachment #501780 - Attachment is obsolete: false

Updated

7 years ago
Attachment #502881 - Attachment is obsolete: true

Updated

7 years ago
Attachment #502893 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 12

7 years ago
I don't really see that this is important enough to backport past 3.6.
(Reporter)

Updated

7 years ago
Flags: blocking3.4.10?
Flags: blocking3.2.10?

Updated

7 years ago
Target Milestone: Bugzilla 3.2 → Bugzilla 3.6

Updated

7 years ago
Flags: blocking4.0?
Flags: blocking4.0+
Flags: blocking3.6.4?
Flags: blocking3.6.4+

Comment 13

7 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

7 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

7 years ago
Also, whoever writes the patches is on the hook to help provide any newly-needed APIs for clients that break.

Comment 17

7 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

7 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.
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

7 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

7 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

7 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

7 years ago
Target Milestone: Bugzilla 3.6 → Bugzilla 3.2

Comment 23

7 years ago
No backport needed. The patch applies cleanly and works correctly.

Updated

7 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

7 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

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

Comment 25

7 years ago
Security advisory sent. Removing the security flag.
Group: bugzilla-security

Updated

5 years ago
Blocks: 835424
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.