From 3c958e1d86c526e12b8c98ff6e468199baeafaed Mon Sep 17 00:00:00 2001 From: Sean Lilley Date: Tue, 4 Apr 2017 16:45:21 -0400 Subject: [PATCH] Add secure checking of paths --- README.md | 1 + bin/obj2gltf.js | 8 +++++++- lib/convert.js | 1 + lib/image.js | 4 ---- lib/mtl.js | 4 ---- lib/obj.js | 33 +++++++++++++++++++++++++++++---- specs/lib/imageSpec.js | 9 --------- specs/lib/mtlSpec.js | 9 --------- specs/lib/objSpec.js | 28 ++++++++++++++++++++++++++++ 9 files changed, 66 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index a1b625a..7b7dc08 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,7 @@ Using obj2gltf as a command-line tool: |`--ao`|Apply ambient occlusion to the converted model.|No, default `false`| |`--bypassPipeline`|Bypass the gltf-pipeline for debugging purposes. This option overrides many of the options above and will save the glTF with the KHR_materials_common extension.|No, default `false`| |`--hasTransparency`|Do a more exhaustive check for texture transparency by looking at the alpha channel of each pixel. By default textures with an alpha channel are considered to be transparent.|No, default `false`| +|`--secure`|Prevent the converter from reading image or mtl files outside of the input obj directory.|No, default `false`| ## Build Instructions diff --git a/bin/obj2gltf.js b/bin/obj2gltf.js index 56e02e5..654d9f8 100644 --- a/bin/obj2gltf.js +++ b/bin/obj2gltf.js @@ -84,6 +84,11 @@ var argv = yargs describe: 'Do a more exhaustive check for texture transparency by looking at the alpha channel of each pixel. By default textures with an alpha channel are considered to be transparent.', type: 'boolean', default: false + }, + secure : { + describe: 'Prevent the converter from reading image or mtl files outside of the input obj directory.', + type: 'boolean', + default: false } }).parse(args); @@ -111,7 +116,8 @@ var options = { ao : argv.ao, optimizeForCesium : argv.cesium, bypassPipeline : argv.bypassPipeline, - hasTransparency : argv.hasTransparency + hasTransparency : argv.hasTransparency, + secure : argv.secure }; console.time('Total'); diff --git a/lib/convert.js b/lib/convert.js index aef3037..0014f28 100644 --- a/lib/convert.js +++ b/lib/convert.js @@ -33,6 +33,7 @@ module.exports = convert; * @param {Boolean} [options.textureCompressionOptions] Options sent to the compressTextures stage of gltf-pipeline. * @param {Boolean} [options.bypassPipeline=false] Bypass the gltf-pipeline for debugging purposes. This option overrides many of the options above and will save the glTF with the KHR_materials_common extension. * @param {Boolean} [options.hasTransparency=false] Do a more exhaustive check for texture transparency by looking at the alpha channel of each pixel. + * @param {Boolean} [options.secure=false] Prevent the converter from reading image or mtl files outside of the input obj directory. */ function convert(objPath, gltfPath, options) { diff --git a/lib/image.js b/lib/image.js index f596dbf..e5cbc45 100644 --- a/lib/image.js +++ b/lib/image.js @@ -56,10 +56,6 @@ function loadImage(imagePath, options) { } return info; - }) - .catch(function() { - console.log('Could not read image file at ' + imagePath + '. Material will ignore this image.'); - return undefined; }); } diff --git a/lib/mtl.js b/lib/mtl.js index 50a93fe..4ced92e 100644 --- a/lib/mtl.js +++ b/lib/mtl.js @@ -101,10 +101,6 @@ function loadMtl(mtlPath) { return readLines(mtlPath, parseLine) .then(function() { return materials; - }) - .catch(function() { - console.log('Could not read material file at ' + mtlPath + '. Using default material instead.'); - return {}; }); } diff --git a/lib/obj.js b/lib/obj.js index af6be9d..455709a 100644 --- a/lib/obj.js +++ b/lib/obj.js @@ -53,12 +53,18 @@ var facePattern4 = /f( +(-?\d+)\/\/(-?\d+))( +(-?\d+)\/\/(-?\d+))( +(-?\d+)\/\/( * @param {String} objPath Path to the obj file. * @param {Object} [options] An object with the following properties: * @param {Boolean} [options.hasTransparency=false] Do a more exhaustive check for texture transparency by looking at the alpha channel of each pixel. + * @param {Boolean} [options.secure=false] Prevent the converter from reading image or mtl files outside of the input obj directory. * @returns {Promise} A promise resolving to the obj data. * @exception {RuntimeError} The file does not have any geometry information in it. * * @private */ function loadObj(objPath, options) { + options = combine(options, { + hasTransparency : false, + secure : false + }); + // Global store of vertex attributes listed in the obj file var positions = new ArrayStorage(ComponentDatatype.FLOAT); var normals = new ArrayStorage(ComponentDatatype.FLOAT); @@ -284,10 +290,10 @@ function finishLoading(nodes, mtlPaths, objPath, options) { if (nodes.length === 0) { throw new RuntimeError(objPath + ' does not have any geometry data'); } - return loadMaterials(mtlPaths, objPath) + return loadMaterials(mtlPaths, objPath, options) .then(function(materials) { var imagePaths = getImagePaths(materials); - return loadImages(imagePaths, options) + return loadImages(imagePaths, objPath, options) .then(function(images) { return { nodes : nodes, @@ -305,27 +311,46 @@ function getAbsolutePath(mtlPath, objPath) { return mtlPath; } -function loadMaterials(mtlPaths, objPath) { +function outsideDirectory(filePath, objPath) { + return (path.relative(path.dirname(objPath), filePath).indexOf('..') === 0); +} + +function loadMaterials(mtlPaths, objPath, options) { var materials = {}; return Promise.map(mtlPaths, function(mtlPath) { mtlPath = getAbsolutePath(mtlPath, objPath); + if (options.secure && outsideDirectory(mtlPath, objPath)) { + console.log('Could not read mtl file at ' + mtlPath + ' because it is outside of the obj directory and the secure flag is true. Using default material instead.'); + return; + } return loadMtl(mtlPath) .then(function(materialsInMtl) { materials = combine(materials, materialsInMtl); + }) + .catch(function() { + console.log('Could not read mtl file at ' + mtlPath + '. Using default material instead.'); }); }).then(function() { return materials; }); } -function loadImages(imagePaths, options) { +function loadImages(imagePaths, objPath, options) { var images = {}; return Promise.map(imagePaths, function(imagePath) { + if (options.secure && outsideDirectory(imagePath, objPath)) { + console.log('Could not read image file at ' + imagePath + ' because it is outside of the obj directory and the secure flag is true. Material will ignore this image.'); + return; + } return loadImage(imagePath, options) .then(function(image) { if (defined(image)) { images[imagePath] = image; } + }) + .catch(function() { + console.log('Could not read image file at ' + imagePath + '. Material will ignore this image.'); + return undefined; }); }).then(function() { return images; diff --git a/specs/lib/imageSpec.js b/specs/lib/imageSpec.js index 9fe5277..e1eff09 100644 --- a/specs/lib/imageSpec.js +++ b/specs/lib/imageSpec.js @@ -91,13 +91,4 @@ describe('image', function() { expect(info.transparent).toBe(false); }), done).toResolve(); }); - - it('handles invalid image file', function(done) { - spyOn(console, 'log'); - expect(loadImage(invalidImage) - .then(function(image) { - expect(image).toBeUndefined(); - expect(console.log.calls.argsFor(0)[0].indexOf('Could not read image file') >= 0).toBe(true); - }), done).toResolve(); - }); }); diff --git a/specs/lib/mtlSpec.js b/specs/lib/mtlSpec.js index 36be197..0be943c 100644 --- a/specs/lib/mtlSpec.js +++ b/specs/lib/mtlSpec.js @@ -41,13 +41,4 @@ describe('mtl', function() { expect(materials.Blue.diffuseColor).toEqual([0.0, 0.0, 0.64, 1.0]); }), done).toResolve(); }); - - it('handles invalid mtl file', function(done) { - spyOn(console, 'log'); - expect(loadMtl(invalidMaterialUrl) - .then(function(materials) { - expect(materials).toEqual({}); - expect(console.log.calls.argsFor(0)[0].indexOf('Could not read material file') >= 0).toBe(true); - }), done).toResolve(); - }); }); diff --git a/specs/lib/objSpec.js b/specs/lib/objSpec.js index b064bdc..c1aeac3 100644 --- a/specs/lib/objSpec.js +++ b/specs/lib/objSpec.js @@ -21,6 +21,7 @@ var objMultipleMaterialsUrl = 'specs/data/box-multiple-materials/box-multiple-ma var objUncleanedUrl = 'specs/data/box-uncleaned/box-uncleaned.obj'; var objMtllibUrl = 'specs/data/box-mtllib/box-mtllib.obj'; var objMissingMtllibUrl = 'specs/data/box-missing-mtllib/box-missing-mtllib.obj'; +var objExternalResourcesUrl = 'specs/data/box-external-resources/box-external-resources.obj'; var objTexturedUrl = 'specs/data/box-textured/box-textured.obj'; var objMissingTextureUrl = 'specs/data/box-missing-texture/box-missing-texture.obj'; var objSubdirectoriesUrl = 'specs/data/box-subdirectories/box-textured.obj'; @@ -269,6 +270,32 @@ describe('obj', function() { expect(loadObj(objMissingMtllibUrl) .then(function(data) { expect(data.materials).toEqual({}); + expect(console.log.calls.argsFor(0)[0].indexOf('Could not read mtl file') >= 0).toBe(true); + }), done).toResolve(); + }); + + it('loads resources outside of the obj directory', function(done) { + expect(loadObj(objExternalResourcesUrl) + .then(function(data) { + var imagePath = getImagePath(objTexturedUrl, 'cesium.png'); + expect(data.images[imagePath]).toBeDefined(); + expect(data.materials.MaterialTextured.diffuseColorMap).toEqual(imagePath); + }), done).toResolve(); + }); + + it('does not load resources outside of the obj directory when secure is true', function(done) { + spyOn(console, 'log'); + var options = { + secure : true + }; + expect(loadObj(objExternalResourcesUrl, options) + .then(function(data) { + var imagePath = getImagePath(objMissingTextureUrl, 'cesium.png'); + expect(data.images[imagePath]).toBeUndefined(); + expect(data.materials.MaterialTextured).toBeDefined(); + expect(data.materials.Material).toBeUndefined(); // Not in directory, so not included + expect(console.log.calls.argsFor(0)[0].indexOf('Could not read mtl file') >= 0).toBe(true); + expect(console.log.calls.argsFor(1)[0].indexOf('Could not read image file') >= 0).toBe(true); }), done).toResolve(); }); @@ -288,6 +315,7 @@ describe('obj', function() { var imagePath = getImagePath(objMissingTextureUrl, 'cesium.png'); expect(data.images[imagePath]).toBeUndefined(); expect(data.materials.Material.diffuseColorMap).toEqual(imagePath); + expect(console.log.calls.argsFor(0)[0].indexOf('Could not read image file') >= 0).toBe(true); }), done).toResolve(); });