in_kubernetes_events: fix OOB reads from non-NUL-terminated msgpack strings - #12180
in_kubernetes_events: fix OOB reads from non-NUL-terminated msgpack strings#12180zanarellidev wants to merge 1 commit into
Conversation
…trings record_get_field_uint64() and record_get_field_time() called strtoul()/flb_strptime() directly on msgpack_object.via.str.ptr. msgpack strings are raw, length-prefixed bytes into the decode buffer, not NUL-terminated, so these C-string functions could read past the field's true boundary. record_get_field_ptr()'s strncmp() key match had the same latent issue (a key that is a prefix of fieldname could false-match, and a short key could still be read past its bounds by strncmp with a longer fieldname length). A spec-compliant Kubernetes Event field (e.g. resourceVersion as a digit-only JSON string) placed at the edge of the decode buffer is enough to trigger an out-of-bounds read; confirmed via a guard-page harness that reproduces EXC_BAD_ACCESS inside strtoul_l, called from record_get_field_uint64. This is the same bug class fixed same-day for the sibling out_stackdriver plugin (fluent#12022, backported in fluent#12170), and the same class that produced GHSA-5rjf-prwh-pp7q in this project before. Applies the same fix pattern here: copy the field into a bounded, NUL-terminated stack buffer before parsing, and require an exact length match before the key strncmp. A prior contributor flagged the same underlying issue in fluent#12073, but it was self-closed without a fix landing; the vulnerable code is still present at HEAD.
📝 WalkthroughWalkthroughKubernetes event MessagePack parsing now requires exact field-name lengths and safely handles timestamp and numeric string values through bounded buffers with validation for empty, oversized, and partially parsed inputs. ChangesKubernetes event parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/in_kubernetes_events/kubernetes_events.c`:
- Around line 250-251: Update item_get_timestamp() to treat
record_get_field_time() results other than 0 as failures, including -2 for empty
or oversized timestamp values. Ensure each lastTimestamp, firstTimestamp, and
metadata.creationTimestamp path only returns success when ret == 0, allowing
fallback timestamps to be attempted after malformed values.
- Line 256: Update the timestamp parsing around flb_strptime() to capture its
returned end pointer and require both successful parsing and *end == '\0'. Apply
this validation consistently to lastTimestamp, firstTimestamp, and
metadata.creationTimestamp so trailing characters are rejected.
- Around line 287-295: Update record_get_field_uint64() to parse the copied
string with an explicit unsigned, digits-only conversion instead of accepting
strtoul’s signed or overflowing results. Clear errno before conversion, reject
any non-digit input including signs, reject ERANGE and values exceeding
UINT64_MAX, and preserve the existing invalid-field return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df686c53-d865-4bd7-8b94-188fa68f3a35
📒 Files selected for processing (1)
plugins/in_kubernetes_events/kubernetes_events.c
| if (v->via.str.size == 0 || v->via.str.size >= sizeof(buf)) { | ||
| return -2; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate malformed timestamps as failures.
record_get_field_time() returns -2 for empty or oversized values, but item_get_timestamp() treats any result other than -1 as success at Line 320, Line 325, and Line 335. An invalid lastTimestamp can therefore suppress fallback to firstTimestamp or metadata.creationTimestamp. Accept only ret == 0 downstream, or normalize parse failures to the caller’s failure sentinel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/in_kubernetes_events/kubernetes_events.c` around lines 250 - 251,
Update item_get_timestamp() to treat record_get_field_time() results other than
0 as failures, including -2 for empty or oversized timestamp values. Ensure each
lastTimestamp, firstTimestamp, and metadata.creationTimestamp path only returns
success when ret == 0, allowing fallback timestamps to be attempted after
malformed values.
| memcpy(buf, v->via.str.ptr, v->via.str.size); | ||
| buf[v->via.str.size] = '\0'; | ||
|
|
||
| if (flb_strptime(buf, "%Y-%m-%dT%H:%M:%SZ", &tm) == NULL) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
rg -n -C 8 '\bflb_strptime\s*\(' .
rg -n -C 6 'record_get_field_time|item_get_timestamp' \
plugins/in_kubernetes_events tests/runtime/in_kubernetes_events.cRepository: fluent/fluent-bit
Length of output: 38889
🏁 Script executed:
#!/bin/sh
set -eu
echo "== kubernetes_events.c parse function =="
sed -n '220,270p' plugins/in_kubernetes_events/kubernetes_events.c
echo "== flb_strptime return usage in flb_parser.c =="
sed -n '1985,2022p' src/flb_parser.c
echo "== kubernetes_events runtime tests mentioning timestamps =="
rg -n -C 4 'timestamp|creationTimestamp|lastTimestamp|firstTimestamp|20.*T[0-9]|invalid' tests/runtime/in_kubernetes_events.c plugins/in_kubernetes_tests || true
echo "== deterministic model of current behavior vs proposed end check =="
python3 - <<'PY'
from datetime import datetime
samples = [
("2023-01-15T12:34:56Z", True),
("2023-01-15T12:34:56Z extra", False),
("", False),
]
for s, exact_ok in samples:
prefix = "2023-01-15T12:34:56Z"
# Mimic: parsing succeeds if prefix matches ISO timestamp pattern exactly
# current NULL-only check accepts valid prefix.
parsed_ok = s.startswith(prefix)
end_consumed = len(s) == len(prefix)
print(f"{s!r}: parsed_ok={parsed_ok}, end_at_null={end_consumed}, current_accepts={parsed_ok}, proposed_accepts={parsed_ok and end_consumed}")
PYRepository: fluent/fluent-bit
Length of output: 6433
Require flb_strptime() to consume the entire timestamp buffer.
The NULL check accepts valid ISO timestamp prefixes with trailing content, so lastTimestamp, firstTimestamp, or metadata.creationTimestamp could be parsed even when the string contains malformed/garbage suffixes. Capture the return pointer and require *end == '\0'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/in_kubernetes_events/kubernetes_events.c` at line 256, Update the
timestamp parsing around flb_strptime() to capture its returned end pointer and
require both successful parsing and *end == '\0'. Apply this validation
consistently to lastTimestamp, firstTimestamp, and metadata.creationTimestamp so
trailing characters are rejected.
| len = v->via.str.size; | ||
| if (len == 0 || len > sizeof(buf) - 1) { | ||
| return -1; | ||
| } | ||
| memcpy(buf, v->via.str.ptr, len); | ||
| buf[len] = '\0'; | ||
|
|
||
| *val = strtoul(buf, &end, 10); | ||
| if (end == NULL || end == buf || *end != '\0') { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
rg -n -C 8 \
'\b(record_get_field_uint64|strtoul|strtoull|strtoumax|ERANGE)\b' .Repository: fluent/fluent-bit
Length of output: 50374
🌐 Web query:
C strtoul overflow ERANGE unsigned long documentation
💡 Result:
When using the C standard library function strtoul, an overflow occurs if the converted value is outside the range of representable values for an unsigned long [1][2]. When this range error occurs: 1. The function returns ULONG_MAX [3][4][5]. 2. The global variable errno is set to ERANGE [3][1][2]. Because ULONG_MAX is a valid return value for successful conversions, you cannot rely solely on the return value to detect an error [1][5]. To correctly check for overflow: 1. Set errno to 0 before calling strtoul [1][2][5]. 2. Perform the conversion [1][2]. 3. Check if errno is equal to ERANGE after the call [3][4][5]. If the input string begins with a minus sign, strtoul performs the conversion as if the value were positive and then negates the result within the unsigned type [4][5]. However, if the absolute value (the non-negated value) is so large that it would overflow even before negation, the function will still trigger the ERANGE error and return ULONG_MAX [6][5].
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/9699919799/functions/strtoul.html
- 2: https://pubs.opengroup.org/onlinepubs/9799919799/functions/strtoul.html
- 3: https://en.cppreference.com/c/string/byte/strtoul
- 4: https://sourceware.org/glibc/manual/2.39/html_node/Parsing-of-Integers.html
- 5: https://man7.org/linux/man-pages/man3/strtoul.3.html
- 6: https://man.openbsd.org/man3/strtoul.3
🏁 Script executed:
#!/bin/sh
set -eu
printf '--- kubernetes_events.c relevant function ---\n'
sed -n '266,306p' plugins/in_kubernetes_events/kubernetes_events.c | cat -n
printf '\n--- behavioral probe if compiler is available ---\n'
if command -v cc >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
cat > "$tmpdir/strtoul_probe.c" <<'C'
`#define` _GNU_SOURCE
`#include` <stdio.h>
`#include` <stdlib.h>
`#include` <string.h>
`#include` <errno.h>
`#include` <limits.h>
int main(void) {
char buf[32];
char *end;
unsigned long ul;
int tests;
tests = snprintf(buf, sizeof(buf), "%lu", ULONG_MAX);
ul = (unsigned long) 0xdeadbeefdeafbeef; errno = 0; end = buf + strlen(buf) + 1;
ul = strtoul(buf, &end, 10);
printf("test_unchanged_buffer_returned_ULONG_MAX=%u\n", ul == ULONG_MAX);
printf("test_unchanged_buffer_errno_ERANGE=%u\n", (int)(errno == ERANGE));
printf("test_unchanged_buffer_consumed_chars=%ld\n", (long)(end - buf));
tests = snprintf(buf, sizeof(buf), "%lu", ULONG_MAX + 1ULL);
ul = (unsigned long) 0xdeadbeefdeafbeef; errno = 0; end = buf + strlen(buf) + 1;
ul = strtoul(buf, &end, 10);
printf("test_oversized_returned_ULONG_MAX=%u\n", ul == ULONG_MAX);
printf("test_oversized_errno_ERANGE=%u\n", (int)(errno == ERANGE));
printf("test_oversized_consumed_chars=%ld\n", (long)(end - buf));
memset(buf, 0, sizeof(buf));
ul = (unsigned long) 0xdeadbeefdeafbeef; errno = 0; end = buf + strlen(buf) + 1;
ul = strtoul("-1", &end, 10);
printf("test_minus_one_ret=%08lx end=%ld\n", ul, (long)(end - "-1"));
return 0;
}
C
if cc -O2 -o "$tmpdir/strtoul_probe" "$tmpdir/strtoul_probe.c" 2>/tmp/strtoul_compile.log; then
"$tmpdir/strtoul_probe"
else
printf 'compile_failed\n'
cat /tmp/strtoul_compile.log
fi
rm -rf "$tmpdir"
else
printf 'cc not available\n'
fiRepository: fluent/fluent-bit
Length of output: 246
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- kubernetes_events.c relevant function ---'
sed -n '266,306p' plugins/in_kubernetes_events/kubernetes_events.c | cat -n
printf '%s\n' ''
printf '%s\n' '--- behavioral probe if compiler is available ---'
if command -v cc >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
cat > "$tmpdir/strtoul_probe.c" <<'C'
`#define` _GNU_SOURCE
`#include` <stdio.h>
`#include` <stdlib.h>
`#include` <string.h>
`#include` <errno.h>
`#include` <limits.h>
int main(void) {
char buf[32];
char *end;
unsigned long ul;
int tests;
tests = snprintf(buf, sizeof(buf), "%lu", ULONG_MAX);
ul = (unsigned long) 0xdeadbeefdeafbeef; errno = 0; end = buf + strlen(buf) + 1;
ul = strtoul(buf, &end, 10);
printf("test_unchanged_buffer_returned_ULONG_MAX=%u\n", ul == ULONG_MAX);
printf("test_unchanged_buffer_errno_ERANGE=%u\n", (int)(errno == ERANGE));
printf("test_unchanged_buffer_consumed_chars=%ld\n", (long)(end - buf));
tests = snprintf(buf, sizeof(buf), "%lu", ULONG_MAX + 1ULL);
ul = (unsigned long) 0xdeadbeefdeafbeef; errno = 0; end = buf + strlen(buf) + 1;
ul = strtoul(buf, &end, 10);
printf("test_oversized_returned_ULONG_MAX=%u\n", ul == ULONG_MAX);
printf("test_oversized_errno_ERANGE=%u\n", (int)(errno == ERANGE));
printf("test_oversized_consumed_chars=%ld\n", (long)(end - buf));
memset(buf, 0, sizeof(buf));
ul = (unsigned long) 0xdeadbeefdeafbeef; errno = 0; end = buf + strlen(buf) + 1;
ul = strtoul("-1", &end, 10);
printf("test_minus_one_ret=%08lx end=%ld\n", ul, (long)(end - "-1"));
return 0;
}
C
if cc -O2 -o "$tmpdir/strtoul_probe" "$tmpdir/strtoul_probe.c" 2>/tmp/strtoul_compile.log; then
"$tmpdir/strtoul_probe"
else
printf '%s\n' 'compile_failed'
cat /tmp/strtoul_compile.log
fi
rm -rf "$tmpdir"
else
printf '%s\n' 'cc not available'
fiRepository: fluent/fluent-bit
Length of output: 1834
🌐 Web query:
C strtoull base 10 optional sign documentation ERANGE
💡 Result:
The C standard library function strtoull (and its related functions like strtoul) performs integer conversion on a string and is defined to handle optional signs and potential range errors as follows: Optional Signs The input string may begin with an arbitrary amount of whitespace (as determined by isspace) followed by an optional plus (+) or minus (-) sign [1][2]. This applies regardless of the specified base (including base 10) [3][4]. If a minus sign is present, the resulting value is negated [3][5][4]. Since the return type of strtoull is unsigned, this negation follows the rules of unsigned integer arithmetic (often referred to as modular arithmetic or unsigned wraparound) [6]. Range Errors (ERANGE) If the correct value of the conversion falls outside the range of representable values for the unsigned long long type, the function returns ULLONG_MAX and sets errno to ERANGE [1][7]. Usage Note Because ULLONG_MAX is a valid return value on success (it is the maximum representable value), a program should reset errno to 0 before calling strtoull and check it after the call to reliably distinguish between a successful conversion to ULLONG_MAX and a range error [7][8]. Summary of Behavior 1. Whitespace is skipped [1][4]. 2. An optional sign (+ or -) is parsed [3][5]. 3. The remaining characters are converted based on the provided base (or auto-detected if base is 0) [1][2]. 4. If the final value is out of range, ULLONG_MAX is returned and ERANGE is set [1][8]. Top Results: [1], [2], [5], [7], [8]
Citations:
- 1: https://en.cppreference.com/c/string/byte/strtoul
- 2: https://man7.org/linux/man-pages/man3/strtoul.3.html
- 3: https://pubs.opengroup.org/onlinepubs/9699959099/functions/strtoull.html
- 4: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strtoull-strtoull-l-wcstoull-wcstoull-l?view=msvc-170
- 5: https://manpages.ubuntu.com/manpages/questing/man3/strtoull.3.html
- 6: https://en.cppreference.com/cpp/string/byte/strtoul
- 7: https://pubs.opengroup.org/onlinepubs/9799919799/functions/strtoul.html
- 8: https://pubs.opengroup.org/onlinepubs/9699919799/functions/strtoull.html
Reject signed or overflowing numeric fields.
record_get_field_uint64() accepts 31-character strings, stores the result of strtoul() in a uint64_t, and never checks errno == ERANGE. This allows out-of-range values to saturate to ULONG_MAX and signs like -1 to be accepted. Use an explicit unsigned parser that accepts digits only, clears/checks errno, and rejects values larger than UINT64_MAX.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/in_kubernetes_events/kubernetes_events.c` around lines 287 - 295,
Update record_get_field_uint64() to parse the copied string with an explicit
unsigned, digits-only conversion instead of accepting strtoul’s signed or
overflowing results. Clear errno before conversion, reject any non-digit input
including signs, reject ERANGE and values exceeding UINT64_MAX, and preserve the
existing invalid-field return behavior.
1 participant
Summary
record_get_field_uint64()andrecord_get_field_time()inplugins/in_kubernetes_events/kubernetes_events.ccallstrtoul()/flb_strptime()directly onmsgpack_object.via.str.ptr. msgpack strings are raw, length-prefixed bytes pointing directly into the decode buffer — they are not NUL-terminated — so these C-string functions can read past the field's true boundary.record_get_field_ptr()'s key match viastrncmp(k->via.str.ptr, fieldname, strlen(fieldname))had the same class of issue: no exact-length check, so a key that's a prefix offieldnamecould false-match, and comparison length wasn't bounded by the key's own size.A spec-compliant Kubernetes Event field (e.g.
resourceVersionas a digit-only JSON string, per the Kubernetes API docs) placed near the edge of the decode buffer is enough to trigger an out-of-bounds read. Reproduced locally with a harness that calls the realflb_pack_json()on{"resourceVersion":"999999"}, places the resulting buffer against aPROT_NONEguard page, and calls the real (recompiled) plugin function on it — confirmed vialldb:EXC_BAD_ACCESSinsidestrtoul_l, called fromrecord_get_field_uint64at the line doing the unboundedstrtoulcall.This is the same bug class fixed same-day for the sibling
out_stackdriverplugin (#12022, backported in #12170), and the same class that produced a real CVE in this project before (GHSA-5rjf-prwh-pp7q).A prior contributor flagged the same underlying issue in #12073 but self-closed it without a fix landing; the vulnerable code is still present at HEAD. Filing this with a fix and full explanation/repro so it doesn't fall through again.
Fix
Same pattern the maintainers already established for the sibling
out_stackdriverfix: copy the field into a bounded, NUL-terminated stack buffer before parsing (strtoul,flb_strptime), and require an exact length match before the keystrncmp.Test plan
ctestsuite (90 tests) passes clean after the fix.in_kubernetes_eventsruntime test suite (flb-rt-in_kubernetes_events) passes clean after the fix.lldb-confirmedEXC_BAD_ACCESSinstrtoul_l); same harness runs clean (resource_version=999999, exit 0) after the fix.Happy to share the repro harness if useful for a permanent regression test — it needs an
mmap'd guard page, which doesn't fit cleanly into the existing runtime-test infra (same constraint the maintainers' own #12022 fix had, which also didn't add a crash-reproducing test).Summary by CodeRabbit