Harden express-processimage for use in production

[papandreou]

• Validate the GET parameters so we don't pass arbitrary data to command line programs
• Make it possible to disable certain filters and operations so things like inkscape and svgfilter don't have to be exposed in cases where they aren't needed
• Add a configurable max resolution of input and output images to prevent DOSing the server by eg. scaling an icon to 100000x100000 pixels. Sort of like gm -limit Pixels <numberOfPixels>.

[Munter]

👍 from here :)

[papandreou]

Actually, let's keep this open until all parameters are validated.

[dbohannon]

What's the current status with respect to hardening this package for use in production environments? I don't see any instances where remote code execution is possible. It looks like all parameters are passed to other dependency functions rather than to command line tools. Is this correct? Is the statement "Parts of the query string will be passed directly to various command line tools." in the README still accurate?

[papandreou]

The current status is that a bunch of graphicsmagick operations and all parameters related to pngcrush, pngquant, jpegtran, optipng, svgfilter, and inkscape aren't validated:

express-processimage/lib/getFilterInfosAndTargetContentTypeFromQueryString.js

Lines 145 to 361 in cbd932d

	// FIXME: Add validation code for all the below.
	// https://github.com/papandreou/express-processimage/issues/4
	// Other engines:
	case 'pngcrush':
	case 'pngquant':
	case 'jpegtran':
	case 'optipng':
	case 'svgfilter':
	case 'inkscape':
	return true;
	// Graphicsmagick specific operations:
	// FIXME: Add validation code for all the below.
	// https://github.com/papandreou/express-processimage/issues/4
	case 'setFormat':
	case 'identify':
	case 'selectFrame':
	case 'subCommand':
	case 'adjoin':
	case 'affine':
	case 'alpha':
	case 'append':
	case 'authenticate':
	case 'average':
	case 'backdrop':
	case 'blackThreshold':
	case 'bluePrimary':
	case 'border':
	case 'borderColor':
	case 'box':
	case 'channel':
	case 'chop':
	case 'clip':
	case 'coalesce':
	case 'colorize':
	case 'colorMap':
	case 'compose':
	case 'compress':
	case 'convolve':
	case 'createDirectories':
	case 'deconstruct':
	case 'define':
	case 'delay':
	case 'displace':
	case 'display':
	case 'dispose':
	case 'dissolve':
	case 'encoding':
	case 'endian':
	case 'file':
	case 'flatten':
	case 'foreground':
	case 'frame':
	case 'fuzz':
	case 'gaussian':
	case 'geometry':
	case 'greenPrimary':
	case 'highlightColor':
	case 'highlightStyle':
	case 'iconGeometry':
	case 'intent':
	case 'lat':
	case 'level':
	case 'list':
	case 'log':
	case 'loop':
	case 'map':
	case 'mask':
	case 'matte':
	case 'matteColor':
	case 'maximumError':
	case 'mode':
	case 'monitor':
	case 'mosaic':
	case 'motionBlur':
	case 'name':
	case 'noop':
	case 'normalize':
	case 'opaque':
	case 'operator':
	case 'orderedDither':
	case 'outputDirectory':
	case 'page':
	case 'pause':
	case 'pen':
	case 'ping':
	case 'pointSize':
	case 'preview':
	case 'process':
	case 'profile':
	case 'progress':
	case 'randomThreshold':
	case 'recolor':
	case 'redPrimary':
	case 'remote':
	case 'render':
	case 'repage':
	case 'sample':
	case 'samplingFactor':
	case 'scene':
	case 'scenes':
	case 'screen':
	case 'set':
	case 'segment':
	case 'shade':
	case 'shadow':
	case 'sharedMemory':
	case 'shave':
	case 'shear':
	case 'silent':
	case 'rawSize':
	case 'snaps':
	case 'stegano':
	case 'stereo':
	case 'textFont':
	case 'texture':
	case 'threshold':
	case 'thumbnail':
	case 'tile':
	case 'title':
	case 'transform':
	case 'transparent':
	case 'treeDepth':
	case 'update':
	case 'units':
	case 'unsharp':
	case 'usePixmap':
	case 'view':
	case 'virtualPixel':
	case 'visual':
	case 'watermark':
	case 'wave':
	case 'whitePoint':
	case 'whiteThreshold':
	case 'window':
	case 'windowGroup':
	case 'strip':
	case 'interlace':
	case 'setFormat':
	case 'resizeExact':
	case 'scale':
	case 'filter':
	case 'density':
	case 'noProfile':
	case 'resample':
	case 'rotate':
	case 'magnify':
	case 'minify':
	case 'quality':
	case 'charcoal':
	case 'modulate':
	case 'antialias':
	case 'bitdepth':
	case 'colors':
	case 'colorspace':
	case 'comment':
	case 'contrast':
	case 'cycle':
	case 'despeckle':
	case 'dither':
	case 'monochrome':
	case 'edge':
	case 'emboss':
	case 'enhance':
	case 'equalize':
	case 'gamma':
	case 'implode':
	case 'label':
	case 'limit':
	case 'median':
	case 'negative':
	case 'noise':
	case 'paint':
	case 'raise':
	case 'lower':
	case 'region':
	case 'roll':
	case 'sharpen':
	case 'solarize':
	case 'spread':
	case 'swirl':
	case 'type':
	case 'trim':
	case 'extent':
	case 'gravity':
	case 'background':
	case 'fill':
	case 'stroke':
	case 'strokeWidth':
	case 'font':
	case 'fontSize':
	case 'draw':
	case 'drawPoint':
	case 'drawLine':
	case 'drawRectangle':
	case 'drawArc':
	case 'drawEllipse':
	case 'drawCircle':
	case 'drawPolyline':
	case 'drawPolygon':
	case 'drawBezier':
	case 'drawText':
	case 'setDraw':
	case 'thumb':
	case 'thumbExact':
	case 'morph':
	case 'sepia':
	case 'autoOrient':
	case 'in':
	case 'out':
	case 'preprocessor':
	case 'addSrcFormatter':
	case 'inputIs':
	case 'compare':
	case 'composite':
	case 'montage':
	return true;

... so all of those should be considered unsafe.

However, I'm running it in production myself with a conservative whitelist of allowed operations configured via the allowOperation option:

const allowedOperations = [
    'withoutEnlargement',
    'progressive',
    'ignoreAspectRatio',
    'interpolateWith',
    'metadata',
    'resize',
    'extract',
    'crop',
    'rotate',
    'quality',
    'format',
    'png',
    'jpeg',
    'gm'
];

const app = require('express')().use(require('express-processimage')({
    allowOperation: (name, args) => {
        return allowedOperations.indexOf(name) !== -1;
    }
}));

Remember to specify maxInputPixels and maxOutputPixels as well.