Hello @ifried, @ldelench_wmf, @gonyeahialam

Is there any status update for this project? Is there any code available for us to review? Is it nearing being ready for review?

If it's not, it might need to be pushed back. There may also be scope for doing a different type of review (concept or similar).

Hey @ifried, @ldelench_wmf, @gonyeahialam -

Just wanted to confirm that you saw @Reedy's message above. If this work isn't ready for security review just yet, we'll plan to move it back to our backlog for next quarter (or whenever). Thanks.

Thank you for checking in -- and apologies, I must have missed the earlier message.

We're not ready for a security review. We don't have any finalized code yet, but we can let you know when the situation changes. It makes sense to move this back to next quarter or whenever the team is ready for review (we don't have a projected date yet).

Also, we now have an engineering team: @cmelo, @JCarvalho, and @Daimona, who can be consulted directly on questions related to the code.

Ok, thanks, @ifried. We'll go ahead and backlog this task for now.

Sounds good. Thanks, @sbassett.

Hello @ifried @vyuen -

Looking at T302317, do we have a better idea for when this task might be actionable? The Security-Team was going to include it within our Q4 2022 appsec planning session (happening in a couple of weeks) but if the code is a ways off yet, we won't.

@sbassett we are about 1 month into the development cycle, and we are aiming to release the first version of the product by end of June. However, if there is an opportunity to conduct a security review before development work is fully complete, that would be fantastic!

I see it was mentioned above there can be "a different type of review (concept or similar)", could you (or @Reedy) clarify what that means, or point me to the right documentation, please? Essentially, I am interested in understanding what is the earliest point in the development cycle can this security review be conducted with confidence. Thank you!

Hey @vyuen -

Currently, the Security-Team offers security preview/concept review/threat modeling sessions, which are completely optional. These are generally recommended to be performed early on in a codebase's development cycle. If there's interest in this, please go ahead and submit a new Phabricator request via the form link within the aforementioned process documentation.

Unfortunately, security previews are not a substitute for our application security reviews. Application security reviews are generally considered mandatory for anything new that will be going into production and really need to be performed upon as close to production-ready code as possible. So it sounds like we should probably plan for this review to be performed sometime towards the beginning or middle of June. I think we could plan for that time frame and then work with the assigned engineer in determining a cutoff commit within the relevant codebase(s) where we would conduct our review.

Please feel free to let me know if you have any other questions.

Thank you for the information @sbassett! I read through the documentation you linked, and in particular the section "Work product this may be relevant for", and it looks like our project does not check any of those boxes. So I believe we will not require security preview at the moment.

Application security reviews are generally considered mandatory for anything new that will be going into production and really need to be performed upon as close to production-ready code as possible. So it sounds like we should probably plan for this review to be performed sometime towards the beginning or middle of June. I think we could plan for that time frame and then work with the assigned engineer in determining a cutoff commit within the relevant codebase(s) where we would conduct our review.

I think planning for beginning/mid June is very reasonable. @cmelo is the tech lead on this project; it will make sense for us to circle back on this closer to that date.

Thanks again!

@vyuen - We've tentatively accepted this review to be completed this quarter. However that will be entirely conditional upon when there is some working code to review. If we can get something that is getting close to production-ready by mid-May, then this should remain very doable. If not, we may need to move the review into the next quarter.

@vyuen @cmelo @ifried do we have any updates on code or deployment?

Hi @sbassett @Mstyles,

From the previous discussion, I was under the impression that we could have the review performed around mid-June, and by then we will have close to production-ready code. I am afraid mid-May will be too soon for us.
For our team, it will be ideal to plan for this review sometime between mid-June to early-July, will that be doable for y'all? We do not plan to ship this to production until at least the beginning of next quarter (July 2022) so we don't need the review to be wrapped up by the end of this quarter.

Happy to jump on a call to clarify things as well, if need be. Thanks!

@vyuen since the quarter ends on June 30, the middle of June is the absolute latest we can schedule a review for the quarter. If you think this is most likely going to be completed late June/early July, then it would make more sense to push this review to next quarter.

@Mstyles apologies as I was not aware that your team plans the reviews out on a quarterly basis. I think for planning purposes, pushing the review to next quarter works for our timeline, as long as we can be "queued up" for the beginning of the quarter (early July). Will that work for you?

Hi again @Mstyles @sbassett, I wanted to give you an update that we are slated for deploying v0 to beta cluster on July 13, and then the code of the extension will be ready to be reviewed. We are targeting late September/early October for our v1 production deployment. Just for your information and clarity: there will be additional features in v1 that won't be in v0. We think it makes sense to have a security review done with v0 code and then later with v1 additional codes, rather than waiting for v1 to do an extensive security review. But please let me know if you have a better strategy in mind.

On the note of the v0 deployment to beta cluster, I see that from the documentation Writing_an_extension_for_deployment#Preparing_for_deployment: "4. [...] While it is strongly recommended to have a security readiness review performed prior to beta cluster deployment, the timing of various project milestones and the nature of the project itself may not accommodate this. In this case, it is best to discuss any proposed beta cluster deployments with the Security Team outside of any requested reviews." So we would like to confirm with your team that we are okay with deploying to beta cluster next week?

Please let me know if I can provide any more information. Thank you!

thanks @vyuen for the update, that sounds good to have a v0 review for this quarter.

Hello @vyuen -

Just wanted to check in and see how the beta deployment has gone. Would the relevant code (so far just the CampaignEvents extension?) be ready for review now or sometime soon? Thanks.

Hi @Mstyles @sbassett, we deployed the CampaignsEvents extension to beta wiki last week! Please let me know if there is anything else you need from our end. Thanks!

Hello @vyuen -

If you and your team are comfortable with where master is (a531508c37) for the CampaignEvents extension, we can go ahead and begin the review up until that commit. I'd note that we try to "freeze" the code at a good point for our reviews at a point where it should be unlikely that major new changes, refactors or features will be added in the near future.

A lack of basic logging is mentioned in https://meta.wikimedia.org/w/index.php?title=Talk:Campaigns/Foundation_Product_Team/Registration&diff=23551847&oldid=23551567 ; does the extension comply with https://www.mediawiki.org/wiki/Everything_is_a_wiki_page ? If not, it would be useful to share a design explaining how it will implement the various security basics.

See Potential registration userflows doc > Experience of participants who fill out registration form (V1)

This document is private. Please share on a wiki page.

Completed threat model: https://docs.google.com/document/d/1JS33r9O1YNlHEz65lGwyq9Yb95dPDEg7fTc4IrdoiI8 (WMF Internal)

Hello @ifried, @ldelench_wmf, @vyuen -

With the initial threat model now completed, I wanted to check in to see if there was a good commit to select for the application security review of ext:CampaignEvents. The code seems somewhat stable IMO (master log, gerrit change sets) but I wanted to confirm that your team didn't have any major changes or refactors in progress at this time. If you do, then we may need to revisit the timeline for this review given that the current quarter ends in a few more weeks. Thanks.

@sbassett thanks for checking in! I think we are about ready for the security review. The only patch we have pending that we would like to ship to production after the review is one that changes how event addresses are stored and handled (for T316128). Can you please advise on whether this is considered a major change that blocks the security review? If not, I think we can go ahead with starting the review. Thanks again!

@sbassett another follow-up: the patch previously mentioned has been merged. The latest commit would be a good point to code freeze for review.

In T290248#8245363, @vyuen wrote:

@sbassett another follow-up: the patch previously mentioned has been merged. The latest commit would be a good point to code freeze for review.

Ok, thanks, @vyuen. I'll plan to review up to b50ce32455. I hope to have the review posted likely by the middle of next week.

Security Review Summary - T290248 - 2022-09-30
Last commit reviewed: b50ce32455

Summary

Overall, the current CampaignEvents extension appears to be in good shape and currently has an overall risk rating of: medium, which will require mitigations to reduce to a lower risk level or risk acceptance/ownership at the manager/director level at the WMF, per the current risk management framework. The primary driver of this risk rating are the remaining open items discussed during the threat modeling sessions and again summarized within the General Security Issues section below.

Vulnerable Packages - Production

No production javascript or php dependencies are used by this extension - low risk

Vulnerable Packages - Development

Overall risk rating: low risk

As reported via npm audit:

As reported via scan.sh:

Outdated Packages
As reported via npm outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)

As reported via composer outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)

Static Analysis Findings

1. lockfile-lint returned no results. low risk
2. gitleaks returned no results. low risk
3. git secrets run with a custom pattern set, returned no actionable results. low risk
4. whispers returned no results. low risk
5. scan scan:latest returned no results, outside of the previously-mentioned vulnerable packages. low risk
6. phan with the phan-taint-check plugin was run against the extension as installed with a recent verison of mediawiki core. No results were found. low risk
7. semgrep 0.115.0 was run with several popular and custom rule-sets - we are happy to review these rule-sets in detail with the requesters. The raw results are found within P35239, most of which should be false positives and none of which appeared to be legitimate vulnerabilities at this time. It would likely be worthwhile for the developers of the extension to give these results a cursory glance and review various data sinks (especially within a file like resources/ext.campaignEvents.specialeventdetails/ParticipantsManager.js) to ensure their expectations are correct.

DAST Findings

1. I would like to run some baseline scans via Zap (and possibly other, similar FOSS scanners) if I have time next week, but these should not block on the completion of this review.

General Security Issues

1. I assume that since I don't see any messages listed under a RawHtmlMessages key within extension.json, that no messages relevant to this extension utilize raw html. This would really only have implications for the Html::rawElement calls within src/EventPage/EventPageDecorator.php and various message-rendering snippets within certain javascript front-end files (e.g. resources/ext.campaignEvents.specialeventdetails/ParticipantsManager.js) which either passed phan-taint-check and/or didn't appear problematic from a cursory manual review. low risk
2. Per the related threat modeling exercise, the issue of requiring authenticated users should now be resolved via T304645: Update 'permission error' page when user tries to access event center & not logged in. This is classified as low risk.
3. Per the related threat modeling exercise, the issue of allowing users more privacy while using the Campaign Registration system is being tracked at T316405: [EPIC] Allow participant option of joining event privately. This is classified as a medium risk until resolved.
4. Per the related threat modeling exercise, the issue of better vetting/enforcement for the selection of event organizers is tracked at T316227: Specify initial list of organizers via configuration. This is classified as a medium risk until resolved.
5. Per the related threat modeling exercise, the issue of potentially retaining somewhat-sensitive date related to events is tracked at both T308582: Implement ability for organizer to delete registration [completed] and the related T316405: [EPIC] Allow participant option of joining event privately. This is classified as a high risk until resolved.
6. Per the related threat modeling exercise, the issue of improved user action logging is currently untracked. This is classified as a low risk until resolved.
7. Per the related threat modeling exercise, the issue of making users aware that certain quasi-sensitive data may be public via the CampaignEvents extension should now be resolved via T316408: Improve policy notification when registering and T309333: Create popup with policy acknowledgement for when users register on the event page. This is classified as low risk.
8. Per the related threat modeling exercise, the issue of securely accessing a third party API (Pelias) is tracked via the additional security review task {T309410}, the public "RFC" task T312677: [Request for Comment] Campaigns Geolocation API proposal and related tasks like T309325: Cross-team review of Geolocation API proposal. This is classified as a high risk until resolved.

Policy Compliance

1. As confirmed during the threat modeling sessions, WMF-Legal has approved the general functionality, data collection and data visibility for the overall system which incorporates this MediaWiki extension.

DoS and Performance Vectors

1. A separate performance review has been created at T302858: Performance review of CampaignEvents extension which should address these and related concerns.

Hi @sbassett, thank you so much for this comprehensive review!

I am noting the acceptance for shipping the extension with medium risk level here with the following justifications:

3. Per the related threat modeling exercise, the issue of allowing users more privacy while using the Campaign Registration system is being tracked at T316405: [EPIC] Allow participant option of joining event privately. This is classified as a medium risk until resolved.

5. Per the related threat modeling exercise, the issue of potentially retaining somewhat-sensitive date related to events is tracked at both T308582: Implement ability for organizer to delete registration [completed] and the related T316405: [EPIC] Allow participant option of joining event privately. This is classified as a high risk until resolved.

Our deployment strategy is first to only enable this extension on officewiki, testwiki, and test2wiki (T318256), which will not have the same concerns for private registration and sensitive data retention as it does when we later enable it on meta wiki. We are actively working on private registration (T316405), and we will not enable the extension on meta wiki for public usage until that work is complete. Therefore, we can go ahead with shipping the extension to officewiki, testwiki, and test2wiki without the private registration feature at the moment.

4. Per the related threat modeling exercise, the issue of better vetting/enforcement for the selection of event organizers is tracked at T316227: Specify initial list of organizers via configuration. This is classified as a medium risk until resolved.

Similar to the previous items, we will allow anyone on officewiki, testwiki, and test2wiki to become organizers, and only apply an "allow-list" for organizers when we ship to meta wiki. Per this comment, we will add the finalised list of vetted organizers to the config for meta wiki when we are ready to deploy to there.

8. Per the related threat modeling exercise, the issue of securely accessing a third party API (Pelias) is tracked via the additional security review task {T309410}, the public "RFC" task T312677: [Request for Comment] Campaigns Geolocation API proposal and related tasks like T309325: Cross-team review of Geolocation API proposal. This is classified as a high risk until resolved.

These should be resolved already as part of the contract approval process. We are working on properly closing these tasks on Phabricator to not cause confusion.

Finally, I have also created a separate task (T319228) to update the vulnerable packages you identified, even though they are of "low" risk level.

Change 838140 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Minor fixes following security review

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

In T290248#8276687, @sbassett wrote:

1. semgrep 0.115.0 was run with several popular and custom rule-sets - we are happy to review these rule-sets in detail with the requesters. The raw results are found within P35239, most of which should be false positives and none of which appeared to be legitimate vulnerabilities at this time. It would likely be worthwhile for the developers of the extension to give these results a cursory glance and review various data sinks (especially within a file like resources/ext.campaignEvents.specialeventdetails/ParticipantsManager.js) to ensure their expectations are correct.

Thank you, I don't see any obvious vulnerability in there.

General Security Issues

1. I assume that since I don't see any messages listed under a RawHtmlMessages key within extension.json, that no messages relevant to this extension utilize raw html. This would really only have implications for the Html::rawElement calls within src/EventPage/EventPageDecorator.php and various message-rendering snippets within certain javascript front-end files (e.g. resources/ext.campaignEvents.specialeventdetails/ParticipantsManager.js) which either passed phan-taint-check and/or didn't appear problematic from a cursory manual review. low risk

There was a minor escaping issue with a system message, for which I sent a patch. Somewhat relatedly, I'm not sure if taint-check can understand the new ITextFormatter syntax, I hope to be able to patch it someday.

Hello @vyuen et al -

Apologies for the delayed response.

In T290248#8280673, @vyuen wrote:

I am noting the acceptance for shipping the extension with medium risk level here with the following justifications:

Our deployment strategy is first to only enable this extension on officewiki, testwiki, and test2wiki (T318256), which will not have the same concerns for private registration and sensitive data retention as it does when we later enable it on meta wiki. We are actively working on private registration (T316405), and we will not enable the extension on meta wiki for public usage until that work is complete. Therefore, we can go ahead with shipping the extension to officewiki, testwiki, and test2wiki without the private registration feature at the moment.

Similar to the previous items, we will allow anyone on officewiki, testwiki, and test2wiki to become organizers, and only apply an "allow-list" for organizers when we ship to meta wiki. Per this comment, we will add the finalised list of vetted organizers to the config for meta wiki when we are ready to deploy to there.

Sure, though there is still a medium risk for these wikis; officewiki being a private wiki with real users and the test/test2 wikipedias making use of real-world data (including real user data) even if their primary purpose is for production-testing various features and code. Per the current risk management framework, medium risks can be accepted at the WMF managerial level, which would likely imply you or @ldelench_wmf. Once these remaining issues have been dealt with, the entry within our risk register will be closed and marked as completed.

These should be resolved already as part of the contract approval process. We are working on properly closing these tasks on Phabricator to not cause confusion.

Excellent. If you or someone else could follow up with the Security-Team once this has been completed, that would be great.

Finally, I have also created a separate task (T319228) to update the vulnerable packages you identified, even though they are of "low" risk level.

Sounds great, thanks.

In T290248#8282904, @Daimona wrote:

Thank you, I don't see any obvious vulnerability in there.

Ok, great.

There was a minor escaping issue with a system message, for which I sent a patch. Somewhat relatedly, I'm not sure if taint-check can understand the new ITextFormatter syntax, I hope to be able to patch it someday.

Ok, better to be safe than sorry, I suppose. And it should be easy to see if this results in anything unintended like a double-escape, I'd guess. Should we file a separate bug re: phan-taint-check-plugin and ITextFormatter?

In T290248#8300566, @sbassett wrote:

There was a minor escaping issue with a system message, for which I sent a patch. Somewhat relatedly, I'm not sure if taint-check can understand the new ITextFormatter syntax, I hope to be able to patch it someday.

Ok, better to be safe than sorry, I suppose. And it should be easy to see if this results in anything unintended like a double-escape, I'd guess. Should we file a separate bug re: phan-taint-check-plugin and ITextFormatter?

Sure: T320268: Support ITextFormatter and friends in taint-check. Unfortunately, that's really hard at the moment due to how the output format is handled in ITextFormatter.

Change 838140 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Minor escaping fix following security review

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

Change 841552 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Fix double-escaping in "more details" dialog

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

Change 841552 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Fix double-escaping in "more details" dialog

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

In T290248#8381307, @ldelench_wmf wrote:

Thank you, Security team! Please let us know if there are followup tasks we should capture separately (I don't see anything immediate for V1).

Sure, I think we should be able to catch up on any outstanding items or concerns at our sync tomorrow (11/10/2022).