See https://www.mediawiki.org/wiki/Security/SOP/Security_Readiness_Reviews and the form at https://phabricator.wikimedia.org/maniphest/task/edit/form/79/ for requested information

@sbassett what's missing from this task? I copied over the template to the description here but I did it manually so I might be missing something crucial.
This is ready for review now.

In T269291#6673735, @Jdlrobson wrote:

@sbassett what's missing from this task? I copied over the template to the description here but I did it manually so I might be missing something crucial.
This is ready for review now.

Ok, I see it now, thanks. We'll try to get this scheduled for next quarter.

Hi @Jdlrobson - we're revamping our scheduling process a bit to provide more clarity and transparency around our workload management and prioritization process. You'll see that we've placed you into queue for this quarter, and will do our best to meet your target deployment date: https://phabricator.wikimedia.org/tag/secscrum/ and @sbassett or @Reedy will be in touch with any questions or concerns. Thanks for your patience as we work through this, and please feel free to reach out at any time!

Just wanted to check in on this one given my target deployment date has passed. No urgency from my side, but I'd like to have a clearer idea on when I can expect to schedule this work.

In T269291#6887593, @Jdlrobson wrote:

Just wanted to check in on this one given my target deployment date has passed. No urgency from my side, but I'd like to have a clearer idea on when I can expect to schedule this work.

Hey @Jdlrobson - apologies for the delay. We have our appsec scrum this Wednesday and should be able to provide more details at that time. I will note that part of the delay for this review is that we may end up outsourcing it to one of our security vendors for completion. Again, we should have more details about this after our Wednesday scrum. Thank you for your patience.

Thanks for the update!

Apologies for the lack of updates @Jdlrobson - we do have a vendor lined up to complete this and will be in touch as they move forward. Thank you for your patience.

Thanks for the update! FYI I am off for a week on vacation starting tomorrow.

Hello @sbassett I see a vendor is confirmed. When should I plan for feedback?

@Jdlrobson - yes, this review is part of our latest contract with ROS. We are hoping to have report deliverables for this and other reviews by the end the current quarter (Q4 2021) or shortly after.

In T269291#7086049, @sbassett wrote:

@Jdlrobson - yes, this review is part of our latest contract with ROS. We are hoping to have report deliverables for this and other reviews by the end the current quarter (Q4 2021) or shortly after.

Is this still working going to plan?

@Jdlrobson - We're hoping to have the report deliverable for this from the vendor within the next week or so.

Exciting! Thanks for the update!

Update: we plan to review the report deliverables for this and other vendor-performed reviews towards the end of next week (2021-07-29).

@sbassett sorry to be some bothersome, but this task relates to an OKR of mine and I'd like to plan some changes relating to this change. Is there a date I can work towards when the security review will be complete? Thanks in advance.

@Jdlrobson - we have a Security-Team session this afternoon to review the security issues for this and a few other security reviews this afternoon. Just glancing at the vendor's report, it appears there were two issues for ext:NearbyPages that resulted from their pentesting. So we'll likely make this task private and report both of those here.

Security Review Summary - T269291 - 2021-08-04

Overall, the current vendor code under consideration...
with an overall risk rating of: medium.

Extension:NearbyPages

General Security Issues

1. Misuse of application component - moderate threat level The API key is valid for the geolocation API and can be misused This key could be used to send out requests and cause higher usage. The key should not be committed with the code
2. Possible DOM data manipulation - input validation vulnerability with a low threat level The recommendation is to use a whitelist of permitted values

Misuse of application component - moderate threat level The API key is valid for the geolocation API and can be misused This key could be used to send out requests and cause higher usage. The key should not be committed with the code

Could I ask which API key is being referred to here? There is no API key being used inside the extension that I am aware of, so I'm not sure what this is referring to and is possibly a misunderstanding of what the code is doing.

Possible DOM data manipulation - input validation vulnerability with a low threat level The recommendation is to use a whitelist of permitted values

Would it be possible to point to the line of code that's a concern here? The only user input that is possible in this app that I am aware of is from the address bar and that code is already in core and I don't know how it could be used to perform DOM data manipulation.

@Jdlrobson -

A bit of follow-up on these:

1. The API key issue looks like a false positive. We'll need to go back to the vendor and ask where they found this key within ext:NearbyPages. Some sleuthing on my part indicates it might be a widely-distributed test key used by various pentesting tools.
2. For the DOM manipulation issue, here is the full report text from the vendor:

Description

The application may be vulnerable to DOM-based data manipulation, as it trusts data from controllable parts of the DOM (such as the URL) and processes this data in an unsafe way.

Technical description

ext.nearby.scripts.2d0caa8d.js contains the following code:

if ( path === '' ) {
      path = window.location.href.replace( /#.*$/, '' );
      history.pushState( null, document.title, path );
      this.checkRoute();
}

The data is read from window.location.href and passed to history.pushState().

DOM data manipulation might happen when a script takes user-controllable data to a field within the DOM that is used
within the visible UI or client-side application logic.

Another instance of this issue can be found in jquery.7a6e0748.js, with the following code:

if ( path === '' ) {
      path = window.location.href.replace( /#.*$/, '' );
      history.pushState( null, document.title, path );
      this.checkRoute();
}
if ( !context ) {
    if ( support.createHTMLDocument ) {
        context = document.implementation.createHTMLDocument( "" );
        base = context.createElement( "base" );
  Findings 17
base.href = document.location.href;
        context.head.appendChild( base );
    } else {
        context = document;
    }
}

Here, data is read from document.location.href and passed to the href property of a DOM element.

Impact

An attacker may be able to use this vulnerability to construct a URL that, if visited by another application user, would modify the value in the DOM, which might affect the UI of the application.

Recommendation

• Do not dynamically write to DOM data fields any data that originates from any untrusted source.
• Use a whitelist of permitted values.

@sbassett I'm sorry, I'm still very confused with the recommendations here.

path = window.location.href.replace( /#.*$/, '' );
history.pushState( null, document.title, path );

The history API is a browser API, that sets the URL based on the current URL. The code basically says take the current URI and drop anything after the hash fragment and update the browser address bar so that the hash fragment is not visible there. It's my understanding that there can only ever be one "#" in the browser address bar so I'm not sure how an attacker could use this to their advantage, so I'm struggling to see where the vulnerability is here. It's more or less the same as:

 path = window.location.href.replace( /#.*$/, '' );
window.location = path;

URLs are also escaped by design as far as I understand?

Another instance of this issue can be found in jquery.7a6e0748.js, with the following code:

An issue with jQuery would impact all our products, not just Nearby so I'm also not sure what to do with this feedback.

@Jdlrobson - some follow-up:

1. For the API key issue, @Mstyles and I have now both installed all of the dependencies for recent versions of master NearbyPages and searched for the referenced API key. No luck. So I think we can officially label that a false positive. Apologies for the confusion there.
2. For the DOM manipulation issues, I don't believe those are specifically related to an actual XSS vulnerability, but are rather code weaknesses in that user-controlled data can be injected into the DOM or a user's history, particularly via the contents of the query string. The mitigation would be guarding against unwanted URLs and unwanted data within the query string via allow-lists. If such allow-lists are not feasible because the code needs to accept any valid URL, then there isn't much that can be done in these cases. This issue was also rated as low risk, so I don't personally find it particularly concerning, especially since no actual vulnerability was found. The bottom line is that if the relevant code can be conveniently hardened with relevant allow-lists, then we should do so.

@sbassett now I'm even more confused as I can't find the line of code being pointed to.

The following file they reference doesn't exist so I'm not sure where that's coming from

ext.nearby.scripts.2d0caa8d.js

The code seems to exist in core already as part of https://gerrit.wikimedia.org/g/mediawiki/core/+/dc321179c9f20334cfae86bbf9a6c6cd15b3a85b/resources/lib/oojs-router/oojs-router.js#170 which is already been used in production. I'd suggest filing a security bug against MediaWiki-Core-Router if that's something we want to look at.

With this in mind, can I assume it's okay to deploy this extension to production environments?

In T269291#7282548, @Jdlrobson wrote:

The code seems to exist in core already as part of https://gerrit.wikimedia.org/g/mediawiki/core/+/dc321179c9f20334cfae86bbf9a6c6cd15b3a85b/resources/lib/oojs-router/oojs-router.js#170 which is already been used in production. I'd suggest filing a security bug against MediaWiki-Core-Router if that's something we want to look at.

Ah, ok, I'm going to assume there was some confusion here with our vendor and that they may have reviewed NearbyPages within the context of it being installed with MediaWiki as opposed to just the relevant NearbyPages code. Apologies on that.

With this in mind, can I assume it's okay to deploy this extension to production environments?

Yes. The items surfaced by the vendor were all fairly low-risk items anyway, in my opinion.

Thanks @sbassett !