Skip to content

Commit e50f2ea

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. 86/86 tests pass (was 81 + 5).
1 parent ac2f0f6 commit e50f2ea

3 files changed

Lines changed: 180 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('clusterSpecific', null, 'manufacturerSpecific', 'directionToClient', 'disableDefaultResponse');
67

@@ -71,6 +72,16 @@ const ZCLAttributeDataRecord = (withStatus, attributes) => new ZCLDataType(NaN,
7172
res.name = attributes[res.id].name || `unknown_attr_${res.id}`;
7273
}
7374

75+
// Validate that enough bytes remain for the value field before parsing.
76+
// Without this, fixed-width primitives in @athombv/data-types silently
77+
// return 0 on short input - which for IAS Zone `zoneStatus` (map16)
78+
// produces an all-bits-false bitmap that looks like "all alarms clear",
79+
// and for any status-bearing attribute promotes a truncated value into
80+
// a SUCCESS-looking enum.
81+
if (DataType.length > 0 && buf.length - i < DataType.length) {
82+
throw new ZCLError('MALFORMED_COMMAND');
83+
}
84+
7485
const entry = DataType.fromBuffer(buf, i, true);
7586
if (DataType.length > 0) {
7687
i += DataType.length;

‎test/testMalformedFrames.js‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,17 @@
1+
// eslint-disable-next-line max-classes-per-file,lines-around-directive
12
'use strict';
23

34
const assert = require('assert');
45
const Node = require('../lib/Node');
56
const BoundCluster = require('../lib/BoundCluster');
67
const IASZoneCluster = require('../lib/clusters/iasZone');
8+
const OTACluster = require('../lib/clusters/ota');
79
const { ZCLStandardHeader } = require('../lib/zclFrames');
810
const { ZCLDataTypes } = require('../lib/zclTypes');
911

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

0 commit comments

Comments
 (0)