in_kubernetes_events: fix OOB reads from non-NUL-terminated msgpack strings - #12187
in_kubernetes_events: fix OOB reads from non-NUL-terminated msgpack strings#12187zanarellidev wants to merge 3 commits 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. Signed-off-by: zanarelli <zanarelli.dev@gmail.com>
Require flb_strptime to consume the full copied buffer, treat only ret==0 as a successful timestamp in item_get_timestamp(), and parse uint64 strings with strtoull+errno so malformed or overflowing values fall through to the next timestamp field instead of being accepted. Signed-off-by: Raphael Zanarelli <zanarelli.dev@gmail.com> Signed-off-by: zanarelli <zanarelli.dev@gmail.com>
…d_uint64 strtoull() itself accepts a leading '+'/'-' and skips leading whitespace, so a resourceVersion string like "-5" silently wrapped around into 18446744073709551611 instead of being rejected. Kubernetes always serializes resourceVersion as a plain unsigned digits-only decimal string, so require the first byte to be a digit before calling strtoull(), on top of the existing errno/ERANGE and full-consumption checks. Confirmed via a standalone guard-page harness (same shape as the existing OOB reproduction in this PR): "-5" and "+5" are now rejected (previously accepted, wrapping "-5" to UINT64_MAX-4), a valid digits-only value still round-trips correctly, and overflow beyond UINT64_MAX still correctly fails via the existing ERANGE check. Addresses a CodeRabbit review comment on this PR. Signed-off-by: Raphael Zanarelli <zanarelli.dev@gmail.com> Signed-off-by: zanarelli <zanarelli.dev@gmail.com>
📝 WalkthroughWalkthroughThe Kubernetes events input now validates MessagePack key lengths, copies strings into bounded NUL-terminated buffers, strictly parses timestamps and resource versions, and rejects negative integers. ChangesKubernetes event parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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.
🧹 Nitpick comments (1)
plugins/in_kubernetes_events/kubernetes_events.c (1)
258-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
endto the function declaration block.Line 259 declares
endafter executable statements. Declare it withvandtmat the start ofrecord_get_field_time.As per coding guidelines, declare variables at the start of functions rather than mid-block.
🤖 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 258 - 265, Move the end variable declaration from the inner parsing block to the declaration section at the start of record_get_field_time, alongside v and tm. Keep its assignment and validation logic unchanged.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@plugins/in_kubernetes_events/kubernetes_events.c`:
- Around line 258-265: Move the end variable declaration from the inner parsing
block to the declaration section at the start of record_get_field_time,
alongside v and tm. Keep its assignment and validation logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30a45a1b-6ecc-4261-b043-70614fa7f223
📒 Files selected for processing (1)
plugins/in_kubernetes_events/kubernetes_events.c
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10e922c8a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ((size_t) k->via.str.size == fieldname_len && | ||
| strncmp(k->via.str.ptr, fieldname, fieldname_len) == 0) { |
There was a problem hiding this comment.
When FLB_HAVE_SQLDB is enabled and the db option is configured, this exact-length lookup makes k8s_events_sql_insert_event() stop finding the event metadata map: that function still calls record_get_field_ptr(item, "meta"), which previously matched the Kubernetes "metadata" key only because the old code accepted prefixes. With this change the insert path always logs unable to find metadata to save event, so the database never records processed UIDs and DB-backed duplicate filtering no longer works; update that caller to request "metadata" before relying on exact key matching.
Useful? React with 👍 / 👎.
|
Cherry picked the changes on this PR and merge them as part of #12222 since I needed to add other changes (author changes are honored) |
2 participants
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.
Supersedes #12180, closed by GitHub after a force-push rewrote that branch's
history (stripped an unrelated commit trailer) while the PR was closed, which
blocks reopening. Same fixes, clean history on top of current
master.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.On top of that, addresses a CodeRabbit review comment from the original PR:
strtoull()accepts a leading+/-and would silently wrap a value like"-5"into a huge positive number instead of rejecting it.resourceVersionis always a plain unsigned digits-only decimal string, so the fix now requires the first byte to be a digit before parsing.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."-5"/"+5"cases: rejected after the fix, previously accepted ("-5"silently wrapped toUINT64_MAX-4).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