fix(fs): check resolved path against root (#224)

* fix(fs): check resolved path against root This should prevent paths from being resolved above the root. Should affect all commands that utilize the FS functions. Fixes #167 * test: use __dirname for relative certs * fix: improve path resolution * chore: remove unused package * fix: normalize resolve path if absolute Otherwise join will normalize Co-authored-by: Tyler Stewart <tyler@autovance.com>
QuorumDMS · Dec 16, 2020 · 457b859 · 457b859
1 parent 722da60
commit 457b859
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 20 deletions.
diff --git a/src/fs.js b/src/fs.js
@@ -6,10 +6,13 @@ const {createReadStream, createWriteStream, constants} = require('fs');
 const fsAsync = require('./helpers/fs-async');
 const errors = require('./errors');
 
+const UNIX_SEP_REGEX = /\//g;
+const WIN_SEP_REGEX = /\\/g;
+
 class FileSystem {
   constructor(connection, {root, cwd} = {}) {
     this.connection = connection;
-    this.cwd = nodePath.normalize(cwd ? nodePath.join(nodePath.sep, cwd) : nodePath.sep);
+    this.cwd = nodePath.normalize((cwd || '/').replace(WIN_SEP_REGEX, '/'));
     this._root = nodePath.resolve(root || process.cwd());
   }
 
@@ -18,19 +21,21 @@ class FileSystem {
   }
 
   _resolvePath(path = '.') {
-    const clientPath = (() => {
-      path = nodePath.normalize(path);
-      if (nodePath.isAbsolute(path)) {
-        return nodePath.join(path);
-      } else {
-        return nodePath.join(this.cwd, path);
-      }
-    })();
-
-    const fsPath = (() => {
-      const resolvedPath = nodePath.join(this.root, clientPath);
-      return nodePath.resolve(nodePath.normalize(nodePath.join(resolvedPath)));
-    })();
+    // Unix separators normalize nicer on both unix and win platforms
+    const resolvedPath = path.replace(WIN_SEP_REGEX, '/');
+
+    // Join cwd with new path
+    const joinedPath = nodePath.isAbsolute(resolvedPath)
+      ? nodePath.normalize(resolvedPath)
+      : nodePath.join('/', this.cwd, resolvedPath);
+
+    // Create local filesystem path using the platform separator
+    const fsPath = nodePath.resolve(nodePath.join(this.root, joinedPath)
+      .replace(UNIX_SEP_REGEX, nodePath.sep)
+      .replace(WIN_SEP_REGEX, nodePath.sep));
+
+    // Create FTP client path using unix separator
+    const clientPath = joinedPath.replace(WIN_SEP_REGEX, '/');
 
     return {
       clientPath,

diff --git a/test/fs.spec.js b/test/fs.spec.js
@@ -36,7 +36,7 @@ describe('FileSystem', function () {
 
   describe('#_resolvePath', function () {
     it('gets correct relative path', function () {
-      const result = fs._resolvePath();
+      const result = fs._resolvePath('.');
       expect(result).to.be.an('object');
       expect(result.clientPath).to.equal(
         nodePath.normalize('/file/1/2/3'));
@@ -53,6 +53,15 @@ describe('FileSystem', function () {
         nodePath.resolve('/tmp/ftp-srv/file/1/2'));
     });
 
+    it('gets correct relative path', function () {
+      const result = fs._resolvePath('other');
+      expect(result).to.be.an('object');
+      expect(result.clientPath).to.equal(
+        nodePath.normalize('/file/1/2/3/other'));
+      expect(result.fsPath).to.equal(
+        nodePath.resolve('/tmp/ftp-srv/file/1/2/3/other'));
+    });
+
     it('gets correct absolute path', function () {
       const result = fs._resolvePath('/other');
       expect(result).to.be.an('object');
@@ -62,7 +71,7 @@ describe('FileSystem', function () {
         nodePath.resolve('/tmp/ftp-srv/other'));
     });
 
-    it('cannot escape root', function () {
+    it('cannot escape root - unix', function () {
       const result = fs._resolvePath('../../../../../../../../../../..');
       expect(result).to.be.an('object');
       expect(result.clientPath).to.equal(
@@ -71,6 +80,24 @@ describe('FileSystem', function () {
         nodePath.resolve('/tmp/ftp-srv'));
     });
 
+    it('cannot escape root - win', function () {
+      const result = fs._resolvePath('.\\..\\..\\..\\..\\..\\..\\');
+      expect(result).to.be.an('object');
+      expect(result.clientPath).to.equal(
+        nodePath.normalize('/'));
+      expect(result.fsPath).to.equal(
+        nodePath.resolve('/tmp/ftp-srv'));
+    });
+
+    it('cannot escape root - backslash prefix', function () {
+      const result = fs._resolvePath('\\/../../../../../../');
+      expect(result).to.be.an('object');
+      expect(result.clientPath).to.equal(
+        nodePath.normalize('/'));
+      expect(result.fsPath).to.equal(
+        nodePath.resolve('/tmp/ftp-srv'));
+    });
+
     it('resolves to file', function () {
       const result = fs._resolvePath('/cool/file.txt');
       expect(result).to.be.an('object');

diff --git a/test/start.js b/test/start.js
@@ -9,16 +9,16 @@ const server = new FtpServer({
   pasv_min: 8881,
   greeting: ['Welcome', 'to', 'the', 'jungle!'],
   tls: {
-    key: fs.readFileSync(`${process.cwd()}/test/cert/server.key`),
-    cert: fs.readFileSync(`${process.cwd()}/test/cert/server.crt`),
-    ca: fs.readFileSync(`${process.cwd()}/test/cert/server.csr`)
+    key: fs.readFileSync(`${__dirname}/cert/server.key`),
+    cert: fs.readFileSync(`${__dirname}/cert/server.crt`),
+    ca: fs.readFileSync(`${__dirname}/cert/server.csr`)
   },
   file_format: 'ep',
   anonymous: 'sillyrabbit'
 });
 server.on('login', ({username, password}, resolve, reject) => {
   if (username === 'test' && password === 'test' || username === 'anonymous') {
-    resolve({root: require('os').homedir()});
+    resolve({root: __dirname});
   } else reject('Bad username or password');
 });
 server.listen();