Repository navigation
ext/session: reject cookie separators in cookie_path and cookie_domain - #22925
Conversation
8404a77 to
54a0a18
Compare
php_session_send_cookie() appends session.cookie_path and session.cookie_domain to the Set-Cookie header verbatim, and neither setting rejected the separators that terminate a cookie attribute. An application could therefore append its own Domain, Path, HttpOnly, Secure, or SameSite attribute through either value. Apply the character set setcookie() already enforces on its $path and $domain options. Closes phpGH-22925
Sjord
left a comment
Sjord
left a comment
There was a problem hiding this comment.
I think it is a good idea to validate cookie settings, but this currently seems to reject too much.
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| Warning: session_set_cookie_params(): "session.cookie_path" must not contain any of the following characters ",; \t\r\n\013\014" in %s on line %d |
There was a problem hiding this comment.
Some of these characters seem as they may be legitimately used in paths. It could be possible to have a path with a space or a comma in it.
Perhaps https://datatracker.ietf.org/doc/html/rfc6265 can be used to more precisely determine which characters can be in which field.
There was a problem hiding this comment.
RFC 6265 Path allows space and comma; Domain does not. Browsers split cookie-av on ;.
This copies setcookie()'s path/domain check. Loosening session.cookie_path alone would accept values setcookie() still rejects.
There was a problem hiding this comment.
Ok. Making this consistent with setcookie makes sense. Even so, there is a slight risk of BC break when someone configured /my app/ as their cookie path and that breaks when upgrading. I think rejecting ; is fine because that was an obvious invalid value and wouldn't have worked. But this does not hold for rejecting space or comma.
There was a problem hiding this comment.
this does not hold for rejecting space or comma
setcookie() has rejected both in path since 7.3. I'd rather keep that set than special-case session.cookie_path.
There was a problem hiding this comment.
someone configured
/my app/as their cookie path and that breaks when upgrading
I was wrong about this. Having a space in the cookie path doesn't work. It seems that the browser automatically encodes the space in the path to %20, so the cookie path should also have %20 for it to match.
A comma works though. I consider it less likely that people use a comma in their path, but having session.cookie_path=foo,bar works and would break when this PR is merged.
@cmb69 do you have any thoughts on this?
There was a problem hiding this comment.
It seems that @cmb69 copied the validation from cookie value to cookie path. This is not entirely correct, since these can contain different characters according to the RFC. I don't think it is a good idea to persist the mistake into the INI values.
I can see the following ways forward:
- @cmb69 explains why the validation in setcookie actually is correct, instead of a copy-paste error like I assumed.
- the validation for the INI setting is changed to conform to the RFC (optionally also the validation for setcookie).
- we discuss on the internals mailing list or even via an PHP RFC what the validation should be.
There was a problem hiding this comment.
Not a copy-paste error. 5cb825d's message is explicit: "characters not allowed for name and value should also be forbidden for path and domain."
Comma in a path is RFC-legal, but RFC 6265 section 3 bans folding Set-Cookie precisely because "," conflicts with standard header folding, so anything that folds or re-splits on commas misreads a comma'd path. I'd keep setcookie's set here; relaxing both to the RFC grammar is a setcookie() question, and this INI handler would follow it.
There was a problem hiding this comment.
I agree with @Sjord on this one. Session module is too sensitive to introduce breaking change without clear purpose. If , is allowed for path then it should stay this way, and alternatively setcookie() grammar should be relaxed. Not the other way around.
Domain on the other hand should have stricter validation as it's still possible to pass invalid form of domains with those subset of invalid characters. But it's story for another PR I think.
There was a problem hiding this comment.
Narrowed cookie_path to ; and CR/LF, so a comma'd path keeps working. cookie_domain keeps setcookie()'s set.
|
@jorgsowa Could you take a look at this? |
php_session_send_cookie() appends session.cookie_path and session.cookie_domain to the Set-Cookie header verbatim, and neither setting rejected the separators that terminate a cookie attribute. An application could therefore append its own Domain, Path, HttpOnly, Secure, or SameSite attribute through either value. cookie_path now rejects ";" and CR/LF, which RFC 6265 path-value excludes, while keeping the comma it allows; cookie_domain takes the set setcookie() enforces on $domain. Closes phpGH-22925
54a0a18 to
434b4f7
Compare
|
Thanks, looks good to me. |
php_session_send_cookie() appends session.cookie_path and session.cookie_domain to the Set-Cookie header verbatim, and neither setting rejected the separators that terminate a cookie attribute. An application could therefore append its own Domain, Path, HttpOnly, Secure, or SameSite attribute through either value. cookie_path now rejects ";" and CR/LF, which RFC 6265 path-value excludes, while keeping the comma it allows; cookie_domain takes the set setcookie() enforces on $domain. Closes phpGH-22925
434b4f7 to
4e8efc5
Compare
|
It looks like this PR is causing CI failures. |
|
@kamil-tekiela fix #24325 |
4 participants
php_session_send_cookie()appendssession.cookie_pathandsession.cookie_domainto the Set-Cookie header verbatim, and neither setting rejects the characters that terminate a cookie attribute, so an application can append its own Domain, Path, HttpOnly, Secure or SameSite.session_set_cookie_params(['path' => '/; Domain=evil.example'])emitsSet-Cookie: PHPSESSID=<id>; path=/; Domain=evil.example.This applies the byte set
setcookie()already enforces on its $path and $domain options, and does it in the ini handler so that both session_set_cookie_params() and a direct ini_set() are covered. Master only, since it starts rejecting configuration that is accepted today.