Page MenuHomePhabricator

Security review of 3d2png
Closed, ResolvedPublic

Description

  • Name of tool/project: 3d2png
  • Description of the tool/project: A backend command line tool in node.js to render thumbnails of 3d models files (currently the STL and AMF file formats).
  • Description of how the tool will be used at WMF: This will down-render 3D files into PNGs so that non-JS users can see the files (on articles, on File: pages, etc.).
  • Name of individual/group requesting review and primary contact: @Gilles and Multimedia
  • Name of individual/group responsible for tool/project after deployment and primary contact: Multimedia
  • Target date for deployment (or approximate date deployed if already in production or labs): When available.
  • Information from any review of the tool that has already been conducted:

The dependencies I need pull quite a lot of subdependencies (maybe that's usual for node.js, I don't know), which might need security review.
Some of those node.js also require some debian packages, namely:

pkg-config, libcairo2-dev, libjpeg-dev, libxi-dev, libglu1-mesa-dev libglew-dev

I want to make sure that those are ok too.
Regarding the file formats themselves (AMF and STL). AMF is XML-based can be zipped. STL is binary or text. In both cases the parsing is done with custom code copied from three.js's examples (AMFLoader.js is slightly modified and be found at the root of 3d2png, STLLoader is taken directly from the three.js module's examples folder).

Revisions and Commits

rTDTP 3d2png
Restricted Differential Revision

Event Timeline

@Gilles Thanks for submitting this. To help us in planning this review, please update the description of this ticket with the information noted at https://www.mediawiki.org/wiki/Wikimedia_Security_Team/Security_reviews#Requesting_a_review.

Jdforrester-WMF updated the task description. (Show Details)
Jdforrester-WMF subscribed.

@dpatrick, not sure if this is enough. :-)

This has been scheduled for the week of June 27th.

General Observations

Support for 3D files has been a long-requested feature addition. I'm glad this work is being done. However, a few issues were found, as noted below.

There are no unit tests for the application. Use of a test harness, and inclusion of tests verifying (at least) proper parsing and rendering of .amf and .stl, would aid in ensuring code and dependency quality and implementation correctness, and could be used to verify protection against the malformed input issue listed below (once it is fixed upstream).

Files

3d2png.js
NOTE

  • There are very few code comments, and no method documentation. This, combined with the coding style used, reduces comprehensibility, and therefore, auditability.

AMFLoader.js
HIGH

NOTE

  • JSZip is used in AMFLoader.js, but is not listed as a dependency in package.json. Are zipped .amf files not supported?
  • Line 68, exception handling should be improved to differentiate between JSZip unavailability and an actual error in unzipping the supplied input
Gilles added a revision: Restricted Differential Revision.Aug 15 2016, 3:14 PM

@dpatrick hey, we just landed a patch that should fix the concerns. I don't know if you want to confirm that, but if so we can close this task.