Hardening: normalize on-disk attachment paths before save/load

app/attachments.js

@@ -37,7 +37,11 @@ exports.createReader = root => {
     }
 
     const absolutePath = path.join(root, relativePath);
-    const buffer = await fse.readFile(absolutePath);
+    const normalized = path.normalize(absolutePath);
+    if (!normalized.startsWith(root)) {
+      throw new Error('Invalid relative path');
+    }
+    const buffer = await fse.readFile(normalized);
     return toArrayBuffer(buffer);
   };
 };
@@ -83,8 +87,13 @@ exports.createWriterForExisting = root => {
 
     const buffer = Buffer.from(arrayBuffer);
     const absolutePath = path.join(root, relativePath);
-    await fse.ensureFile(absolutePath);
+    const normalized = path.normalize(absolutePath);
-    await fse.writeFile(absolutePath, buffer);
+    if (!normalized.startsWith(root)) {
+      throw new Error('Invalid relative path');
+    }
+
+    await fse.ensureFile(normalized);
+    await fse.writeFile(normalized, buffer);
     return relativePath;
   };
 };
@@ -103,6 +112,10 @@ exports.createDeleter = root => {
     }
 
     const absolutePath = path.join(root, relativePath);
+    const normalized = path.normalize(absolutePath);
+    if (!normalized.startsWith(root)) {
+      throw new Error('Invalid relative path');
+    }
     await fse.remove(absolutePath);
   };
 };
@@ -124,5 +137,11 @@ exports.getRelativePath = name => {
 };
 
 //      createAbsolutePathGetter :: RoothPath -> RelativePath -> AbsolutePath
-exports.createAbsolutePathGetter = rootPath => relativePath =>
+exports.createAbsolutePathGetter = rootPath => relativePath => {
-  path.join(rootPath, relativePath);
+  const absolutePath = path.join(rootPath, relativePath);
+  const normalized = path.normalize(absolutePath);
+  if (!normalized.startsWith(rootPath)) {
+    throw new Error('Invalid relative path');
+  }
+  return normalized;
+};

test/app/attachments_test.js

@@ -77,6 +77,28 @@ describe('Attachments', () => {
       const inputBuffer = Buffer.from(input);
       assert.deepEqual(inputBuffer, output);
     });
+
+    it('throws if relative path goes higher than root', async () => {
+      const input = stringToArrayBuffer('test string');
+      const tempDirectory = path.join(
+        tempRootDirectory,
+        'Attachments_createWriterForExisting'
+      );
+
+      const relativePath = '../../parent';
+      const attachment = {
+        path: relativePath,
+        data: input,
+      };
+      try {
+        await Attachments.createWriterForExisting(tempDirectory)(attachment);
+      } catch (error) {
+        assert.strictEqual(error.message, 'Invalid relative path');
+        return;
+      }
+
+      throw new Error('Expected an error');
+    });
   });
 
   describe('createReader', () => {
@@ -110,6 +132,24 @@ describe('Attachments', () => {
 
       assert.deepEqual(input, output);
     });
+
+    it('throws if relative path goes higher than root', async () => {
+      const tempDirectory = path.join(
+        tempRootDirectory,
+        'Attachments_createReader'
+      );
+
+      const relativePath = '../../parent';
+
+      try {
+        await Attachments.createReader(tempDirectory)(relativePath);
+      } catch (error) {
+        assert.strictEqual(error.message, 'Invalid relative path');
+        return;
+      }
+
+      throw new Error('Expected an error');
+    });
   });
 
   describe('createDeleter', () => {
@@ -142,6 +182,24 @@ describe('Attachments', () => {
       const existsFile = await fse.exists(fullPath);
       assert.isFalse(existsFile);
     });
+
+    it('throws if relative path goes higher than root', async () => {
+      const tempDirectory = path.join(
+        tempRootDirectory,
+        'Attachments_createDeleter'
+      );
+
+      const relativePath = '../../parent';
+
+      try {
+        await Attachments.createDeleter(tempDirectory)(relativePath);
+      } catch (error) {
+        assert.strictEqual(error.message, 'Invalid relative path');
+        return;
+      }
+
+      throw new Error('Expected an error');
+    });
   });
 
   describe('createName', () => {
@@ -157,4 +215,30 @@ describe('Attachments', () => {
       assert.lengthOf(Attachments.getRelativePath(name), PATH_LENGTH);
     });
   });
+
+  describe('createAbsolutePathGetter', () => {
+    it('combines root and relative path', () => {
+      const root = '/tmp';
+      const relative = 'ab/abcdef';
+      const pathGetter = Attachments.createAbsolutePathGetter(root);
+      const absolutePath = pathGetter(relative);
+
+      assert.strictEqual(absolutePath, '/tmp/ab/abcdef');
+    });
+
+    it('throws if relative path goes higher than root', () => {
+      const root = '/tmp';
+      const relative = '../../ab/abcdef';
+      const pathGetter = Attachments.createAbsolutePathGetter(root);
+
+      try {
+        pathGetter(relative);
+      } catch (error) {
+        assert.strictEqual(error.message, 'Invalid relative path');
+        return;
+      }
+
+      throw new Error('Expected an error');
+    });
+  });
 });