Hey @matmarex can you fill in the

Link to code repository / patchset:

and

Working test environment

Portion of this ticket

I removed references to the Android app, as there exists no patch or plans to implement this outside of mobile web.

https://serratus.github.io/quaggaJS/

Code is https://github.com/serratus/quaggaJS/blob/v0.12.1/lib/quagga.js

I note https://github.com/serratus/quaggaJS/releases/tag/v0.12.1 isn't very... comforting

This is just a patch release for those who are interested in testing it with the new Safari Technology Preview. So far it works on iOS (11 Beta) and macOS, with some limitations: The video is upside-down on iOS devices. Haven't figured out why. Have fun testing!

Last release 7 Jun 2017. 14 commits to master since this release

Also, 18 months since any commits on https://github.com/serratus/quaggaJS/commits/master

If it didn't work on iOS 11 beta... Does it work on 11 RTM? 12? 13? 14?

I just spotted https://github.com/serratus/quaggaJS/issues/444

Stopped working after upgrade to Safari 14.0 and iOS 14

If it's not working on the current default browser on the current stable release of iOS... This doesn't sound helpful or productive for anyone to be deployed in its current state :)

187 open issues in https://github.com/serratus/quaggaJS/issues. 11 PR open in https://github.com/serratus/quaggaJS/pulls

The developer doesn't seem to have been active very recently at all in this repo

With this cursory look at the library, it seems this will likely end up as a "High" risk rating. For more information on that, see T249039#6309061

In my last few rounds of testing product everything was working fine on iOS. I'll test iOS 14 and if it is broken we can disable in those browsers. However these are all product issues, not security issues.

Update: I just tested this on iOS 14 and it worked fine

11 PR open in https://github.com/serratus/quaggaJS/pulls

PRs have been merged as recently as April 2019. I don't a lack of very recent active development points to any specific security issues.

@Reedy, we appreciate you having an initial look at this work. A resulting question for you (I think): What – if any – further review is required before the Security team is able to assign this work a risk level?

Two notes:

• We're saw the preliminary rating you mentioned in T269007#6663726
• In T269007#6663916, @Esanders offered responses to some of the points you raised in T269007#6663726

@Jcross Thanks for taking a look at this! I saw you added a priority, which is a handy coincidence after I had a chat with @ppelberg about this task today. Given your activity here, I assume you're aware there is an open question remaining. Our esteemed colleagues on the Editing-team would value this review happening sometime in Q4. Given your team's shorthandedness ATM, does that seem like a reasonable expectation? :)

Hi @MBinder_WMF - I noted that we still had it marked "Needs Triage" and wanted to correct that error. Adjust priority as needed. There is discussion happening around the remaining question and it was raised again just yesterday. We'll pursue resolution for you as soon as possible and apologize for the delay.

Security Review Summary - T269007 - 2020-03-09
Last commit reviewed: 0.12.1/eff0c5a of quaggaJS

Summary

Overall, the current state of the code (of the quaggaJS library) is mostly unauditable, due to the lack of a package-lock.json and WebPack resulting in a non reproducible build. This gives an overall risk rating of: medium

In T269007#6663916, @Esanders wrote:

I don't a lack of very recent active development points to any specific security issues.

Sure, but it also doesn't mean there aren't any. It's certainly possible some were reported and not addressed. While this may not be the biggest issue on its own, it's hardly helpful in lowering the security risk.

Relatedly.. There's no package-lock.json file in the repo or the distribution (or similarly committed node_modules/similar for verifiability). This makes auditing this (noting we don't do code review of every single line of code, or ever different upstream dependancy) basically impossible. There is no way of reproducing the exact same quagga.min.js file that is in the repo/distribution. We have no idea of what version of what library has been actually included.

This is even more relevant when we look at the unminified code, and notice WebPack is also in use... https://github.com/serratus/quaggaJS/blob/e8352c7/dist/quagga.js.

I know we usually try and include the non minified source (needed for licensing in certain cases), but as is, this is kind of impossible with what is there currently.

Vulnerable Packages - Production

Vulnerable Packages - Development

Nearly 900 lines of output. So put in a paste for reference rather than making this comment super long!

If we clone the repo, change to the v0.12.1 tag, run npm i --package-lock-only to create a lock file (based on what is in package.json), and we start getting the usual "fun" with npm libraries...

$ npm i --package-lock-only
npm WARN deprecated babel-eslint@7.2.3: babel-eslint is now @babel/eslint-parser. This package will no longer receive updates.
npm WARN deprecated core-js@2.6.12: core-js@<3 is no longer maintained and not recommended for usage due to the number of issues. Please, upgrade your dependencies to the actual version of core-js@3.
npm WARN deprecated babel-istanbul-loader@0.1.0: Package is deprecated, use https://github.com/deepsweet/istanbul-instrumenter-loader
npm WARN deprecated chokidar@1.7.0: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
npm WARN deprecated isparta-loader@2.0.0: Package is deprecated, use https://github.com/deepsweet/istanbul-instrumenter-loader
npm WARN deprecated minimatch@0.2.14: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated coffee-script@1.3.3: CoffeeScript on NPM has moved to "coffeescript" (no hyphen)
npm WARN deprecated nodeunit@0.9.5: you are strongly encouraged to use other testing options
npm WARN deprecated istanbul@0.4.5: This module is no longer maintained, try this instead:
npm WARN deprecated   npm i nyc
npm WARN deprecated Visit https://istanbul.js.org/integrations for other alternatives.
npm WARN deprecated mkdirp@0.5.1: Legacy versions of mkdirp are no longer supported. Please update to mkdirp 1.x. (Note that the API surface has changed to use Promises in 1.x.)
npm WARN deprecated to-iso-string@0.0.2: to-iso-string has been deprecated, use @segment/to-iso-string instead.
npm WARN deprecated jade@0.26.3: Jade has been renamed to pug, please install the latest version of pug instead of jade
npm WARN deprecated request@2.67.0: request has been deprecated, see https://github.com/request/request/issues/3142
npm WARN deprecated formatio@1.1.1: This package is unmaintained. Use @sinonjs/formatio instead
npm WARN deprecated request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
npm WARN deprecated samsam@1.1.2: This package has been deprecated in favour of @sinonjs/samsam
npm WARN deprecated minimatch@2.0.10: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated fsevents@1.2.13: fsevents 1 will break on node v14+ and could be using insecure binaries. Upgrade to fsevents 2.
npm WARN deprecated graceful-fs@1.2.3: please upgrade to graceful-fs 4 for compatibility with current and future versions of Node.js
npm WARN deprecated minimatch@0.3.0: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated mkdirp@0.5.0: Legacy versions of mkdirp are no longer supported. Please update to mkdirp 1.x. (Note that the API surface has changed to use Promises in 1.x.)
npm WARN deprecated mkdirp@0.3.0: Legacy versions of mkdirp are no longer supported. Please update to mkdirp 1.x. (Note that the API surface has changed to use Promises in 1.x.)
npm WARN deprecated tough-cookie@2.2.2: ReDoS vulnerability parsing Set-Cookie https://nodesecurity.io/advisories/130
npm WARN deprecated samsam@1.1.3: This package has been deprecated in favour of @sinonjs/samsam
npm WARN deprecated hawk@3.1.3: This module moved to @hapi/hawk. Please make sure to switch over as this distribution is no longer supported and may contain bugs and critical security issues.
npm WARN deprecated node-uuid@1.4.8: Use uuid module instead
npm WARN deprecated har-validator@2.0.6: this library is no longer supported
npm WARN deprecated har-validator@5.1.5: this library is no longer supported
npm WARN deprecated circular-json@0.3.3: CircularJSON is in maintenance only, flatted is its successor.
npm WARN deprecated json3@3.3.2: Please use the native JSON object instead of JSON 3
npm WARN deprecated cryptiles@2.0.5: This version has been deprecated in accordance with the hapi support policy (hapi.im/support). Please upgrade to the latest version to get the best features, bug fixes, and security patches. If you are unable to upgrade at this time, paid support is available for older versions (hapi.im/commercial).
npm WARN deprecated hoek@2.16.3: This version has been deprecated in accordance with the hapi support policy (hapi.im/support). Please upgrade to the latest version to get the best features, bug fixes, and security patches. If you are unable to upgrade at this time, paid support is available for older versions (hapi.im/commercial).
npm WARN deprecated sntp@1.0.9: This module moved to @hapi/sntp. Please make sure to switch over as this distribution is no longer supported and may contain bugs and critical security issues.
npm WARN deprecated boom@2.10.1: This version has been deprecated in accordance with the hapi support policy (hapi.im/support). Please upgrade to the latest version to get the best features, bug fixes, and security patches. If you are unable to upgrade at this time, paid support is available for older versions (hapi.im/commercial).
npm WARN deprecated chokidar@2.1.8: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
npm WARN deprecated request@2.79.0: request has been deprecated, see https://github.com/request/request/issues/3142
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN notsup Unsupported engine for karma@1.7.1: wanted: {"node":"0.10 || 0.12 || 4 || 5 || 6 || 7 || 8"} (current: {"node":"10.19.0","npm":"6.14.4"})
npm WARN notsup Not compatible with your version of node/npm: karma@1.7.1
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@^1.0.0 (node_modules/chokidar/node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.13: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@~2.3.1 (node_modules/watchpack/node_modules/chokidar/node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@2.3.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
npm WARN karma-sinon-chai@1.3.4 requires a peer of sinon@>=2.1.0 <5 but none is installed. You must install peer dependencies yourself.

added 1234 packages from 125 contributors and audited 1240 packages in 8.984s
found 214 vulnerabilities (88 low, 32 moderate, 92 high, 2 critical)
  run `npm audit fix` to fix them, or `npm audit` for details

And as that will be at the bottom of a scrollable part... For highlighting purposes:

added 1234 packages from 125 contributors and audited 1240 packages in 8.984s
found 214 vulnerabilities (88 low, 32 moderate, 92 high, 2 critical)

Sure, most of the dependancies are from dev dependancies (as noted above), but there are still 6 non dev (and 56 dev) dependancies. But still. It's high in SNR.

And that they obviously doesn't necessarily even relate to the versions used in the built version.

In both production and dev we have use of deprecated and no longer supported/abandoned repos.

Even npm audit knows what's up ;).

npm notice created a lockfile as package-lock.json. You should commit this file.

So as currently stands, this is a default medium risk. There's no way for humans to (easily, without a time machine) to audit the code that is there. dist/quagga.js itself is 11.5K LOC, without any dependancies brought in (via whatever method). As per T249039#6309061 this requires manager (or director) acceptance of the risk.

Requires Manger level oversight and risk treatment plan creation within 30 days. Risk treatment must be applied with 30 days of plan creation. Risk acceptance by Management level.

And depending on what happens (lack of risk treatment plan etc), this potentially bubbles up to become a high risk, requiring C-level acceptance.

It's also more difficult for people to maintain/patch if there was a security issue found in production (as was found with an XSS in MobileFrontend for example), and a fix needed rolling out quickly (though we can at least seemingly turn off the feature using $wgISBNScannerDesktopEnable on desktop, it's default always on on mobile devices)?

	isbnEnabled = OO.ui.isMobile() || mw.config.get( 'wgCitoidConfig' ).ISBNScannerDesktopEnable;
	isbnSupported =
		// Reflects browser security policy
		( location.protocol === 'https:' || location.hostname === 'localhost' ) &&
		navigator.mediaDevices && navigator.mediaDevices.getUserMedia;

Mitigations

For a good example of "what to do next", see T260236#6825798 from Section Translation.

This is where the upstream developer not being active, or seemingly even responsive at all in a long time comes in. It might not be easy to get these issues fixed upstream.

The easiest way I can see forward from here is to fork the library, and fix the auditability concerns. Add and commit a package-lock.json. That way we should have more reproducible builds, and insight into what actually went into making the minified and packed js. Make sure various the npm dependancies are up to date (dev too, to make things generally quieter; mostly probably just npm audit fix), and don't include the potential 2 x high "Prototype Pollution" vulnerabilities in lodash; whether by replacing the 3rd party code entirely (if feasible), or at least upgrading the version. Obviously, feel free to try and upstream any and all changes if you wish.

This then mostly just leaves the WebPack issue of any significance. Which is part of a larger ongoing discussion around the movement, more so for in-house built stuff, but it's similarly (or potentially even more) relevant for 3rd party code where we can't even necessarily speak to the developers, and they're not actually active or the audit trail may not be so obvious.

I believe the longer term plan is to move away from WebPack entirely (use of Rollup in some cases) where we are currently using it in MW/WMF production in some cases, so I'd advise against introducing it at all if at all feasible. I've filed T275047: Create webpack security standard for MediaWiki development wrt to documenting this more formally, and Scott's comments and links help point in the direction things should be heading. And it's probably "better" to fix this before it's committed and deployed, rather than being tech debt to fix up after the fact.

Removing both WebPack (or at least having the artefacts somewhere), and adding the package-lock.json should be enough to keep the risk to at least a medium (manager level acceptance) or even a low (which is little/no action required in most cases) at the other end. Obviously, a second look would be required at that point anyway.

It also seems better to me not introducing further WebPack usages in production, rather than having to go back and remove/rework them later, taking further time and effort.

It would also be appreciated to add a foreign-resources.yaml file as appropriate depending on what happens with the library.

Thanks @Reedy. I'll look into https://github.com/ericblade/quagga2 which appears to be more actively maintained with more up-to-date dependencies (and a tracked package-lock file).

If it were just the WebPack issue left, would that likely give us a lower risk rating?

It would also be appreciated to add a foreign-resources.yaml file as appropriate depending on what happens with the library.

This is already present in the patch, and will be kept going forward.

That library does look a better starting point, helping address at least some of the auditability concerns and also keeping up to date with the moving target that is npm package related vulnerabilities, and in theory helps improve the reproducible build situation.

I think the current position is that generally using WebPack results in a default Medium risk. It is generally being discouraged, and alternative solutions (ie rollup or similar) being pursued in other code.

As part of that risk mitigation plan, including (either by committing to our repos, or at least, having the relevant upstream commit references for the same files if the repo includes them) the unminified webpack artifacts does definitely help. I don't think it necessarily lowers the risk, but should be enough to prevent it bubbling up to being a high risk at least.

@Esanders @marcella @Reedy -

I've gone ahead and added an entry to the Security-Team's risk registry for a medium risk for use of the quaggajs library with Citoid. As this is a medium risk, per our risk management framework, it will be accepted at the managerial level (e.g. @marcella). If quaggajs is further mitigated to reduce risk or never makes it into production or is replaced with quagga2 (after a new security review), then we can adjust or remove this entry within the risk registry. Let me know if you have any further questions. Thanks.