router: fix null-deref in addDownstreamSetCookie when downstream connection is null#45134
Open
omkhar wants to merge 4 commits into
Open
router: fix null-deref in addDownstreamSetCookie when downstream connection is null#45134omkhar wants to merge 4 commits into
omkhar wants to merge 4 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>
|
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>
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
Per @phlax's review feedback (envoyproxy#45134 review comment on changelogs/current.yaml:54): the bug_fix entry moves out of the legacy current.yaml (which is now mostly defunct after envoyproxy#45093) and into a per-file RST under changelogs/current/bug_fixes/ matching the new layout described in envoyproxy#45095. Signed-off-by: Omkhar Arasaratnam <omkhar@linkedin.com>
Contributor
Author
|
Addressed @phlax — migrated the bug_fix entry to |
envoy's tools/spelling/check_spelling_pedantic.py dictionary has
'dereference', 'dereferences', 'dereferencing' but not the past-tense
'dereferenced'. Reword to past-conditional ('would unconditionally
dereference') in both the source comment and the matching changelog
RST. No behavior change.
Signed-off-by: Omkhar Arasaratnam <omkhar@linkedin.com>
mattklein123
requested changes
May 29, 2026
mattklein123
left a comment
Member
There was a problem hiding this comment.
This needs a test.
/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
3 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.