Skip to content

Commit 86d0274

Browse files
maor-rozenfeldmaor-rozenfeld
committed
Prevent remote code execution in the terraform block
1 parent ce03f39 commit 86d0274

2 files changed

Lines changed: 162 additions & 16 deletions

File tree

‎packages/blocks/terraform/src/lib/hcledit-cli.ts‎

Lines changed: 63 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,26 @@ export interface TerraformResource {
77
type: string;
88
}
99

10+
const SAFE_IDENTIFIER_PATTERN = /^[a-zA-Z0-9_-]+$/;
11+
12+
function validateIdentifier(value: string, fieldName: string): void {
13+
if (!value || !SAFE_IDENTIFIER_PATTERN.test(value)) {
14+
throw new Error(
15+
`${fieldName} contains invalid characters. Only alphanumeric, underscore, and hyphen are allowed. Received: ${value}`,
16+
);
17+
}
18+
}
19+
20+
function escapeShellArgument(value: string): string {
21+
return `'${value.replace(/'/g, `'\\''`)}'`;
22+
}
23+
1024
export async function getResources(
1125
template: string,
1226
): Promise<TerraformResource[]> {
1327
const commandResult = await useTempFile(template, async (filePath) => {
1428
return await executeHclEditCommand(
15-
`-f ${filePath} block get resource | hcledit block list`,
29+
`-f ${escapeShellArgument(filePath)} block get resource | hcledit block list`,
1630
);
1731
});
1832

@@ -35,15 +49,29 @@ export async function getResources(
3549
return [];
3650
}
3751

38-
const resources = commandResult.stdOut.split('\n').map((line) => {
39-
// eslint-disable-next-line @typescript-eslint/no-unused-vars
40-
const [_, type, name] = line.split('.').map((value) => value.trim());
41-
42-
return {
43-
name: name,
44-
type: type,
45-
};
46-
});
52+
const resources = commandResult.stdOut
53+
.split('\n')
54+
.map((line) => line.trim())
55+
.filter((line) => line.length > 0)
56+
.map((line) => {
57+
const [blockType, type, name] = line
58+
.split('.')
59+
.map((value) => value.trim());
60+
61+
if (blockType !== 'resource' || !type || !name) {
62+
throw new Error(
63+
`Unexpected resource format received from hcledit. Line: ${line}`,
64+
);
65+
}
66+
67+
validateIdentifier(type, 'Resource type');
68+
validateIdentifier(name, 'Resource name');
69+
70+
return {
71+
name,
72+
type,
73+
};
74+
});
4775

4876
return resources;
4977
}
@@ -54,13 +82,21 @@ export async function updateResourceProperties(
5482
resourceName: string,
5583
modifications: { propertyName: string; propertyValue: string }[],
5684
): Promise<string> {
85+
validateIdentifier(resourceType, 'Resource type');
86+
validateIdentifier(resourceName, 'Resource name');
87+
88+
modifications.forEach((modification, index) => {
89+
validateIdentifier(modification.propertyName, `Property name at index ${index}`);
90+
});
91+
5792
const providedTemplate = template.trim() as string;
5893

5994
const result = await useTempFile(providedTemplate, async (filePath) => {
6095
const updates = [];
6196
for (const modification of modifications) {
6297
const propertyName = modification.propertyName;
6398
const propertyValue = sanitizePropertyValue(modification.propertyValue);
99+
const safePropertyValue = escapeShellArgument(propertyValue);
64100

65101
const attributeCommand = await getAttributeCommand(
66102
filePath,
@@ -70,7 +106,7 @@ export async function updateResourceProperties(
70106
);
71107

72108
updates.push(
73-
`attribute ${attributeCommand} resource.${resourceType}.${resourceName}.${propertyName} ${propertyValue}`,
109+
`attribute ${attributeCommand} resource.${resourceType}.${resourceName}.${propertyName} ${safePropertyValue}`,
74110
);
75111
}
76112

@@ -88,6 +124,13 @@ export async function updateVariablesFile(
88124
): Promise<string> {
89125
const providedTemplate = template.trim();
90126

127+
modifications.forEach((modification, index) => {
128+
validateIdentifier(
129+
modification.variableName,
130+
`Variable name at index ${index}`,
131+
);
132+
});
133+
91134
return await useTempFile(providedTemplate, async (filePath) => {
92135
const updates = [];
93136

@@ -107,9 +150,12 @@ export async function deleteResource(
107150
resourceType: string,
108151
resourceName: string,
109152
): Promise<string> {
153+
validateIdentifier(resourceType, 'Resource type');
154+
validateIdentifier(resourceName, 'Resource name');
155+
110156
const commandResult = await useTempFile(template, async (filePath) => {
111157
return await executeHclEditCommand(
112-
`-f ${filePath} block rm resource.${resourceType}.${resourceName}`,
158+
`-f ${escapeShellArgument(filePath)} block rm resource.${resourceType}.${resourceName}`,
113159
);
114160
});
115161

@@ -131,7 +177,9 @@ export async function listBlocksCommand(
131177
template: string,
132178
): Promise<CommandResult> {
133179
return await useTempFile(template, async (filePath) => {
134-
return await executeHclEditCommand(`-f ${filePath} block list`);
180+
return await executeHclEditCommand(
181+
`-f ${escapeShellArgument(filePath)} block list`,
182+
);
135183
});
136184
}
137185

@@ -141,7 +189,7 @@ async function getAttributeCommand(
141189
resourceName: string,
142190
propertyName: string,
143191
): Promise<string> {
144-
const command = `-f ${filePath} attribute get resource.${resourceType}.${resourceName}.${propertyName}`;
192+
const command = `-f ${escapeShellArgument(filePath)} attribute get resource.${resourceType}.${resourceName}.${propertyName}`;
145193

146194
const commandResult = await executeHclEditCommand(command);
147195

@@ -160,7 +208,7 @@ async function updateTemplateCommand(
160208
filePath: string,
161209
modifications: string[],
162210
): Promise<string> {
163-
const command = `-f ${filePath} ${modifications.join(' | hcledit ')}`;
211+
const command = `-f ${escapeShellArgument(filePath)} ${modifications.join(' | hcledit ')}`;
164212

165213
const commandResult = await executeHclEditCommand(command);
166214

‎packages/blocks/terraform/test/hcledit-cli.test.ts‎

Lines changed: 99 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,19 @@ describe('Get Resources', () => {
7979
'Failed to execute the command to get resources.',
8080
);
8181
});
82+
83+
test('should throw when resource identifiers contain invalid characters', async () => {
84+
mockExecuteCommand.mockResolvedValue({
85+
exitCode: 0,
86+
stdOut: 'resource.aws_instance.example;rm -rf /',
87+
stdError: '',
88+
});
89+
90+
await expect(getResources(testTemplate)).rejects.toThrow(
91+
'Resource name contains invalid characters',
92+
);
93+
expect(mockExecuteCommand).toHaveBeenCalledTimes(1);
94+
});
8295
});
8396

8497
describe('Update Resource Properties', () => {
@@ -102,6 +115,8 @@ describe('Update Resource Properties', () => {
102115
{ propertyName: 'property_name1', propertyValue: setPropertyValue },
103116
{ propertyName: 'property_name2', propertyValue: appendPropertyValue },
104117
];
118+
const setValueArgument = `'${setPropertyValueExpected}'`;
119+
const appendValueArgument = `'${appendPropertyValueExpected}'`;
105120

106121
mockExecuteCommand
107122
.mockResolvedValueOnce({
@@ -146,7 +161,7 @@ describe('Update Resource Properties', () => {
146161
'/bin/bash',
147162
expect.arrayContaining([
148163
expect.stringContaining(
149-
`attribute set resource.aws_instance.example.property_name1 ${setPropertyValueExpected} | hcledit attribute append resource.aws_instance.example.property_name2 ${appendPropertyValueExpected}`,
164+
`attribute set resource.aws_instance.example.property_name1 ${setValueArgument} | hcledit attribute append resource.aws_instance.example.property_name2 ${appendValueArgument}`,
150165
),
151166
]),
152167
);
@@ -175,6 +190,73 @@ describe('Update Resource Properties', () => {
175190
'Failed to modify the template. {"exitCode":1,"stdOut":"","stdError":"Error"}',
176191
);
177192
});
193+
194+
test('should escape property values to prevent shell injection', async () => {
195+
const modifications = [
196+
{ propertyName: 'property_name1', propertyValue: 'value; $(whoami)' },
197+
];
198+
199+
mockExecuteCommand
200+
.mockResolvedValueOnce({ exitCode: 1, stdOut: '', stdError: 'missing' })
201+
.mockResolvedValueOnce({ exitCode: 0, stdOut: 'result', stdError: '' });
202+
203+
const result = await updateResourceProperties(
204+
testTemplate,
205+
'aws_instance',
206+
'example',
207+
modifications,
208+
);
209+
210+
expect(result).toContain('result');
211+
const executedCommand = mockExecuteCommand.mock.calls[1][1][1];
212+
expect(executedCommand).toContain(
213+
`resource.aws_instance.example.property_name1 '\\"value; $(whoami)\\"'`,
214+
);
215+
});
216+
217+
test.each([
218+
{
219+
description: 'resource type contains invalid characters',
220+
resourceType: 'aws_instance; rm -rf /',
221+
resourceName: 'example',
222+
propertyName: 'property_name1',
223+
expectedMessage: 'Resource type contains invalid characters',
224+
},
225+
{
226+
description: 'resource name contains invalid characters',
227+
resourceType: 'aws_instance',
228+
resourceName: 'example; rm -rf /',
229+
propertyName: 'property_name1',
230+
expectedMessage: 'Resource name contains invalid characters',
231+
},
232+
{
233+
description: 'property name contains invalid characters',
234+
resourceType: 'aws_instance',
235+
resourceName: 'example',
236+
propertyName: 'property;name',
237+
expectedMessage: 'Property name at index 0 contains invalid characters',
238+
},
239+
])(
240+
'should throw when $description',
241+
async ({
242+
description,
243+
resourceType,
244+
resourceName,
245+
propertyName,
246+
expectedMessage,
247+
}) => {
248+
await expect(
249+
updateResourceProperties(
250+
testTemplate,
251+
resourceType,
252+
resourceName,
253+
[{ propertyName, propertyValue: 'value' }],
254+
),
255+
).rejects.toThrow(expectedMessage);
256+
expect(mockExecuteCommand).not.toHaveBeenCalled();
257+
expect(mockWriteFile).not.toHaveBeenCalled();
258+
},
259+
);
178260
});
179261

180262
describe('Delete Resource', () => {
@@ -219,6 +301,14 @@ describe('Delete Resource', () => {
219301
'Failed to modify the template. {"exitCode":1,"stdOut":"","stdError":"Error"}',
220302
);
221303
});
304+
305+
test('should reject unsafe resource identifiers when deleting', async () => {
306+
await expect(
307+
deleteResource(testTemplate, 'aws_instance', 'example; rm -rf /'),
308+
).rejects.toThrow('Resource name contains invalid characters');
309+
expect(mockExecuteCommand).not.toHaveBeenCalled();
310+
expect(mockWriteFile).not.toHaveBeenCalled();
311+
});
222312
});
223313

224314
describe('Update Variables File', () => {
@@ -266,6 +356,14 @@ describe('Update Variables File', () => {
266356
'Failed to modify the template. {"exitCode":1,"stdOut":"","stdError":"err"}',
267357
);
268358
});
359+
360+
test('should reject invalid variable names', async () => {
361+
await expect(
362+
updateVariablesFile('a=1', [{ variableName: 'name; rm -rf /', variableValue: 3 }]),
363+
).rejects.toThrow('Variable name at index 0 contains invalid characters');
364+
expect(mockExecuteCommand).not.toHaveBeenCalled();
365+
expect(mockWriteFile).not.toHaveBeenCalled();
366+
});
269367
});
270368

271369
describe('List Blocks Command', () => {

0 commit comments

Comments
 (0)