From 8efd36648fd27c3d50701acb09286ef3f8b0b9e2 Mon Sep 17 00:00:00 2001 From: galz10 Date: Fri, 13 Feb 2026 13:55:04 -0800 Subject: [PATCH] revert: fix(security): enforce strict policy directory permissions (#17353) --- docs/core/policy-engine.md | 32 ---- packages/core/src/config/storage.test.ts | 55 +------ packages/core/src/config/storage.ts | 20 +-- packages/core/src/policy/config.test.ts | 51 ------ packages/core/src/policy/config.ts | 32 +--- packages/core/src/utils/security.test.ts | 189 ----------------------- packages/core/src/utils/security.ts | 107 ------------- 7 files changed, 10 insertions(+), 476 deletions(-) delete mode 100644 packages/core/src/utils/security.test.ts delete mode 100644 packages/core/src/utils/security.ts diff --git a/docs/core/policy-engine.md b/docs/core/policy-engine.md index a99a6652d8..e6422dedc2 100644 --- a/docs/core/policy-engine.md +++ b/docs/core/policy-engine.md @@ -154,38 +154,6 @@ A rule matches a tool call if all of its conditions are met: Policies are defined in `.toml` files. The CLI loads these files from Default, User, and (if configured) Admin directories. -### Policy locations - -| Tier | Type | Location | -| :-------- | :----- | :-------------------------- | -| **User** | Custom | `~/.gemini/policies/*.toml` | -| **Admin** | System | _See below (OS specific)_ | - -#### System-wide policies (Admin) - -Administrators can enforce system-wide policies (Tier 3) that override all user -and default settings. These policies must be placed in specific, secure -directories: - -| OS | Policy Directory Path | -| :---------- | :------------------------------------------------ | -| **Linux** | `/etc/gemini-cli/policies` | -| **macOS** | `/Library/Application Support/GeminiCli/policies` | -| **Windows** | `C:\ProgramData\gemini-cli\policies` | - -**Security Requirements:** - -To prevent privilege escalation, the CLI enforces strict security checks on -admin directories. If checks fail, system policies are **ignored**. - -- **Linux / macOS:** Must be owned by `root` (UID 0) and NOT writable by group - or others (e.g., `chmod 755`). -- **Windows:** Must be in `C:\ProgramData`. Standard users (`Users`, `Everyone`) - must NOT have `Write`, `Modify`, or `Full Control` permissions. _Tip: If you - see a security warning, use the folder properties to remove write permissions - for non-admin groups. You may need to "Disable inheritance" in Advanced - Security Settings._ - ### TOML rule schema Here is a breakdown of the fields available in a TOML policy rule: diff --git a/packages/core/src/config/storage.test.ts b/packages/core/src/config/storage.test.ts index 8d91ca1a3e..0cbdaa5393 100644 --- a/packages/core/src/config/storage.test.ts +++ b/packages/core/src/config/storage.test.ts @@ -4,12 +4,11 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { beforeEach, describe, it, expect, vi, afterEach } from 'vitest'; +import { beforeEach, describe, it, expect, vi } from 'vitest'; vi.unmock('./storage.js'); vi.unmock('./projectRegistry.js'); vi.unmock('./storageMigration.js'); - import * as os from 'node:os'; import * as path from 'node:path'; @@ -173,55 +172,3 @@ describe('Storage – additional helpers', () => { expect(storageWithSession.getProjectTempPlansDir()).toBe(expected); }); }); - -describe('Storage - System Paths', () => { - const originalEnv = process.env['GEMINI_CLI_SYSTEM_SETTINGS_PATH']; - - afterEach(() => { - if (originalEnv !== undefined) { - process.env['GEMINI_CLI_SYSTEM_SETTINGS_PATH'] = originalEnv; - } else { - delete process.env['GEMINI_CLI_SYSTEM_SETTINGS_PATH']; - } - }); - - it('getSystemSettingsPath returns correct path based on platform (default)', () => { - delete process.env['GEMINI_CLI_SYSTEM_SETTINGS_PATH']; - - const platform = os.platform(); - const result = Storage.getSystemSettingsPath(); - - if (platform === 'darwin') { - expect(result).toBe( - '/Library/Application Support/GeminiCli/settings.json', - ); - } else if (platform === 'win32') { - expect(result).toBe('C:\\ProgramData\\gemini-cli\\settings.json'); - } else { - expect(result).toBe('/etc/gemini-cli/settings.json'); - } - }); - - it('getSystemSettingsPath follows GEMINI_CLI_SYSTEM_SETTINGS_PATH if set', () => { - const customPath = '/custom/path/settings.json'; - process.env['GEMINI_CLI_SYSTEM_SETTINGS_PATH'] = customPath; - expect(Storage.getSystemSettingsPath()).toBe(customPath); - }); - - it('getSystemPoliciesDir returns correct path based on platform and ignores env var', () => { - process.env['GEMINI_CLI_SYSTEM_SETTINGS_PATH'] = - '/custom/path/settings.json'; - const platform = os.platform(); - const result = Storage.getSystemPoliciesDir(); - - expect(result).not.toContain('/custom/path'); - - if (platform === 'darwin') { - expect(result).toBe('/Library/Application Support/GeminiCli/policies'); - } else if (platform === 'win32') { - expect(result).toBe('C:\\ProgramData\\gemini-cli\\policies'); - } else { - expect(result).toBe('/etc/gemini-cli/policies'); - } - }); -}); diff --git a/packages/core/src/config/storage.ts b/packages/core/src/config/storage.ts index bd0fec1c8e..5f059a12c9 100644 --- a/packages/core/src/config/storage.ts +++ b/packages/core/src/config/storage.ts @@ -93,25 +93,21 @@ export class Storage { ); } - private static getSystemConfigDir(): string { - if (os.platform() === 'darwin') { - return '/Library/Application Support/GeminiCli'; - } else if (os.platform() === 'win32') { - return 'C:\\ProgramData\\gemini-cli'; - } else { - return '/etc/gemini-cli'; - } - } - static getSystemSettingsPath(): string { if (process.env['GEMINI_CLI_SYSTEM_SETTINGS_PATH']) { return process.env['GEMINI_CLI_SYSTEM_SETTINGS_PATH']; } - return path.join(Storage.getSystemConfigDir(), 'settings.json'); + if (os.platform() === 'darwin') { + return '/Library/Application Support/GeminiCli/settings.json'; + } else if (os.platform() === 'win32') { + return 'C:\\ProgramData\\gemini-cli\\settings.json'; + } else { + return '/etc/gemini-cli/settings.json'; + } } static getSystemPoliciesDir(): string { - return path.join(Storage.getSystemConfigDir(), 'policies'); + return path.join(path.dirname(Storage.getSystemSettingsPath()), 'policies'); } static getGlobalTempDir(): string { diff --git a/packages/core/src/policy/config.test.ts b/packages/core/src/policy/config.test.ts index 620cdd8500..650d2a0cc6 100644 --- a/packages/core/src/policy/config.test.ts +++ b/packages/core/src/policy/config.test.ts @@ -10,14 +10,9 @@ import nodePath from 'node:path'; import type { PolicySettings } from './types.js'; import { ApprovalMode, PolicyDecision, InProcessCheckerType } from './types.js'; -import { isDirectorySecure } from '../utils/security.js'; vi.unmock('../config/storage.js'); -vi.mock('../utils/security.js', () => ({ - isDirectorySecure: vi.fn().mockResolvedValue({ secure: true }), -})); - afterEach(() => { vi.clearAllMocks(); vi.restoreAllMocks(); @@ -35,53 +30,7 @@ describe('createPolicyEngineConfig', () => { vi.spyOn(Storage, 'getSystemPoliciesDir').mockReturnValue( '/non/existent/system/policies', ); - // Reset security check to default secure - vi.mocked(isDirectorySecure).mockResolvedValue({ secure: true }); }); - - it('should filter out insecure system policy directories', async () => { - const { Storage } = await import('../config/storage.js'); - const systemPolicyDir = '/insecure/system/policies'; - vi.spyOn(Storage, 'getSystemPoliciesDir').mockReturnValue(systemPolicyDir); - - vi.mocked(isDirectorySecure).mockImplementation(async (path: string) => { - if (nodePath.resolve(path) === nodePath.resolve(systemPolicyDir)) { - return { secure: false, reason: 'Insecure directory' }; - } - return { secure: true }; - }); - - // We need to spy on loadPoliciesFromToml to verify which directories were passed - // But it is not exported from config.js, it is imported. - // We can spy on the module it comes from. - const tomlLoader = await import('./toml-loader.js'); - const loadPoliciesSpy = vi.spyOn(tomlLoader, 'loadPoliciesFromToml'); - loadPoliciesSpy.mockResolvedValue({ - rules: [], - checkers: [], - errors: [], - }); - - const { createPolicyEngineConfig } = await import('./config.js'); - const settings: PolicySettings = {}; - - await createPolicyEngineConfig( - settings, - ApprovalMode.DEFAULT, - '/tmp/mock/default/policies', - ); - - // Verify loadPoliciesFromToml was called - expect(loadPoliciesSpy).toHaveBeenCalled(); - const calledDirs = loadPoliciesSpy.mock.calls[0][0]; - - // The system directory should NOT be in the list - expect(calledDirs).not.toContain(systemPolicyDir); - // But other directories (user, default) should be there - expect(calledDirs).toContain('/non/existent/user/policies'); - expect(calledDirs).toContain('/tmp/mock/default/policies'); - }); - it('should return ASK_USER for write tools and ALLOW for read-only tools by default', async () => { const actualFs = await vi.importActual( diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index ca641d09ea..0da61a3604 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -30,8 +30,6 @@ import { debugLogger } from '../utils/debugLogger.js'; import { SHELL_TOOL_NAMES } from '../utils/shell-utils.js'; import { SHELL_TOOL_NAME } from '../tools/tool-names.js'; -import { isDirectorySecure } from '../utils/security.js'; - const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); export const DEFAULT_CORE_POLICIES_DIR = path.join(__dirname, 'policies'); @@ -115,47 +113,19 @@ export function formatPolicyError(error: PolicyFileError): string { return message; } -/** - * Filters out insecure policy directories (specifically the system policy directory). - * Emits warnings if insecure directories are found. - */ -async function filterSecurePolicyDirectories( - dirs: string[], -): Promise { - const systemPoliciesDir = path.resolve(Storage.getSystemPoliciesDir()); - - const results = await Promise.all( - dirs.map(async (dir) => { - // Only check security for system policies - if (path.resolve(dir) === systemPoliciesDir) { - const { secure, reason } = await isDirectorySecure(dir); - if (!secure) { - const msg = `Security Warning: Skipping system policies from ${dir}: ${reason}`; - coreEvents.emitFeedback('warning', msg); - return null; - } - } - return dir; - }), - ); - - return results.filter((dir): dir is string => dir !== null); -} - export async function createPolicyEngineConfig( settings: PolicySettings, approvalMode: ApprovalMode, defaultPoliciesDir?: string, ): Promise { const policyDirs = getPolicyDirectories(defaultPoliciesDir); - const securePolicyDirs = await filterSecurePolicyDirectories(policyDirs); // Load policies from TOML files const { rules: tomlRules, checkers: tomlCheckers, errors, - } = await loadPoliciesFromToml(securePolicyDirs, (dir) => + } = await loadPoliciesFromToml(policyDirs, (dir) => getPolicyTier(dir, defaultPoliciesDir), ); diff --git a/packages/core/src/utils/security.test.ts b/packages/core/src/utils/security.test.ts deleted file mode 100644 index aef11ca753..0000000000 --- a/packages/core/src/utils/security.test.ts +++ /dev/null @@ -1,189 +0,0 @@ -/** - * @license - * Copyright 2026 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ -import { describe, it, expect, vi, afterEach } from 'vitest'; -import { isDirectorySecure } from './security.js'; -import * as fs from 'node:fs/promises'; -import { constants, type Stats } from 'node:fs'; -import * as os from 'node:os'; -import { spawnAsync } from './shell-utils.js'; - -vi.mock('node:fs/promises'); -vi.mock('node:fs'); -vi.mock('node:os'); -vi.mock('./shell-utils.js', () => ({ - spawnAsync: vi.fn(), -})); - -describe('isDirectorySecure', () => { - afterEach(() => { - vi.clearAllMocks(); - }); - - it('returns secure=true on Windows if ACL check passes', async () => { - vi.spyOn(os, 'platform').mockReturnValue('win32'); - vi.mocked(fs.stat).mockResolvedValue({ - isDirectory: () => true, - } as unknown as Stats); - vi.mocked(spawnAsync).mockResolvedValue({ stdout: '', stderr: '' }); - - const result = await isDirectorySecure('C:\\Some\\Path'); - expect(result.secure).toBe(true); - expect(spawnAsync).toHaveBeenCalledWith( - 'powershell', - expect.arrayContaining(['-Command', expect.stringContaining('Get-Acl')]), - ); - }); - - it('returns secure=false on Windows if ACL check fails', async () => { - vi.spyOn(os, 'platform').mockReturnValue('win32'); - vi.mocked(fs.stat).mockResolvedValue({ - isDirectory: () => true, - } as unknown as Stats); - vi.mocked(spawnAsync).mockResolvedValue({ - stdout: 'BUILTIN\\Users', - stderr: '', - }); - - const result = await isDirectorySecure('C:\\Some\\Path'); - - expect(result.secure).toBe(false); - - expect(result.reason).toBe( - "Directory 'C:\\Some\\Path' is insecure. The following user groups have write permissions: BUILTIN\\Users. To fix this, remove Write and Modify permissions for these groups from the directory's ACLs.", - ); - }); - - it('returns secure=false on Windows if spawnAsync fails', async () => { - vi.spyOn(os, 'platform').mockReturnValue('win32'); - - vi.mocked(fs.stat).mockResolvedValue({ - isDirectory: () => true, - } as unknown as Stats); - - vi.mocked(spawnAsync).mockRejectedValue( - new Error('PowerShell is not installed'), - ); - - const result = await isDirectorySecure('C:\\Some\\Path'); - - expect(result.secure).toBe(false); - - expect(result.reason).toBe( - "A security check for the system policy directory 'C:\\Some\\Path' failed and could not be completed. Please file a bug report. Original error: PowerShell is not installed", - ); - }); - - it('returns secure=true if directory does not exist (ENOENT)', async () => { - vi.spyOn(os, 'platform').mockReturnValue('linux'); - - const error = new Error('ENOENT'); - - Object.assign(error, { code: 'ENOENT' }); - - vi.mocked(fs.stat).mockRejectedValue(error); - - const result = await isDirectorySecure('/some/path'); - - expect(result.secure).toBe(true); - }); - - it('returns secure=false if path is not a directory', async () => { - vi.spyOn(os, 'platform').mockReturnValue('linux'); - - vi.mocked(fs.stat).mockResolvedValue({ - isDirectory: () => false, - - uid: 0, - - mode: 0o700, - } as unknown as Stats); - - const result = await isDirectorySecure('/some/file'); - - expect(result.secure).toBe(false); - - expect(result.reason).toBe('Not a directory'); - }); - - it('returns secure=false if not owned by root (uid 0) on POSIX', async () => { - vi.spyOn(os, 'platform').mockReturnValue('linux'); - - vi.mocked(fs.stat).mockResolvedValue({ - isDirectory: () => true, - - uid: 1000, // Non-root - - mode: 0o755, - } as unknown as Stats); - - const result = await isDirectorySecure('/some/path'); - - expect(result.secure).toBe(false); - - expect(result.reason).toBe( - 'Directory \'/some/path\' is not owned by root (uid 0). Current uid: 1000. To fix this, run: sudo chown root:root "/some/path"', - ); - }); - - it('returns secure=false if writable by group (020) on POSIX', async () => { - vi.spyOn(os, 'platform').mockReturnValue('linux'); - Object.assign(constants, { S_IWGRP: 0o020, S_IWOTH: 0 }); - - vi.mocked(fs.stat).mockResolvedValue({ - isDirectory: () => true, - - uid: 0, - - mode: 0o775, // rwxrwxr-x (group writable) - } as unknown as Stats); - - const result = await isDirectorySecure('/some/path'); - - expect(result.secure).toBe(false); - - expect(result.reason).toBe( - 'Directory \'/some/path\' is writable by group or others (mode: 775). To fix this, run: sudo chmod g-w,o-w "/some/path"', - ); - }); - - it('returns secure=false if writable by others (002) on POSIX', async () => { - vi.spyOn(os, 'platform').mockReturnValue('linux'); - Object.assign(constants, { S_IWGRP: 0, S_IWOTH: 0o002 }); - - vi.mocked(fs.stat).mockResolvedValue({ - isDirectory: () => true, - - uid: 0, - - mode: 0o757, // rwxr-xrwx (others writable) - } as unknown as Stats); - - const result = await isDirectorySecure('/some/path'); - - expect(result.secure).toBe(false); - - expect(result.reason).toBe( - 'Directory \'/some/path\' is writable by group or others (mode: 757). To fix this, run: sudo chmod g-w,o-w "/some/path"', - ); - }); - - it('returns secure=true if owned by root and secure permissions on POSIX', async () => { - vi.spyOn(os, 'platform').mockReturnValue('linux'); - Object.assign(constants, { S_IWGRP: 0, S_IWOTH: 0 }); - - vi.mocked(fs.stat).mockResolvedValue({ - isDirectory: () => true, - - uid: 0, - - mode: 0o755, // rwxr-xr-x - } as unknown as Stats); - - const result = await isDirectorySecure('/some/path'); - - expect(result.secure).toBe(true); - }); -}); diff --git a/packages/core/src/utils/security.ts b/packages/core/src/utils/security.ts deleted file mode 100644 index 448776e1b1..0000000000 --- a/packages/core/src/utils/security.ts +++ /dev/null @@ -1,107 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import * as fs from 'node:fs/promises'; -import { constants } from 'node:fs'; -import * as os from 'node:os'; -import { spawnAsync } from './shell-utils.js'; - -export interface SecurityCheckResult { - secure: boolean; - reason?: string; -} - -/** - * Verifies if a directory is secure (owned by root and not writable by others). - * - * @param dirPath The path to the directory to check. - * @returns A promise that resolves to a SecurityCheckResult. - */ -export async function isDirectorySecure( - dirPath: string, -): Promise { - try { - const stats = await fs.stat(dirPath); - - if (!stats.isDirectory()) { - return { secure: false, reason: 'Not a directory' }; - } - - if (os.platform() === 'win32') { - try { - // Check ACLs using PowerShell to ensure standard users don't have write access - const escapedPath = dirPath.replace(/'/g, "''"); - const script = ` - $path = '${escapedPath}'; - $acl = Get-Acl -LiteralPath $path; - $rules = $acl.Access | Where-Object { - $_.AccessControlType -eq 'Allow' -and - (($_.FileSystemRights -match 'Write') -or ($_.FileSystemRights -match 'Modify') -or ($_.FileSystemRights -match 'FullControl')) - }; - $insecureIdentity = $rules | Where-Object { - $_.IdentityReference.Value -match 'Users' -or $_.IdentityReference.Value -eq 'Everyone' - } | Select-Object -ExpandProperty IdentityReference; - Write-Output ($insecureIdentity -join ', '); - `; - - const { stdout } = await spawnAsync('powershell', [ - '-NoProfile', - '-NonInteractive', - '-Command', - script, - ]); - - const insecureGroups = stdout.trim(); - if (insecureGroups) { - return { - secure: false, - reason: `Directory '${dirPath}' is insecure. The following user groups have write permissions: ${insecureGroups}. To fix this, remove Write and Modify permissions for these groups from the directory's ACLs.`, - }; - } - - return { secure: true }; - } catch (error) { - return { - secure: false, - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - reason: `A security check for the system policy directory '${dirPath}' failed and could not be completed. Please file a bug report. Original error: ${(error as Error).message}`, - }; - } - } - - // POSIX checks - // Check ownership: must be root (uid 0) - if (stats.uid !== 0) { - return { - secure: false, - reason: `Directory '${dirPath}' is not owned by root (uid 0). Current uid: ${stats.uid}. To fix this, run: sudo chown root:root "${dirPath}"`, - }; - } - - // Check permissions: not writable by group (S_IWGRP) or others (S_IWOTH) - const mode = stats.mode; - if ((mode & (constants.S_IWGRP | constants.S_IWOTH)) !== 0) { - return { - secure: false, - reason: `Directory '${dirPath}' is writable by group or others (mode: ${mode.toString( - 8, - )}). To fix this, run: sudo chmod g-w,o-w "${dirPath}"`, - }; - } - - return { secure: true }; - } catch (error) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - return { secure: true }; - } - return { - secure: false, - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - reason: `Failed to access directory: ${(error as Error).message}`, - }; - } -}