Skip to content

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND - #193

Open
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening
Open

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND#193
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening

Conversation

@RobinBol

@RobinBolRobinBol commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens command-argument and attribute-record parsing against truncated wire input. Without these checks, @athombv/data-types silently zero-fills missing bytes, producing a plausible-looking parse result that:

  • For IAS Zone zoneStatusChangeNotification and attribute reports of zoneStatus: reads as zoneStatus.alarm1 === false (i.e. "all clear" while truncated frames look indistinguishable from a real "no alarm" report at the consumer layer).
  • For any status-bearing response: reads as status === 'SUCCESS' (because the zero value is what every ZCL status enum maps to SUCCESS).

This is a robustness / defense-in-depth change. The framing is correctness-first: safety-critical cluster handlers should never observe args from a payload too short to populate them, and "downstream code cannot tell whether the device reported a valid false value or whether the parser invented it from missing bytes" is a trust-boundary problem worth closing.

Changes

1. Command arg parsing - length precheck + try/catch

Cluster.handleFrame (Cluster.js:780) and BoundCluster.handleFrame (BoundCluster.js:312) now route command-arg parsing through a new helper, lib/util/parseCommandArgs.js:

  • Payload shorter than Math.abs(command.args.length) is rejected before the parser runs.
  • Payload that throws mid-parse (e.g. an octstr length prefix overruns the remaining buffer) is caught and converted to MALFORMED_COMMAND instead of leaking a RangeError as a generic FAILURE.
  • Endpoint.handleFrame already converts thrown ZCLErrors into proper ZCL default-response frames back to the sender, so no surrounding wiring changes are needed.

2. Attribute report (reportAttributes) value-length validation

ZCLAttributeDataRecord.fromBuf in zclFrames.js now validates that enough bytes remain for the value field before calling DataType.fromBuffer. Without this check, a reportAttributes frame carrying a valid attribute header but a truncated value bypassed the command-arg precheck entirely (because reportAttributes declares args as a raw buffer) and re-introduced the same silent zero-fill at the inner record layer. Concretely, a zoneStatus (map16) attribute report missing its value bytes parsed as an all-bits-false bitmap, emitted attr.zoneStatus, and reached IAS-Zone drivers as "all alarms clear".

On insufficient bytes, throws ZCLError('MALFORMED_COMMAND'). Variable-length value fields are not covered by this check (see "Out of scope" below).

3. Tighter encodeMissingFieldsBehavior: 'skip' exemption

The initial precheck fully exempted 'skip' commands (OTA) because they allow trailing fields to be omitted. That was broader than needed: every 'skip' command in the codebase has a mandatory leading discriminator (status / fieldControl / payloadType) whose presence determines which subsequent fields are valid. Without that byte, the parse is meaningless.

The exemption now requires at least the first field's byte width. This preserves the legitimate "omit trailing optional fields" path (e.g. queryNextImageRequest without hardwareVersion) while rejecting empty / sub-leading-byte payloads.

Test plan

All new tests written TDD-style (red first, then implementation).

  • Command path - truncated zoneStatusChangeNotification, truncated zoneEnrollResponse:
    • Handlers not invoked.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed equivalents still invoke the handler with the expected args (regression).
  • Attribute report path - truncated zoneStatus (map16) value:
    • attr.zoneStatus event does not fire.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed zoneStatus report still fires attr.zoneStatus with alarm1 === true (regression).
  • 'skip' exemption - empty queryNextImageRequest:
    • Rejected with MALFORMED_COMMAND.
    • 9-byte queryNextImageRequest without trailing hardwareVersion still parses successfully and reaches the handler with the expected manufacturerCode / imageType (regression - preserves existing OTA tolerance).
  • Full suite: 86/86 passing (76 pre-existing + 10 new). No regressions.
  • Lint clean.

Out of scope

This PR is consumer-side hardening. The deeper fix sits in @athombv/data-types and includes:

  • Fixed-width reads (uintFromBuf, enumFromBuf, dataFromBuf) failing on insufficient bytes instead of returning 0 / a truncated buffer.
  • Variable-length reads (bufferFromBuf) failing instead of returning a truncated buffer with isPartial = true (an undocumented flag no consumer checks - the source even contains a // TODO: FIXME next to it).
  • Bitmap slices not aliasing the source buffer (TOCTOU hazard if a radio driver reuses receive buffers).
  • Struct constructor rejecting prototype-chain keys like constructor as field names.

Two of those (Bitmap defensive copy, Struct prototype-chain guard) are already in flight upstream at athombv/node-data-types#44. The *FromBuf truncation behavior is the breaking change that needs a separate discussion (either a strict opt-in flag or a major-version flip).

As a consequence, this PR does not close:

  • Variable-length attribute / command field overrun via bufferFromBuf's isPartial path. The try/catch in parseCommandArgs catches nothing here because nothing throws.
  • Attribute reports carrying a variable-length value field (e.g. octstr-typed attributes) where the length prefix overruns the remaining buffer.

These can be closed either upstream (cleanest) or by recursing parsed args downstream to reject any Buffer with isPartial === true (less clean, but unblocks without coordination). Tracked separately.

The parser primitives in @athombv/data-types silently substitute zero
values when the source buffer is shorter than the declared field length.
For a ZCL command struct, this means a truncated payload parses into an
object where every numeric field is 0 and every bitmap field has all
bits clear - which for alarm-routing clusters (IAS Zone) happens to look
like "all clear" and for status-bearing response frames happens to look
like "SUCCESS".
This change adds a length precheck and try/catch around command arg
parsing in Cluster.handleFrame and BoundCluster.handleFrame. Truncated
payloads now throw ZCLError('MALFORMED_COMMAND'), which Endpoint.handleFrame
already converts into a proper default-response back to the sender.
Commands declared with encodeMissingFieldsBehavior: 'skip' (OTA) are
exempt from the strict length precheck because they intentionally allow
trailing fields to be omitted; for those, only the try/catch fallback
applies.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens inbound ZCL command argument parsing so truncated/unparseable command payloads are rejected with ZCLError('MALFORMED_COMMAND') instead of being silently parsed (often as all-zero “SUCCESS”/“all-clear” values). This leverages existing Endpoint.handleFrame behavior to automatically emit a proper ZCL Default Response without additional wiring.

Changes:

  • Added a shared parseCommandArgs() helper that performs a minimum-length precheck (when applicable) and converts parse-time exceptions into MALFORMED_COMMAND.
  • Updated Cluster.handleFrame and BoundCluster.handleFrame to use the new helper instead of calling command.args.fromBuffer(...) directly.
  • Added tests to ensure truncated IAS Zone frames don’t invoke handlers and do produce MALFORMED_COMMAND Default Responses (with regressions for well-formed frames).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

FileDescription
test/testMalformedFrames.jsAdds regression + hardening tests verifying truncated payloads yield MALFORMED_COMMAND and do not call handlers.
lib/util/parseCommandArgs.jsIntroduces defensive argument parsing helper with length precheck + parse error normalization.
lib/Cluster.jsRoutes inbound cluster command arg parsing through parseCommandArgs().
lib/BoundCluster.jsRoutes inbound bound-cluster command arg parsing through parseCommandArgs().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/Cluster.js Outdated
Comment threadtest/testMalformedFrames.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@RobinBol@nozols
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(cluster): reject truncated command payloads with MALFORMED_COMMAND by RobinBol · Pull Request #193 · athombv/node-zigbee-clusters · GitHub
Skip to content

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND - #193

Open
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening
Open

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND#193
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening

Conversation

@RobinBol

@RobinBolRobinBol commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens command-argument and attribute-record parsing against truncated wire input. Without these checks, @athombv/data-types silently zero-fills missing bytes, producing a plausible-looking parse result that:

  • For IAS Zone zoneStatusChangeNotification and attribute reports of zoneStatus: reads as zoneStatus.alarm1 === false (i.e. "all clear" while truncated frames look indistinguishable from a real "no alarm" report at the consumer layer).
  • For any status-bearing response: reads as status === 'SUCCESS' (because the zero value is what every ZCL status enum maps to SUCCESS).

This is a robustness / defense-in-depth change. The framing is correctness-first: safety-critical cluster handlers should never observe args from a payload too short to populate them, and "downstream code cannot tell whether the device reported a valid false value or whether the parser invented it from missing bytes" is a trust-boundary problem worth closing.

Changes

1. Command arg parsing - length precheck + try/catch

Cluster.handleFrame (Cluster.js:780) and BoundCluster.handleFrame (BoundCluster.js:312) now route command-arg parsing through a new helper, lib/util/parseCommandArgs.js:

  • Payload shorter than Math.abs(command.args.length) is rejected before the parser runs.
  • Payload that throws mid-parse (e.g. an octstr length prefix overruns the remaining buffer) is caught and converted to MALFORMED_COMMAND instead of leaking a RangeError as a generic FAILURE.
  • Endpoint.handleFrame already converts thrown ZCLErrors into proper ZCL default-response frames back to the sender, so no surrounding wiring changes are needed.

2. Attribute report (reportAttributes) value-length validation

ZCLAttributeDataRecord.fromBuf in zclFrames.js now validates that enough bytes remain for the value field before calling DataType.fromBuffer. Without this check, a reportAttributes frame carrying a valid attribute header but a truncated value bypassed the command-arg precheck entirely (because reportAttributes declares args as a raw buffer) and re-introduced the same silent zero-fill at the inner record layer. Concretely, a zoneStatus (map16) attribute report missing its value bytes parsed as an all-bits-false bitmap, emitted attr.zoneStatus, and reached IAS-Zone drivers as "all alarms clear".

On insufficient bytes, throws ZCLError('MALFORMED_COMMAND'). Variable-length value fields are not covered by this check (see "Out of scope" below).

3. Tighter encodeMissingFieldsBehavior: 'skip' exemption

The initial precheck fully exempted 'skip' commands (OTA) because they allow trailing fields to be omitted. That was broader than needed: every 'skip' command in the codebase has a mandatory leading discriminator (status / fieldControl / payloadType) whose presence determines which subsequent fields are valid. Without that byte, the parse is meaningless.

The exemption now requires at least the first field's byte width. This preserves the legitimate "omit trailing optional fields" path (e.g. queryNextImageRequest without hardwareVersion) while rejecting empty / sub-leading-byte payloads.

Test plan

All new tests written TDD-style (red first, then implementation).

  • Command path - truncated zoneStatusChangeNotification, truncated zoneEnrollResponse:
    • Handlers not invoked.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed equivalents still invoke the handler with the expected args (regression).
  • Attribute report path - truncated zoneStatus (map16) value:
    • attr.zoneStatus event does not fire.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed zoneStatus report still fires attr.zoneStatus with alarm1 === true (regression).
  • 'skip' exemption - empty queryNextImageRequest:
    • Rejected with MALFORMED_COMMAND.
    • 9-byte queryNextImageRequest without trailing hardwareVersion still parses successfully and reaches the handler with the expected manufacturerCode / imageType (regression - preserves existing OTA tolerance).
  • Full suite: 86/86 passing (76 pre-existing + 10 new). No regressions.
  • Lint clean.

Out of scope

This PR is consumer-side hardening. The deeper fix sits in @athombv/data-types and includes:

  • Fixed-width reads (uintFromBuf, enumFromBuf, dataFromBuf) failing on insufficient bytes instead of returning 0 / a truncated buffer.
  • Variable-length reads (bufferFromBuf) failing instead of returning a truncated buffer with isPartial = true (an undocumented flag no consumer checks - the source even contains a // TODO: FIXME next to it).
  • Bitmap slices not aliasing the source buffer (TOCTOU hazard if a radio driver reuses receive buffers).
  • Struct constructor rejecting prototype-chain keys like constructor as field names.

Two of those (Bitmap defensive copy, Struct prototype-chain guard) are already in flight upstream at athombv/node-data-types#44. The *FromBuf truncation behavior is the breaking change that needs a separate discussion (either a strict opt-in flag or a major-version flip).

As a consequence, this PR does not close:

  • Variable-length attribute / command field overrun via bufferFromBuf's isPartial path. The try/catch in parseCommandArgs catches nothing here because nothing throws.
  • Attribute reports carrying a variable-length value field (e.g. octstr-typed attributes) where the length prefix overruns the remaining buffer.

These can be closed either upstream (cleanest) or by recursing parsed args downstream to reject any Buffer with isPartial === true (less clean, but unblocks without coordination). Tracked separately.

The parser primitives in @athombv/data-types silently substitute zero
values when the source buffer is shorter than the declared field length.
For a ZCL command struct, this means a truncated payload parses into an
object where every numeric field is 0 and every bitmap field has all
bits clear - which for alarm-routing clusters (IAS Zone) happens to look
like "all clear" and for status-bearing response frames happens to look
like "SUCCESS".
This change adds a length precheck and try/catch around command arg
parsing in Cluster.handleFrame and BoundCluster.handleFrame. Truncated
payloads now throw ZCLError('MALFORMED_COMMAND'), which Endpoint.handleFrame
already converts into a proper default-response back to the sender.
Commands declared with encodeMissingFieldsBehavior: 'skip' (OTA) are
exempt from the strict length precheck because they intentionally allow
trailing fields to be omitted; for those, only the try/catch fallback
applies.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens inbound ZCL command argument parsing so truncated/unparseable command payloads are rejected with ZCLError('MALFORMED_COMMAND') instead of being silently parsed (often as all-zero “SUCCESS”/“all-clear” values). This leverages existing Endpoint.handleFrame behavior to automatically emit a proper ZCL Default Response without additional wiring.

Changes:

  • Added a shared parseCommandArgs() helper that performs a minimum-length precheck (when applicable) and converts parse-time exceptions into MALFORMED_COMMAND.
  • Updated Cluster.handleFrame and BoundCluster.handleFrame to use the new helper instead of calling command.args.fromBuffer(...) directly.
  • Added tests to ensure truncated IAS Zone frames don’t invoke handlers and do produce MALFORMED_COMMAND Default Responses (with regressions for well-formed frames).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

FileDescription
test/testMalformedFrames.jsAdds regression + hardening tests verifying truncated payloads yield MALFORMED_COMMAND and do not call handlers.
lib/util/parseCommandArgs.jsIntroduces defensive argument parsing helper with length precheck + parse error normalization.
lib/Cluster.jsRoutes inbound cluster command arg parsing through parseCommandArgs().
lib/BoundCluster.jsRoutes inbound bound-cluster command arg parsing through parseCommandArgs().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/Cluster.js Outdated
Comment threadtest/testMalformedFrames.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@RobinBol@nozols
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(cluster): reject truncated command payloads with MALFORMED_COMMAND by RobinBol · Pull Request #193 · athombv/node-zigbee-clusters · GitHub
Skip to content

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND - #193

Open
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening
Open

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND#193
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening

Conversation

@RobinBol

@RobinBolRobinBol commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens command-argument and attribute-record parsing against truncated wire input. Without these checks, @athombv/data-types silently zero-fills missing bytes, producing a plausible-looking parse result that:

  • For IAS Zone zoneStatusChangeNotification and attribute reports of zoneStatus: reads as zoneStatus.alarm1 === false (i.e. "all clear" while truncated frames look indistinguishable from a real "no alarm" report at the consumer layer).
  • For any status-bearing response: reads as status === 'SUCCESS' (because the zero value is what every ZCL status enum maps to SUCCESS).

This is a robustness / defense-in-depth change. The framing is correctness-first: safety-critical cluster handlers should never observe args from a payload too short to populate them, and "downstream code cannot tell whether the device reported a valid false value or whether the parser invented it from missing bytes" is a trust-boundary problem worth closing.

Changes

1. Command arg parsing - length precheck + try/catch

Cluster.handleFrame (Cluster.js:780) and BoundCluster.handleFrame (BoundCluster.js:312) now route command-arg parsing through a new helper, lib/util/parseCommandArgs.js:

  • Payload shorter than Math.abs(command.args.length) is rejected before the parser runs.
  • Payload that throws mid-parse (e.g. an octstr length prefix overruns the remaining buffer) is caught and converted to MALFORMED_COMMAND instead of leaking a RangeError as a generic FAILURE.
  • Endpoint.handleFrame already converts thrown ZCLErrors into proper ZCL default-response frames back to the sender, so no surrounding wiring changes are needed.

2. Attribute report (reportAttributes) value-length validation

ZCLAttributeDataRecord.fromBuf in zclFrames.js now validates that enough bytes remain for the value field before calling DataType.fromBuffer. Without this check, a reportAttributes frame carrying a valid attribute header but a truncated value bypassed the command-arg precheck entirely (because reportAttributes declares args as a raw buffer) and re-introduced the same silent zero-fill at the inner record layer. Concretely, a zoneStatus (map16) attribute report missing its value bytes parsed as an all-bits-false bitmap, emitted attr.zoneStatus, and reached IAS-Zone drivers as "all alarms clear".

On insufficient bytes, throws ZCLError('MALFORMED_COMMAND'). Variable-length value fields are not covered by this check (see "Out of scope" below).

3. Tighter encodeMissingFieldsBehavior: 'skip' exemption

The initial precheck fully exempted 'skip' commands (OTA) because they allow trailing fields to be omitted. That was broader than needed: every 'skip' command in the codebase has a mandatory leading discriminator (status / fieldControl / payloadType) whose presence determines which subsequent fields are valid. Without that byte, the parse is meaningless.

The exemption now requires at least the first field's byte width. This preserves the legitimate "omit trailing optional fields" path (e.g. queryNextImageRequest without hardwareVersion) while rejecting empty / sub-leading-byte payloads.

Test plan

All new tests written TDD-style (red first, then implementation).

  • Command path - truncated zoneStatusChangeNotification, truncated zoneEnrollResponse:
    • Handlers not invoked.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed equivalents still invoke the handler with the expected args (regression).
  • Attribute report path - truncated zoneStatus (map16) value:
    • attr.zoneStatus event does not fire.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed zoneStatus report still fires attr.zoneStatus with alarm1 === true (regression).
  • 'skip' exemption - empty queryNextImageRequest:
    • Rejected with MALFORMED_COMMAND.
    • 9-byte queryNextImageRequest without trailing hardwareVersion still parses successfully and reaches the handler with the expected manufacturerCode / imageType (regression - preserves existing OTA tolerance).
  • Full suite: 86/86 passing (76 pre-existing + 10 new). No regressions.
  • Lint clean.

Out of scope

This PR is consumer-side hardening. The deeper fix sits in @athombv/data-types and includes:

  • Fixed-width reads (uintFromBuf, enumFromBuf, dataFromBuf) failing on insufficient bytes instead of returning 0 / a truncated buffer.
  • Variable-length reads (bufferFromBuf) failing instead of returning a truncated buffer with isPartial = true (an undocumented flag no consumer checks - the source even contains a // TODO: FIXME next to it).
  • Bitmap slices not aliasing the source buffer (TOCTOU hazard if a radio driver reuses receive buffers).
  • Struct constructor rejecting prototype-chain keys like constructor as field names.

Two of those (Bitmap defensive copy, Struct prototype-chain guard) are already in flight upstream at athombv/node-data-types#44. The *FromBuf truncation behavior is the breaking change that needs a separate discussion (either a strict opt-in flag or a major-version flip).

As a consequence, this PR does not close:

  • Variable-length attribute / command field overrun via bufferFromBuf's isPartial path. The try/catch in parseCommandArgs catches nothing here because nothing throws.
  • Attribute reports carrying a variable-length value field (e.g. octstr-typed attributes) where the length prefix overruns the remaining buffer.

These can be closed either upstream (cleanest) or by recursing parsed args downstream to reject any Buffer with isPartial === true (less clean, but unblocks without coordination). Tracked separately.

The parser primitives in @athombv/data-types silently substitute zero
values when the source buffer is shorter than the declared field length.
For a ZCL command struct, this means a truncated payload parses into an
object where every numeric field is 0 and every bitmap field has all
bits clear - which for alarm-routing clusters (IAS Zone) happens to look
like "all clear" and for status-bearing response frames happens to look
like "SUCCESS".
This change adds a length precheck and try/catch around command arg
parsing in Cluster.handleFrame and BoundCluster.handleFrame. Truncated
payloads now throw ZCLError('MALFORMED_COMMAND'), which Endpoint.handleFrame
already converts into a proper default-response back to the sender.
Commands declared with encodeMissingFieldsBehavior: 'skip' (OTA) are
exempt from the strict length precheck because they intentionally allow
trailing fields to be omitted; for those, only the try/catch fallback
applies.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens inbound ZCL command argument parsing so truncated/unparseable command payloads are rejected with ZCLError('MALFORMED_COMMAND') instead of being silently parsed (often as all-zero “SUCCESS”/“all-clear” values). This leverages existing Endpoint.handleFrame behavior to automatically emit a proper ZCL Default Response without additional wiring.

Changes:

  • Added a shared parseCommandArgs() helper that performs a minimum-length precheck (when applicable) and converts parse-time exceptions into MALFORMED_COMMAND.
  • Updated Cluster.handleFrame and BoundCluster.handleFrame to use the new helper instead of calling command.args.fromBuffer(...) directly.
  • Added tests to ensure truncated IAS Zone frames don’t invoke handlers and do produce MALFORMED_COMMAND Default Responses (with regressions for well-formed frames).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

FileDescription
test/testMalformedFrames.jsAdds regression + hardening tests verifying truncated payloads yield MALFORMED_COMMAND and do not call handlers.
lib/util/parseCommandArgs.jsIntroduces defensive argument parsing helper with length precheck + parse error normalization.
lib/Cluster.jsRoutes inbound cluster command arg parsing through parseCommandArgs().
lib/BoundCluster.jsRoutes inbound bound-cluster command arg parsing through parseCommandArgs().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/Cluster.js Outdated
Comment threadtest/testMalformedFrames.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@RobinBol@nozols
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(cluster): reject truncated command payloads with MALFORMED_COMMAND by RobinBol · Pull Request #193 · athombv/node-zigbee-clusters · GitHub
Skip to content

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND - #193

Open
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening
Open

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND#193
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening

Conversation

@RobinBol

@RobinBolRobinBol commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens command-argument and attribute-record parsing against truncated wire input. Without these checks, @athombv/data-types silently zero-fills missing bytes, producing a plausible-looking parse result that:

  • For IAS Zone zoneStatusChangeNotification and attribute reports of zoneStatus: reads as zoneStatus.alarm1 === false (i.e. "all clear" while truncated frames look indistinguishable from a real "no alarm" report at the consumer layer).
  • For any status-bearing response: reads as status === 'SUCCESS' (because the zero value is what every ZCL status enum maps to SUCCESS).

This is a robustness / defense-in-depth change. The framing is correctness-first: safety-critical cluster handlers should never observe args from a payload too short to populate them, and "downstream code cannot tell whether the device reported a valid false value or whether the parser invented it from missing bytes" is a trust-boundary problem worth closing.

Changes

1. Command arg parsing - length precheck + try/catch

Cluster.handleFrame (Cluster.js:780) and BoundCluster.handleFrame (BoundCluster.js:312) now route command-arg parsing through a new helper, lib/util/parseCommandArgs.js:

  • Payload shorter than Math.abs(command.args.length) is rejected before the parser runs.
  • Payload that throws mid-parse (e.g. an octstr length prefix overruns the remaining buffer) is caught and converted to MALFORMED_COMMAND instead of leaking a RangeError as a generic FAILURE.
  • Endpoint.handleFrame already converts thrown ZCLErrors into proper ZCL default-response frames back to the sender, so no surrounding wiring changes are needed.

2. Attribute report (reportAttributes) value-length validation

ZCLAttributeDataRecord.fromBuf in zclFrames.js now validates that enough bytes remain for the value field before calling DataType.fromBuffer. Without this check, a reportAttributes frame carrying a valid attribute header but a truncated value bypassed the command-arg precheck entirely (because reportAttributes declares args as a raw buffer) and re-introduced the same silent zero-fill at the inner record layer. Concretely, a zoneStatus (map16) attribute report missing its value bytes parsed as an all-bits-false bitmap, emitted attr.zoneStatus, and reached IAS-Zone drivers as "all alarms clear".

On insufficient bytes, throws ZCLError('MALFORMED_COMMAND'). Variable-length value fields are not covered by this check (see "Out of scope" below).

3. Tighter encodeMissingFieldsBehavior: 'skip' exemption

The initial precheck fully exempted 'skip' commands (OTA) because they allow trailing fields to be omitted. That was broader than needed: every 'skip' command in the codebase has a mandatory leading discriminator (status / fieldControl / payloadType) whose presence determines which subsequent fields are valid. Without that byte, the parse is meaningless.

The exemption now requires at least the first field's byte width. This preserves the legitimate "omit trailing optional fields" path (e.g. queryNextImageRequest without hardwareVersion) while rejecting empty / sub-leading-byte payloads.

Test plan

All new tests written TDD-style (red first, then implementation).

  • Command path - truncated zoneStatusChangeNotification, truncated zoneEnrollResponse:
    • Handlers not invoked.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed equivalents still invoke the handler with the expected args (regression).
  • Attribute report path - truncated zoneStatus (map16) value:
    • attr.zoneStatus event does not fire.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed zoneStatus report still fires attr.zoneStatus with alarm1 === true (regression).
  • 'skip' exemption - empty queryNextImageRequest:
    • Rejected with MALFORMED_COMMAND.
    • 9-byte queryNextImageRequest without trailing hardwareVersion still parses successfully and reaches the handler with the expected manufacturerCode / imageType (regression - preserves existing OTA tolerance).
  • Full suite: 86/86 passing (76 pre-existing + 10 new). No regressions.
  • Lint clean.

Out of scope

This PR is consumer-side hardening. The deeper fix sits in @athombv/data-types and includes:

  • Fixed-width reads (uintFromBuf, enumFromBuf, dataFromBuf) failing on insufficient bytes instead of returning 0 / a truncated buffer.
  • Variable-length reads (bufferFromBuf) failing instead of returning a truncated buffer with isPartial = true (an undocumented flag no consumer checks - the source even contains a // TODO: FIXME next to it).
  • Bitmap slices not aliasing the source buffer (TOCTOU hazard if a radio driver reuses receive buffers).
  • Struct constructor rejecting prototype-chain keys like constructor as field names.

Two of those (Bitmap defensive copy, Struct prototype-chain guard) are already in flight upstream at athombv/node-data-types#44. The *FromBuf truncation behavior is the breaking change that needs a separate discussion (either a strict opt-in flag or a major-version flip).

As a consequence, this PR does not close:

  • Variable-length attribute / command field overrun via bufferFromBuf's isPartial path. The try/catch in parseCommandArgs catches nothing here because nothing throws.
  • Attribute reports carrying a variable-length value field (e.g. octstr-typed attributes) where the length prefix overruns the remaining buffer.

These can be closed either upstream (cleanest) or by recursing parsed args downstream to reject any Buffer with isPartial === true (less clean, but unblocks without coordination). Tracked separately.

The parser primitives in @athombv/data-types silently substitute zero
values when the source buffer is shorter than the declared field length.
For a ZCL command struct, this means a truncated payload parses into an
object where every numeric field is 0 and every bitmap field has all
bits clear - which for alarm-routing clusters (IAS Zone) happens to look
like "all clear" and for status-bearing response frames happens to look
like "SUCCESS".
This change adds a length precheck and try/catch around command arg
parsing in Cluster.handleFrame and BoundCluster.handleFrame. Truncated
payloads now throw ZCLError('MALFORMED_COMMAND'), which Endpoint.handleFrame
already converts into a proper default-response back to the sender.
Commands declared with encodeMissingFieldsBehavior: 'skip' (OTA) are
exempt from the strict length precheck because they intentionally allow
trailing fields to be omitted; for those, only the try/catch fallback
applies.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens inbound ZCL command argument parsing so truncated/unparseable command payloads are rejected with ZCLError('MALFORMED_COMMAND') instead of being silently parsed (often as all-zero “SUCCESS”/“all-clear” values). This leverages existing Endpoint.handleFrame behavior to automatically emit a proper ZCL Default Response without additional wiring.

Changes:

  • Added a shared parseCommandArgs() helper that performs a minimum-length precheck (when applicable) and converts parse-time exceptions into MALFORMED_COMMAND.
  • Updated Cluster.handleFrame and BoundCluster.handleFrame to use the new helper instead of calling command.args.fromBuffer(...) directly.
  • Added tests to ensure truncated IAS Zone frames don’t invoke handlers and do produce MALFORMED_COMMAND Default Responses (with regressions for well-formed frames).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

FileDescription
test/testMalformedFrames.jsAdds regression + hardening tests verifying truncated payloads yield MALFORMED_COMMAND and do not call handlers.
lib/util/parseCommandArgs.jsIntroduces defensive argument parsing helper with length precheck + parse error normalization.
lib/Cluster.jsRoutes inbound cluster command arg parsing through parseCommandArgs().
lib/BoundCluster.jsRoutes inbound bound-cluster command arg parsing through parseCommandArgs().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/Cluster.js Outdated
Comment threadtest/testMalformedFrames.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@RobinBol@nozols
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(cluster): reject truncated command payloads with MALFORMED_COMMAND by RobinBol · Pull Request #193 · athombv/node-zigbee-clusters · GitHub
Skip to content

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND - #193

Open
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening
Open

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND#193
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening

Conversation

@RobinBol

@RobinBolRobinBol commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens command-argument and attribute-record parsing against truncated wire input. Without these checks, @athombv/data-types silently zero-fills missing bytes, producing a plausible-looking parse result that:

  • For IAS Zone zoneStatusChangeNotification and attribute reports of zoneStatus: reads as zoneStatus.alarm1 === false (i.e. "all clear" while truncated frames look indistinguishable from a real "no alarm" report at the consumer layer).
  • For any status-bearing response: reads as status === 'SUCCESS' (because the zero value is what every ZCL status enum maps to SUCCESS).

This is a robustness / defense-in-depth change. The framing is correctness-first: safety-critical cluster handlers should never observe args from a payload too short to populate them, and "downstream code cannot tell whether the device reported a valid false value or whether the parser invented it from missing bytes" is a trust-boundary problem worth closing.

Changes

1. Command arg parsing - length precheck + try/catch

Cluster.handleFrame (Cluster.js:780) and BoundCluster.handleFrame (BoundCluster.js:312) now route command-arg parsing through a new helper, lib/util/parseCommandArgs.js:

  • Payload shorter than Math.abs(command.args.length) is rejected before the parser runs.
  • Payload that throws mid-parse (e.g. an octstr length prefix overruns the remaining buffer) is caught and converted to MALFORMED_COMMAND instead of leaking a RangeError as a generic FAILURE.
  • Endpoint.handleFrame already converts thrown ZCLErrors into proper ZCL default-response frames back to the sender, so no surrounding wiring changes are needed.

2. Attribute report (reportAttributes) value-length validation

ZCLAttributeDataRecord.fromBuf in zclFrames.js now validates that enough bytes remain for the value field before calling DataType.fromBuffer. Without this check, a reportAttributes frame carrying a valid attribute header but a truncated value bypassed the command-arg precheck entirely (because reportAttributes declares args as a raw buffer) and re-introduced the same silent zero-fill at the inner record layer. Concretely, a zoneStatus (map16) attribute report missing its value bytes parsed as an all-bits-false bitmap, emitted attr.zoneStatus, and reached IAS-Zone drivers as "all alarms clear".

On insufficient bytes, throws ZCLError('MALFORMED_COMMAND'). Variable-length value fields are not covered by this check (see "Out of scope" below).

3. Tighter encodeMissingFieldsBehavior: 'skip' exemption

The initial precheck fully exempted 'skip' commands (OTA) because they allow trailing fields to be omitted. That was broader than needed: every 'skip' command in the codebase has a mandatory leading discriminator (status / fieldControl / payloadType) whose presence determines which subsequent fields are valid. Without that byte, the parse is meaningless.

The exemption now requires at least the first field's byte width. This preserves the legitimate "omit trailing optional fields" path (e.g. queryNextImageRequest without hardwareVersion) while rejecting empty / sub-leading-byte payloads.

Test plan

All new tests written TDD-style (red first, then implementation).

  • Command path - truncated zoneStatusChangeNotification, truncated zoneEnrollResponse:
    • Handlers not invoked.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed equivalents still invoke the handler with the expected args (regression).
  • Attribute report path - truncated zoneStatus (map16) value:
    • attr.zoneStatus event does not fire.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed zoneStatus report still fires attr.zoneStatus with alarm1 === true (regression).
  • 'skip' exemption - empty queryNextImageRequest:
    • Rejected with MALFORMED_COMMAND.
    • 9-byte queryNextImageRequest without trailing hardwareVersion still parses successfully and reaches the handler with the expected manufacturerCode / imageType (regression - preserves existing OTA tolerance).
  • Full suite: 86/86 passing (76 pre-existing + 10 new). No regressions.
  • Lint clean.

Out of scope

This PR is consumer-side hardening. The deeper fix sits in @athombv/data-types and includes:

  • Fixed-width reads (uintFromBuf, enumFromBuf, dataFromBuf) failing on insufficient bytes instead of returning 0 / a truncated buffer.
  • Variable-length reads (bufferFromBuf) failing instead of returning a truncated buffer with isPartial = true (an undocumented flag no consumer checks - the source even contains a // TODO: FIXME next to it).
  • Bitmap slices not aliasing the source buffer (TOCTOU hazard if a radio driver reuses receive buffers).
  • Struct constructor rejecting prototype-chain keys like constructor as field names.

Two of those (Bitmap defensive copy, Struct prototype-chain guard) are already in flight upstream at athombv/node-data-types#44. The *FromBuf truncation behavior is the breaking change that needs a separate discussion (either a strict opt-in flag or a major-version flip).

As a consequence, this PR does not close:

  • Variable-length attribute / command field overrun via bufferFromBuf's isPartial path. The try/catch in parseCommandArgs catches nothing here because nothing throws.
  • Attribute reports carrying a variable-length value field (e.g. octstr-typed attributes) where the length prefix overruns the remaining buffer.

These can be closed either upstream (cleanest) or by recursing parsed args downstream to reject any Buffer with isPartial === true (less clean, but unblocks without coordination). Tracked separately.

The parser primitives in @athombv/data-types silently substitute zero
values when the source buffer is shorter than the declared field length.
For a ZCL command struct, this means a truncated payload parses into an
object where every numeric field is 0 and every bitmap field has all
bits clear - which for alarm-routing clusters (IAS Zone) happens to look
like "all clear" and for status-bearing response frames happens to look
like "SUCCESS".
This change adds a length precheck and try/catch around command arg
parsing in Cluster.handleFrame and BoundCluster.handleFrame. Truncated
payloads now throw ZCLError('MALFORMED_COMMAND'), which Endpoint.handleFrame
already converts into a proper default-response back to the sender.
Commands declared with encodeMissingFieldsBehavior: 'skip' (OTA) are
exempt from the strict length precheck because they intentionally allow
trailing fields to be omitted; for those, only the try/catch fallback
applies.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens inbound ZCL command argument parsing so truncated/unparseable command payloads are rejected with ZCLError('MALFORMED_COMMAND') instead of being silently parsed (often as all-zero “SUCCESS”/“all-clear” values). This leverages existing Endpoint.handleFrame behavior to automatically emit a proper ZCL Default Response without additional wiring.

Changes:

  • Added a shared parseCommandArgs() helper that performs a minimum-length precheck (when applicable) and converts parse-time exceptions into MALFORMED_COMMAND.
  • Updated Cluster.handleFrame and BoundCluster.handleFrame to use the new helper instead of calling command.args.fromBuffer(...) directly.
  • Added tests to ensure truncated IAS Zone frames don’t invoke handlers and do produce MALFORMED_COMMAND Default Responses (with regressions for well-formed frames).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

FileDescription
test/testMalformedFrames.jsAdds regression + hardening tests verifying truncated payloads yield MALFORMED_COMMAND and do not call handlers.
lib/util/parseCommandArgs.jsIntroduces defensive argument parsing helper with length precheck + parse error normalization.
lib/Cluster.jsRoutes inbound cluster command arg parsing through parseCommandArgs().
lib/BoundCluster.jsRoutes inbound bound-cluster command arg parsing through parseCommandArgs().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/Cluster.js Outdated
Comment threadtest/testMalformedFrames.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@RobinBol@nozols
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(cluster): reject truncated command payloads with MALFORMED_COMMAND by RobinBol · Pull Request #193 · athombv/node-zigbee-clusters · GitHub
Skip to content

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND - #193

Open
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening
Open

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND#193
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening

Conversation

@RobinBol

@RobinBolRobinBol commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens command-argument and attribute-record parsing against truncated wire input. Without these checks, @athombv/data-types silently zero-fills missing bytes, producing a plausible-looking parse result that:

  • For IAS Zone zoneStatusChangeNotification and attribute reports of zoneStatus: reads as zoneStatus.alarm1 === false (i.e. "all clear" while truncated frames look indistinguishable from a real "no alarm" report at the consumer layer).
  • For any status-bearing response: reads as status === 'SUCCESS' (because the zero value is what every ZCL status enum maps to SUCCESS).

This is a robustness / defense-in-depth change. The framing is correctness-first: safety-critical cluster handlers should never observe args from a payload too short to populate them, and "downstream code cannot tell whether the device reported a valid false value or whether the parser invented it from missing bytes" is a trust-boundary problem worth closing.

Changes

1. Command arg parsing - length precheck + try/catch

Cluster.handleFrame (Cluster.js:780) and BoundCluster.handleFrame (BoundCluster.js:312) now route command-arg parsing through a new helper, lib/util/parseCommandArgs.js:

  • Payload shorter than Math.abs(command.args.length) is rejected before the parser runs.
  • Payload that throws mid-parse (e.g. an octstr length prefix overruns the remaining buffer) is caught and converted to MALFORMED_COMMAND instead of leaking a RangeError as a generic FAILURE.
  • Endpoint.handleFrame already converts thrown ZCLErrors into proper ZCL default-response frames back to the sender, so no surrounding wiring changes are needed.

2. Attribute report (reportAttributes) value-length validation

ZCLAttributeDataRecord.fromBuf in zclFrames.js now validates that enough bytes remain for the value field before calling DataType.fromBuffer. Without this check, a reportAttributes frame carrying a valid attribute header but a truncated value bypassed the command-arg precheck entirely (because reportAttributes declares args as a raw buffer) and re-introduced the same silent zero-fill at the inner record layer. Concretely, a zoneStatus (map16) attribute report missing its value bytes parsed as an all-bits-false bitmap, emitted attr.zoneStatus, and reached IAS-Zone drivers as "all alarms clear".

On insufficient bytes, throws ZCLError('MALFORMED_COMMAND'). Variable-length value fields are not covered by this check (see "Out of scope" below).

3. Tighter encodeMissingFieldsBehavior: 'skip' exemption

The initial precheck fully exempted 'skip' commands (OTA) because they allow trailing fields to be omitted. That was broader than needed: every 'skip' command in the codebase has a mandatory leading discriminator (status / fieldControl / payloadType) whose presence determines which subsequent fields are valid. Without that byte, the parse is meaningless.

The exemption now requires at least the first field's byte width. This preserves the legitimate "omit trailing optional fields" path (e.g. queryNextImageRequest without hardwareVersion) while rejecting empty / sub-leading-byte payloads.

Test plan

All new tests written TDD-style (red first, then implementation).

  • Command path - truncated zoneStatusChangeNotification, truncated zoneEnrollResponse:
    • Handlers not invoked.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed equivalents still invoke the handler with the expected args (regression).
  • Attribute report path - truncated zoneStatus (map16) value:
    • attr.zoneStatus event does not fire.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed zoneStatus report still fires attr.zoneStatus with alarm1 === true (regression).
  • 'skip' exemption - empty queryNextImageRequest:
    • Rejected with MALFORMED_COMMAND.
    • 9-byte queryNextImageRequest without trailing hardwareVersion still parses successfully and reaches the handler with the expected manufacturerCode / imageType (regression - preserves existing OTA tolerance).
  • Full suite: 86/86 passing (76 pre-existing + 10 new). No regressions.
  • Lint clean.

Out of scope

This PR is consumer-side hardening. The deeper fix sits in @athombv/data-types and includes:

  • Fixed-width reads (uintFromBuf, enumFromBuf, dataFromBuf) failing on insufficient bytes instead of returning 0 / a truncated buffer.
  • Variable-length reads (bufferFromBuf) failing instead of returning a truncated buffer with isPartial = true (an undocumented flag no consumer checks - the source even contains a // TODO: FIXME next to it).
  • Bitmap slices not aliasing the source buffer (TOCTOU hazard if a radio driver reuses receive buffers).
  • Struct constructor rejecting prototype-chain keys like constructor as field names.

Two of those (Bitmap defensive copy, Struct prototype-chain guard) are already in flight upstream at athombv/node-data-types#44. The *FromBuf truncation behavior is the breaking change that needs a separate discussion (either a strict opt-in flag or a major-version flip).

As a consequence, this PR does not close:

  • Variable-length attribute / command field overrun via bufferFromBuf's isPartial path. The try/catch in parseCommandArgs catches nothing here because nothing throws.
  • Attribute reports carrying a variable-length value field (e.g. octstr-typed attributes) where the length prefix overruns the remaining buffer.

These can be closed either upstream (cleanest) or by recursing parsed args downstream to reject any Buffer with isPartial === true (less clean, but unblocks without coordination). Tracked separately.

The parser primitives in @athombv/data-types silently substitute zero
values when the source buffer is shorter than the declared field length.
For a ZCL command struct, this means a truncated payload parses into an
object where every numeric field is 0 and every bitmap field has all
bits clear - which for alarm-routing clusters (IAS Zone) happens to look
like "all clear" and for status-bearing response frames happens to look
like "SUCCESS".
This change adds a length precheck and try/catch around command arg
parsing in Cluster.handleFrame and BoundCluster.handleFrame. Truncated
payloads now throw ZCLError('MALFORMED_COMMAND'), which Endpoint.handleFrame
already converts into a proper default-response back to the sender.
Commands declared with encodeMissingFieldsBehavior: 'skip' (OTA) are
exempt from the strict length precheck because they intentionally allow
trailing fields to be omitted; for those, only the try/catch fallback
applies.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens inbound ZCL command argument parsing so truncated/unparseable command payloads are rejected with ZCLError('MALFORMED_COMMAND') instead of being silently parsed (often as all-zero “SUCCESS”/“all-clear” values). This leverages existing Endpoint.handleFrame behavior to automatically emit a proper ZCL Default Response without additional wiring.

Changes:

  • Added a shared parseCommandArgs() helper that performs a minimum-length precheck (when applicable) and converts parse-time exceptions into MALFORMED_COMMAND.
  • Updated Cluster.handleFrame and BoundCluster.handleFrame to use the new helper instead of calling command.args.fromBuffer(...) directly.
  • Added tests to ensure truncated IAS Zone frames don’t invoke handlers and do produce MALFORMED_COMMAND Default Responses (with regressions for well-formed frames).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

FileDescription
test/testMalformedFrames.jsAdds regression + hardening tests verifying truncated payloads yield MALFORMED_COMMAND and do not call handlers.
lib/util/parseCommandArgs.jsIntroduces defensive argument parsing helper with length precheck + parse error normalization.
lib/Cluster.jsRoutes inbound cluster command arg parsing through parseCommandArgs().
lib/BoundCluster.jsRoutes inbound bound-cluster command arg parsing through parseCommandArgs().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/Cluster.js Outdated
Comment threadtest/testMalformedFrames.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@RobinBol@nozols
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(cluster): reject truncated command payloads with MALFORMED_COMMAND by RobinBol · Pull Request #193 · athombv/node-zigbee-clusters · GitHub
Skip to content

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND - #193

Open
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening
Open

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND#193
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening

Conversation

@RobinBol

@RobinBolRobinBol commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens command-argument and attribute-record parsing against truncated wire input. Without these checks, @athombv/data-types silently zero-fills missing bytes, producing a plausible-looking parse result that:

  • For IAS Zone zoneStatusChangeNotification and attribute reports of zoneStatus: reads as zoneStatus.alarm1 === false (i.e. "all clear" while truncated frames look indistinguishable from a real "no alarm" report at the consumer layer).
  • For any status-bearing response: reads as status === 'SUCCESS' (because the zero value is what every ZCL status enum maps to SUCCESS).

This is a robustness / defense-in-depth change. The framing is correctness-first: safety-critical cluster handlers should never observe args from a payload too short to populate them, and "downstream code cannot tell whether the device reported a valid false value or whether the parser invented it from missing bytes" is a trust-boundary problem worth closing.

Changes

1. Command arg parsing - length precheck + try/catch

Cluster.handleFrame (Cluster.js:780) and BoundCluster.handleFrame (BoundCluster.js:312) now route command-arg parsing through a new helper, lib/util/parseCommandArgs.js:

  • Payload shorter than Math.abs(command.args.length) is rejected before the parser runs.
  • Payload that throws mid-parse (e.g. an octstr length prefix overruns the remaining buffer) is caught and converted to MALFORMED_COMMAND instead of leaking a RangeError as a generic FAILURE.
  • Endpoint.handleFrame already converts thrown ZCLErrors into proper ZCL default-response frames back to the sender, so no surrounding wiring changes are needed.

2. Attribute report (reportAttributes) value-length validation

ZCLAttributeDataRecord.fromBuf in zclFrames.js now validates that enough bytes remain for the value field before calling DataType.fromBuffer. Without this check, a reportAttributes frame carrying a valid attribute header but a truncated value bypassed the command-arg precheck entirely (because reportAttributes declares args as a raw buffer) and re-introduced the same silent zero-fill at the inner record layer. Concretely, a zoneStatus (map16) attribute report missing its value bytes parsed as an all-bits-false bitmap, emitted attr.zoneStatus, and reached IAS-Zone drivers as "all alarms clear".

On insufficient bytes, throws ZCLError('MALFORMED_COMMAND'). Variable-length value fields are not covered by this check (see "Out of scope" below).

3. Tighter encodeMissingFieldsBehavior: 'skip' exemption

The initial precheck fully exempted 'skip' commands (OTA) because they allow trailing fields to be omitted. That was broader than needed: every 'skip' command in the codebase has a mandatory leading discriminator (status / fieldControl / payloadType) whose presence determines which subsequent fields are valid. Without that byte, the parse is meaningless.

The exemption now requires at least the first field's byte width. This preserves the legitimate "omit trailing optional fields" path (e.g. queryNextImageRequest without hardwareVersion) while rejecting empty / sub-leading-byte payloads.

Test plan

All new tests written TDD-style (red first, then implementation).

  • Command path - truncated zoneStatusChangeNotification, truncated zoneEnrollResponse:
    • Handlers not invoked.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed equivalents still invoke the handler with the expected args (regression).
  • Attribute report path - truncated zoneStatus (map16) value:
    • attr.zoneStatus event does not fire.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed zoneStatus report still fires attr.zoneStatus with alarm1 === true (regression).
  • 'skip' exemption - empty queryNextImageRequest:
    • Rejected with MALFORMED_COMMAND.
    • 9-byte queryNextImageRequest without trailing hardwareVersion still parses successfully and reaches the handler with the expected manufacturerCode / imageType (regression - preserves existing OTA tolerance).
  • Full suite: 86/86 passing (76 pre-existing + 10 new). No regressions.
  • Lint clean.

Out of scope

This PR is consumer-side hardening. The deeper fix sits in @athombv/data-types and includes:

  • Fixed-width reads (uintFromBuf, enumFromBuf, dataFromBuf) failing on insufficient bytes instead of returning 0 / a truncated buffer.
  • Variable-length reads (bufferFromBuf) failing instead of returning a truncated buffer with isPartial = true (an undocumented flag no consumer checks - the source even contains a // TODO: FIXME next to it).
  • Bitmap slices not aliasing the source buffer (TOCTOU hazard if a radio driver reuses receive buffers).
  • Struct constructor rejecting prototype-chain keys like constructor as field names.

Two of those (Bitmap defensive copy, Struct prototype-chain guard) are already in flight upstream at athombv/node-data-types#44. The *FromBuf truncation behavior is the breaking change that needs a separate discussion (either a strict opt-in flag or a major-version flip).

As a consequence, this PR does not close:

  • Variable-length attribute / command field overrun via bufferFromBuf's isPartial path. The try/catch in parseCommandArgs catches nothing here because nothing throws.
  • Attribute reports carrying a variable-length value field (e.g. octstr-typed attributes) where the length prefix overruns the remaining buffer.

These can be closed either upstream (cleanest) or by recursing parsed args downstream to reject any Buffer with isPartial === true (less clean, but unblocks without coordination). Tracked separately.

The parser primitives in @athombv/data-types silently substitute zero
values when the source buffer is shorter than the declared field length.
For a ZCL command struct, this means a truncated payload parses into an
object where every numeric field is 0 and every bitmap field has all
bits clear - which for alarm-routing clusters (IAS Zone) happens to look
like "all clear" and for status-bearing response frames happens to look
like "SUCCESS".
This change adds a length precheck and try/catch around command arg
parsing in Cluster.handleFrame and BoundCluster.handleFrame. Truncated
payloads now throw ZCLError('MALFORMED_COMMAND'), which Endpoint.handleFrame
already converts into a proper default-response back to the sender.
Commands declared with encodeMissingFieldsBehavior: 'skip' (OTA) are
exempt from the strict length precheck because they intentionally allow
trailing fields to be omitted; for those, only the try/catch fallback
applies.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens inbound ZCL command argument parsing so truncated/unparseable command payloads are rejected with ZCLError('MALFORMED_COMMAND') instead of being silently parsed (often as all-zero “SUCCESS”/“all-clear” values). This leverages existing Endpoint.handleFrame behavior to automatically emit a proper ZCL Default Response without additional wiring.

Changes:

  • Added a shared parseCommandArgs() helper that performs a minimum-length precheck (when applicable) and converts parse-time exceptions into MALFORMED_COMMAND.
  • Updated Cluster.handleFrame and BoundCluster.handleFrame to use the new helper instead of calling command.args.fromBuffer(...) directly.
  • Added tests to ensure truncated IAS Zone frames don’t invoke handlers and do produce MALFORMED_COMMAND Default Responses (with regressions for well-formed frames).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

FileDescription
test/testMalformedFrames.jsAdds regression + hardening tests verifying truncated payloads yield MALFORMED_COMMAND and do not call handlers.
lib/util/parseCommandArgs.jsIntroduces defensive argument parsing helper with length precheck + parse error normalization.
lib/Cluster.jsRoutes inbound cluster command arg parsing through parseCommandArgs().
lib/BoundCluster.jsRoutes inbound bound-cluster command arg parsing through parseCommandArgs().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/Cluster.js Outdated
Comment threadtest/testMalformedFrames.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@RobinBol@nozols
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(cluster): reject truncated command payloads with MALFORMED_COMMAND by RobinBol · Pull Request #193 · athombv/node-zigbee-clusters · GitHub
Skip to content

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND - #193

Open
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening
Open

fix(cluster): reject truncated command payloads with MALFORMED_COMMAND#193
RobinBol wants to merge 3 commits into
masterfrom
fix/malformed-frame-hardening

Conversation

@RobinBol

@RobinBolRobinBol commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens command-argument and attribute-record parsing against truncated wire input. Without these checks, @athombv/data-types silently zero-fills missing bytes, producing a plausible-looking parse result that:

  • For IAS Zone zoneStatusChangeNotification and attribute reports of zoneStatus: reads as zoneStatus.alarm1 === false (i.e. "all clear" while truncated frames look indistinguishable from a real "no alarm" report at the consumer layer).
  • For any status-bearing response: reads as status === 'SUCCESS' (because the zero value is what every ZCL status enum maps to SUCCESS).

This is a robustness / defense-in-depth change. The framing is correctness-first: safety-critical cluster handlers should never observe args from a payload too short to populate them, and "downstream code cannot tell whether the device reported a valid false value or whether the parser invented it from missing bytes" is a trust-boundary problem worth closing.

Changes

1. Command arg parsing - length precheck + try/catch

Cluster.handleFrame (Cluster.js:780) and BoundCluster.handleFrame (BoundCluster.js:312) now route command-arg parsing through a new helper, lib/util/parseCommandArgs.js:

  • Payload shorter than Math.abs(command.args.length) is rejected before the parser runs.
  • Payload that throws mid-parse (e.g. an octstr length prefix overruns the remaining buffer) is caught and converted to MALFORMED_COMMAND instead of leaking a RangeError as a generic FAILURE.
  • Endpoint.handleFrame already converts thrown ZCLErrors into proper ZCL default-response frames back to the sender, so no surrounding wiring changes are needed.

2. Attribute report (reportAttributes) value-length validation

ZCLAttributeDataRecord.fromBuf in zclFrames.js now validates that enough bytes remain for the value field before calling DataType.fromBuffer. Without this check, a reportAttributes frame carrying a valid attribute header but a truncated value bypassed the command-arg precheck entirely (because reportAttributes declares args as a raw buffer) and re-introduced the same silent zero-fill at the inner record layer. Concretely, a zoneStatus (map16) attribute report missing its value bytes parsed as an all-bits-false bitmap, emitted attr.zoneStatus, and reached IAS-Zone drivers as "all alarms clear".

On insufficient bytes, throws ZCLError('MALFORMED_COMMAND'). Variable-length value fields are not covered by this check (see "Out of scope" below).

3. Tighter encodeMissingFieldsBehavior: 'skip' exemption

The initial precheck fully exempted 'skip' commands (OTA) because they allow trailing fields to be omitted. That was broader than needed: every 'skip' command in the codebase has a mandatory leading discriminator (status / fieldControl / payloadType) whose presence determines which subsequent fields are valid. Without that byte, the parse is meaningless.

The exemption now requires at least the first field's byte width. This preserves the legitimate "omit trailing optional fields" path (e.g. queryNextImageRequest without hardwareVersion) while rejecting empty / sub-leading-byte payloads.

Test plan

All new tests written TDD-style (red first, then implementation).

  • Command path - truncated zoneStatusChangeNotification, truncated zoneEnrollResponse:
    • Handlers not invoked.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed equivalents still invoke the handler with the expected args (regression).
  • Attribute report path - truncated zoneStatus (map16) value:
    • attr.zoneStatus event does not fire.
    • Sender receives MALFORMED_COMMAND default-response.
    • Well-formed zoneStatus report still fires attr.zoneStatus with alarm1 === true (regression).
  • 'skip' exemption - empty queryNextImageRequest:
    • Rejected with MALFORMED_COMMAND.
    • 9-byte queryNextImageRequest without trailing hardwareVersion still parses successfully and reaches the handler with the expected manufacturerCode / imageType (regression - preserves existing OTA tolerance).
  • Full suite: 86/86 passing (76 pre-existing + 10 new). No regressions.
  • Lint clean.

Out of scope

This PR is consumer-side hardening. The deeper fix sits in @athombv/data-types and includes:

  • Fixed-width reads (uintFromBuf, enumFromBuf, dataFromBuf) failing on insufficient bytes instead of returning 0 / a truncated buffer.
  • Variable-length reads (bufferFromBuf) failing instead of returning a truncated buffer with isPartial = true (an undocumented flag no consumer checks - the source even contains a // TODO: FIXME next to it).
  • Bitmap slices not aliasing the source buffer (TOCTOU hazard if a radio driver reuses receive buffers).
  • Struct constructor rejecting prototype-chain keys like constructor as field names.

Two of those (Bitmap defensive copy, Struct prototype-chain guard) are already in flight upstream at athombv/node-data-types#44. The *FromBuf truncation behavior is the breaking change that needs a separate discussion (either a strict opt-in flag or a major-version flip).

As a consequence, this PR does not close:

  • Variable-length attribute / command field overrun via bufferFromBuf's isPartial path. The try/catch in parseCommandArgs catches nothing here because nothing throws.
  • Attribute reports carrying a variable-length value field (e.g. octstr-typed attributes) where the length prefix overruns the remaining buffer.

These can be closed either upstream (cleanest) or by recursing parsed args downstream to reject any Buffer with isPartial === true (less clean, but unblocks without coordination). Tracked separately.

The parser primitives in @athombv/data-types silently substitute zero
values when the source buffer is shorter than the declared field length.
For a ZCL command struct, this means a truncated payload parses into an
object where every numeric field is 0 and every bitmap field has all
bits clear - which for alarm-routing clusters (IAS Zone) happens to look
like "all clear" and for status-bearing response frames happens to look
like "SUCCESS".
This change adds a length precheck and try/catch around command arg
parsing in Cluster.handleFrame and BoundCluster.handleFrame. Truncated
payloads now throw ZCLError('MALFORMED_COMMAND'), which Endpoint.handleFrame
already converts into a proper default-response back to the sender.
Commands declared with encodeMissingFieldsBehavior: 'skip' (OTA) are
exempt from the strict length precheck because they intentionally allow
trailing fields to be omitted; for those, only the try/catch fallback
applies.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens inbound ZCL command argument parsing so truncated/unparseable command payloads are rejected with ZCLError('MALFORMED_COMMAND') instead of being silently parsed (often as all-zero “SUCCESS”/“all-clear” values). This leverages existing Endpoint.handleFrame behavior to automatically emit a proper ZCL Default Response without additional wiring.

Changes:

  • Added a shared parseCommandArgs() helper that performs a minimum-length precheck (when applicable) and converts parse-time exceptions into MALFORMED_COMMAND.
  • Updated Cluster.handleFrame and BoundCluster.handleFrame to use the new helper instead of calling command.args.fromBuffer(...) directly.
  • Added tests to ensure truncated IAS Zone frames don’t invoke handlers and do produce MALFORMED_COMMAND Default Responses (with regressions for well-formed frames).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

FileDescription
test/testMalformedFrames.jsAdds regression + hardening tests verifying truncated payloads yield MALFORMED_COMMAND and do not call handlers.
lib/util/parseCommandArgs.jsIntroduces defensive argument parsing helper with length precheck + parse error normalization.
lib/Cluster.jsRoutes inbound cluster command arg parsing through parseCommandArgs().
lib/BoundCluster.jsRoutes inbound bound-cluster command arg parsing through parseCommandArgs().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/Cluster.js Outdated
Comment threadtest/testMalformedFrames.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@RobinBol@nozols