From b7428ab6276a38dd1184ae096306843e7cada917 Mon Sep 17 00:00:00 2001 From: Mark Hindess Date: Tue, 29 Jul 2014 13:51:21 +0100 Subject: [PATCH] Prohibit library entry names from containing '../'. --- red/library.js | 16 ++++++++++++ red/storage/index.js | 16 ++++++++++++ test/red/library_spec.js | 45 ++++++++++++++++++++++++++++++++++ test/red/storage/index_spec.js | 16 ++++++------ 4 files changed, 85 insertions(+), 8 deletions(-) diff --git a/red/library.js b/red/library.js index 4c3f57b3f..cc4199f27 100644 --- a/red/library.js +++ b/red/library.js @@ -34,6 +34,10 @@ function init() { res.send(204); }).otherwise(function(err) { util.log("[red] Error loading flow '"+req.params[0]+"' : "+err); + if (err.message.indexOf('forbidden') === 0) { + res.send(403); + return; + } res.send(500); }); }); @@ -52,6 +56,10 @@ function init() { }).otherwise(function(err) { if (err) { util.log("[red] Error loading flow '"+req.params[0]+"' : "+err); + if (err.message.indexOf('forbidden') === 0) { + res.send(403); + return; + } } res.send(404); }); @@ -75,6 +83,10 @@ function createLibrary(type) { }).otherwise(function(err) { if (err) { util.log("[red] Error loading library entry '"+path+"' : "+err); + if (err.message.indexOf('forbidden') === 0) { + res.send(403); + return; + } } res.send(404); }); @@ -91,6 +103,10 @@ function createLibrary(type) { res.send(204); }).otherwise(function(err) { util.log("[red] Error saving library entry '"+path+"' : "+err); + if (err.message.indexOf('forbidden') === 0) { + res.send(403); + return; + } res.send(500); }); }); diff --git a/red/storage/index.js b/red/storage/index.js index ad55fa5b7..836947593 100644 --- a/red/storage/index.js +++ b/red/storage/index.js @@ -33,6 +33,10 @@ function moduleSelector(aSettings) { return toReturn; } +function is_malicious(path) { + return path.indexOf('../') != -1 || path.indexOf('..\\') != -1; +} + var storageModuleInterface = { init : function(settings) { try { @@ -58,15 +62,27 @@ var storageModuleInterface = { return storageModule.getAllFlows(); }, getFlow : function(fn) { + if (is_malicious(fn)) { + return when.reject(new Error('forbidden flow name')); + } return storageModule.getFlow(fn); }, saveFlow : function(fn, data) { + if (is_malicious(fn)) { + return when.reject(new Error('forbidden flow name')); + } return storageModule.saveFlow(fn, data); }, getLibraryEntry : function(type, path) { + if (is_malicious(path)) { + return when.reject(new Error('forbidden flow name')); + } return storageModule.getLibraryEntry(type, path); }, saveLibraryEntry : function(type, path, meta, body) { + if (is_malicious(path)) { + return when.reject(new Error('forbidden flow name')); + } return storageModule.saveLibraryEntry(type, path, meta, body); } } diff --git a/test/red/library_spec.js b/test/red/library_spec.js index e472cda62..522552872 100644 --- a/test/red/library_spec.js +++ b/test/red/library_spec.js @@ -120,6 +120,27 @@ describe("library", function() { }); }); }); + + it('returns 403 for malicious access attempt', function(done) { + // without the userDir override the malicious url would be + // http://127.0.0.1:1880/library/flows/../../package to + // obtain package.json from the node-red root. + request(RED.httpAdmin) + .get('/library/flows/../../../../../package') + .expect(403) + .end(done); + }); + + it('returns 403 for malicious access attempt', function(done) { + // without the userDir override the malicious url would be + // http://127.0.0.1:1880/library/flows/../../package to + // obtain package.json from the node-red root. + request(RED.httpAdmin) + .post('/library/flows/../../../../../package') + .expect(403) + .end(done); + }); + }); describe("type", function() { @@ -188,5 +209,29 @@ describe("library", function() { }); }); + + it('returns 403 for malicious access attempt', function(done) { + request(RED.httpAdmin) + .get('/library/test/../../../../../../../../../../etc/passwd') + .expect(403) + .end(done); + }); + + it('returns 403 for malicious access attempt', function(done) { + request(RED.httpAdmin) + .get('/library/test/..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\etc\\passwd') + .expect(403) + .end(done); + }); + + it('returns 403 for malicious access attempt', function(done) { + request(RED.httpAdmin) + .post('/library/test/../../../../../../../../../../etc/passwd') + .set('Content-Type', 'text/plain') + .send('root:x:0:0:root:/root:/usr/bin/tclsh') + .expect(403) + .end(done); + }); + }); }); diff --git a/test/red/storage/index_spec.js b/test/red/storage/index_spec.js index d6a30f75c..4b60ba835 100644 --- a/test/red/storage/index_spec.js +++ b/test/red/storage/index_spec.js @@ -83,19 +83,19 @@ describe("red/storage/index", function() { calledFlagGetAllFlows = true; }, getFlow : function(fn) { - fn.should.be.true; + fn.should.equal("name"); }, saveFlow : function(fn, data) { - fn.should.be.true; + fn.should.equal("name"); data.should.be.true; }, getLibraryEntry : function(type, path) { type.should.be.true; - path.should.be.true; + path.should.equal("name"); }, saveLibraryEntry : function(type, path, meta, body) { type.should.be.true; - path.should.be.true; + path.should.equal("name"); meta.should.be.true; body.should.be.true; } @@ -112,10 +112,10 @@ describe("red/storage/index", function() { storage.getCredentials(); storage.saveCredentials(true); storage.getAllFlows(); - storage.getFlow(true); - storage.saveFlow(true, true); - storage.getLibraryEntry(true, true); - storage.saveLibraryEntry(true, true, true, true); + storage.getFlow("name"); + storage.saveFlow("name", true); + storage.getLibraryEntry(true, "name"); + storage.saveLibraryEntry(true, "name", true, true); calledInit.should.be.true; calledFlagGetFlows.should.be.true;