Skip to content

Commit

Permalink
resolve #340208 - Command injection in 'pdf-image', Severity:Medium (…
Browse files Browse the repository at this point in the history 
…6.1) - fix mooz#38 - solution for v2
  • Loading branch information
@roest01
roest01 committed May 13, 2018
1 parent d8c0dca commit 5467949
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 65 deletions.
116 changes: 59 additions & 57 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,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 = {};
Expand All @@ -23,10 +23,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 = {};
Expand All @@ -40,20 +40,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) {
Expand Down Expand Up @@ -84,46 +76,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());
});
});
},
Expand Down Expand Up @@ -167,16 +166,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);
});
});
}

Expand All @@ -194,7 +195,9 @@ PDFImage.prototype = {

if (imageNotExists) {
// (1)
convertPageToImage();
convertPageToImage().then(function(result){
resolve(result);
}).catch(reject);
return;
}

Expand All @@ -209,11 +212,10 @@ PDFImage.prototype = {

if (imageFileStat.mtime < pdfFileStat.mtime) {
// (2)
convertPageToImage();
return;
convertPageToImage().then(function(result){
resolve(result);
}).catch(reject);
}

return resolve(outputImagePath);
});
});
});
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
37 changes: 30 additions & 7 deletions tests/test-main.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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){
Expand Down

0 comments on commit 5467949

Please sign in to comment.