router: fix null-deref in addDownstreamSetCookie when downstream connection is null#45134
Open
omkhar wants to merge 2 commits into
Open
router: fix null-deref in addDownstreamSetCookie when downstream connection is null#45134omkhar wants to merge 2 commits into
omkhar wants to merge 2 commits into
Conversation
…tion Filter::addDownstreamSetCookie() dereferences downstreamConnection() unconditionally to compose the per-connection seed for the generated cookie value. When the router is invoked without a downstream connection (Http::AsyncClient-driven paths: ext_authz / ratelimit side calls, mirror/shadow, and health checks) the connection pointer is null and the worker process segfaults whenever a cookie hash policy is configured. Skip the connection-derived seed and return an empty cookie value for those callers. The resulting cookie hashes identically across all async invocations, which preserves hash-stability for the cookie hash policy on this path. Reported privately via GHSA-47qr-fw7j-6m57; envoy security team asked for this to be fixed in the open as a non-security bug because the misconfiguration would surface in basic functional testing (the affected configuration does not appear in production deployments that exercise the async-router paths). Signed-off-by: Omkhar Arasaratnam <omkhar@linkedin.com>
omkhar
temporarily deployed
to
external-contributors
GitHub Actions
Inactive
|
Hi @omkhar, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
Replace 'segfault' with 'crash' in the bug_fixes entry from the previous commit; envoy's pedantic spell check does not recognize 'segfault'. Signed-off-by: Omkhar Arasaratnam <omkhar@linkedin.com>
omkhar
requested a deployment
to
external-contributors
GitHub Actions
Waiting
phlax
reviewed
May 19, 2026
|
|
||
| bug_fixes: | ||
| # *Changes expected to improve the state of the world and are unlikely to have negative effects* | ||
| - area: router |
Member
There was a problem hiding this comment.
needs to use new changelog layout - see #45095
/wait
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.
Commit Message:
router: fix crash in addDownstreamSetCookie when no downstream connection
Filter::addDownstreamSetCookie()unconditionally dereferencesdownstreamConnection()to compose the per-connection seed for thegenerated cookie value. When the router is invoked without a downstream
connection (e.g.
Http::AsyncClient-driven paths such asext_authzorratelimit side calls, mirror/shadow, and health checks) the connection
pointer is null and the worker process segfaults whenever a cookie hash
policy is configured.
Skip the connection-derived seed and return an empty cookie value for
those callers. The resulting cookie hashes identically across all async
invocations, which preserves hash-stability for the cookie hash policy on
this path.
Additional Description:
The TODO comment in
router.hline 500 had already flagged the risk("Need to check for null conn if this is ever used by Http::AsyncClient
in the future"); this PR addresses that TODO.
Reported privately via GHSA-47qr-fw7j-6m57; envoy security team asked
for this to be fixed in the open as a non-security bug because the
misconfiguration would surface in basic functional testing.
Fixes #45133.
Risk Level: Low.
Defensive null check; no behavior change for the existing (downstream-
connection-present) path. Affected async-router paths previously
crashed and now degrade to an empty cookie value, which is a strict
improvement.
Testing: Manual review of the affected code path against the
private-fork regression test. Happy to add a focused regression test
in
test/common/router/router_test.ccthat exercises the null-connection path if maintainers would like; it adds enough scope that
I kept it out of this initial PR to keep the fix small.
Docs Changes: None required.
Release Notes: Added to
changelogs/current.yamlunderbug_fixes(area: router).
Platform Specific Features: None.