Skip to content

mod_proxy_balancer: Fix a read past the end of the string when looking up the sticky session route. - #843

Open
alepacheco wants to merge 2 commits into
apache:trunkfrom
alepacheco:balancer-sticky-overread
Open

alepacheco wants to merge 2 commits into
apache:trunkfrom
alepacheco:balancer-sticky-overread

Conversation

@alepacheco

Copy link
Copy Markdown

get_path_param() and get_cookie_param() advance the for-loop variable over the parameter name and its '=' inside the loop body, and the loop increment then calls strstr(p + 1, name). When the sticky session name is the last thing in the URL or Cookie header and has no value after it (for example Cookie: a=b; JSESSIONID or /app;jsessionid=), the variable is left on or past the terminating NUL, and the next strstr() starts reading after the end of the string.

This change steps over the name with a separate pointer, so the loop variable always stays where strstr() found the name and the next search starts inside the string. The returned values are otherwise unchanged, with one exception: a match that overlaps an earlier partial match is no longer skipped. get_path_param() also now returns no route for an empty name, which stickysession=NAME| configures and which strstr() would match at every position up to and past the NUL.

These are the only loops of this shape in server/ and modules/.

I checked the old and new versions of both functions with exact-size heap buffers under ASan. The old ones overflow on the inputs above, and the new ones return the same values for the other cases. The test/modules/proxy suite passes. I can add a pyhttpd test for a balancer with stickysession if you'd like one.

…g up the sticky session route.

* modules/proxy/mod_proxy_balancer.c (get_path_param, get_cookie_param):
  Use a separate pointer to step over the parameter name and its '=',
  leaving the loop variable where strstr() found the name. When the name
  was at the end of the URL or Cookie header with no value after it, the
  loop variable was left on (or one past) the terminating NUL, so the
  next strstr(p + 1, name) started reading after the end of the string.
  The next search now also starts one character into the previous match,
  so a name that overlaps an earlier partial match is no longer skipped.
  (get_path_param): Return no route for an empty name, as configured by
  "stickysession=NAME|", which strstr() would otherwise match up to and
  past the terminating NUL.

* changes-entries/balancer-sticky-overread.txt: Add entry.
@notroj

notroj commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator

I can add a pyhttpd test for a balancer with stickysession if you'd like one.

Please do!

* test/modules/proxy/test_01_http.py (test_proxy_01_004): Check that
  the route is taken from the stickysession cookie and query string
  parameter, and that a session name without a value selects no route.
@alepacheco

Copy link
Copy Markdown
Author

Added test_proxy_01_004 to test/modules/proxy/test_01_http.py: a balancer with two routes and stickysession=ROUTEID|routeid. It checks that the route is taken from the cookie and from the query string, that a session name without a value (ROUTEID, a=b; ROUTEID, routeid, ...) selects no route and the request still succeeds, and that a later cookie with a value is still found. On an ASan build of trunk the value-less cookie case fails; with the fix the proxy suite passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants