Skip to content

Commit ddfe782

Browse files
committed
fix: validate OTA firmware paths to prevent traversal
payload.hex.file_name on bridge/request/device/ota_update/{update, schedule} flowed unvalidated into join(baseDir, fileName), letting MQTT clients write attacker-controlled firmware bytes to arbitrary paths. Resolve the target via the shared resolveSafeChildPath helper and reject anything that isn't a direct child of <data>/ota. Wrap the writeFirmwareHexToDataDir calls in try/catch so the validation throw surfaces as an error response on the bridge topic instead of crashing the handler. Replace the unschedule rmSync prefix check with the same helper. A string-based startsWith with path.sep is bypassable by storing a traversed payload.url ("<data>/ota/../../etc/passwd") at schedule time: the prefix matches but the syscall resolves outside the OTA dir. resolveSafeChildPath normalizes via path.resolve and rejects anything whose parent isn't <data>/ota. Includes regression tests for ".." / "." / "../escape.hex" file_name on update + schedule, and a traversal-url unschedule case.
1 parent 99d41a0 commit ddfe782

2 files changed

Lines changed: 70 additions & 13 deletions

File tree

lib/extension/otaUpdate.ts

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -31,18 +31,13 @@ export interface UpdatePayload {
3131
* Write to `dataDir` and return created path
3232
*/
3333
function writeFirmwareHexToDataDir(hex: string, fileName: string | undefined, deviceIeee: string): string {
34-
if (!fileName) {
35-
fileName = `${deviceIeee}_${Date.now()}`;
36-
}
37-
3834
const baseDir = dataDir.joinPath("ota");
35+
const filePath = fileName ? utils.resolveSafeChildPath(baseDir, fileName) : join(baseDir, `${deviceIeee}_${Date.now()}`);
3936

4037
if (!existsSync(baseDir)) {
4138
mkdirSync(baseDir, {recursive: true});
4239
}
4340

44-
const filePath = join(baseDir, fileName);
45-
4641
writeFileSync(filePath, Buffer.from(hex, "hex"));
4742

4843
return filePath;
@@ -340,8 +335,13 @@ export default class OTAUpdate extends Extension {
340335
if (payload.hex) {
341336
assert(payload.hex.data);
342337

343-
// write to `dataDir` and pass created path as source URL
344-
source.url = writeFirmwareHexToDataDir(payload.hex.data, payload.hex.file_name, device.ieeeAddr);
338+
try {
339+
// write to `dataDir` and pass created path as source URL
340+
source.url = writeFirmwareHexToDataDir(payload.hex.data, payload.hex.file_name, device.ieeeAddr);
341+
} catch (e) {
342+
error = (e as Error).message;
343+
break;
344+
}
345345
} else if (payload.url) {
346346
source.url = payload.url;
347347
} else if (!device.definition?.ota) {
@@ -411,8 +411,13 @@ export default class OTAUpdate extends Extension {
411411
if (payload.hex) {
412412
assert(payload.hex.data);
413413

414-
// write to `dataDir` and pass created path as source URL
415-
source.url = writeFirmwareHexToDataDir(payload.hex.data, payload.hex.file_name, device.ieeeAddr);
414+
try {
415+
// write to `dataDir` and pass created path as source URL
416+
source.url = writeFirmwareHexToDataDir(payload.hex.data, payload.hex.file_name, device.ieeeAddr);
417+
} catch (e) {
418+
error = (e as Error).message;
419+
break;
420+
}
416421
} else if (payload.url) {
417422
source.url = payload.url;
418423
} else if (!device.definition?.ota) {
@@ -435,8 +440,14 @@ export default class OTAUpdate extends Extension {
435440
}
436441

437442
case "unschedule": {
438-
if (device.zh.scheduledOta?.url?.startsWith(dataDir.joinPath("ota"))) {
439-
rmSync(device.zh.scheduledOta.url, {force: true});
443+
const url = device.zh.scheduledOta?.url;
444+
445+
if (url) {
446+
try {
447+
rmSync(utils.resolveSafeChildPath(dataDir.joinPath("ota"), url), {force: true});
448+
} catch {
449+
// not a safe child of OTA dir (remote URL or traversal) — skip
450+
}
440451
}
441452

442453
device.zh.unscheduleOta();

test/extensions/otaUpdate.test.ts

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import {flushPromises} from "../mocks/utils";
88
import {devices, events as mockZHEvents} from "../mocks/zigbeeHerdsman";
99

1010
import {join} from "node:path";
11-
import {existsSync, readFileSync, rmSync} from "node:fs";
11+
import {existsSync, mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync} from "node:fs";
1212
import stringify from "json-stable-stringify-without-jsonify";
1313
import {Controller} from "../../lib/controller";
1414
import OTAUpdate from "../../lib/extension/otaUpdate";
@@ -409,6 +409,31 @@ describe("Extension: OTAUpdate", () => {
409409
expect(readFileSync(saveFilePath)).toStrictEqual(Buffer.from([0x01, 0x02, 0x03]));
410410
});
411411

412+
it.each([
413+
["..", "update"],
414+
[".", "update"],
415+
["../escape.hex", "update"],
416+
["..", "schedule"],
417+
["../escape.hex", "schedule"],
418+
])("rejects %s file_name on %s", async (badName, op) => {
419+
const otaDir = join(data.mockDir, "ota");
420+
const before = existsSync(otaDir) ? readdirSync(otaDir) : [];
421+
422+
mockMQTTEvents.message(
423+
`zigbee2mqtt/bridge/request/device/ota_update/${op}`,
424+
stringify({id: "bulb", hex: {data: "010203", file_name: badName}}),
425+
);
426+
await flushPromises();
427+
428+
expect(mockMQTTPublishAsync).toHaveBeenCalledWith(
429+
`zigbee2mqtt/bridge/response/device/ota_update/${op}`,
430+
expect.stringContaining(`Invalid file name '${badName}'`),
431+
{},
432+
);
433+
const after = existsSync(otaDir) ? readdirSync(otaDir) : [];
434+
expect(after).toStrictEqual(before);
435+
});
436+
412437
it("updates with override data settings", async () => {
413438
devices.bulb.updateOta.mockImplementationOnce(
414439
async (_source, _requestPayload, _requestTsn, _extraMetas, _onProgress, _dataSettings, _endpoint) => {
@@ -1212,6 +1237,27 @@ describe("Extension: OTAUpdate", () => {
12121237
expect(devices.bulb.scheduledOta).toStrictEqual(undefined);
12131238
});
12141239

1240+
it("does not rmSync outside ota dir on unschedule with traversal url", async () => {
1241+
const otaDir = join(data.mockDir, "ota");
1242+
mkdirSync(otaDir, {recursive: true});
1243+
const sentinelFile = join(data.mockDir, "canary.txt");
1244+
writeFileSync(sentinelFile, "do-not-delete");
1245+
1246+
// schedule with a payload.url that escapes the ota dir via traversal
1247+
const traversalUrl = join(otaDir, "..", "canary.txt");
1248+
mockMQTTEvents.message("zigbee2mqtt/bridge/request/device/ota_update/schedule", stringify({id: "bulb", url: traversalUrl}));
1249+
await flushPromises();
1250+
expect(devices.bulb.scheduledOta).toStrictEqual({url: traversalUrl, downgrade: false});
1251+
1252+
mockMQTTEvents.message("zigbee2mqtt/bridge/request/device/ota_update/unschedule", stringify({id: "bulb"}));
1253+
await flushPromises();
1254+
1255+
expect(existsSync(sentinelFile)).toStrictEqual(true);
1256+
expect(devices.bulb.scheduledOta).toStrictEqual(undefined);
1257+
1258+
rmSync(sentinelFile);
1259+
});
1260+
12151261
it("responds with NO_IMAGE_AVAILABLE when not supporting OTA", async () => {
12161262
const device = devices.HGZB04D;
12171263
const data = {imageType: 12382};

0 commit comments

Comments
 (0)