Fix buffer overflow / off-by-one in claim and freebsd_ipfw collectors by vkalintiris · Pull Request #22710 · netdata/netdata

vkalintiris · 2026-06-14T13:43:48Z

Summary

Three related memory-safety and correctness fixes in the Windows claimer and the FreeBSD IPFW collector. All are pre-existing bugs surfaced during a focused audit of sprintf/calloc sizing in platform-specific collectors.

Changes

1. `src/claim/main.c` — off-by-one heap overflow in wide-char conversion

netdata_claim_convert_str() (claim/main.h:14) writes a NUL terminator at dst[copied] after wcstombs, where copied can equal the len limit. The buffer must therefore hold len + 1 bytes. The aToken allocation already obeys this (calloc(length) with convert_str(..., length - 1)), but aRoom, aProxy, and aURL allocated calloc(length - 1) — one byte short. For ASCII-only room/proxy/url, wcstombs fills the buffer and the NUL write overflows the heap by exactly one byte. Triggered on essentially every Windows claim.

Fix: the three allocations now match aToken (calloc(length)); the convert_str calls stay length - 1.

2. `src/collectors/freebsd.plugin/freebsd_ipfw.c` — stack buffer overflow in rule number formatting

char rule_num_str[12] was formatted with sprintf(..., "%"PRIu32"_%"PRIu32"", rulenum, id). Two uint32_t values joined by _ can need up to 22 bytes (4294967295_4294967295\0). Any IPFW rule number or id ≥ 7 digits overflowed the 12-byte stack buffer.

Fix: size the buffer as (MAX_INT_DIGITS * 2) + 2 (= 22, exact worst case) and switch sprintf to the in-tree clamping snprintfz (already used elsewhere in the plugin). Both call sites updated.

3. `src/claim/main.c` — `/I` insecure flag silently ignored

The /I parser set length = wcslen(argv[i]) without +1, so for the only meaningful input /I 1 it called wcstombs(..., 0), wrote no bytes, and atoi("") always returned 0. The Windows claimer therefore could not disable TLS verification via the documented /I flag — insecure was always no in claim.conf regardless of the user-supplied value.

Fix: mirror the correct aToken pattern (length = wcslen(argv[i]) + 1, calloc(length), convert_str(..., length - 1)).

Impact

Fix	Severity	Trigger	Platform
claim heap off-by-one	High (memory safety)	Every Windows claim with ASCII room/proxy/url	Windows
ipfw stack overflow	High (memory safety)	Any IPFW rule rulenum/id ≥ 7 digits	FreeBSD 11+
claim `/I` dead flag	Medium (functional)	Any `/I <n>` invocation	Windows

No public contract changes. No schema/config/metric/alert changes.

Notes

These files are platform-specific (<windows.h> / FreeBSD <netinet/ip_fw.h>) and not built on Linux; fixes were verified by inspection against the existing-correct patterns in the same files.
Pre-existing, intentionally out of scope: netdata_claim_convert_str does not check the (size_t)-1 return of wcstombs on encoding error, and three sibling sprintf calls in freebsd_sysctl.c / freebsd_devstat.c currently fit by construction but could be migrated to snprintfz for defense-in-depth. Tracked separately if desired.

Summary by cubic

Fixes off-by-one buffer overflows in the Windows claimer and the FreeBSD IPFW collector, and makes the Windows /I flag work as documented. Prevents memory corruption during claims and ensures TLS verification can be toggled.

Bug Fixes
- Windows claim: allocate aRoom, aProxy, and aURL with length bytes and keep convert_str(..., length - 1) to avoid a 1-byte heap overflow on the NUL.
- Windows claim: /I parsing now uses wcslen(...) + 1 so the value converts correctly; TLS verification can be disabled as intended.
- FreeBSD IPFW: enlarge rule_num_str to (MAX_INT_DIGITS * 2) + 2 and replace sprintf with snprintfz to prevent stack overflow when formatting <rulenum>_<id>.

^{Written for commit 4a12012. Summary will update on new commits.}

netdata_claim_convert_str() writes a NUL terminator at dst[copied], where copied can equal the len limit, so the buffer must hold len+1 bytes. The aToken allocation already obeys this (calloc(length), convert with length-1), but aRoom, aProxy, and aURL allocated calloc(length-1), one byte short. For ASCII-only room/proxy/url, wcstombs fills the entire buffer and the NUL terminator overflows the heap by one byte on every Windows claim operation.

rule_num_str was a 12-byte buffer formatted with sprintf() using "<uint32>_<uint32>", which can need up to 22 bytes (10 digits per uint32 plus separator and NUL). Any IPFW rule number or id with more than ~5 digits overflowed the stack. Enlarge the buffer to hold two MAX_INT_DIGITS values plus separator and NUL, and switch sprintf() to the clamping snprintfz() used by the rest of the plugin.

The /I (insecure) parser set length = wcslen(argv[i]) without +1, so for the only meaningful input "/I 1" it called wcstombs with len=0, wrote no bytes, and atoi("") always returned 0. As a result the Windows claimer could never disable TLS verification via the documented /I flag (insecure was always "no" in claim.conf). Mirror the correct aToken pattern: length = wcslen(argv[i]) + 1, then calloc(length) and convert_str(..., length - 1).

cubic-dev-ai

No issues found across 2 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

Architecture diagram

sequenceDiagram
    participant ClaimCLI as Claimer CLI
    participant ParseArgs as nd_claim_parse_args()
    participant PrepStrings as netdata_claim_prepare_strings()
    participant WCharConv as netdata_claim_convert_str()
    participant Heap as Windows Heap
    participant IpfwPlugin as FreeBSD IPFW Plugin
    participant IpfwLoop as do_ipfw() main loop
    participant Stack as Stack Buffer

    Note over ClaimCLI,Heap: Windows Claim Flow

    ClaimCLI->>ParseArgs: Parse command-line argv (LPWSTR)
    alt /I flag present
        ParseArgs->>ParseArgs: wcslen(argv[i]) + 1 (now correct)
        ParseArgs->>WCharConv: convert_str(tmp, length - 1)
        WCharConv->>WCharConv: wcstombs with correct buffer size
        WCharConv-->>ParseArgs: insecure flag value
    else no /I flag
        ParseArgs->>ParseArgs: continue processing
    end

    ParseArgs-->>PrepStrings: token, room, proxy, url strings

    PrepStrings->>Heap: calloc(length) for aRoom (now length, not length-1)
    PrepStrings->>Heap: calloc(length) for aProxy (now length, not length-1)
    PrepStrings->>Heap: calloc(length) for aURL (now length, not length-1)
    PrepStrings->>WCharConv: convert_str(buf, length - 1)
    WCharConv->>Heap: wcstombs fills buffer
    WCharConv->>Heap: Write NUL at dst[copied] (now safe, buffer has space)

    Note over IpfwPlugin,Stack: FreeBSD IPFW Flow

    IpfwPlugin->>IpfwLoop: do_ipfw(update_every, dt)
    IpfwLoop->>IpfwLoop: Allocate rule_num_str on stack (now 22 bytes)
    IpfwLoop->>Stack: snprintfz(rule_num_str, sizeof, format, rulenum, id)
    alt Static rules
        IpfwLoop->>Stack: Format "%"PRIu32"_%"PRIu32"" (safe with snprintfz)
        Stack-->>IpfwLoop: rule_num_str content (no overflow)
    else Dynamic rules
        IpfwLoop->>Stack: Format "%d" (safe with snprintfz)
        Stack-->>IpfwLoop: rule_num_str content (no overflow)
    end
    IpfwLoop->>IpfwLoop: Use rule_num_str as RRD dimension name

_{Re-trigger cubic}

sonarqubecloud · 2026-06-14T13:46:09Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

thiagoftsm

PR is working as expected on FreeBSD and Windows. LGTM!

…#22710) * claim: fix off-by-one buffer allocation in wide-char string conversion netdata_claim_convert_str() writes a NUL terminator at dst[copied], where copied can equal the len limit, so the buffer must hold len+1 bytes. The aToken allocation already obeys this (calloc(length), convert with length-1), but aRoom, aProxy, and aURL allocated calloc(length-1), one byte short. For ASCII-only room/proxy/url, wcstombs fills the entire buffer and the NUL terminator overflows the heap by one byte on every Windows claim operation. * freebsd_ipfw: fix stack buffer overflow in rule number formatting rule_num_str was a 12-byte buffer formatted with sprintf() using "<uint32>_<uint32>", which can need up to 22 bytes (10 digits per uint32 plus separator and NUL). Any IPFW rule number or id with more than ~5 digits overflowed the stack. Enlarge the buffer to hold two MAX_INT_DIGITS values plus separator and NUL, and switch sprintf() to the clamping snprintfz() used by the rest of the plugin. * claim: fix /I insecure flag being silently ignored The /I (insecure) parser set length = wcslen(argv[i]) without +1, so for the only meaningful input "/I 1" it called wcstombs with len=0, wrote no bytes, and atoi("") always returned 0. As a result the Windows claimer could never disable TLS verification via the documented /I flag (insecure was always "no" in claim.conf). Mirror the correct aToken pattern: length = wcslen(argv[i]) + 1, then calloc(length) and convert_str(..., length - 1). (cherry picked from commit 1bbc2cb)

vkalintiris added 3 commits June 14, 2026 16:01

vkalintiris requested review from Ferroin and thiagoftsm as code owners June 14, 2026 13:43

cubic-dev-ai Bot reviewed Jun 14, 2026

View reviewed changes

github-actions Bot added area/collectors Everything related to data collection area/claim collectors/freebsd labels Jun 14, 2026

thiagoftsm approved these changes Jun 15, 2026

View reviewed changes

vkalintiris merged commit 1bbc2cb into netdata:master Jun 16, 2026
153 of 157 checks passed

stelfrag mentioned this pull request Jun 22, 2026

Patch release 2.10.4 #22825

Merged

Navigation Menu

Uh oh!

Fix buffer overflow / off-by-one in claim and freebsd_ipfw collectors - #22710

Fix buffer overflow / off-by-one in claim and freebsd_ipfw collectors#22710
vkalintiris merged 3 commits into
netdata:masterfrom
vkalintiris:fix/claim-ipfw-buffer-overflows

vkalintiris commented Jun 14, 2026 •

edited by cubic-dev-ai Bot

Loading

Uh oh!

cubic-dev-ai Bot left a comment

Uh oh!

sonarqubecloud Bot commented Jun 14, 2026

Uh oh!

thiagoftsm left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vkalintiris commented Jun 14, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

1. src/claim/main.c — off-by-one heap overflow in wide-char conversion

2. src/collectors/freebsd.plugin/freebsd_ipfw.c — stack buffer overflow in rule number formatting

3. src/claim/main.c — /I insecure flag silently ignored

Impact

Notes

Summary by cubic

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jun 14, 2026

Quality Gate passed

Uh oh!

thiagoftsm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vkalintiris commented Jun 14, 2026 •

edited by cubic-dev-ai Bot

Loading

1. `src/claim/main.c` — off-by-one heap overflow in wide-char conversion

2. `src/collectors/freebsd.plugin/freebsd_ipfw.c` — stack buffer overflow in rule number formatting

3. `src/claim/main.c` — `/I` insecure flag silently ignored