Skip to content

Commit cfaaec0

Browse files
committed
fix(cluster): Harden attribute reports and tighten 'skip' exemption
Follow-up to the command-arg hardening. Addresses two bypasses that remained after the first pass: 1. Attribute reports (cmdId 0x0A reportAttributes) parse a raw payload into ZCLAttributeDataRecord entries, where each entry's value field was read via `DataType.fromBuffer` without bounds checking. For fixed-width types like map16 (IAS Zone `zoneStatus`, attribute id 0x0002), a missing or short value silently produced an all-bits-false bitmap that propagates to drivers as "all alarms clear". Same fail- open path as the command-arg case from PR #193, just routed through the attribute-report channel. Add a length precheck in `ZCLAttributeDataRecord.fromBuf` before calling the value's `fromBuffer`. On short input, throw `ZCLError('MALFORMED_COMMAND')` which `Endpoint.handleFrame` already converts to a proper default-response back to the sender. 2. The previous `encodeMissingFieldsBehavior: 'skip'` exemption in `parseCommandArgs` was too broad: it skipped *all* length checking for those commands, so an empty payload to e.g. OTA `queryNextImageRequest` parsed as all-zero fields. Tighten to require at least the first field's byte width, which preserves the legitimate "omit trailing optional fields" case while rejecting truncated leading-discriminator payloads. 5 new tests, including regression guards for well-formed reports and for OTA's omit-trailing-fields path.
1 parent 178ab7e commit cfaaec0

3 files changed

Lines changed: 194 additions & 14 deletions

File tree

‎lib/util/parseCommandArgs.js‎

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,49 @@ const { ZCLError } = require('./index');
2323
* @throws {ZCLError} With zclStatus `MALFORMED_COMMAND` when parsing fails or
2424
* the payload is shorter than the args struct's minimum byte length.
2525
*/
26-
function parseCommandArgs(command, data) {
27-
const argsType = command.args;
28-
29-
// Commands declared with `encodeMissingFieldsBehavior: 'skip'` allow trailing
30-
// fields to be omitted (e.g. OTA `hardwareVersion` is only present when its
31-
// fieldControl bit is set). For those we cannot derive a strict minimum from
32-
// the struct definition and rely on the try/catch below to catch overruns.
33-
if (command.encodeMissingFieldsBehavior !== 'skip') {
26+
/**
27+
* Minimum payload length we'll accept before invoking the parser.
28+
*
29+
* For commands declared with `encodeMissingFieldsBehavior: 'skip'`, trailing
30+
* fields may be intentionally omitted (e.g. OTA `hardwareVersion` is only
31+
* present when its `fieldControl` bit is set). We can't derive the strict
32+
* minimum from the struct alone, but every such command in practice has a
33+
* mandatory leading discriminator (status, fieldControl, payloadType) that
34+
* determines which subsequent fields are present. Without that leading byte,
35+
* the parsed result is meaningless; with it, the parser correctly omits
36+
* optional trailing fields.
37+
*
38+
* So for 'skip' commands we require at least the first field's byte width.
39+
* For non-'skip' commands we require the full fixed-portion length.
40+
*
41+
* @param {object} argsType - The ZCLStruct class for this command's args.
42+
* @param {boolean} skipMode - Whether the command uses `encodeMissingFieldsBehavior: 'skip'`.
43+
* @returns {number} Minimum byte length the payload must carry.
44+
*/
45+
function computeMinLength(argsType, skipMode) {
46+
if (!skipMode) {
3447
// Struct.length convention:
3548
// > 0 : exact byte count for an all-fixed-size struct
3649
// < 0 : negative of the fixed-portion size for a varsize struct
3750
// = 0 : no payload required
38-
// For both fixed and varsize structs, |declaredLength| is the minimum
39-
// number of bytes the payload must carry to populate the fixed fields.
40-
const minLength = Math.abs(argsType.length);
41-
if (data.length < minLength) {
42-
throw new ZCLError('MALFORMED_COMMAND');
43-
}
51+
return Math.abs(argsType.length);
52+
}
53+
54+
const firstField = Object.values(argsType.fields)[0];
55+
if (!firstField) return 0;
56+
// Fixed-width field's length is a positive integer; varsize is negative or 0.
57+
// For varsize first fields (uncommon for 'skip' commands), require at least
58+
// 1 byte so the length prefix is present.
59+
return firstField.length > 0 ? firstField.length : 1;
60+
}
61+
62+
function parseCommandArgs(command, data) {
63+
const argsType = command.args;
64+
const skipMode = command.encodeMissingFieldsBehavior === 'skip';
65+
66+
const minLength = computeMinLength(argsType, skipMode);
67+
if (data.length < minLength) {
68+
throw new ZCLError('MALFORMED_COMMAND');
4469
}
4570

4671
try {

‎lib/zclFrames.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict';
22

33
const { ZCLDataType, ZCLDataTypes, ZCLStruct } = require('./zclTypes');
4+
const { ZCLError } = require('./util');
45

56
const ZCLFrameControlBitmap = ZCLDataTypes.map8(
67
'clusterSpecific',
@@ -84,6 +85,16 @@ const ZCLAttributeDataRecord = (withStatus, attributes) =>
8485
res.name = attributes[res.id].name || `unknown_attr_${res.id}`;
8586
}
8687

88+
// Validate that enough bytes remain for the value field before parsing.
89+
// Without this, fixed-width primitives in @athombv/data-types silently
90+
// return 0 on short input - which for IAS Zone `zoneStatus` (map16)
91+
// produces an all-bits-false bitmap that looks like "all alarms clear",
92+
// and for any status-bearing attribute promotes a truncated value into
93+
// a SUCCESS-looking enum.
94+
if (DataType.length > 0 && buf.length - i < DataType.length) {
95+
throw new ZCLError('MALFORMED_COMMAND');
96+
}
97+
8798
const entry = DataType.fromBuffer(buf, i, true);
8899
if (DataType.length > 0) {
89100
i += DataType.length;

‎test/testMalformedFrames.js‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,13 @@ const assert = require('node:assert');
55
const Node = require('../lib/Node');
66
const BoundCluster = require('../lib/BoundCluster');
77
const IASZoneCluster = require('../lib/clusters/iasZone');
8+
const OTACluster = require('../lib/clusters/ota');
89
const { ZCLStandardHeader } = require('../lib/zclFrames');
910
const { ZCLDataTypes } = require('../lib/zclTypes');
1011

12+
// ZCL data-type id for map16 (declared in @athombv/data-types).
13+
const ZCL_TYPE_ID_MAP16 = 25;
14+
1115
/**
1216
* Helper: build a mock Node that captures every frame sent back via sendFrame.
1317
* Unlike `createMockNode({loopback:true})`, this lets the test inspect the
@@ -173,4 +177,144 @@ describe('Malformed frame hardening', function () {
173177
);
174178
});
175179
});
180+
181+
describe('Attribute report (reportAttributes / cmdId 0x0A)', function () {
182+
/**
183+
* Build a reportAttributes frame carrying one attribute record.
184+
* @param {number} attrId
185+
* @param {number} typeId
186+
* @param {Buffer} value - the raw value bytes (may be truncated for tests).
187+
*/
188+
function buildReportAttributesFrame(attrId, typeId, value) {
189+
const frame = new ZCLStandardHeader();
190+
frame.cmdId = 0x0a;
191+
frame.frameControl.directionToClient = true;
192+
frame.frameControl.clusterSpecific = false; // global command
193+
const header = Buffer.alloc(3);
194+
header.writeUInt16LE(attrId, 0);
195+
header.writeUInt8(typeId, 2);
196+
frame.data = Buffer.concat([header, value]);
197+
return frame;
198+
}
199+
200+
it('should not emit attr.zoneStatus on truncated map16 value', async function () {
201+
const { node } = createCapturingNode(1, IASZoneCluster.ID);
202+
203+
let emitted = false;
204+
node.endpoints[1].clusters.iasZone.on('attr.zoneStatus', () => {
205+
emitted = true;
206+
});
207+
208+
// Attribute id=0x0002 (zoneStatus), type=map16, value bytes missing entirely.
209+
const frame = buildReportAttributesFrame(0x0002, ZCL_TYPE_ID_MAP16, Buffer.alloc(0));
210+
await node.handleFrame(1, IASZoneCluster.ID, frame.toBuffer(), {});
211+
212+
assert.strictEqual(
213+
emitted,
214+
false,
215+
'attr.zoneStatus must not fire when the map16 value bytes are missing',
216+
);
217+
});
218+
219+
it('should respond with MALFORMED_COMMAND on truncated attribute value', async function () {
220+
const { node, sentFrames } = createCapturingNode(1, IASZoneCluster.ID);
221+
222+
// 1 byte of map16 instead of 2.
223+
const frame = buildReportAttributesFrame(0x0002, ZCL_TYPE_ID_MAP16, Buffer.from([0x01]));
224+
await node.handleFrame(1, IASZoneCluster.ID, frame.toBuffer(), {});
225+
226+
const response = sentFrames
227+
.map((f) => parseDefaultResponse(f.raw))
228+
.find((r) => r && r.forCmdId === 0x0a);
229+
230+
assert.ok(response, 'expected a default-response frame to be sent back');
231+
assert.strictEqual(
232+
response.status,
233+
'MALFORMED_COMMAND',
234+
`expected MALFORMED_COMMAND, got ${response.status}`,
235+
);
236+
});
237+
238+
it('should still emit attr.zoneStatus on well-formed report (regression)', async function () {
239+
const { node } = createCapturingNode(1, IASZoneCluster.ID);
240+
241+
let received = null;
242+
node.endpoints[1].clusters.iasZone.on('attr.zoneStatus', (val) => {
243+
received = val;
244+
});
245+
246+
// Full report: attrId=0x0002, typeId=map16, value=[0x01, 0x00] (alarm1 bit set).
247+
const value = Buffer.from([0x01, 0x00]);
248+
const frame = buildReportAttributesFrame(0x0002, ZCL_TYPE_ID_MAP16, value);
249+
await node.handleFrame(1, IASZoneCluster.ID, frame.toBuffer(), {});
250+
251+
assert.ok(received, 'attr.zoneStatus should have fired');
252+
assert.strictEqual(received.alarm1, true);
253+
});
254+
});
255+
256+
describe('encodeMissingFieldsBehavior: "skip" - empty payload', function () {
257+
it('should reject empty queryNextImageRequest', async function () {
258+
const { node, sentFrames } = createCapturingNode(1, OTACluster.ID);
259+
node.endpoints[1].bind('ota', new BoundCluster());
260+
261+
const frame = new ZCLStandardHeader();
262+
frame.cmdId = OTACluster.COMMANDS.queryNextImageRequest.id;
263+
frame.frameControl.directionToClient = false;
264+
frame.frameControl.clusterSpecific = true;
265+
frame.data = Buffer.alloc(0); // Pre-fix: parses with all zero-filled fields.
266+
267+
await node.handleFrame(1, OTACluster.ID, frame.toBuffer(), {});
268+
269+
const response = sentFrames
270+
.map((f) => parseDefaultResponse(f.raw))
271+
.find((r) => r && r.forCmdId === OTACluster.COMMANDS.queryNextImageRequest.id);
272+
273+
assert.ok(response, 'expected a default-response frame');
274+
assert.strictEqual(
275+
response.status,
276+
'MALFORMED_COMMAND',
277+
`expected MALFORMED_COMMAND, got ${response.status}`,
278+
);
279+
});
280+
281+
it('should still accept queryNextImageRequest without trailing hardwareVersion (regression)', async function () {
282+
const { node } = createCapturingNode(1, OTACluster.ID);
283+
284+
let receivedArgs = null;
285+
node.endpoints[1].bind(
286+
'ota',
287+
new (class extends BoundCluster {
288+
async queryNextImageRequest(args) {
289+
receivedArgs = args;
290+
return { status: 'NO_IMAGE_AVAILABLE' };
291+
}
292+
})(),
293+
);
294+
295+
const frame = new ZCLStandardHeader();
296+
frame.cmdId = OTACluster.COMMANDS.queryNextImageRequest.id;
297+
frame.frameControl.directionToClient = false;
298+
frame.frameControl.clusterSpecific = true;
299+
// 9 bytes: fieldControl(1) + manufacturerCode(2) + imageType(2) + fileVersion(4).
300+
// Trailing hardwareVersion(2) omitted because fieldControl bit 0 is not set.
301+
frame.data = Buffer.from([
302+
0x00, // fieldControl
303+
0x34,
304+
0x12, // manufacturerCode = 0x1234
305+
0x21,
306+
0x43, // imageType = 0x4321
307+
0x01,
308+
0x00,
309+
0x00,
310+
0x00, // fileVersion = 1
311+
]);
312+
313+
await node.handleFrame(1, OTACluster.ID, frame.toBuffer(), {});
314+
315+
assert.ok(receivedArgs, 'handler should have been called');
316+
assert.strictEqual(receivedArgs.manufacturerCode, 0x1234);
317+
assert.strictEqual(receivedArgs.imageType, 0x4321);
318+
});
319+
});
176320
});

0 commit comments

Comments
 (0)