From 153fa17d334959fef48ecd211e9dc63731a26a63 Mon Sep 17 00:00:00 2001 From: Ying Xue Date: Thu, 6 Feb 2025 17:54:44 -0800 Subject: [PATCH] fix codeql 0217 --- src/app/api/services/localRepoService.ts | 11 +--- .../listItemLocalLabel.spec.tsx.snap | 5 ++ .../components/listItemLocalLabel.tsx | 7 ++- src/localization/locales/en.json | 1 + src/localization/resourceKeys.ts | 1 + src/server/serverBase.ts | 10 +-- src/server/utils.ts | 62 ++++++------------- 7 files changed, 37 insertions(+), 60 deletions(-) diff --git a/src/app/api/services/localRepoService.ts b/src/app/api/services/localRepoService.ts index ee0bcfc3..ef67a916 100644 --- a/src/app/api/services/localRepoService.ts +++ b/src/app/api/services/localRepoService.ts @@ -30,14 +30,5 @@ export const fetchLocalFileNaive = async (path: string, fileName: string): Promi export const fetchDirectories = async (path: string): Promise => { const response = await fetch(`${CONTROLLER_API_ENDPOINT}${GET_DIRECTORIES}/${encodeURIComponent(path || DEFAULT_DIRECTORY)}`); - if (!path) { - // only possible when platform is windows, expecting drives to be returned - const responseText = await response.text(); - const drives = responseText.split(/\r\n/).map(drive => drive.trim()).filter(drive => drive !== ''); - drives.shift(); // remove header - return drives.map(drive => `${drive}/`); // add trailing slash for drives - } - else { - return response.json(); - } + return response.json(); }; diff --git a/src/app/modelRepository/components/__snapshots__/listItemLocalLabel.spec.tsx.snap b/src/app/modelRepository/components/__snapshots__/listItemLocalLabel.spec.tsx.snap index 237368f6..061f43f1 100644 --- a/src/app/modelRepository/components/__snapshots__/listItemLocalLabel.spec.tsx.snap +++ b/src/app/modelRepository/components/__snapshots__/listItemLocalLabel.spec.tsx.snap @@ -15,6 +15,11 @@ exports[`ListItemLocalLabel matches snapshot when repo type equals Local 1`] = ` > modelRepository.types.local.infoText + + modelRepository.types.local.warning + `; diff --git a/src/app/modelRepository/components/listItemLocalLabel.tsx b/src/app/modelRepository/components/listItemLocalLabel.tsx index 74bc1638..205c3d0d 100644 --- a/src/app/modelRepository/components/listItemLocalLabel.tsx +++ b/src/app/modelRepository/components/listItemLocalLabel.tsx @@ -4,7 +4,7 @@ **********************************************************/ import * as React from 'react'; import { Trans, useTranslation } from 'react-i18next'; -import { Link } from '@fluentui/react'; +import { Link, MessageBar, MessageBarType } from '@fluentui/react'; import { REPOSITORY_LOCATION_TYPE } from '../../constants/repositoryLocationTypes'; import { ResourceKeys } from '../../../localization/resourceKeys'; @@ -17,7 +17,10 @@ export const ListItemLocalLabel: React.FC<{repoType: REPOSITORY_LOCATION_TYPE}>
{t(ResourceKeys.modelRepository.types.local.label)}
{t(ResourceKeys.modelRepository.types.local.infoText)}
-
} + + {t(ResourceKeys.modelRepository.types.local.warning)} + + } {repoType === REPOSITORY_LOCATION_TYPE.LocalDMR &&
{t(ResourceKeys.modelRepository.types.dmr.label)}
diff --git a/src/localization/locales/en.json b/src/localization/locales/en.json index 00262398..cb3fd8c2 100644 --- a/src/localization/locales/en.json +++ b/src/localization/locales/en.json @@ -977,6 +977,7 @@ "local": { "label": "Local Folder", "infoText": "Use your local folder as a model repository. The model definition files you wish to use should have a json extension. The first JSON file with an '@id' property matching the digital twin model identifier (DTMI) will be retrieved.", + "warning": "For security reasons, please select a subdirectory inside C:\\Users\\ (Windows) or /home/ (Linux/macOS).", "textBoxLabel": "Selected folder", "folderPicker": { "command": { diff --git a/src/localization/resourceKeys.ts b/src/localization/resourceKeys.ts index eedfcc13..5158ce2e 100644 --- a/src/localization/resourceKeys.ts +++ b/src/localization/resourceKeys.ts @@ -857,6 +857,7 @@ export class ResourceKeys { infoText : "modelRepository.types.local.infoText", label : "modelRepository.types.local.label", textBoxLabel : "modelRepository.types.local.textBoxLabel", + warning : "modelRepository.types.local.warning", }, mandatory : "modelRepository.types.mandatory", notAvailable : "modelRepository.types.notAvailable", diff --git a/src/server/serverBase.ts b/src/server/serverBase.ts index 0075a015..b93fd20e 100644 --- a/src/server/serverBase.ts +++ b/src/server/serverBase.ts @@ -13,7 +13,7 @@ import fetch from 'node-fetch'; import { EventHubConsumerClient, Subscription, ReceivedEventData, earliestEventPosition } from '@azure/event-hubs'; import { generateDataPlaneRequestBody, generateDataPlaneResponse } from './dataPlaneHelper'; import { convertIotHubToEventHubsConnectionString } from './eventHubHelper'; -import { fetchDirectories, fetchDrivesOnWindows, findMatchingFile, isSafeUrl, readFileFromLocal } from './utils'; +import { fetchDirectories, findMatchingFile, isSafeUrl, readFileFromLocal, SAFE_ROOT } from './utils'; export const SERVER_ERROR = 500; export const SUCCESS = 200; @@ -134,7 +134,7 @@ export const handleGetDirectoriesRequest = (req: express.Request, res: express.R try { const dir = req.params.path; if (dir === '$DEFAULT') { - fetchDrivesOnWindows(res); + res.status(SUCCESS).send([SAFE_ROOT]); } else { fetchDirectories(dir, res); @@ -208,9 +208,11 @@ export const handleModelRepoPostRequest = async (req: express.Request, res: expr if (!(await isSafeUrl(userUri))) { return res.status(403).send({ error: "Forbidden: Unsafe URL." }); } - + + // Reconstruct a sanitized URL + const safeUrl = `${userUri.origin}${userUri.pathname}`; try { - const response = await fetch(userUri, + const response = await fetch(safeUrl, { body: controllerRequest.body || null, headers: controllerRequest.headers || null, diff --git a/src/server/utils.ts b/src/server/utils.ts index 18893d36..fdcf5be4 100644 --- a/src/server/utils.ts +++ b/src/server/utils.ts @@ -17,63 +17,37 @@ export const fetchDrivesOnWindows = (res: express.Response) => { }); }; -const SAFE_ROOTS_WINDOWS = [ - "C:\\Users", // User home directories - "D:\\", "E:\\", // Additional drives (non-system) - "C:\\ProgramData", // Application-wide data storage - "C:\\inetpub", // IIS web server root - "C:\\Temp" // Temporary storage (watch for security concerns) -]; - -const SAFE_ROOTS_LINUX = [ - "/home", // User home directories - "/Users", // macOS user home directories - "/var/www", // Web server content - "/var/data", // Common data storage - "/srv", // Server files - "/opt", // Optional software installs - "/workspace", // Dev environments - "/app", // Application root - "/tmp", "/var/tmp" // Temporary storage (watch for security concerns) -]; - -const isPathSafe = (dir: string): string | null => { - const resolvedPath = path.resolve(fs.realpathSync(dir)); - - // Use platform-specific safe roots - const safeRoots = process.platform === "win32" ? SAFE_ROOTS_WINDOWS : SAFE_ROOTS_LINUX; - - for (const root of safeRoots) { - const resolvedRoot = path.resolve(root); - - if (resolvedPath.startsWith(resolvedRoot + path.sep)) { - // Normalize the path within the safe root - return path.resolve(resolvedRoot, path.relative(resolvedRoot, resolvedPath)); - } +// Dynamically determine a "Safe Root Directory" +const getSafeRoot = (): string => { + if (process.platform === "win32") { + return "C:\\Users"; // Restrict access to user directories only } - - return null; // Not in a safe root + return "/home"; // Restrict access to home directories on Linux/macOS }; +export const SAFE_ROOT = getSafeRoot(); + export const fetchDirectories = (dir: string, res: express.Response) => { try { - const safePath = isPathSafe(dir); + // Resolve the requested directory relative to the safe root + const resolvedPath = fs.realpathSync(path.resolve(SAFE_ROOT, path.relative(SAFE_ROOT, dir))); - if (!safePath) { - return res.status(403).send({ error: `Access denied: Unsafe directory ${dir}.` }); + // Ensure resolvedPath is still inside SAFE_ROOT (prevents traversal attacks) + if (!resolvedPath.startsWith(SAFE_ROOT)) { + return res.status(403).send({ error: "Access denied. Unsafe directory." }); } const result: string[] = []; - for (const item of fs.readdirSync(safePath)) { + for (const item of fs.readdirSync(resolvedPath)) { try { - const itemPath = fs.realpathSync(path.join(safePath, item)); + const itemPath = fs.realpathSync(path.join(resolvedPath, item)); - // Ensure itemPath is still within safePath - if (itemPath.startsWith(safePath + path.sep)) { + // Ensure itemPath is still inside resolvedPath (protects against symlink attacks) + if (itemPath.startsWith(resolvedPath + path.sep)) { const stat = fs.statSync(itemPath); if (stat.isDirectory()) { - result.push(item); + result.push(escape(item)); } } } catch { @@ -83,7 +57,7 @@ export const fetchDirectories = (dir: string, res: express.Response) => { res.status(200).send(result); } catch { - res.status(500).send({ error: 'Failed to fetch directories.' }); + res.status(500).send({ error: "Failed to fetch directories." }); } };