diff --git a/lib/BoundCluster.js b/lib/BoundCluster.js index 63031bd..18908af 100644 --- a/lib/BoundCluster.js +++ b/lib/BoundCluster.js @@ -4,6 +4,7 @@ let { debug } = require('./util'); const { ZCLDataType } = require('./zclTypes'); const { getLogId, getPropertyDescriptor, ZCLError } = require('./util'); const Cluster = require('./Cluster'); +const parseCommandArgs = require('./util/parseCommandArgs'); debug = debug.extend('bound-cluster'); @@ -364,7 +365,8 @@ class BoundCluster { .pop(); if (command) { - const args = command.args ? command.args.fromBuffer(frame.data, 0) : undefined; + // Validate payload length before parsing; see lib/util/parseCommandArgs.js. + const args = command.args ? parseCommandArgs(command, frame.data) : undefined; if (this[command.name]) { debug(this.logId, 'received command', command.name, args); diff --git a/lib/Cluster.js b/lib/Cluster.js index 14277f4..537ccdb 100644 --- a/lib/Cluster.js +++ b/lib/Cluster.js @@ -4,6 +4,7 @@ const EventEmitter = require('node:events'); let { debug } = require('./util'); const { getLogId, ZCLError } = require('./util'); +const parseCommandArgs = require('./util/parseCommandArgs'); debug = debug.extend('cluster'); @@ -862,8 +863,10 @@ class Cluster extends EventEmitter { if (command) { const handlerName = `on${command.name.charAt(0).toUpperCase()}${command.name.slice(1)}`; - // Parse the command arguments - const args = command.args ? command.args.fromBuffer(frame.data, 0) : undefined; + // Parse the command arguments. Validate the payload length first so that + // truncated frames are rejected instead of silently parsed with zero-filled + // fields (see lib/util/parseCommandArgs.js for details). + const args = command.args ? parseCommandArgs(command, frame.data) : undefined; debug(this.logId, 'received frame', command.name, args); diff --git a/lib/util/parseCommandArgs.js b/lib/util/parseCommandArgs.js new file mode 100644 index 0000000..d1b3948 --- /dev/null +++ b/lib/util/parseCommandArgs.js @@ -0,0 +1,81 @@ +'use strict'; + +const { ZCLError } = require('./index'); + +/** + * Parse a ZCL command's argument payload with defensive length validation. + * + * The underlying parser primitives in `@athombv/data-types` silently fall back + * to zero-filled values when the source buffer is shorter than the declared + * field length. For example, an empty payload parsed against a fixed-size + * struct produces an object where every numeric field is 0 and every bitmap + * field has all bits clear. This is unsafe for alarm-routing clusters (IAS + * Zone) and for status-bearing response frames, where the zero values happen + * to mean "all clear" / "SUCCESS". + * + * This helper rejects truncated payloads up-front by throwing a ZCLError with + * status `MALFORMED_COMMAND`, which `Endpoint.handleFrame` converts into a + * proper default-response frame back to the sender. + * + * @param {object} command - Cluster command definition (with `args` Struct). + * @param {Buffer} data - Frame payload bytes. + * @returns {object} Parsed args instance. + * @throws {ZCLError} With zclStatus `MALFORMED_COMMAND` when parsing fails or + * the payload is shorter than the args struct's minimum byte length. + */ +/** + * Minimum payload length we'll accept before invoking the parser. + * + * For commands declared with `encodeMissingFieldsBehavior: 'skip'`, trailing + * fields may be intentionally omitted (e.g. OTA `hardwareVersion` is only + * present when its `fieldControl` bit is set). We can't derive the strict + * minimum from the struct alone, but every such command in practice has a + * mandatory leading discriminator (status, fieldControl, payloadType) that + * determines which subsequent fields are present. Without that leading byte, + * the parsed result is meaningless; with it, the parser correctly omits + * optional trailing fields. + * + * So for 'skip' commands we require at least the first field's byte width. + * For non-'skip' commands we require the full fixed-portion length. + * + * @param {object} argsType - The ZCLStruct class for this command's args. + * @param {boolean} skipMode - Whether the command uses `encodeMissingFieldsBehavior: 'skip'`. + * @returns {number} Minimum byte length the payload must carry. + */ +function computeMinLength(argsType, skipMode) { + if (!skipMode) { + // Struct.length convention: + // > 0 : exact byte count for an all-fixed-size struct + // < 0 : negative of the fixed-portion size for a varsize struct + // = 0 : no payload required + return Math.abs(argsType.length); + } + + const firstField = Object.values(argsType.fields)[0]; + if (!firstField) return 0; + // Fixed-width field's length is a positive integer; varsize is negative or 0. + // For varsize first fields (uncommon for 'skip' commands), require at least + // 1 byte so the length prefix is present. + return firstField.length > 0 ? firstField.length : 1; +} + +function parseCommandArgs(command, data) { + const argsType = command.args; + const skipMode = command.encodeMissingFieldsBehavior === 'skip'; + + const minLength = computeMinLength(argsType, skipMode); + if (data.length < minLength) { + throw new ZCLError('MALFORMED_COMMAND'); + } + + try { + return argsType.fromBuffer(data, 0); + } catch { + // The parser threw mid-walk (e.g. a varsize length prefix overran the + // payload). Treat as malformed rather than letting the raw RangeError + // propagate as a generic FAILURE. + throw new ZCLError('MALFORMED_COMMAND'); + } +} + +module.exports = parseCommandArgs; diff --git a/lib/zclFrames.js b/lib/zclFrames.js index 9b0c394..8491b37 100644 --- a/lib/zclFrames.js +++ b/lib/zclFrames.js @@ -1,6 +1,7 @@ 'use strict'; const { ZCLDataType, ZCLDataTypes, ZCLStruct } = require('./zclTypes'); +const { ZCLError } = require('./util'); const ZCLFrameControlBitmap = ZCLDataTypes.map8( 'clusterSpecific', @@ -84,6 +85,16 @@ const ZCLAttributeDataRecord = (withStatus, attributes) => res.name = attributes[res.id].name || `unknown_attr_${res.id}`; } + // Validate that enough bytes remain for the value field before parsing. + // Without this, fixed-width primitives in @athombv/data-types silently + // return 0 on short input - which for IAS Zone `zoneStatus` (map16) + // produces an all-bits-false bitmap that looks like "all alarms clear", + // and for any status-bearing attribute promotes a truncated value into + // a SUCCESS-looking enum. + if (DataType.length > 0 && buf.length - i < DataType.length) { + throw new ZCLError('MALFORMED_COMMAND'); + } + const entry = DataType.fromBuffer(buf, i, true); if (DataType.length > 0) { i += DataType.length; diff --git a/test/testMalformedFrames.js b/test/testMalformedFrames.js new file mode 100644 index 0000000..6d6f999 --- /dev/null +++ b/test/testMalformedFrames.js @@ -0,0 +1,100 @@ +'use strict'; + +const { describe, it, mock } = require('node:test'); +const assert = require('node:assert'); +const BoundCluster = require('../lib/BoundCluster'); +const IASZoneCluster = require('../lib/clusters/iasZone'); +const OTACluster = require('../lib/clusters/ota'); +const { createVirtualDevice, frameToClient } = require('./util/virtualDevice'); + +const DEFAULT_RESPONSE = 0x0b; +const REPORT_ATTRIBUTES = 0x0a; +const MALFORMED_COMMAND = 0x80; +const MAP16 = 0x19; +const ZCL_HEADER_SIZE = 3; + +/** The payload, [cmdId, status], of the Default Response `from` sent for `cmdId`. */ +function defaultResponse(frames, from, cmdId) { + const record = frames.find( + ({ from: sender, frame }) => + sender === from && + frame.cmdId === DEFAULT_RESPONSE && + !frame.frameControl.clusterSpecific && + frame.data[0] === cmdId, + ); + return record && record.frame.data; +} + +function createIasZoneSensor() { + return createVirtualDevice({ + endpoints: [{ endpointId: 1, inputClusters: [IASZoneCluster.ID] }], + }); +} + +describe('Malformed frames', function () { + it('rejects a truncated command to a Cluster', async function () { + const { zclNode, virtualDevice, frames, settle } = createIasZoneSensor(); + const handler = mock.fn(); + zclNode.endpoints[1].clusters.iasZone.onZoneStatusChangeNotification = handler; + const { id } = IASZoneCluster.COMMANDS.zoneStatusChangeNotification; + + // zoneStatus only, the other 4 of 6 bytes are missing + const frame = frameToClient({ cmdId: id, clusterSpecific: true, data: Buffer.from([1, 0]) }); + await virtualDevice.sendFrame(1, IASZoneCluster.ID, frame); + await settle(); + + assert.strictEqual(handler.mock.callCount(), 0); + assert.deepStrictEqual( + defaultResponse(frames, 'zclNode', id), + Buffer.from([id, MALFORMED_COMMAND]), + ); + }); + + it('rejects an attribute report with a truncated value', async function () { + const { zclNode, virtualDevice, frames, settle } = createIasZoneSensor(); + const onZoneStatus = mock.fn(); + zclNode.endpoints[1].clusters.iasZone.on('attr.zoneStatus', onZoneStatus); + + // zoneStatus is map16, only 1 of its 2 value bytes is sent + const { id } = IASZoneCluster.ATTRIBUTES.zoneStatus; + const frame = frameToClient({ cmdId: REPORT_ATTRIBUTES, data: Buffer.from([id, 0, MAP16, 1]) }); + await virtualDevice.sendFrame(1, IASZoneCluster.ID, frame); + await settle(); + + assert.strictEqual(onZoneStatus.mock.callCount(), 0); + assert.deepStrictEqual( + defaultResponse(frames, 'zclNode', REPORT_ATTRIBUTES), + Buffer.from([REPORT_ATTRIBUTES, MALFORMED_COMMAND]), + ); + }); + + it('rejects an empty command to a BoundCluster that may skip trailing fields', async function () { + const { zclNode, virtualDevice, intercept } = createVirtualDevice({ + endpoints: [{ endpointId: 1, outputClusters: [OTACluster.ID] }], + }); + const handler = mock.fn(async () => ({ status: 'NO_IMAGE_AVAILABLE' })); + zclNode.endpoints[1].bind( + 'ota', + new (class extends BoundCluster { + queryNextImageRequest(...args) { + return handler(...args); + } + })(), + ); + // Keep the header, drop the payload + intercept(({ from, raw }) => + from === 'virtualDevice' ? [raw.subarray(0, ZCL_HEADER_SIZE)] : undefined, + ); + + await assert.rejects( + virtualDevice.endpoints[1].clusters.ota.queryNextImageRequest({ + fieldControl: 0, + manufacturerCode: 0x1234, + imageType: 1, + fileVersion: 1, + }), + /^Error: MALFORMED_COMMAND$/, + ); + assert.strictEqual(handler.mock.callCount(), 0); + }); +});