in_kubernetes_events: fix unsafe MessagePack field parsing - #12222
Merged
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 (#12022, backported in #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 #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>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
📝 WalkthroughWalkthroughThe Kubernetes events input validates MessagePack keys and values, parses timestamps and resource versions with bounded buffers, validates event-list fields, stores metadata correctly, finalizes SQLite statements during shutdown, and strengthens integration assertions. ChangesKubernetes event processing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 2c6b20703a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 745: In the metadata handling flow around record_get_field_ptr(item,
"metadata"), initialize uid to NULL before record_get_field_sds() and treat a
NULL result as a lookup failure. Ensure SQLite binding, logging, and destruction
only occur after validating uid, while preserving the existing handling for
valid metadata.uid values.
🪄 Autofix
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: c958fdcb-1c04-4085-b465-5ee141ca24dc
📒 Files selected for processing (3)
plugins/in_kubernetes_events/kubernetes_events.cplugins/in_kubernetes_events/kubernetes_events_conf.ctests/integration/scenarios/in_kubernetes_events/tests/test_in_kubernetes_events_001.py
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@tests/integration/scenarios/in_kubernetes_events/tests/test_in_kubernetes_events_001.py`:
- Around line 188-195: Update the assertions around process_event_object() to
match its current behavior: do not expect a UID-less list item to emit "unable
to find uid in metadata to save event" or reach SQLite insertion. Remove or
revise only the conflicting assertion while preserving the existing checks for
the other event-processing messages and successful inserts.
🪄 Autofix
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: 1a5f3ca0-535a-4827-8410-fa823a60823a
📒 Files selected for processing (2)
plugins/in_kubernetes_events/kubernetes_events.ctests/integration/scenarios/in_kubernetes_events/tests/test_in_kubernetes_events_001.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/in_kubernetes_events/kubernetes_events.c
Comment on lines
+188
to
+195
| assert log_text.count(f'"uid"=>"{EVENT_UID}"') == 1 | ||
| assert log_text.count(f'"uid"=>"{RECOVERED_EVENT_UID}"') == 1 | ||
| assert "Streamed Event 'type' not found" in log_text | ||
| assert "Cannot get uid for item in response" in log_text | ||
| assert "unable to find uid in metadata to save event" in log_text | ||
| assert "unable to find metadata to save event" not in log_text | ||
| assert f"inserted k8s event: uid={EVENT_UID}" in log_text | ||
| assert f"inserted k8s event: uid={RECOVERED_EVENT_UID}" in log_text |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the EventList item-extraction loop and save-event call flow in kubernetes_events.c
ast-grep outline plugins/in_kubernetes_events/kubernetes_events.c --items all --match 'items|uid|save'
rg -n -B5 -A25 'Cannot get uid for item in response' plugins/in_kubernetes_events/kubernetes_events.c
rg -n -B15 -A5 'unable to find uid in metadata to save event' plugins/in_kubernetes_events/kubernetes_events.cRepository: fluent/fluent-bit
Length of output: 2135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file size =="
wc -l plugins/in_kubernetes_events/kubernetes_events.c tests/integration/scenarios/in_kubernetes_events/tests/test_in_kubernetes_events_001.py
echo "== functions/sections outline grep =="
rg -n 'static|int |flb_plg_error|static int.*event|save|list|items|metadata|uid|EventList' plugins/in_kubernetes_events/kubernetes_events.c \
| head -n 220
echo "== test relevant sections =="
sed -n '1,130p' tests/integration/scenarios/in_kubernetes_events/tests/test_in_kubernetes_events_001.py
echo "== assertions =="
sed -n '150,210p' tests/integration/scenarios/in_kubernetes_events/tests/test_in_kubernetes_events_001.py
echo "== k8s event extraction/save function sections =="
sed -n '320,430p' plugins/in_kubernetes_events/kubernetes_events.c
sed -n '680,820p' plugins/in_kubernetes_events/kubernetes_events.c
echo "== callers of extraction/save =="
rg -n -B8 -A12 'extract|EventList|unable to find metadata to save event|unable to find uid in metadata to save event|save.*event|kubernetes_event_save|save_event' plugins/in_kubernetes_events/kubernetes_events.cRepository: fluent/fluent-bit
Length of output: 26413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== process_event_list full implementation =="
sed -n '565,670p' plugins/in_kubernetes_events/kubernetes_events.c
echo "== remaining extraction helper usages around process_event_list =="
rg -n -B3 -A8 'process_event_object|check_event_is_filtered|last_resource_version|items->via.array.size' plugins/in_kubernetes_events/kubernetes_events.c
echo "== remaining process_event_object implementation =="
sed -n '441,510p' plugins/in_kubernetes_events/kubernetes_events.cRepository: fluent/fluent-bit
Length of output: 9066
No change needed. process_event_object() does not check metadata.uid, so the UID-less list item does not emit "unable to find uid in metadata to save event" or reach SQLite insert. This assertion will fail unless the extraction check is skipped or the save path is adjusted.
🤖 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
`@tests/integration/scenarios/in_kubernetes_events/tests/test_in_kubernetes_events_001.py`
around lines 188 - 195, Update the assertions around process_event_object() to
match its current behavior: do not expect a UID-less list item to emit "unable
to find uid in metadata to save event" or reach SQLite insertion. Remove or
revise only the conflicting assertion while preserving the existing checks for
the other event-processing messages and successful inserts.
4 tasks
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
2 participants
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
This builds on and supersedes #12187 while preserving its three original
commits and their authorship.
buffers before parsing timestamps and resource versions.
EventList
itemsandmetadatafields.metadataexplicitly.
Attribution
The first three commits are the original work from #12187, authored by
@zanarellidev. The follow-up commits by @edsiper address the outstanding review
feedback, complete the remaining unsafe lookups, add database lifecycle cleanup,
and provide focused integration coverage.
Root cause
MessagePack strings are length-delimited and are not guaranteed to be
NUL-terminated. The Kubernetes Events input passed those buffers directly to
C-string parsing functions and also relied on prefix-based key comparisons.
Changing field lookup to exact matching additionally exposed an existing
metaversusmetadatadependency in the SQL persistence path.Validation
cmake --build build -j8ctest --test-dir build -R '^flb-it-strptime$' --output-on-failuretests/integration/.venv/bin/python -m pytest tests/integration/scenarios/in_kubernetes_events/tests/test_in_kubernetes_events_001.py -qVALGRIND=1 VALGRIND_STRICT=1 tests/integration/.venv/bin/python -m pytest tests/integration/scenarios/in_kubernetes_events/tests/test_in_kubernetes_events_001.py -qGITHUB_EVENT_NAME=pull_request GITHUB_BASE_REF=master tests/integration/.venv/bin/python .github/scripts/commit_prefix_check.pyAll checks passed. The strict Valgrind run completed with zero errors and zero
leaked bytes.
Summary by CodeRabbit
Bug Fixes
Tests