From 4ef55a356706cfaa4c9d36d6e7702cf82a3b994f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Ro=CC=88s?= Date: Mon, 14 May 2018 01:43:20 +0200 Subject: [PATCH] resolve #340208 - Command injection in 'pdf-image', Severity:Medium (6.1) - fix #38 - solution for v2 --- index.js | 117 +++++++++++++++++++++++---------------------- package.json | 3 +- tests/test-main.js | 37 +++++++++++--- 3 files changed, 91 insertions(+), 66 deletions(-) diff --git a/index.js b/index.js index 4fa6231..05c2073 100644 --- a/index.js +++ b/index.js @@ -4,8 +4,7 @@ var Promise = require("es6-promise").Promise; var path = require("path"); var fs = require("fs"); -var util = require("util"); -var exec = require("child_process").exec; +var spawn = require("child-process-promise").spawn; function PDFImage(pdfFilePath, options) { if (!options) options = {}; @@ -23,10 +22,10 @@ function PDFImage(pdfFilePath, options) { PDFImage.prototype = { constructGetInfoCommand: function () { - return util.format( - "pdfinfo \"%s\"", - this.pdfFilePath - ); + return { + cmd: "pdfinfo", + args: [this.pdfFilePath] + }; }, parseGetInfoCommandOutput: function (output) { var info = {}; @@ -40,20 +39,12 @@ PDFImage.prototype = { getInfo: function () { var self = this; var getInfoCommand = this.constructGetInfoCommand(); - var promise = new Promise(function (resolve, reject) { - exec(getInfoCommand, function (err, stdout, stderr) { - if (err) { - return reject({ - message: "Failed to get PDF'S information", - error: err, - stdout: stdout, - stderr: stderr - }); - } - return resolve(self.parseGetInfoCommandOutput(stdout)); - }); + return new Promise(function (resolve, reject) { + spawn(getInfoCommand.cmd, getInfoCommand.args, { capture: [ 'stdout', 'stderr' ]}) + .then(function (cmdResult) { + resolve(self.parseGetInfoCommandOutput(cmdResult.stdout.toString())); + }).catch(reject); }); - return promise; }, numberOfPages: function () { return this.getInfo().then(function (info) { @@ -84,46 +75,53 @@ PDFImage.prototype = { constructConvertCommandForPage: function (pageNumber) { var pdfFilePath = this.pdfFilePath; var outputImagePath = this.getOutputImagePathForPage(pageNumber); - var convertOptionsString = this.constructConvertOptions(); - return util.format( - "%s %s\"%s[%d]\" \"%s\"", - this.useGM ? "gm convert" : "convert", - convertOptionsString ? convertOptionsString + " " : "", - pdfFilePath, pageNumber, outputImagePath - ); + var convertOptions = this.constructConvertOptions(); + var args = []; + if (convertOptions) args = convertOptions.slice(); + args.push(pdfFilePath+"["+pageNumber+"]"); + args.push(outputImagePath); + + return { + cmd: this.useGM ? "gm convert" : "convert", + args: args + }; }, constructCombineCommandForFile: function (imagePaths) { - return util.format( - "%s -append %s \"%s\"", - this.useGM ? "gm convert" : "convert", - imagePaths.join(' '), - this.getOutputImagePathForFile() - ); + var args = imagePaths.slice(); + args.push(this.getOutputImagePathForFile()); + args.unshift("-append"); + return { + cmd: this.useGM ? "gm convert" : "convert", + args: args + }; }, constructConvertOptions: function () { - return Object.keys(this.convertOptions).sort().map(function (optionName) { + var convertOptions = []; + Object.keys(this.convertOptions).sort().map(function (optionName) { if (this.convertOptions[optionName] !== null) { - return optionName + " " + this.convertOptions[optionName]; + convertOptions.push(optionName); + convertOptions.push(this.convertOptions[optionName]); } else { - return optionName; + convertOptions.push(optionName); } - }, this).join(" "); + }, this); + return convertOptions; }, combineImages: function(imagePaths) { var pdfImage = this; var combineCommand = pdfImage.constructCombineCommandForFile(imagePaths); return new Promise(function (resolve, reject) { - exec(combineCommand, function (err, stdout, stderr) { - if (err) { - return reject({ + spawn(combineCommand.cmd, combineCommand.args, { capture: [ 'stdout', 'stderr' ]}) + .then(function () { + spawn("rm", imagePaths); //cleanUp + resolve(pdfImage.getOutputImagePathForFile()); + }).catch(function(error){ + reject({ message: "Failed to combine images", - error: err, - stdout: stdout, - stderr: stderr + error: error.message, + stdout: error.stdout, + stderr: error.stderr }); - } - exec("rm "+imagePaths.join(' ')); //cleanUp - return resolve(pdfImage.getOutputImagePathForFile()); }); }); }, @@ -167,16 +165,18 @@ PDFImage.prototype = { var promise = new Promise(function (resolve, reject) { function convertPageToImage() { - exec(convertCommand, function (err, stdout, stderr) { - if (err) { - return reject({ + return new Promise(function (resolve, reject) { + spawn(convertCommand.cmd, convertCommand.args, { capture: [ 'stdout', 'stderr' ]}) + .then(function () { + resolve(outputImagePath); + }).catch(function(error){ + reject({ message: "Failed to convert page to image", - error: err, - stdout: stdout, - stderr: stderr + error: error.message, + stdout: error.stdout, + stderr: error.stderr }); - } - return resolve(outputImagePath); + }); }); } @@ -194,7 +194,9 @@ PDFImage.prototype = { if (imageNotExists) { // (1) - convertPageToImage(); + convertPageToImage().then(function(result){ + resolve(result); + }).catch(reject); return; } @@ -209,11 +211,10 @@ PDFImage.prototype = { if (imageFileStat.mtime < pdfFileStat.mtime) { // (2) - convertPageToImage(); - return; + convertPageToImage().then(function(result){ + resolve(result); + }).catch(reject); } - - return resolve(outputImagePath); }); }); }); diff --git a/package.json b/package.json index ed1c5fa..fa45bab 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,8 @@ }, "homepage": "https://github.com/mooz/node-pdf-image#readme", "dependencies": { - "es6-promise": "~4.2.4" + "es6-promise": "~4.2.4", + "child-process-promise": "^2.2.1" }, "devDependencies": { "chai": "~4.1.2", diff --git a/tests/test-main.js b/tests/test-main.js index d022d56..612b862 100644 --- a/tests/test-main.js +++ b/tests/test-main.js @@ -45,19 +45,22 @@ describe("PDFImage", function () { }); it("should return correct convert command", function () { - expect(pdfImage.constructConvertCommandForPage(1)) - .equal('convert "/tmp/test.pdf[1]" "/tmp/test-1.png"'); + var convertCommand = pdfImage.constructConvertCommandForPage(1); + expect(convertCommand.cmd).equal("convert"); + expect(convertCommand.args.length).equal(2); }); it("should return correct convert command to combine images", function () { - expect(pdfImage.constructCombineCommandForFile(['/tmp/test-0.png', '/tmp/test-1.png'])) - .equal('convert -append /tmp/test-0.png /tmp/test-1.png "/tmp/test.png"'); + var cmdConfig = pdfImage.constructCombineCommandForFile(['/tmp/test-0.png', '/tmp/test-1.png']); + expect(cmdConfig.cmd).equal('convert'); + expect(cmdConfig.args.length).equal(4); }); it("should use gm when you ask it to", function () { pdfImage = new PDFImage(pdfPath, {graphicsMagick: true}); - expect(pdfImage.constructConvertCommandForPage(1)) - .equal('gm convert "/tmp/test.pdf[1]" "/tmp/test-1.png"'); + var cmdConfig = pdfImage.constructConvertCommandForPage(1); + expect(cmdConfig.cmd).equal('gm convert'); + expect(cmdConfig.args.length).equal(2); }); // TODO: Do page updating test @@ -148,7 +151,27 @@ describe("PDFImage", function () { "-density": 300, "-trim": null }); - expect(pdfImage.constructConvertOptions()).equal("-density 300 -trim"); + expect(pdfImage.constructConvertOptions()[0]).equal("-density 300"); + expect(pdfImage.constructConvertOptions()[1]).equal("-trim"); + }); + + it("should convert all PDF's pages with convertOptions", function () { + return new Promise(function(resolve, reject){ + pdfImage.setConvertOptions({ + "-quality": 100, + "-trim": null + }); + + pdfImage.convertFile().then(function (images) { + images.forEach(function(image){ + expect(fs.existsSync(image)).to.be.true; + }); + generatedFiles = images; + resolve(); + }).catch(function (error) { + reject(error.message + " " + error.stderr); + }); + }) }); afterEach(function(done){