hey @sbassett, could you, please, give us an update on this task:

• when are you planning to tackle it?
• could you tell us something we could tackle proactively that may help?

We are currently implementing the last stories and that information would help us greatly to shape our plans.

Thanks a lot in advance!

@darthmon_wmde - we don't currently have this review assigned/scheduled, though I could probably have a look at it next week. Before we do that, I think we'd need:

1. Confirmed commit shas for the various code bases and files defined within sections one and two within the task description. Basically points to freeze the code so we're not reviewing a moving target. If the code is still volatile, we'd probably want to wait until it becomes a bit more stable before selecting the commit shas.
2. Confirmation as to what the three unlinked config changes above imply: Upcoming: Set dataBridgeEnabled repo setting on Wikidata, Upcoming: Set dataBridgeEnabled client setting on certain client wikis and Upcoming: Set dataBridgeEnabled client setting on all Wikibase clients. If these are merely config variables within Wikibase.php or IS.php, then we likely wouldn't care about them for this review, unless they have security implications, like perhaps wmgWikibaseClientDataBridgeHrefRegExp.
3. For the working test environment, it would be nice to have instructions on how to get a local development environment of this system/config up and running via the Wikibase docker or similar. Beta/test wiki setups can be helpful but local development environments that mimic what is intended to eventually exist within production are the most helpful for security reviews as we can then often perform more in-depth pen-tests and analyses.

Hi Scott!

I am back from asking the team of engineers for input:

1. Confirmed commit shas for the various code bases and files defined within sections one and two within the task description. Basically points to freeze the code so we're not reviewing a moving target. If the code is still volatile, we'd probably want to wait until it becomes a bit more stable before selecting the commit shas.

We have not frozen the code yet, are finishing the last 2.5 stories. Excuse my ignorance but, do we need to be 100% finished before the security review can happen?

1. Confirmation as to what the three unlinked config changes above imply: Upcoming: Set dataBridgeEnabled repo setting on Wikidata, Upcoming: Set dataBridgeEnabled client setting on certain client wikis and Upcoming: Set dataBridgeEnabled client setting on all Wikibase clients. If these are merely config variables within Wikibase.php or IS.php, then we likely wouldn't care about them for this review, unless they have security implications, like perhaps wmgWikibaseClientDataBridgeHrefRegExp.

Those are indeed config changes, already uploaded:

• https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/595542/
• https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/595543/

Other than that, please find here the documentation for the RegExp: https://www.mediawiki.org/wiki/Wikidata_Bridge/Development/DocDrafts/How_to_Enable_Wikidata_Bridge_for_your_Infobox

1. For the working test environment, it would be nice to have instructions on how to get a local development environment of this system/config up and running via the Wikibase docker or similar. Beta/test wiki setups can be helpful but local development environments that mimic what is intended to eventually exist within production are the most helpful for security reviews as we can then often perform more in-depth pen-tests and analyses.

I sent you a google doc today with the documentation on how to reproduce the DataBridge locally. Please, do no hesitate to ask if needed.

Hey @darthmon_wmde -

We have not frozen the code yet, are finishing the last 2.5 stories. Excuse my ignorance but, do we need to be 100% finished before the security review can happen?

Although it has not been officially documented within our Security Readiness Review SOP, we typically ask code owners to select a "stopping point" for a review. This is ideally (a) commit sha(s) where the code is as close to production-ready as it can be so that we do not have to review a volatile codebase which may change substantially within a few days. Our team simply does not have the resources and sanity to conduct numerous reviews of volatile code. We understand this may not be possible in certain situations and that small changes (i18n, doc, etc.) can occur with little overall impact, but we strive to have the code in a stable, finished state prior to our reviews. So if there is more work to be completed (which seems to be the case with your mention of outstanding stories), we should likely wait until that work is completed before agreeing upon commit sha(s) for the review.

Other than that, please find here the documentation for the RegExp: https://www.mediawiki.org/wiki/Wikidata_Bridge/Development/DocDrafts/How_to_Enable_Wikidata_Bridge_for_your_Infobox

Thanks.

I sent you a google doc today with the documentation on how to reproduce the DataBridge locally. Please, do no hesitate to ask if needed.

Thanks, I'll review that and let you know.

Thanks a lot, @sbassett for your quick reply!

Another thing that I failed to mention is that the performance review (T246456) is also being currently done.

It might make sense then to wait a bit and I will ping you when the code is more stable =)

Hey @sbassett !

as a heads up: the bridge team has finished the implementation and the performance review (T246456) has been passed.

Could you please make an estimation when this Security Review can be done so that we can coordinate accordingly?

thanks a lot in advance!!

@darthmon_wmde - I can look at this next. Did you have an updated target date for deployment?

In T249039#6203730, @sbassett wrote:

@darthmon_wmde - I can look at this next.

that'd be sweet! thanks!

In T249039#6203730, @sbassett wrote:

Did you have an updated target date for deployment?

as soon as possible would be good - we have already the ok from the catalan wikipedia. @Lydia_Pintscher has more concrete information on this matter if needed.

If at all possible it'd be <3 to be ready for deployment at the beginning of July.

In T249039#6224698, @Lydia_Pintscher wrote:

If at all possible it'd be <3 to be ready for deployment at the beginning of July.

We can at least have a minimal, due-diligence review performed by then. Which will likely be the sole deliverable here given Security-Team resourcing and the continued Covid19-related disruptions.

@Lydia_Pintscher @darthmon_wmde - I hope to have the aforementioned due-diligence security review completed by the end of next week (Friday, July 3rd).

In T249039#6260739, @sbassett wrote:

@Lydia_Pintscher @darthmon_wmde - I hope to have the aforementioned due-diligence security review completed by the end of next week (Friday, July 3rd).

thanks a lot for the update :)

So https://gerrit.wikimedia.org/g/mediawiki/extensions/Wikibase/+/master/client/resources/Resources.php no longer appears to exist, as it is ref'd in the task description. Does that live somewhere else or is it just gone now?

In T249039#6273038, @sbassett wrote:

So https://gerrit.wikimedia.org/g/mediawiki/extensions/Wikibase/+/master/client/resources/Resources.php no longer appears to exist, as it is ref'd in the task description. Does that live somewhere else or is it just gone now?

This was talking about e.g. https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Wikibase/+/908df45/client/resources/Resources.php#28 ff.

which now is configured in
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Wikibase/+/077e63c/extension-client-wip.json#252

Heavy refactoring (Wikibase Extension Decoupling and Registration, not bridge-related) going on, sorry.

Though the Data Bridge-related modules actually moved to DataBridgeResourceLoaderModules.php, since they can’t be configured statically in the JSON file yet.

Update: I still hope to have this security review completed by EOBD tomorrow (10:00 PM UTC for me) but note that the review may have to be posted on Monday 2020-07-06 due to some delays. Apologies and thanks for your patience.

Update: Apologies, but this is going to have to wait until Monday 2020-07-06.

Security Review Summary - T249039 - 2020-07-06
Last commit reviewed:

1. Wikibase: cbfd8bbca3bf816ace5bafdfbd112ddaa44274da

Summary

For this review, I focused mainly upon the TypeScript app within Wikibase's client/data-bridge directory, with a cursory glance at the config files within client/includes/DataBridge/ and the generic wikidata item regex component of the IS.php config changes (((Q[1-9][0-9]*)).*#(P[1-9][0-9]*)), which all seem fine. I didn't find anything significantly disturbing with the TypeScript app other than it being a substantial amount of complex code with myriad dependencies. Overall, I would currently assign a risk rating of medium given the dependency issues and potential vue v-html issues below.

Vulnerable Packages

1. No production vulnerabilities found with npm audit --production, though a significant number (4,343!) were found within dev dependencies. Please run an npm audit to confirm and address as needed. Risk: medium
2. No production vulnerabilities found with snyk test, though a significant number (26 issues, 4,391 vulnerable paths) were found within dev dependencies. See attached file (P11943) output of snyk report. Risk: medium

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

As reported by retirejs:
(Risk: medium)

/src/node_modules/tinycolor2/demo/jquery-1.9.1.js

1. jquery 1.9.1
   1. https://nvd.nist.gov/vuln/detail/CVE-2015-9251
   2. https://nvd.nist.gov/vuln/detail/CVE-2015-9251
   3. https://nvd.nist.gov/vuln/detail/CVE-2019-11358
   4. https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/

General Security Issues

1. njsscan did find some potential issues with vue's v-html attribute. I'd guess most of these are false positives given that they render messages which shouldn't be vulnerable as they are used within the TypeScript app and likely secure content-rendering functions (title, getBodyMessage, referenceHTML et al). Still posting the results here (P11942) for review and confirmation with a probable Risk: low.

hi @sbassett !

thanks a lot for that assessment!

sorry if this is a stupid question but could you please say clearly whether we need to lower the risk on any of the points? I am not sure whether what you define as medium or low risk are acceptable to go to production or not.

Thanks a lot in advance!

In T249039#6307879, @darthmon_wmde wrote:

sorry if this is a stupid question but could you please say clearly whether we need to lower the risk on any of the points? I am not sure whether what you define as medium or low risk are acceptable to go to production or not.

Hey @darthmon_wmde -

Apologies if our current risk management policy (on officewiki, which sadly I don't believe wmde folks can view) hasn't been as well-socialized as I would like, but whenever the Security-Team performs any kind of security or risk review, including application security reviews, we assign an overall risk which then needs to be mitigated or accepted. We obviously prefer mitigation, as it actually reduces risk for a given code base or system, but we also allow for individuals to fully accept and own any risk established during a review. Here is a simple table from the aforementioned risk management policy detailing levels of risks and the required steps for approval:

thanks a lot @sbassett for your complete answer!

(...) our current risk management policy (on officewiki, which sadly I don't believe wmde folks can view) ...

You are probably right. Are the credentials for this page shared with another system within the wikimedia world? I have tried a couple without success.

In T249039#6313032, @darthmon_wmde wrote:

(...) our current risk management policy (on officewiki, which sadly I don't believe wmde folks can view) ...

You are probably right. Are the credentials for this page shared with another system within the wikimedia world? I have tried a couple without success.

Sadly, I do not believe so. officewiki accounts are local (not SUL or shared in any way) and are granted upon being employed by the WMF. There are definitely various policies that live there which should probably have some public version on mw.org or wherever.

General Security Issues

1. njsscan did find some potential issues with vue's v-html attribute. I'd guess most of these are false positives given that they render messages which shouldn't be vulnerable as they are used within the TypeScript app and likely secure content-rendering functions (title, getBodyMessage, referenceHTML et al). Still posting the results here (P11942) for review and confirmation with a probable Risk: low.

I looked at these earlier and thought they all looked like false positives, but I seem to have lost access to the paste now for some reason, so I can’t say for sure. But looking through v-html codesearch results:

• All instances of v-html="$messages.get( … )" should be safe. $messages.get() is a wrapper around mw.message().parse(), where mediawiki.jqueryMsg (which our app module depends on) will escape HTML. (See also T115888.)

• <span v-html="title" /> in AppHeader.vue should be safe. title is a getter wrapping $messages.get(), see above.

• v-html="messageHeader" and v-html="messageBody" in ErrorPermissionInfo.vue should be safe. messageHeader and messageBody are properties of the component; ErrorPermission.vue fills them with its getMessageHeader() and getMessageBody() methods, respectively, which both wrap $messages.get(), see above.

• v-html="getBodyMessage" in License.vue should be safe. getBodyMessage is a getter wrapping, you guessed it, $messages.get().

• v-html="referenceHTML" in ReferenceSection.vue should be safe. referenceHTML comes from the renderedTargetReferences array in the app state, which is populated in the renderReferences action using the ApiRenderReferencesRepository, which gets the HTML from the wbformatreference API, where the HTML ultimately comes from a Parser based on wikitext input.

• v-html="message" in ReportIssue.vue should be safe. message is a getter wrapping $.messages.get().

Vulnerable Packages
Risk: medium
[...]
Outdated Packages

There recurringly are and recently were efforts to get those numbers down, maybe a recheck (e.g. after sha 5f1d7d106f47dbe7738efb788144d7f2fe391f39) is all it takes to find more acceptable counts (is 0 the success criterion?).
This is a moving target, however. At WMDE we are in the process of finding a structured workflow (for the products' and the developers' sake) which prevents those counts climbing again. A push on T228527: Support nested package.json files from people with an official security hat would be of great help to make this happen in (ever more popular) monorepos.

As reported by retirejs:
(Risk: medium)

/src/node_modules/tinycolor2/demo/jquery-1.9.1.js

I believe this is a false positive. TinyColor (which we depend on via @storybook/addon-knobs@5.3.19 > react-color@2.18.1 > tinycolor 1.4.1) does contain a copy of jquery 1.9.1 for its own demo page, but it is not part of its package, and consequently not loaded in the bridge product.

Thanks for making sure we deliver quality work to our users!

In T249039#6322813, @Lucas_Werkmeister_WMDE wrote:

I looked at these earlier and thought they all looked like false positives

Great, thanks for confirming and for your detailed analysis, with which I concur. I'll change this to a risk rating of: none.

but I seem to have lost access to the paste now for some reason, so I can’t say for sure.

This was due to some mitigations for a non-issue (T258239), the referenced pastes should now be viewable to you.

In T249039#6323388, @Pablo-WMDE wrote:

There recurringly are and recently were efforts to get those numbers down, maybe a recheck (e.g. after sha 5f1d7d106f47dbe7738efb788144d7f2fe391f39) is all it takes to find more acceptable counts (is 0 the success criterion?).
This is a moving target, however. At WMDE we are in the process of finding a structured workflow (for the products' and the developers' sake) which prevents those counts climbing again. A push on T228527: Support nested package.json files from people with an official security hat would be of great help to make this happen in (ever more popular) monorepos.

0 is of course ideal, though likely not realistic. As noted within the review, outdated packages by themselves, without any additional mention of specific security vulnerabilities, would have a risk of: low. Per the risk acceptance chart within T249039#6309061, these issues can be addressed outside of any timeline and the risk is automatically accepted without managerial+ approval. I'm also hopeful that we'll have better automated security monitoring in place both as stand-alone solutions and within CI in the near future. Though that work is likely not to be completed for a while and so we try to call out such issues during manual security readiness reviews when prudent.

I believe this is a false positive. TinyColor (which we depend on via @storybook/addon-knobs@5.3.19 > react-color@2.18.1 > tinycolor 1.4.1) does contain a copy of jquery 1.9.1 for its own demo page, but it is not part of its package, and consequently not loaded in the bridge product.

Ok, I'd barely call that a dev dependency then, so the risk would be: low.

Given the volume of issues returned by npm audit and snyk test, and that while such packages might not be directly deployed to wikimedia production hardware, they are still likely used during critical doc, test and build stages and I would still rate the overall risk at medium. This risk can be accepted by a manager (I assume @darthmon_wmde) and a risk plan could be as simple as committing to review vulnerable dependencies for security updates within 30 days (for which there obviously may or may not be updates.)

Ping @darthmon_wmde et al - just wanted to check on where we're at here with mediations and/or risk acceptance per my previous comment. Thanks!

Hey @sbassett , thanks for checking in! I have talked with the PM of the project about the possibly soon activation of the Bridge for Wikicat, as planned, and I have set a discussion meeting next week with the developers to craft the plan. I will update you next week :)

Hey @sbassett ,

heads up: I am accepting the risk and we programmed the deploy to production.

We have already fixed some of the dev dependencies - by yesterday there were no high vulnerabilities, only low ones.

You mentioned that we need to commit to a risk plan to review the vulnerable dependencies e.g. in the next 30 days. From talking to the team the issue here is rather a continuous than a milestone, meaning that this is a moving target and we need a process to periodically check and fix the dependencies of our projects (To this aim we could really benefit from https://phabricator.wikimedia.org/T228527)

With all this in mind, could you please specify the kind of commitment that you expect from me?

thanks a lot in advance!

In T249039#6362819, @darthmon_wmde wrote:

heads up: I am accepting the risk and we programmed the deploy to production.

Great, thanks.

We have already fixed some of the dev dependencies - by yesterday there were no high vulnerabilities, only low ones.

Ok, great.

You mentioned that we need to commit to a risk plan to review the vulnerable dependencies e.g. in the next 30 days. From talking to the team the issue here is rather a continuous than a milestone, meaning that this is a moving target and we need a process to periodically check and fix the dependencies of our projects (To this aim we could really benefit from https://phabricator.wikimedia.org/T228527)

With all this in mind, could you please specify the kind of commitment that you expect from me?

The expectations the Security-Team would have are:

1. Accepting the risk resulting from this review would mean accepting accountability for any potential issue which might arise from this code being deployed upon Wikimedia hardware. e.g. being fully accountable if, say, a vulnerability from a deployed npm package resulted in a security incident.
2. Regarding the risk plan, what you've described seems reasonable. Given the vast amount of upstream code used for wikidata-bridge and other projects, it's likely infeasible to get to a point any time soon where every vulnerability has been addressed and resolved. Committing to constant vigilance of dependency vulnerabilities and working to remediate those via patches to upstream, upgrading to secure versions or using alternative packages are all acceptable solutions. To help with this, it might make sense to set up automated jobs (outside of publicly-viewable jenkins CI jobs) to run tools like npm audit, retirejs, outdated and snyk against the code base, which would then inform developers of current statuses.

In T249039#6400360, @sbassett wrote:

@darthmon_wmde - I assume there are no further questions about my above explanation? I'll plan to resolve this task for now. We can create new tasks for any additional, more focused follow-ups.

yes, thanks a lot @sbassett !