Skip to content

Commit fe3092f

Browse files
jesussamuel-bytejesussamuel-byte
committed
fix(config): prevent insecure system-wide configuration loading
## Summary Fixes insecure system-wide configuration loading on Windows and POSIX systems that could enable local privilege escalation and cross-user arbitrary command execution. ## Details - Implemented isPathSecureSync and isFileAndDirectorySecureSync in @google/gemini-cli-core to validate directory and file security before loading system-level configurations. - On Windows: Validates ACLs to ensure standard users/groups (e.g. Users, Authenticated Users, Everyone) do not have Write, Modify, or FullControl permissions. - On POSIX: Verifies root ownership (uid 0) and verifies that group and others do not have write permissions (S_IWGRP, S_IWOTH). Checks symlinks and resolved targets. - Updated _doLoadSettings in @google/gemini-cli to validate system configuration files (systemSettingsPath and systemDefaultsPath) before loading; securely skips insecure files and logs warnings. - Added comprehensive unit tests in packages/core/src/utils/security.test.ts and packages/cli/src/config/settings.test.ts. ## How to Validate 1. Run targeted security and settings unit tests: ```bash npx vitest run packages/core/src/utils/security.test.ts packages/cli/src/config/settings.test.ts ``` 2. Run full lint and typecheck: ```bash npm run lint:ci && npm run typecheck ```
1 parent 3c311be commit fe3092f

5 files changed

Lines changed: 902 additions & 49 deletions

File tree

‎packages/cli/src/config/settings.test.ts‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ import {
8585
AuthType,
8686
type MCPServerConfig,
8787
} from '@google/gemini-cli-core';
88+
import * as core from '@google/gemini-cli-core';
8889
import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js';
8990
import {
9091
getSettingsSchema,
@@ -3530,6 +3531,106 @@ MALICIOUS_VAR=allowed-because-trusted
35303531
);
35313532
});
35323533
});
3534+
3535+
describe('system configuration security', () => {
3536+
beforeEach(() => {
3537+
vi.mocked(isWorkspaceTrusted).mockReturnValue({
3538+
isTrusted: true,
3539+
source: 'file',
3540+
});
3541+
});
3542+
3543+
it('should skip system-defaults.json when insecure and record a warning', () => {
3544+
resetSettingsCacheForTesting();
3545+
vi.mocked(fs.existsSync).mockImplementation(
3546+
(p) => String(p) === getSystemDefaultsPath(),
3547+
);
3548+
vi.mocked(fs.readFileSync).mockImplementation((p) => {
3549+
if (String(p) === getSystemDefaultsPath()) {
3550+
return JSON.stringify({
3551+
hooks: {
3552+
onSessionStart: { command: 'malicious-command.exe' },
3553+
},
3554+
});
3555+
}
3556+
return '';
3557+
});
3558+
vi.spyOn(core, 'isFileAndDirectorySecureSync').mockReturnValue({
3559+
secure: false,
3560+
reason:
3561+
'Directory is insecure. User group Users has write permissions.',
3562+
});
3563+
3564+
const settings = loadSettings(MOCK_WORKSPACE_DIR);
3565+
3566+
expect(settings.systemDefaults.settings).toEqual({});
3567+
expect(settings.errors).toEqual(
3568+
expect.arrayContaining([
3569+
expect.objectContaining({
3570+
message: expect.stringContaining('Skipping system defaults file'),
3571+
severity: 'warning',
3572+
}),
3573+
]),
3574+
);
3575+
});
3576+
3577+
it('should skip system settings.json when insecure and record a warning', () => {
3578+
resetSettingsCacheForTesting();
3579+
vi.mocked(fs.existsSync).mockImplementation(
3580+
(p) => String(p) === getSystemSettingsPath(),
3581+
);
3582+
vi.mocked(fs.readFileSync).mockImplementation((p) => {
3583+
if (String(p) === getSystemSettingsPath()) {
3584+
return JSON.stringify({
3585+
hooks: {
3586+
onSessionStart: { command: 'malicious-command.exe' },
3587+
},
3588+
});
3589+
}
3590+
return '';
3591+
});
3592+
vi.spyOn(core, 'isFileAndDirectorySecureSync').mockReturnValue({
3593+
secure: false,
3594+
reason: 'File is not owned by root (uid 0).',
3595+
});
3596+
3597+
const settings = loadSettings(MOCK_WORKSPACE_DIR);
3598+
3599+
expect(settings.system.settings).toEqual({});
3600+
expect(settings.errors).toEqual(
3601+
expect.arrayContaining([
3602+
expect.objectContaining({
3603+
message: expect.stringContaining('Skipping system settings file'),
3604+
severity: 'warning',
3605+
}),
3606+
]),
3607+
);
3608+
});
3609+
3610+
it('should load system-defaults.json when secure', () => {
3611+
resetSettingsCacheForTesting();
3612+
vi.mocked(fs.existsSync).mockImplementation(
3613+
(p) => String(p) === getSystemDefaultsPath(),
3614+
);
3615+
vi.mocked(fs.readFileSync).mockImplementation((p) => {
3616+
if (String(p) === getSystemDefaultsPath()) {
3617+
return JSON.stringify({
3618+
ui: { theme: 'corporate-theme' },
3619+
});
3620+
}
3621+
return '';
3622+
});
3623+
vi.spyOn(core, 'isFileAndDirectorySecureSync').mockReturnValue({
3624+
secure: true,
3625+
});
3626+
3627+
const settings = loadSettings(MOCK_WORKSPACE_DIR);
3628+
3629+
expect(settings.systemDefaults.settings).toEqual({
3630+
ui: { theme: 'corporate-theme' },
3631+
});
3632+
});
3633+
});
35333634
});
35343635
});
35353636

‎packages/cli/src/config/settings.ts‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
AuthType,
2222
type AdminControlsSettings,
2323
createCache,
24+
isFileAndDirectorySecureSync,
2425
} from '@google/gemini-cli-core';
2526
import stripJsonComments from 'strip-json-comments';
2627
import { DefaultLight } from '../ui/themes/builtin/light/default-light.js';
@@ -844,8 +845,32 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings {
844845
return { settings: {}, rawSettings: {} };
845846
};
846847

847-
const systemResult = load(systemSettingsPath);
848-
const systemDefaultsResult = load(systemDefaultsPath);
848+
const loadSystemFile = (
849+
filePath: string,
850+
fileLabel: string,
851+
): { settings: Settings; rawSettings: Settings; rawJson?: string } => {
852+
if (!fs.existsSync(filePath)) {
853+
return { settings: {}, rawSettings: {} };
854+
}
855+
856+
const check = isFileAndDirectorySecureSync(filePath);
857+
if (!check.secure) {
858+
settingsErrors.push({
859+
message: `Security Warning: Skipping ${fileLabel} file '${filePath}': ${check.reason}`,
860+
path: filePath,
861+
severity: 'warning',
862+
});
863+
return { settings: {}, rawSettings: {} };
864+
}
865+
866+
return load(filePath);
867+
};
868+
869+
const systemResult = loadSystemFile(systemSettingsPath, 'system settings');
870+
const systemDefaultsResult = loadSystemFile(
871+
systemDefaultsPath,
872+
'system defaults',
873+
);
849874
const userResult = load(USER_SETTINGS_PATH);
850875

851876
let workspaceResult: {
@@ -898,7 +923,7 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings {
898923
);
899924
const isTrusted =
900925
isWorkspaceTrusted(initialTrustCheckSettings as Settings, workspaceDir)
901-
.isTrusted ?? false;
926+
?.isTrusted ?? false;
902927

903928
// Create a temporary merged settings object to pass to loadEnvironment.
904929
const tempMergedSettings = mergeSettings(

‎packages/core/src/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ export * from './utils/path-validator.js';
9797
export * from './utils/atCommandUtils.js';
9898
export * from './utils/retry.js';
9999
export * from './utils/shell-utils.js';
100+
export * from './utils/security.js';
100101
export {
101102
PolicyDecision,
102103
ApprovalMode,

0 commit comments

Comments
 (0)