Secure BACnet decoders and service requests - #1244
Merged
skarg merged 36 commits intoFeb 26, 2026
Merged
Conversation
…en recipient address is missing the router MAC address.
…er-Request decoders.
Base automatically changed from
feature/notification-class-who-is-router-for-address-recipients
to
master
February 25, 2026 15:58
…ers-by-replacing-deprecated-functions
…d list structure into bacdest where the recipient encoder and decoder already existed.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to harden multiple BACnet service/type decoders by replacing deprecated/unbounded decode helpers with size-aware decoding APIs, and updates unit tests and service handlers accordingly.
Changes:
- Replace several legacy tag/primitive decode patterns with
bacnet_*_decode(..., apdu_size, ...)style routines to improve bounds checking. - Introduce/standardize
BACNET_RECIPIENT_LISTviabacdest.hand update TimeSync recipient list encoding/decoding and related handlers/tests. - Expand/adjust tests for WPM, TimeSync, and Private Transfer to validate updated encode/decode behavior.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| test/bacnet/wpm/src/main.c | Adds additional WPM decode/ack coverage (including NULL-length checks). |
| test/bacnet/timesync/src/main.c | Refactors recipient list tests to use shared recipient helpers and adds negative/short-length cases. |
| test/bacnet/ptransfer/src/main.c | Updates Private Transfer decode helpers and strengthens length assertions. |
| src/bacnet/wpm.h | Exposes new WPM error-ack service-parameter encoder. |
| src/bacnet/wpm.c | Reworks WPM decoders/encoders to use size-bounded helpers (but contains a closing-tag length bug and potential overflow). |
| src/bacnet/timesync.h | Switches to bacdest.h recipient list type and adds a new encode helper (typoed name). |
| src/bacnet/timesync.c | Adds a shared TimeSync service-parameter encoder and updates recipient list encode/decode implementation. |
| src/bacnet/rpm.c | Updates RPM request decoding to size-bounded helpers. |
| src/bacnet/ptransfer.c | Refactors Private Transfer request/error encoding/decoding to more consistently use bounded primitives. |
| src/bacnet/list_element.c | Replaces deprecated Add/Remove ListElement request decoding with size-aware decode utilities. |
| src/bacnet/ihave.c | Updates I-Have encode/decode to size-bounded helpers (but now can NULL-deref when data == NULL). |
| src/bacnet/event.c | Switches property-state context decoding call site to the new context decode API. |
| src/bacnet/credential_authentication_factor.h | Adds apdu_size parameters to credential-authentication-factor decode APIs. |
| src/bacnet/credential_authentication_factor.c | Updates credential-authentication-factor decode (but still uses an unbounded deprecated nested decoder). |
| src/bacnet/basic/service/h_wpm.c | Uses bacnet_is_opening/closing_tag_number with tag-length outputs and returns decoded offset. |
| src/bacnet/basic/service/h_ts.c | Migrates TimeSync recipients storage to the unified BACNET_RECIPIENT_LIST shape and helpers. |
| src/bacnet/basic/service/h_rpm_a.c | Updates RPM Ack decoding to tag-length-aware helpers (but introduces a pointer-advance bug in error decoding). |
| src/bacnet/basic/service/h_rpm.c | Uses tag-length-aware closing-tag checks in RPM request handling. |
| src/bacnet/basic/service/h_rp_a.c | Uses tag-length-aware closing-tag checks when decoding RP-Ack lists. |
| src/bacnet/basic/object/trendlog.c | Replaces deprecated primitive decoding with tag-based and size-aware decode helpers for Trend Log fetch. |
| src/bacnet/bacpropstates.h | Adds bacapp_property_state_context_decode() and deprecates old context property state decoder. |
| src/bacnet/bacpropstates.c | Implements bacapp_property_state_context_decode() (but currently calls an unbounded deprecated decoder internally). |
| src/bacnet/bacdest.h | Defines BACNET_RECIPIENT_LIST and declares list encode/link helpers. |
| src/bacnet/bacdest.c | Implements recipient-list encode and array-to-linked-list linking helper. |
| src/bacnet/assigned_access_rights.h | Adds apdu_size parameters to assigned-access-rights decode APIs. |
| src/bacnet/assigned_access_rights.c | Updates assigned-access-rights decode implementation (but currently treats len==0 tag mismatches as success). |
| CHANGELOG.md | Documents the security hardening changes for multiple decoders/services. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…SafetyOperation-Request service. Secured BACnetAuthenticationFactor decoding in the Crediential Data Input object.
…iteProperty error reporting by adding specific reject codes during decoding similar to WritePropertyMultiple. Deduplicated the WriteProperty handling of abort, reject and error codes.
…cated decoding functions to ensure they are not used except intentionally for legacy code bases.
skarg
deleted the
bugfix/secure-decoders-by-replacing-deprecated-functions
branch
February 26, 2026 16:48
skarg
added a commit
that referenced
this pull request
Apr 10, 2026
* Secured BACnetAssignedAccessRights decoder. * Secured BACnetPropertyState decoder. * Secured BACnetCredentialAuthenticationFactor decoder. * Secured BACnetEventState change-of-state [1] SEQUENCE decoder. * Secured I-Have-Request service decoder. * Secured Add/Remove ListElement service request decoder. * Secured ConfirmedPrivateTransfer-Request and UnconfirmedPrivateTransfer-Request decoders. * Secured ReadPropertyMultiple-Request and -Ack decoders. * Secured TimeSynchronization-Request decoder. * Secured WritePropertyMultiple service decoders * Secured Trend Log object TL_fetch_property() function. * Secured ReadProperty-Ack decider, * Refactor BACnet time sync recipient handling by moving timesync linked list structure into bacdest where the recipient encoder and decoder already existed. * Secured decoding of BACnetPropertyState. * Secured decoding in the LifeSafetyOperation-Request service. * Secured BACnetAuthenticationFactor decoding in the Credential Data Input object. * Fixed WriteProperty decoder to avoid read buffer overrun. Improved WriteProperty error reporting by adding specific reject codes during decoding similar to WritePropertyMultiple. Deduplicated the WriteProperty handling of abort, reject and error codes. * Added BACNET_STACK_DEPRECATED_DISABLE guards around all of the deprecated decoding functions to ensure they are not used except intentionally for legacy code bases. * Changed version to 1.5.0.rc5 for security fix tracking in branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Secure various BACnet type and service request decoders by replacing deprecated decoders.
Secured ReadProperty-Ack decoder, Trend Log object TL_fetch_property() function, WritePropertyMultiple service decoders, TimeSynchronization-Request decoder, ReadPropertyMultiple-Request and -Ack decoders, ConfirmedPrivateTransfer-Request and UnconfirmedPrivateTransfer-Request decoders, Add/Remove ListElement service request decoder, I-Have-Request service decoder, BACnetEventState change-of-state [1] SEQUENCE decoder, BACnetCredentialAuthenticationFactor decoder, BACnetPropertyState decoder, BACnetAssignedAccessRights decoder.