Просмотр исходного кода

fix: prevent path traversal attacks in file handling

Yuki Takei 3 месяцев назад
Родитель
Сommit
898f6ba8e7

+ 25 - 2
apps/app/src/server/routes/apiv3/g2g-transfer.ts

@@ -1,11 +1,11 @@
+import { createReadStream } from 'node:fs';
 import { SCOPE } from '@growi/core/dist/interfaces';
 import { ErrorV3 } from '@growi/core/dist/models';
 import type { NextFunction, Request, Router } from 'express';
 import express from 'express';
 import { body } from 'express-validator';
-import { createReadStream } from 'fs';
 import multer from 'multer';
-import path from 'path';
+import path from 'pathe';
 
 import type { GrowiArchiveImportOption } from '~/models/admin/growi-archive-import-option';
 import { accessTokenParser } from '~/server/middlewares/access-token-parser';
@@ -22,6 +22,7 @@ import { TransferKey } from '~/utils/vo/transfer-key';
 import type Crowi from '../../crowi';
 import { apiV3FormValidator } from '../../middlewares/apiv3-form-validator';
 import { Attachment } from '../../models/attachment';
+import { isPathWithinBase } from '../../util/path-utils';
 import type { ApiV3Response } from './interfaces/apiv3-response';
 
 interface AuthorizedRequest extends Request {
@@ -511,6 +512,28 @@ module.exports = (crowi: Crowi): Router => {
         );
       }
 
+      // Validate file path to prevent path traversal attack
+      const importService = getImportService();
+      if (importService == null) {
+        return res.apiv3Err(
+          new ErrorV3(
+            'Import service is not available.',
+            'service_unavailable',
+          ),
+          500,
+        );
+      }
+      if (!isPathWithinBase(file.path, importService.baseDir)) {
+        logger.error('Path traversal attack detected', {
+          filePath: file.path,
+          baseDir: importService.baseDir,
+        });
+        return res.apiv3Err(
+          new ErrorV3('Invalid file path.', 'invalid_path'),
+          400,
+        );
+      }
+
       const fileStream = createReadStream(file.path, {
         flags: 'r',
         mode: 0o666,

+ 123 - 0
apps/app/src/server/service/growi-bridge/index.spec.ts

@@ -0,0 +1,123 @@
+import fs from 'node:fs';
+import path from 'pathe';
+import { mock } from 'vitest-mock-extended';
+
+import type Crowi from '~/server/crowi';
+
+import { GrowiBridgeService } from './index';
+
+vi.mock('fs');
+
+describe('GrowiBridgeService', () => {
+  let growiBridgeService: GrowiBridgeService;
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+    const crowiMock = mock<Crowi>();
+    growiBridgeService = new GrowiBridgeService(crowiMock);
+  });
+
+  describe('getFile', () => {
+    const baseDir = '/tmp/growi-export';
+
+    beforeEach(() => {
+      // Mock fs.accessSync to not throw (file exists)
+      vi.mocked(fs.accessSync).mockImplementation(() => undefined);
+    });
+
+    describe('valid file paths', () => {
+      test('should return resolved path for a simple filename', () => {
+        const fileName = 'test.json';
+        const result = growiBridgeService.getFile(fileName, baseDir);
+
+        expect(result).toBe(path.resolve(baseDir, fileName));
+        expect(fs.accessSync).toHaveBeenCalledWith(
+          path.resolve(baseDir, fileName),
+        );
+      });
+
+      test('should return resolved path for a filename in subdirectory', () => {
+        const fileName = 'subdir/test.json';
+        const result = growiBridgeService.getFile(fileName, baseDir);
+
+        expect(result).toBe(path.resolve(baseDir, fileName));
+      });
+
+      test('should handle baseDir with trailing slash', () => {
+        const fileName = 'test.json';
+        const baseDirWithSlash = '/tmp/growi-export/';
+        const result = growiBridgeService.getFile(fileName, baseDirWithSlash);
+
+        expect(result).toBe(path.resolve(baseDirWithSlash, fileName));
+      });
+    });
+
+    describe('path traversal attack prevention', () => {
+      test('should throw error for path traversal with ../', () => {
+        const fileName = '../etc/passwd';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw error for path traversal with multiple ../', () => {
+        const fileName = '../../etc/passwd';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw error for path traversal in middle of path', () => {
+        const fileName = 'subdir/../../../etc/passwd';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw error for absolute path outside baseDir', () => {
+        const fileName = '/etc/passwd';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw error for path traversal at the end of path', () => {
+        // e.g., trying to access sibling directory
+        const fileName = 'subdir/../../other-dir/file.json';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+    });
+
+    describe('file access check', () => {
+      test('should throw error if file does not exist', () => {
+        const fileName = 'nonexistent.json';
+        vi.mocked(fs.accessSync).mockImplementation(() => {
+          throw new Error('ENOENT: no such file or directory');
+        });
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('ENOENT: no such file or directory');
+      });
+    });
+  });
+
+  describe('getEncoding', () => {
+    test('should return utf-8', () => {
+      expect(growiBridgeService.getEncoding()).toBe('utf-8');
+    });
+  });
+
+  describe('getMetaFileName', () => {
+    test('should return meta.json', () => {
+      expect(growiBridgeService.getMetaFileName()).toBe('meta.json');
+    });
+  });
+});

+ 9 - 5
apps/app/src/server/service/growi-bridge/index.ts

@@ -1,12 +1,13 @@
-import fs from 'fs';
-import path from 'path';
-import { pipeline } from 'stream';
-import { finished } from 'stream/promises';
+import fs from 'node:fs';
+import { pipeline } from 'node:stream';
+import { finished } from 'node:stream/promises';
+import path from 'pathe';
 import unzipStream, { type Entry } from 'unzip-stream';
 
 import type Crowi from '~/server/crowi';
 import loggerFactory from '~/utils/logger';
 
+import { assertFileNameSafeForBaseDir } from '../../util/path-utils';
 import type { ZipFileStat } from '../interfaces/export';
 import { tapStreamDataByPromise } from './unzip-stream-utils';
 
@@ -55,7 +56,10 @@ export class GrowiBridgeService {
    * @memberOf GrowiBridgeService
    */
   getFile(fileName: string, baseDir: string): string {
-    const jsonFile = path.join(baseDir, fileName);
+    // Prevent path traversal attack
+    assertFileNameSafeForBaseDir(fileName, baseDir);
+
+    const jsonFile = path.resolve(baseDir, fileName);
 
     // throws err if the file does not exist
     fs.accessSync(jsonFile);

+ 169 - 0
apps/app/src/server/util/path-utils.spec.ts

@@ -0,0 +1,169 @@
+import path from 'pathe';
+
+import {
+  assertFileNameSafeForBaseDir,
+  isFileNameSafeForBaseDir,
+  isPathWithinBase,
+} from './path-utils';
+
+describe('path-utils', () => {
+  describe('isPathWithinBase', () => {
+    const baseDir = '/tmp/growi-export';
+
+    describe('valid paths', () => {
+      test('should return true for file directly in baseDir', () => {
+        const filePath = '/tmp/growi-export/test.json';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(true);
+      });
+
+      test('should return true for file in subdirectory', () => {
+        const filePath = '/tmp/growi-export/subdir/test.json';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(true);
+      });
+
+      test('should return true for baseDir itself', () => {
+        expect(isPathWithinBase(baseDir, baseDir)).toBe(true);
+      });
+
+      test('should handle relative paths correctly', () => {
+        const filePath = path.join(baseDir, 'test.json');
+        expect(isPathWithinBase(filePath, baseDir)).toBe(true);
+      });
+    });
+
+    describe('invalid paths (path traversal attacks)', () => {
+      test('should return false for path outside baseDir', () => {
+        const filePath = '/etc/passwd';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(false);
+      });
+
+      test('should return false for path traversal with ../', () => {
+        const filePath = '/tmp/growi-export/../etc/passwd';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(false);
+      });
+
+      test('should return false for sibling directory', () => {
+        const filePath = '/tmp/other-dir/test.json';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(false);
+      });
+
+      test('should return false for directory with similar prefix', () => {
+        // /tmp/growi-export-evil should not match /tmp/growi-export
+        const filePath = '/tmp/growi-export-evil/test.json';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(false);
+      });
+    });
+  });
+
+  describe('isFileNameSafeForBaseDir', () => {
+    const baseDir = '/tmp/growi-export';
+
+    describe('valid file names', () => {
+      test('should return true for simple filename', () => {
+        expect(isFileNameSafeForBaseDir('test.json', baseDir)).toBe(true);
+      });
+
+      test('should return true for filename in subdirectory', () => {
+        expect(isFileNameSafeForBaseDir('subdir/test.json', baseDir)).toBe(
+          true,
+        );
+      });
+
+      test('should return true for deeply nested file', () => {
+        expect(isFileNameSafeForBaseDir('a/b/c/d/test.json', baseDir)).toBe(
+          true,
+        );
+      });
+    });
+
+    describe('path traversal attacks', () => {
+      test('should return false for ../etc/passwd', () => {
+        expect(isFileNameSafeForBaseDir('../etc/passwd', baseDir)).toBe(false);
+      });
+
+      test('should return false for ../../etc/passwd', () => {
+        expect(isFileNameSafeForBaseDir('../../etc/passwd', baseDir)).toBe(
+          false,
+        );
+      });
+
+      test('should return false for subdir/../../../etc/passwd', () => {
+        expect(
+          isFileNameSafeForBaseDir('subdir/../../../etc/passwd', baseDir),
+        ).toBe(false);
+      });
+
+      test('should return false for absolute path outside baseDir', () => {
+        expect(isFileNameSafeForBaseDir('/etc/passwd', baseDir)).toBe(false);
+      });
+
+      test('should return false for path escaping to sibling directory', () => {
+        expect(
+          isFileNameSafeForBaseDir('subdir/../../other-dir/file.json', baseDir),
+        ).toBe(false);
+      });
+    });
+
+    describe('edge cases', () => {
+      test('should handle empty filename', () => {
+        // Empty filename resolves to baseDir itself, which is valid
+        expect(isFileNameSafeForBaseDir('', baseDir)).toBe(true);
+      });
+
+      test('should handle . (current directory)', () => {
+        expect(isFileNameSafeForBaseDir('.', baseDir)).toBe(true);
+      });
+
+      test('should handle ./filename', () => {
+        expect(isFileNameSafeForBaseDir('./test.json', baseDir)).toBe(true);
+      });
+    });
+  });
+
+  describe('assertFileNameSafeForBaseDir', () => {
+    const baseDir = '/tmp/growi-export';
+
+    describe('valid file names (should not throw)', () => {
+      test('should not throw for simple filename', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('test.json', baseDir);
+        }).not.toThrow();
+      });
+
+      test('should not throw for filename in subdirectory', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('subdir/test.json', baseDir);
+        }).not.toThrow();
+      });
+    });
+
+    describe('path traversal attacks (should throw)', () => {
+      test('should throw for ../etc/passwd', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('../etc/passwd', baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw for ../../etc/passwd', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('../../etc/passwd', baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw for absolute path outside baseDir', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('/etc/passwd', baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw for path escaping to sibling directory', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir(
+            'subdir/../../other-dir/file.json',
+            baseDir,
+          );
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+    });
+  });
+});

+ 69 - 0
apps/app/src/server/util/path-utils.ts

@@ -0,0 +1,69 @@
+import path from 'pathe';
+
+/**
+ * Validates that the given file path is within the base directory.
+ * This prevents path traversal attacks where an attacker could use sequences
+ * like '../' to access files outside the intended directory.
+ *
+ * @param filePath - The file path to validate
+ * @param baseDir - The base directory that the file path should be within
+ * @returns true if the path is valid, false otherwise
+ */
+export function isPathWithinBase(filePath: string, baseDir: string): boolean {
+  const resolvedBaseDir = path.resolve(baseDir);
+  const resolvedFilePath = path.resolve(filePath);
+
+  // Check if the resolved path starts with the base directory
+  // We add path.sep to ensure we're checking a directory boundary
+  // (e.g., /tmp/foo should not match /tmp/foobar)
+  return (
+    resolvedFilePath.startsWith(resolvedBaseDir + path.sep) ||
+    resolvedFilePath === resolvedBaseDir
+  );
+}
+
+/**
+ * Validates that joining baseDir with fileName results in a path within baseDir.
+ * This is useful for validating user-provided file names before using them.
+ *
+ * @param fileName - The file name to validate
+ * @param baseDir - The base directory
+ * @returns true if the resulting path is valid, false otherwise
+ * @throws Error if path traversal is detected
+ */
+export function assertFileNameSafeForBaseDir(
+  fileName: string,
+  baseDir: string,
+): void {
+  const resolvedBaseDir = path.resolve(baseDir);
+  const resolvedFilePath = path.resolve(baseDir, fileName);
+
+  const isValid =
+    resolvedFilePath.startsWith(resolvedBaseDir + path.sep) ||
+    resolvedFilePath === resolvedBaseDir;
+
+  if (!isValid) {
+    throw new Error('Invalid file path: path traversal detected');
+  }
+}
+
+/**
+ * Validates that joining baseDir with fileName results in a path within baseDir.
+ * This is useful for validating user-provided file names before using them.
+ *
+ * @param fileName - The file name to validate
+ * @param baseDir - The base directory
+ * @returns true if the resulting path is valid, false otherwise
+ */
+export function isFileNameSafeForBaseDir(
+  fileName: string,
+  baseDir: string,
+): boolean {
+  const resolvedBaseDir = path.resolve(baseDir);
+  const resolvedFilePath = path.resolve(baseDir, fileName);
+
+  return (
+    resolvedFilePath.startsWith(resolvedBaseDir + path.sep) ||
+    resolvedFilePath === resolvedBaseDir
+  );
+}