@Reedy is this the right form for asking for a security review for the extension? Or is this one better?

In T260822#6397379, @Niharika wrote:

@Reedy is this the right form for asking for a security review for the extension? Or is this one better?

One isn't necessarily "better" than the other, it's for slightly different purposes. I think this one came from the template on https://phabricator.wikimedia.org/maniphest/task/edit/form/72/ which is for more overall/general requests; especially if you don't quite know what service/whatever you actually need, and want help being pointed in the right direction

You're presumably going to want the one from form 79, which is the readiness review (and can be a subtask of this). The question being if you require any other help/services? :)

In T260822#6399984, @Reedy wrote:

In T260822#6397379, @Niharika wrote:

@Reedy is this the right form for asking for a security review for the extension? Or is this one better?

One isn't necessarily "better" than the other, it's for slightly different purposes. I think this one came from the template on https://phabricator.wikimedia.org/maniphest/task/edit/form/72/ which is for more overall/general requests; especially if you don't quite know what service/whatever you actually need, and want help being pointed in the right direction

You're presumably going to want the one from form 79, which is the readiness review (and can be a subtask of this). The question being if you require any other help/services? :)

Got it. Thanks! I think we are looking for a general direction on this project. I'll chat with Thalia about it some more.

@Tchanders This looks good to me. I added some of the Privacy risks to T260821. Might be worth explaining here, but as you say, it might end up being a licensing restriction anyways.

We expect the feature to be deployed to all wikis, and be available on certain pages that show IP addresses, e.g. Special:RecentChanges. (@Niharika do we know which pages?)

We don't know for sure yet. We will start with Special:Investigate and gradually expand to other pages (such as Special:RecentChanges)based on user research and testing.

We expect the feature to be available only to CheckUsers initially, and later to other trusted user groups. (@Niharika Do we know which user groups?)

This will also depend on user research and testing. There is also a possibility of there being a new user group for users who will have access to this information.

[For the records, I have changed the View Policy of this task from "All Users" to "Public" (the former makes no sense) and also fixed https://phabricator.wikimedia.org/transactions/editengine/maniphest.task/view/72/ which had set this strange view policy default.]

Thanks @Niharika and @Aklapper.

In T260822#6448184, @sbassett wrote:

Stalling to our backlog for now until there's some code/commit sha for the review.

Just wanted to note that this is ready for review - thanks.

Thank you @sbassett. We'd like to deploy on beta by the end of the year, but we also understand you're very busy at the moment.

@Niharika mentioned it might be possible to deploy before the reviews are complete if we're able to get manager buy-in (presumably from @aezell)?

In T260822#6631288, @Tchanders wrote:

@Niharika mentioned it might be possible to deploy before the reviews are complete if we're able to get manager buy-in (presumably from @aezell)?

Yes, this portion of the extension deployment documentation was recently refactored by me: https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment#Preparing_for_deployment. Performance and Security reviews are no longer considered hard blockers to deploy to beta (in fact, performance reviews require beta deployment). The way that the Security-Team has been implementing this so far is by assigning a pre-review risk of medium for any code which needs a more hastened beta deployment. This requires manager-level ownership of said risk. And we would still block deployment to production pending a satisfactorily-completed security review unless there was c-level approval and ownership of all associated risk.

This is really helpful - thanks @sbassett

Thanks @sbassett. I approve the expedited release and take ownership of any risk involved.

@Tchanders - I hope to have this review completed by early next week.

Update: I've performed several automated security checks and scans against the code base and have not found anything concerning. I'd like to spend a little more time today manually exploring and pen-testing the extension, installed within a mediawiki instance, and then this review should be complete.

Security Review Summary - T260822 - 2020-12-18
Last commit reviewed: 70676b782489f26bea2a49c06f3c5b4cc932bd9c

Summary

Overall, this extension looks pretty good and I'd rate its overall risk as low, barring any further major changes or refactors. A few notes:

1. I tested all of the OOUI sinks I could find and both the popup and infoBox widgets looked good. And the data in question should be fairly trustworthy to begin with (mw messages, maxmind db values).
2. I didn't really investigate any potential Vuln-DoS issues as those classifications of vulnerabilities appear to have been dealt with (in different ways) during the performance review (T260821).
3. Given some of the issues discussed within T263263 (and my own adventure building out a local development environment to test the extension), it might be worth attempting to incorporate some of the test databases and source data into unit/integration tests for both better code coverage and (hopefully) easier local development and testing.

Thanks @sbassett

it might be worth attempting to incorporate some of the test databases and source data into unit/integration tests for both better code coverage and (hopefully) easier local development and testing.

Filed as T269775

@sbassett Since this security review was done, we've experienced some delays in deployment due to additional product requirements (copying in @Niharika) and the team being unexpectedly asked to work on SecurePoll.

We would hope to deploy IPInfo to testwiki in a month and to the rest of our production sites as a BetaFeature by the end of the quarter. However, more features have been added since T260822#6702741, so we're not sure whether this plan would be blocked on another security review. (And we're aware the security team is very busy at the moment.) What would you advise?

Also copying in @JayCano, who is our (not-so) new engineering manager.

In T260822#7667533, @Tchanders wrote:

@sbassett Since this security review was done, we've experienced some delays in deployment due to additional product requirements (copying in @Niharika) and the team being unexpectedly asked to work on SecurePoll.

We would hope to deploy IPInfo to testwiki in a month and to the rest of our production sites as a BetaFeature by the end of the quarter. However, more features have been added since T260822#6702741, so we're not sure whether this plan would be blocked on another security review. (And we're aware the security team is very busy at the moment.) What would you advise?

Hey @Tchanders - could you provide a diff of the relevant changes since 70676b782489f26bea2a49c06f3c5b4cc932bd9c somewhere? I'd like to get some idea of how much code we're talking about here. I would doubt that, in all but the most extreme cases, it would be enough to block on a new security review. Thanks.

Ping @Tchanders - any feedback on my above suggestion? Thanks.

Apologies @sbassett - this has been on my todo list. I've enumerated the main changes below, but it may be that this review doesn't need re-doing? As with other new projects, we're continuing to develop features according to requirements set out by our product manager @Niharika, and in response to testing and feedback as we deploy. I suppose we're uncertain of when the review process is considered over, given all that. Thanks for your help!

The full diff since the last review is here, but that includes a lot of refactoring and filename changes. Also small changes such as showing more MaxMind data, including the popup in more places, etc.

The main new features are:

• Added src/HookHandler/BetaFeaturePreferencesHandler.php so we can deploy as a beta feature, using the BetaFeatures extension
• Added src/InfoRetriever/BlockInfoRetriever.php and src/InfoRetriever/ContributionInfoRetriever.php, which retrieve on-wiki information (e.g. number of blocks, contributions, etc)
• Added src/Logging/Logger.php so that logs of data access can be browsed on Special:Log (feature still under development)
• src/HookHandler/SchemaHandler.php adds a new table, discussed with DBAs in T297696. The feature is still under development, and isn't a blocker to testwiki deployment

Thanks, @Tchanders. I'll plan to have a look at the diff, with a specific focus on the new features you mention above. At a glance, I'm not seeing anything that would warrant a reclassification of the risk, but I'll confirm that for sure. I can likely get this done by the time you plan to launch in a month or so.

In T260822#7704308, @sbassett wrote:

Thanks, @Tchanders. I'll plan to have a look at the diff, with a specific focus on the new features you mention above. At a glance, I'm not seeing anything that would warrant a reclassification of the risk, but I'll confirm that for sure. I can likely get this done by the time you plan to launch in a month or so.

Thank you @sbassett

@sbassett do you have an update for us? Hoping to deploy by end of this week on testwiki.

In T260822#7745015, @Niharika wrote:

@sbassett do you have an update for us? Hoping to deploy by end of this week on testwiki.

No, not yet. I'm at a team offsite this week, but I'll try to get this done this week. It shouldn't block deployment, regardless, as after glancing at the changes, none of them appeared to be high-risk.

@Niharika @Tchanders - I should have my re-review posted later today. After a cursory glance at the github diff, I'm not yet seeing anything majorly concerning that would adjust the previous risk rating of low.

@Niharika @Tchanders -

After a re-review of the new additions to ext:IPInfo, I'm going to keep the overall risk rating as low, unchanged from the initial review. Here are some notes on what I did for the re-review along with some additional, minor issues which were found:

1. Performed a quick eyeballing of the provided github diff.
2. Spot-checked a handful of recent commits for ext:IPInfo, with particular focus on the output of mwext-php72-phan-docker and the SecCheckPlugin's output. Also ran the plugin locally against IPInfo@master.
3. Ran semgrep static analysis policies and rule sets against IPInfo@master, including specific JavaScript and PHP policies.
4. Ran a number of SCA tools against IPInfo@master for various JavaScript and PHP packages.
5. Ran some custom, security-related JavaScript and PHP greps against IPInfo@master.

Here were a few minor findings:

Vulnerable Packages - Development

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