From 770800910387d34b86cd11df447304e149dd27bb Mon Sep 17 00:00:00 2001 From: Yuna Seol Date: Mon, 26 Jan 2026 19:27:49 -0500 Subject: [PATCH] fix(security): enforce strict policy directory permissions (#17353) Co-authored-by: Yuna Seol --- docs/core/policy-engine.md | 32 ++++ packages/core/src/config/storage.test.ts | 54 ++++++- 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 | 104 +++++++++++++ 7 files changed, 472 insertions(+), 10 deletions(-) create mode 100644 packages/core/src/utils/security.test.ts create mode 100644 packages/core/src/utils/security.ts diff --git a/docs/core/policy-engine.md b/docs/core/policy-engine.md index 6811b8b6fb..30b9a0ac93 100644 --- a/docs/core/policy-engine.md +++ b/docs/core/policy-engine.md @@ -146,6 +146,38 @@ 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 b0b4fa8791..a635bcbf14 100644 --- a/packages/core/src/config/storage.test.ts +++ b/packages/core/src/config/storage.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, afterEach } from 'vitest'; import * as os from 'node:os'; import * as path from 'node:path'; @@ -85,3 +85,55 @@ describe('Storage – additional helpers', () => { expect(storage.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 1f317d4ddf..fc5006d04e 100644 --- a/packages/core/src/config/storage.ts +++ b/packages/core/src/config/storage.ts @@ -74,21 +74,25 @@ 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']; } - 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'; - } + return path.join(Storage.getSystemConfigDir(), 'settings.json'); } static getSystemPoliciesDir(): string { - return path.join(path.dirname(Storage.getSystemSettingsPath()), 'policies'); + return path.join(Storage.getSystemConfigDir(), 'policies'); } static getGlobalTempDir(): string { diff --git a/packages/core/src/policy/config.test.ts b/packages/core/src/policy/config.test.ts index b6dc71c71b..7b310027e0 100644 --- a/packages/core/src/policy/config.test.ts +++ b/packages/core/src/policy/config.test.ts @@ -10,6 +10,11 @@ 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.mock('../utils/security.js', () => ({ + isDirectorySecure: vi.fn().mockResolvedValue({ secure: true }), +})); afterEach(() => { vi.clearAllMocks(); @@ -28,7 +33,53 @@ 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 ccd2df7ec2..7f6f4d9f3d 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -29,6 +29,8 @@ 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'); @@ -112,19 +114,47 @@ 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(policyDirs, (dir) => + } = await loadPoliciesFromToml(securePolicyDirs, (dir) => getPolicyTier(dir, defaultPoliciesDir), ); diff --git a/packages/core/src/utils/security.test.ts b/packages/core/src/utils/security.test.ts new file mode 100644 index 0000000000..aef11ca753 --- /dev/null +++ b/packages/core/src/utils/security.test.ts @@ -0,0 +1,189 @@ +/** + * @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 new file mode 100644 index 0000000000..cd08a34dac --- /dev/null +++ b/packages/core/src/utils/security.ts @@ -0,0 +1,104 @@ +/** + * @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, + 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) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return { secure: true }; + } + return { + secure: false, + reason: `Failed to access directory: ${(error as Error).message}`, + }; + } +}