in_kubernetes_events: bound record field parsing to msgpack string length#12073
in_kubernetes_events: bound record field parsing to msgpack string length#12073saddamr3e wants to merge 1 commit into
Conversation
…ngth The record field getters handed msgpack string bytes to strncmp(), flb_strptime() and strtoul(). msgpack strings are not NUL terminated, so those scans read past via.str.size. Copy each value into a bounded, terminated buffer before parsing and match keys by exact length. Signed-off-by: Saddam <saddamr3e@gmail.com>
📝 WalkthroughWalkthroughThis PR hardens Kubernetes event parsing against unsafe operations on non-NUL-terminated msgpack string fields. Key matching functions now verify exact string size before strncmp comparisons, and value parsing functions copy strings into bounded, NUL-terminated buffers before calling flb_strptime and strtoul. ChangesSafe msgpack string handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/in_kubernetes_events/kubernetes_events.c (1)
260-301: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
bufandlendeclarations to the start of the function.Per the project's coding guidelines, variables should be declared at the start of functions, not mid-block.
bufandlenat lines 272-273 are declared inside theif (v->type == MSGPACK_OBJECT_STR)block.♻️ Proposed refactor
static int record_get_field_uint64(msgpack_object *obj, const char *fieldname, uint64_t *val) { msgpack_object *v; char *end; + char buf[32]; + size_t len; v = record_get_field_ptr(obj, fieldname); if (v == NULL) { return -1; } /* attempt to parse string as number... */ if (v->type == MSGPACK_OBJECT_STR) { - char buf[32]; - size_t len = v->via.str.size; + len = v->via.str.size; /* * msgpack strings are not NUL terminated; strtoul() scans until a🤖 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 260 - 301, The `record_get_field_uint64` function declares `buf` and `len` inside the `MSGPACK_OBJECT_STR` block, which violates the coding guideline requiring variables to be declared at the start of the function. Move those declarations to the top of `record_get_field_uint64` alongside `v` and `end`, then keep the existing string parsing 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 260-301: The `record_get_field_uint64` function declares `buf` and
`len` inside the `MSGPACK_OBJECT_STR` block, which violates the coding guideline
requiring variables to be declared at the start of the function. Move those
declarations to the top of `record_get_field_uint64` alongside `v` and `end`,
then keep the existing string parsing logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23ca2dde-8023-43ad-a25f-4f89edd27a1e
📒 Files selected for processing (1)
plugins/in_kubernetes_events/kubernetes_events.c
The Kubernetes events input decodes the API watch/list response with
flb_pack_jsonand then reads fields out of the resulting msgpack objects.record_get_field_ptr(),record_get_field_time(),record_get_field_uint64()andprocess_event_list()handed the msgpackstring bytes (
via.str.ptr) straight tostrncmp(),flb_strptime()andstrtoul(). msgpack strings are not NUL terminated, so those C-string scansrun past
via.str.sizeinto whatever follows the string in the buffer. Thevalues parsed this way include
metadata.resourceVersionand thelastTimestamp/firstTimestamp/creationTimestampfields, plus the map keys.Guard-page proof (the string is placed flush against a
PROT_NONEpage, so anyread past its length faults inside libc before the call returns):
Copy each value into a bounded, NUL-terminated stack buffer before parsing and
compare keys by exact length first. Same shape as the out_stackdriver over-read
fix that replaced
atoll(obj.via.str.ptr)with a bounded copy. Valid inputbehavior is unchanged; the length compare also stops a key that merely shares a
prefix with a looked-up field from matching.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit