Adding @majavah, who correctly described the cause for removing in a private message.

21:02 <+logmsgbot> !log urbanecm@deploy1002 Synchronized wmf-config/InitialiseSettings.php: 27814b8eaacb5ba2fee1b6167a36ea14356a1ecf: testwiki: Fully remove securepoll-related groups (T290808) (duration: 00m 57s)
21:02 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log

Fixed the removal of the groups.

It might be going on way before the main ticket.

E.g. the non-NDA'ed user has created an election before T277353#7223260 (with id of 701, it doesn't have a vote in it). I think this needs to be looked at in more depth with someone more knowledgeable about SecurePoll.

Tagging AHT as the maintainers of the extension.

Sorry I keep coming up with notes. One thing is that signing an NDA doesn't mean the user can be trusted with private data. For example we have had a user who signed an NDA but globally locked. Handing out this right should have been done way more carefully.

In T290808#7346549, @Ladsgroup wrote:

Sorry I keep coming up with notes. One thing is that signing an NDA doesn't mean the user can be trusted with private data. For example we have had a user who signed an NDA but globally locked. Handing out this right should have been done way more carefully.

I strongly agree with this. Usually, rights that require NDA-level access are handed out by the stewards or T&S, and I think this would be an appropriate setup in testwiki's case as well. We'd need to figure out some criteria to be eligible for access (as voting makes no sense for testwiki).

In T290808#7346549, @Ladsgroup wrote:

Handing out this right should have been done way more carefully.

Yes.

In T290808#7346535, @Ladsgroup wrote:

So assuming securepoll logging is correct. The non-NDA'ed user has created one election (with id of 805) and accessed its logs four times. I can only see one vote in that particular election which is the user himself

select vote_voter_name from securepoll_votes where vote_election = 805;
+-----------------+
| vote_voter_name |
+-----------------+
| 4nn1l2          |
+-----------------+
1 row in set (0.00 sec)

so it means there is no leaked data unless:

• An election admin can look at other elections or

You are considered an admin of a particular election if you are a member of the electionadmin group and in the list of admins for that particular election (see https://gerrit.wikimedia.org/g/mediawiki/extensions/SecurePoll/+/5b31db7e97d399b00370620e0fe296f43cf94501/includes/Entities/Election.php#357).

You may only list the confidential information of voters in an election if you are an admin of that particular election (see https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/SecurePoll/+/refs/heads/master/includes/Pages/ListPager.php#48).

That user is only an admin for two elections, the IDs of which you've already noted:

select el_entity from securepoll_elections join securepoll_properties on el_entity = pr_entity where pr_key = 'admins' and pr_value like '%4nn1l2%';
+-----------+
| el_entity |
+-----------+
|       805 |
|       701 |
+-----------+
2 rows in set (0.00 sec)

<edit>

and only 1 vote was cast across both of those elections:

select count(*) from securepoll_votes where vote_election in (805, 701);
+----------+
| count(*) |
+----------+
|        1 |
+----------+
1 row in set (0.001 sec)

</edit>

• Some votes have been removed this election or

AFAICT only the cli/generateTestElection.php maintenance script deletes rows from the securepoll_votes table (i.e. votes that are struck remain in the table).

• There are more than one user without NDA in that list

I should also say thank you, @Ladsgroup, @Urbanecm, and @Majavah, for catching this and acting on a Saturday.

I understand this didn't have a breach of private information but it was pretty close to it. It would be great to have several layers of checks to make sure such close calls won't happen again. Our luck will run out one day.

@sbassett I think "someone" should help with those things:

• Do we need someone to formally declare "no infoleak happened" (if Ladsgroup's understanding is correct)?
• Draft next steps for re-enabling the group at testwiki
  • This should probably include a substep "Create process for becoming an electionadmin at testwiki"
• Make it more obvious that securepoll's electionadmin group grants access to PII (maybe create a proper right, like securepoll-view-votes and securepoll-view-ip-for-votes, to make it more obvious in the patch that PII is involved?) I think Anti-Harassment can help with this part (CC @Niharika)
• Figure out what to do to notice similar issues sooner. Maybe we should have a list of PII-enabled groups and alerting if someone without a NDA is added?

I'm not sure if Security-Team would be the right team to help with those items, or some other team at the WMF.

In T290808#7349572, @Urbanecm wrote:

• Draft next steps for re-enabling the group at testwiki
  • This should probably include a substep "Create process for becoming an electionadmin at testwiki"

An alternative to testing SecurePoll on testwiki might be to create and maintain an installation using Cloud VPS. This would short-circuit that particular conversation at the cost of increased maintenance burden.

• Make it more obvious that securepoll's electionadmin group grants access to PII (maybe create a proper right, like securepoll-view-votes and securepoll-view-ip-for-votes, to make it more obvious in the patch that PII is involved?) I think Anti-Harassment can help with this part (CC @Niharika)

This is a great suggestion. securepoll-view-ip-for-votes or securepoll-view-voter-ips would be preferable here as they are clearer about the nature of the information that the right gives the user access to. I can create a task to capture this once this task is made public.

Perhaps adding a note (or notes) to https://test.wikipedia.org/wiki/Wikipedia:Requests might be worthwhile as well.

In T290808#7351689, @phuedx wrote:

In T290808#7349572, @Urbanecm wrote:

• Draft next steps for re-enabling the group at testwiki
  • This should probably include a substep "Create process for becoming an electionadmin at testwiki"

An alternative to testing SecurePoll on testwiki might be to create and maintain an installation using Cloud VPS. This would short-circuit that particular conversation at the cost of increased maintenance burden.

Beta cluster seems like a good candidate for it.

In T290808#7349572, @Urbanecm wrote:

• Do we need someone to formally declare "no infoleak happened" (if Ladsgroup's understanding is correct)?

IMO, we can likely safely say this is the case, per @Ladsgroup's findings in T290808#7346535. I'm not sure there's much else the Security-Team (or anyone else) could audit here? We don't really have secret security logs with more wide-ranging data for cases like this. Ping @Dsharpe to confirm.

• Draft next steps for re-enabling the group at testwiki
  • This should probably include a substep "Create process for becoming an electionadmin at testwiki"
• Make it more obvious that securepoll's electionadmin group grants access to PII (maybe create a proper right, like securepoll-view-votes and securepoll-view-ip-for-votes, to make it more obvious in the patch that PII is involved?) I think Anti-Harassment can help with this part (CC @Niharika)

These sound like good ideas that the SecurePoll maintainers should further discuss and implement.

• Figure out what to do to notice similar issues sooner. Maybe we should have a list of PII-enabled groups and alerting if someone without a NDA is added?

That could work as a risk-reducing mitigation. Maybe somewhere in UserrightsPage::doSaveUserGroups?

In T290808#7357037, @sbassett wrote:

In T290808#7349572, @Urbanecm wrote:

• Do we need someone to formally declare "no infoleak happened" (if Ladsgroup's understanding is correct)?

IMO, we can likely safely say this is the case, per @Ladsgroup's findings in T290808#7346535. I'm not sure there's much else the Security-Team (or anyone else) could audit here? We don't really have secret security logs with more wide-ranging data for cases like this. Ping @Dsharpe to confirm.

I meant "give an authoritative interpretation of the data" rather than "add more data", sorry if that wasn't clear. I don't think that this call should be made by me :-).

Here is how I see matters like this: if we looked at everything we could reasonably think of and found no evidence of a leak, then a leak did not happen. That sounds exactly like the situation here. No leak happened.

Does anyone need anything else from me for this matter?

In T290808#7357118, @Urbanecm wrote:

In T290808#7357037, @sbassett wrote:

In T290808#7349572, @Urbanecm wrote:

• Do we need someone to formally declare "no infoleak happened" (if Ladsgroup's understanding is correct)?

IMO, we can likely safely say this is the case, per @Ladsgroup's findings in T290808#7346535. I'm not sure there's much else the Security-Team (or anyone else) could audit here? We don't really have secret security logs with more wide-ranging data for cases like this. Ping @Dsharpe to confirm.

I meant "give an authoritative interpretation of the data" rather than "add more data", sorry if that wasn't clear. I don't think that this call should be made by me :-).

One would be a script that gets run daily and checks members of any group that has access to confidentional information against the meta page.

It clearly is not enough, having volunteer-NDA doesn't mean the person is trusted as exampled above.

In T290808#7351689, @phuedx wrote:

In T290808#7349572, @Urbanecm wrote:

• Make it more obvious that securepoll's electionadmin group grants access to PII (maybe create a proper right, like securepoll-view-votes and securepoll-view-ip-for-votes, to make it more obvious in the patch that PII is involved?) I think Anti-Harassment can help with this part (CC @Niharika)

The above patch introduces a check for the securepoll-view-voter-pii right before showing the fields containing PII.

Could this ticket see some progress, please? Thanks a lot! :)

@phuedx could you please have someone else review your patch so that the security team can move forward and deploy it?

Based on a quick look, it looks good to me. I confirmed with a scrutineer of an ongoing election that they were not able to see PII until I made the config change to assign the right to votewiki election admins.

Sec patch deployed.

22:28 <+logmsgbot> !log urbanecm@deploy1002 Synchronized wmf-config/InitialiseSettings.php: 8f5008d9a043c96cd1dba18bdb38e168b01d63d0: votewiki: Grant election admins securepoll-view-voter-pii (T290808) (duration: 00m 55s)
22:28 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log
22:30 <urbanecm> !log Deploy a security patch for T290808
22:30 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log

@Tks4Fish mentioned PII moved to the end of the table. They said it's not a big deal, but @phuedx, maybe changing it to follow the previous layout is not a big issue?

@Urbanecm - thanks for the deploy, tracking at T292236 and T276237 now.

In T290808#7473300, @Urbanecm wrote:

@Tks4Fish mentioned PII moved to the end of the table. They said it's not a big deal, but @phuedx, maybe changing it to follow the previous layout is not a big issue?

Thanks for deployment, @Urbanecm. You're right that it's not a big issue to fix. I'll take a look.

@sbassett @Reedy @phuedx Hello, any objections to making this public? AFAICS, only a cosmetic change is pending now.

In T290808#7488103, @Urbanecm wrote:

@sbassett @Reedy @phuedx Hello, any objections to making this public? AFAICS, only a cosmetic change is pending now.

None from my side.

In T290808#7488103, @Urbanecm wrote:

@sbassett @Reedy @phuedx Hello, any objections to making this public? AFAICS, only a cosmetic change is pending now.

Given the findings from T290808#7346535, I think we're likely fine to make this public, but I'd defer to @Dsharpe one more time just in case he sees something that I do not.

In T290808#7490954, @sbassett wrote:

In T290808#7488103, @Urbanecm wrote:

@sbassett @Reedy @phuedx Hello, any objections to making this public? AFAICS, only a cosmetic change is pending now.

Given the findings from T290808#7346535, I think we're likely fine to make this public, but I'd defer to @Dsharpe one more time just in case he sees something that I do not.

Any update?

In T290808#7534343, @Urbanecm wrote:

Any update?

Yep, the Security-Team just chatted about this. It's fine to open this up. This is already tracked for the supplemental, could use backports though.

It was funny reading this :)

While I, as a non-paid volunteer, have been working hard helping the Anti-Harassment team and @Niharika implement Meek STV for WMF elections

https://www.mediawiki.org/wiki/Topic:Wdnlzj4xzf0ardcd

https://commons.wikimedia.org/wiki/File:Scottish_STV,_MediaWiki_SecurePoll_example.svg

https://www.mediawiki.org/w/index.php?title=Anti-Harassment_Tools/SecurePoll_Improvements/Understanding_the_STV_result&action=history

T288185

https://meta.wikimedia.org/w/index.php?title=Wikimedia_Foundation_elections/2021/Results&diff=prev&oldid=21993043

https://meta.wikimedia.org/wiki/Talk:Movement_Charter/Drafting_Committee/Elections#Heads_up_for_alternates

https://meta.wikimedia.org/w/index.php?title=Movement_Charter/Drafting_Committee/Elections/Results&diff=prev&oldid=22358958

some other people have been concerned about leaking or breaching private data by bad actors!

I don't need anyone's private data and have signed NDA as a former VRT agent. (I know, I know there are multiple NDAs, no need to mention that, but you should use correct terminology yourselves).

Hope you let innocent users play with SecurePoll on testwiki again. That really helps developing things. Users still cannot test SecurePoll on testwiki and that is frustrating: T277353#7234897

While this discussion is gathering people's attention, may I remind this important security gap: T152972: Accessing private information through SecurePoll should be logged

In T290808#7550272, @4nn1l2 wrote:

It was funny reading this :)

While I, as a non-paid volunteer, have been working hard helping the Anti-Harassment team and @Niharika implement Meek STV for WMF elections

https://www.mediawiki.org/wiki/Topic:Wdnlzj4xzf0ardcd

https://commons.wikimedia.org/wiki/File:Scottish_STV,_MediaWiki_SecurePoll_example.svg

https://www.mediawiki.org/w/index.php?title=Anti-Harassment_Tools/SecurePoll_Improvements/Understanding_the_STV_result&action=history

T288185

https://meta.wikimedia.org/w/index.php?title=Wikimedia_Foundation_elections/2021/Results&diff=prev&oldid=21993043

https://meta.wikimedia.org/wiki/Talk:Movement_Charter/Drafting_Committee/Elections#Heads_up_for_alternates

https://meta.wikimedia.org/w/index.php?title=Movement_Charter/Drafting_Committee/Elections/Results&diff=prev&oldid=22358958

some other people have been concerned about leaking or breaching private data by bad actors!

I don't need anyone's private data and have signed NDA as a former VRT agent. (I know, I know there are multiple NDAs, no need to mention that, but you should use correct terminology yourselves).

Hope you let innocent users play with SecurePoll on testwiki again. That really helps developing things. Users still cannot test SecurePoll on testwiki and that is frustrating: T277353#7234897

You shouldn't take it personally, @4nn1l2. It is not about you. This could have happened with another person who is not as well intended. A safety flaw is a safety flaw, no matter the outcome. In fact, focusing on the outcome is a known mistake (called outcome bias) and what I gather from above is that ultimately the team is aware of it. It shows where @Ladsgroup says "our luck will run out one day".

@Urbanecm in the task description you said "... we effectively allowed testwiki's bureaucrats to grant users access to confidential information".

Isn't that part of the problem? Shouldn't $wgGrantPermissions be overwritten in InitialiseSettings.php to ensure that only stewards can assign electionadmin right on WMF production wikis?

Change 744669 had a related patch set uploaded (by Urbanecm; author: Phuedx):

[mediawiki/extensions/SecurePoll@master] SECURITY: Require securepoll-view-voter-pii right to view voter's PII

https://gerrit.wikimedia.org/r/744669

Change 743940 had a related patch set uploaded (by Urbanecm; author: Phuedx):

[mediawiki/extensions/SecurePoll@REL1_37] SECURITY: Require securepoll-view-voter-pii right to view voter's PII

https://gerrit.wikimedia.org/r/743940

In T290808#7551597, @Huji wrote:

@Urbanecm in the task description you said "... we effectively allowed testwiki's bureaucrats to grant users access to confidential information".

Isn't that part of the problem? Shouldn't $wgGrantPermissions be overwritten in InitialiseSettings.php to ensure that only stewards can assign electionadmin right on WMF production wikis?

Not necessarily. For votewiki, this risk pretty much doesn't exist -- we trust votewiki bureaucrats for this kind of a task. It was only an issue for wikis where bureaucrats have no connection with handling PII. Honestly, I don't think we need stewards to act as a rubber stamp for WMF staff managing securepoll elections and while the new rule you mention could have a votewiki-specific exemption, I'm pretty sure people trying to enable creating pools on new wikis will...try to copy votewiki config :)).

I think a better solution is implemented in the security patch deployed as part of this task (see the one I just merged to master), which requires users having securepoll-view-voter-pii in addition to being election admins before they're allowed to view PII. That right should make it pretty obvious that you're adding PII access, and I believe a change like https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/736067 will not pass review for testwiki (at least not without someone asking questions "are we sure letting users to access PII is a good idea").

Of course, we should have more layers of defense for this. One of the ideas could be to construct a list of PII accessing groups, and audit users for NDA membership regularly. If an issue is found, it can notify...someone (stewards+T&S sound like a reasonable first choice for me).

Change 744669 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@master] SECURITY: Require securepoll-view-voter-pii right to view voter's PII

https://gerrit.wikimedia.org/r/744669

Change 744761 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/SecurePoll@master] Fully add new securepoll-view-voter-pii right

https://gerrit.wikimedia.org/r/744761

Change 744761 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@master] Fully add new securepoll-view-voter-pii right

https://gerrit.wikimedia.org/r/744761

Change 743940 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@REL1_37] SECURITY: Require securepoll-view-voter-pii right to view voter's PII

https://gerrit.wikimedia.org/r/743940

Change 747076 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/SecurePoll@REL1_37] Fully add new securepoll-view-voter-pii right

https://gerrit.wikimedia.org/r/747076

Change 747513 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/SecurePoll@master] listPager: Restore original field order

https://gerrit.wikimedia.org/r/747513

Change 749228 had a related patch set uploaded (by SBassett; author: Phuedx):

[mediawiki/extensions/SecurePoll@REL1_35] SECURITY: Require securepoll-view-voter-pii right to view voter's PII

https://gerrit.wikimedia.org/r/749228

Change 749229 had a related patch set uploaded (by SBassett; author: Phuedx):

[mediawiki/extensions/SecurePoll@REL1_36] SECURITY: Require securepoll-view-voter-pii right to view voter's PII

https://gerrit.wikimedia.org/r/749229

Change 749228 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@REL1_35] SECURITY: Require securepoll-view-voter-pii right to view voter's PII

https://gerrit.wikimedia.org/r/749228

Change 749229 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@REL1_36] SECURITY: Require securepoll-view-voter-pii right to view voter's PII

https://gerrit.wikimedia.org/r/749229

Change 749885 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/SecurePoll@REL1_36] Fully add new securepoll-view-voter-pii right

https://gerrit.wikimedia.org/r/749885

Change 750806 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/SecurePoll@REL1_35] Fully add new securepoll-view-voter-pii right

https://gerrit.wikimedia.org/r/750806

Change 747076 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@REL1_37] Fully add new securepoll-view-voter-pii right

https://gerrit.wikimedia.org/r/747076

Change 749885 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@REL1_36] Fully add new securepoll-view-voter-pii right

https://gerrit.wikimedia.org/r/749885

Change 750806 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@REL1_35] Fully add new securepoll-view-voter-pii right

https://gerrit.wikimedia.org/r/750806