Fix buffer overflows in bsc_node_parse_urls() - #1365
Merged
skarg merged 2 commits intoMay 29, 2026
Conversation
Three bugs in the BACnet/SC Address Resolution ACK URL parser: 1. NUL terminator used absolute string position 'i' as array index instead of relative position 'i - start', writing past the 129-byte utf8_urls buffer for URLs starting past position 128. 2. No bounds check on URL count index 'j', allowing writes past the 10-element utf8_urls array when parsing 11+ URLs, corrupting urls_num, fresh_timer, and adjacent structures. 3. Length check compared absolute position 'i' against the max URI size instead of the actual URL length '(i - start)', incorrectly rejecting valid short URLs at high string offsets. Fixes all three by using relative lengths and adding array bounds checks before every write.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens BACnet/SC Address Resolution ACK URL parsing in bsc_node_parse_urls() to prevent out-of-bounds writes and incorrect rejection of valid URLs.
Changes:
- Uses relative URL length (
i - start) for size checks and NUL terminator placement. - Adds URL-count bounds checks before writing into
utf8_urls. - Stops parsing once the configured URL count limit is reached.
Comment on lines
+408
to
+416
| if ((i - start) > | ||
| BSC_CONF_NODE_MAX_URI_SIZE_IN_ADDRESS_RESOLUTION_ACK || | ||
| (i - start) == 0) { | ||
| start = i + 1; | ||
| continue; | ||
| } else { | ||
| } else if (j < | ||
| BSC_CONF_NODE_MAX_URIS_NUM_IN_ADDRESS_RESOLUTION_ACK) { | ||
| memcpy(&r->utf8_urls[j][0], &url[start], i - start); | ||
| r->utf8_urls[j][i] = 0; | ||
| r->utf8_urls[j][i - start] = 0; |
Collaborator
|
Please run "pre-commit" to fix the style failure in the pipeline. |
Contributor
Author
Hi Steve, |
skarg
added a commit
that referenced
this pull request
Jun 25, 2026
* Fix buffer overflows in bsc_node_parse_urls() Three bugs in the BACnet/SC Address Resolution ACK URL parser: 1. NUL terminator used absolute string position 'i' as array index instead of relative position 'i - start', writing past the 129-byte utf8_urls buffer for URLs starting past position 128. 2. No bounds check on URL count index 'j', allowing writes past the 10-element utf8_urls array when parsing 11+ URLs, corrupting urls_num, fresh_timer, and adjacent structures. 3. Length check compared absolute position 'i' against the max URI size instead of the actual URL length '(i - start)', incorrectly rejecting valid short URLs at high string offsets. Fixes all three by using relative lengths and adding array bounds checks before every write. Add test-only shim for bsc_node_parse_urls() to support unit testing (#1369)
skarg
added a commit
that referenced
this pull request
Jul 5, 2026
* Fix buffer overflows in bsc_node_parse_urls() Three bugs in the BACnet/SC Address Resolution ACK URL parser: 1. NUL terminator used absolute string position 'i' as array index instead of relative position 'i - start', writing past the 129-byte utf8_urls buffer for URLs starting past position 128. 2. No bounds check on URL count index 'j', allowing writes past the 10-element utf8_urls array when parsing 11+ URLs, corrupting urls_num, fresh_timer, and adjacent structures. 3. Length check compared absolute position 'i' against the max URI size instead of the actual URL length '(i - start)', incorrectly rejecting valid short URLs at high string offsets. Fixes all three by using relative lengths and adding array bounds checks before every write.
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.
Summary
Three buffer overflow bugs in the BACnet/SC Address Resolution ACK URL parser (
bsc_node_parse_urls()insrc/bacnet/datalink/bsc/bsc-node.c):Bug 1 -- NUL byte write past buffer (CWE-787): The NUL terminator at lines 414/423 uses the absolute string position
ias an array index (r->utf8_urls[j][i] = 0) instead of the relative URL lengthi - start. For URLs starting past byte 128 in the URI string,iexceeds the 129-byteutf8_urls[j]buffer, writing a zero byte up to ~1389 bytes out of bounds.Bug 2 -- Unbounded URL count (CWE-787): The URL index
jis never checked againstBSC_CONF_NODE_MAX_URIS_NUM_IN_ADDRESS_RESOLUTION_ACK. With 11+ space-separated short URLs in a single Address Resolution ACK, bothmemcpyand the NUL write access past the 10-elementutf8_urlsarray, corruptingurls_num,fresh_timer, and adjacentBSC_ADDRESS_RESOLUTIONentries in the static array.Bug 3 -- Incorrect length comparison: The skip check at line 408 compares the absolute position
iagainst the max URI size instead of the actual URL length(i - start). This causes valid short URLs at high string offsets to be incorrectly rejected.Fix
(i - start)for URL length in both the skip check and NUL terminator indexj < BSC_CONF_NODE_MAX_URIS_NUM_IN_ADDRESS_RESOLUTION_ACKbounds check before every array writeImpact
An authenticated BACnet/SC peer can send a crafted Address Resolution ACK message to trigger out-of-bounds writes, corrupting static data structures. Bug 2 is the most exploitable -- a 21-byte URI string with 11 single-character URLs is sufficient.