fix: prevent signed integer overflow in bacnet_enclosed_data_length() by moroznah · Pull Request #1468 · bacnet-stack/bacnet-stack

moroznah · 2026-08-13T12:48:05Z

fix: prevent signed integer overflow in `bacnet_enclosed_data_length()`

bacnet_enclosed_data_length() guards each tag's length value against INT_MAX
and then adds it to a running length that is already non-zero. The guard
therefore bounds the addend and never the sum, and three separate accumulations
can overflow on attacker-controlled input.

Found by fuzzing a BACnet client's RPM-ack decode path under
UndefinedBehaviorSanitizer, then reduced to a hand-written APDU.

The defect

src/bacnet/bacdcode.c, in the do { … } while (opening_tag_number_counter > 0)
loop:

line	statement	why it overflows
`:869`	`len += tag.len_value_type;`	`len` already holds the tag header size; `uint32_t` addend converted to `int`
`:877`	`len += bacnet_application_data_length(tag.number, tag.len_value_type);`	same, and the callee returns `INT_MAX` itself when the length is out of range
`:884`	`total_len += len;`	accumulates the value produced above
`:890`	`apdu_len += len;`	`apdu_len` already holds the opening tag

The guard in both branches is

if (tag.len_value_type > INT_MAX) {
    /* error: length is out of range */
    return BACNET_STATUS_ERROR;
}

INT_MAX promotes to uint32_t here, so only 0x80000000..0xFFFFFFFF is
rejected — 0x7FFFFFFF passes, and len is already 6 for an extended
32-bit length tag, so 6 + 2147483647 overflows int.

len_value_type is taken verbatim off the wire by bacnet_tag_decode()
(bacdcode.c:526-531) with no bound against apdu_size, so a peer controls it
across the full uint32_t range.

Reachability

Every caller is a service decoder reached from unauthenticated network input:
rp.c:492, rpm.c:743, wp.c:264, wpm.c:172, cov.c:462,
readrange.c:665, ptransfer.c:211,432, create_object.c:208,449,
list_element.c:185, bacapp.c:1934,5675,5817, timer_value.c:427, and
basic/service/h_rpm_a.c:100. The path this was found on is a client decoding
a ReadPropertyMultiple ACK returned by a field device.

Reproducer

APDU: opening tag [0], one application tag (unsigned, tag 2) with extended
length 0x7FFFFFFF, closing tag [0] — 8 octets:

0E 25 FF 7F FF FF FF 0F

Built with clang -fsanitize=undefined,implicit-conversion over
bacdcode.c bacstr.c bacint.c bacreal.c at c19860e77:

bacdcode.c:877:17: runtime error: signed integer overflow: 6 + 2147483647
    cannot be represented in type 'int'
bacdcode.c:890:22: runtime error: signed integer overflow: 1 + 2147483647
    cannot be represented in type 'int'
bacdcode.c:891:30: runtime error: implicit conversion from type 'int' of value
    -2147483648 (32-bit, signed) to type 'size_t' changed the value to
    18446744071562067968 (64-bit, unsigned)
bacdcode.c:869:17: runtime error: implicit conversion from type 'uint32_t' of
    value 2147483653 (32-bit, unsigned) to type 'int' changed the value to
    -2147483643 (32-bit, signed)

(:869 is the context-tag branch, driven by the same APDU with a context tag.)

The fix

Bound the sums rather than the addends, rejecting in exactly the place the
old guards rejected:

if (tag.len_value_type > (uint32_t)(INT_MAX - len)) {
    return BACNET_STATUS_ERROR;
}

plus the same shape for total_len += len and apdu_len += len. len comes
from bacnet_tag_decode(), which returns 0 or a small positive count, so
INT_MAX - len cannot itself overflow.

Valid input is unaffected — the guard only fires where the old code would have
wrapped.

How much this matters — stated plainly

This is undefined behaviour that current code generation happens to render
harmless. The wrapped value is negative, so the existing if (len > 0)
(:882) and apdu_size <= apdu_len (:891) checks still return
BACNET_STATUS_ERROR; I could not produce a wrong length or a memory error on
any compiler I tried. Verified across the probe inputs, before and after:

application tag  lvt=0x7FFFFFFF -> -1      context tag  lvt=0x7FFFFFFF -> -1
application tag  lvt=0x7FFFFFFA -> -1      context tag  lvt=0x7FFFFFFA -> -1
application tag  lvt=0x7FFFFFF9 -> -1      context tag  lvt=0x80000000 -> -1
application tag  lvt=0x80000000 -> -1
application tag  lvt=0xFFFFFFFF -> -1
valid unsigned   apdu_len=4     ->  2      valid nested apdu_len=24    -> 22

So this is a hardening fix, not an exploitable bug report. The reason to take
it anyway is that the checks currently saving it are exactly the checks a
compiler may delete, having been told signed overflow cannot happen — and this
function sits on the unauthenticated decode path of eleven services.

I'd rather say that up front than dress it up.

Tests

Three cases added to test_bacnet_enclosed_data_length in
test/bacnet/bacdcode/src/main.c, each asserting BACNET_STATUS_ERROR:
an application tag at INT_MAX, a context tag at INT_MAX, and an application
tag at INT_MAX - 6 (the value whose data length fits len exactly, so that it
is the enclosing apdu_len that would overflow).

These pass before and after the fix on current compilers — consistent with
the section above; what changes is that the sanitizer stops reporting. They are
there to lock the rejection in, not to prove the bug.

$ cd test/build && cmake .. && cmake --build . --target test_bacdcode
$ ./bacnet/bacdcode/test_bacdcode
Test suite bacdcode_tests succeeded
PROJECT EXECUTION SUCCESSFUL

clang-format --style=file --dry-run is clean on both changed files.

Relationship to #1466

#1466 fixed a different overflow in this same function — uint8_t opening_tag_number_counter wrapping at 256 nested tags. That change is
untouched here; this one is the length arithmetic in the same loop. The
CHANGELOG entry is worded to keep the two distinguishable, and is added without
a PR number — happy to fill it in, or drop it if you would rather write it
yourself.

Also verified while here (no change requested)

Independently of this PR, three MS/TP reproducers from the same fuzzing run hit
the COBS decode overflow you fixed in #1425 / GHSA-8456-m9x4-j6mc. Against
bacnet-stack-1.6.0 all three produce an ASan heap-buffer-overflow WRITE in
cobs_decode; with the bacnet-stack-1.6 branch's two-line fix applied, all
three run clean and still decode their frames (20, 62 and 204 frames
respectively). The fix is good — this is just confirmation from an independent
corpus.

One note in case it is useful: the bacnet-stack-1.6 branch currently carries
only #1425, while SECURITY.md lists 1.6.1 as a patched version for nine other
advisories that were backported to the 1.4 and 1.5 branches but not to 1.6.
Anyone pinned to the bacnet-stack-1.6.0 tag is missing those.

bacnet_enclosed_data_length() guards each tag's length value against INT_MAX, then adds it to a running length that is already non-zero - so the guard bounds the addend and never the sum: * len already holds this tag's header size (up to 6 octets for an extended 32-bit length) when the data length is added at bacdcode.c:869 and :877, so a tag claiming INT_MAX overflows int; * apdu_len already holds the opening tag when len is added at :890; * total_len accumulates the same len at :884. len_value_type is taken straight off the wire by bacnet_tag_decode() with no bound against apdu_size, so a peer controls it up to 0xFFFFFFFF and 0x7FFFFFFF passes the existing "> INT_MAX" guard. Note also that bacnet_application_data_length() returns INT_MAX itself for an out-of-range length, which is what reaches the addition at :877. Every caller is a service decoder reached from unauthenticated network input: rp.c, rpm.c, wp.c, wpm.c, cov.c, readrange.c, ptransfer.c, create_object.c, list_element.c and basic/service/h_rpm_a.c. Reproduced with clang -fsanitize=undefined,implicit-conversion on the APDU [0] <application tag 2, extended length 0x7FFFFFFF> [0]: bacdcode.c:877:17: runtime error: signed integer overflow: 6 + 2147483647 cannot be represented in type 'int' bacdcode.c:890:22: runtime error: signed integer overflow: 1 + 2147483647 cannot be represented in type 'int' bacdcode.c:891:30: runtime error: implicit conversion from type 'int' of value -2147483648 changed the value to 18446744071562067968 bacdcode.c:869:17: runtime error: implicit conversion from type 'uint32_t' of value 2147483653 changed the value to -2147483643 The fix bounds the sums instead of the addends, and rejects with BACNET_STATUS_ERROR exactly where the old guards did. Scope, stated honestly: on current compilers the wrapped value is negative and the existing "len > 0" and "apdu_size <= apdu_len" checks still return BACNET_STATUS_ERROR, so there is no observable misbehaviour today. Those checks are, however, precisely what a compiler is entitled to drop once it has been told signed overflow cannot happen - which is why this is worth closing rather than relying on. The added test cases lock the rejection in. They pass before and after the fix on current codegen; what changes is that the sanitizer no longer reports. Signed-off-by: Igor Sakulin <fxmoroz@gmail.com>

Copilot

Pull request overview

Hardens the BACnet tag-length accumulation logic in bacnet_enclosed_data_length() to avoid signed integer overflow/UB on attacker-controlled tag lengths during APDU decode, and adds regression coverage to keep the rejection behavior stable.

Changes:

Add overflow checks that bound the accumulated sums (len, total_len, apdu_len) before each addition in bacnet_enclosed_data_length().
Add unit tests covering INT_MAX-adjacent application/context tag lengths to prevent sanitizer-reported UB regressions.
Document the hardening change in the Security section of the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/bacnet/bacdcode.c	Adds sum-bounding overflow guards in `bacnet_enclosed_data_length()` before length accumulations.
test/bacnet/bacdcode/src/main.c	Adds regression tests that assert `BACNET_STATUS_ERROR` for INT_MAX-edge tag length cases.
CHANGELOG.md	Adds a Security changelog entry describing the signed-overflow hardening.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…#1468) bacnet_enclosed_data_length() guards each tag's length value against INT_MAX, then adds it to a running length that is already non-zero - so the guard bounds the addend and never the sum: * len already holds this tag's header size (up to 6 octets for an extended 32-bit length) when the data length is added at bacdcode.c:869 and :877, so a tag claiming INT_MAX overflows int; * apdu_len already holds the opening tag when len is added at :890; * total_len accumulates the same len at :884. len_value_type is taken straight off the wire by bacnet_tag_decode() with no bound against apdu_size, so a peer controls it up to 0xFFFFFFFF and 0x7FFFFFFF passes the existing "> INT_MAX" guard. Note also that bacnet_application_data_length() returns INT_MAX itself for an out-of-range length, which is what reaches the addition at :877. Every caller is a service decoder reached from unauthenticated network input: rp.c, rpm.c, wp.c, wpm.c, cov.c, readrange.c, ptransfer.c, create_object.c, list_element.c and basic/service/h_rpm_a.c. Reproduced with clang -fsanitize=undefined,implicit-conversion on the APDU [0] <application tag 2, extended length 0x7FFFFFFF> [0]: bacdcode.c:877:17: runtime error: signed integer overflow: 6 + 2147483647 cannot be represented in type 'int' bacdcode.c:890:22: runtime error: signed integer overflow: 1 + 2147483647 cannot be represented in type 'int' bacdcode.c:891:30: runtime error: implicit conversion from type 'int' of value -2147483648 changed the value to 18446744071562067968 bacdcode.c:869:17: runtime error: implicit conversion from type 'uint32_t' of value 2147483653 changed the value to -2147483643 The fix bounds the sums instead of the addends, and rejects with BACNET_STATUS_ERROR exactly where the old guards did. Scope, stated honestly: on current compilers the wrapped value is negative and the existing "len > 0" and "apdu_size <= apdu_len" checks still return BACNET_STATUS_ERROR, so there is no observable misbehaviour today. Those checks are, however, precisely what a compiler is entitled to drop once it has been told signed overflow cannot happen - which is why this is worth closing rather than relying on. The added test cases lock the rejection in. They pass before and after the fix on current codegen; what changes is that the sanitizer no longer reports. Signed-off-by: Igor Sakulin <fxmoroz@gmail.com>

skarg approved these changes Aug 13, 2026

View reviewed changes

skarg requested a lite review from Copilot August 13, 2026 19:05

Copilot started reviewing on behalf of skarg August 13, 2026 19:06 View session

Copilot AI reviewed Aug 13, 2026

View reviewed changes

Comment thread CHANGELOG.md

skarg merged commit 30fcea8 into bacnet-stack:master Aug 13, 2026
36 checks passed

Navigation Menu

Uh oh!

fix: prevent signed integer overflow in bacnet_enclosed_data_length() - #1468

fix: prevent signed integer overflow in bacnet_enclosed_data_length()#1468
skarg merged 1 commit into
bacnet-stack:masterfrom
moroznah:bugfix/enclosed-data-length-overflow

moroznah commented Aug 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

moroznah commented Aug 13, 2026

fix: prevent signed integer overflow in bacnet_enclosed_data_length()

The defect

Reachability

Reproducer

The fix

How much this matters — stated plainly

Tests

Relationship to #1466

Also verified while here (no change requested)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: prevent signed integer overflow in `bacnet_enclosed_data_length()`