Skip to content

[HttpKernel] Don't use eval() to render ESI/SSI #50013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 17, 2023
Merged

[HttpKernel] Don't use eval() to render ESI/SSI #50013

merged 1 commit into from
Apr 17, 2023

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 6.3
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

Yes, this would not be needed if people were able to use a real kernel.secret. But apparently, many fail.
And it looks like we can do something about it, so let's makes this impossible to exploit :)

@carsonbot carsonbot added this to the 6.3 milestone Apr 13, 2023
@nicolas-grekas nicolas-grekas force-pushed the hk-no-eval branch 5 times, most recently from 711c6ec to 0464c44 Compare April 14, 2023 08:41
stof
stof reviewed Apr 14, 2023
View reviewed changes
@nicolas-grekas nicolas-grekas force-pushed the hk-no-eval branch 5 times, most recently from 952c300 to 44269c3 Compare April 14, 2023 09:35
@fabpot
Copy link
Member

fabpot commented Apr 17, 2023

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 3712d2e into symfony:6.3 Apr 17, 2023
@nicolas-grekas nicolas-grekas deleted the hk-no-eval branch April 19, 2023 09:57
@fritzmg
Copy link
Contributor

fritzmg commented May 3, 2023

Yes, this would not be needed if people were able to use a real kernel.secret. But apparently, many fail.

imho the issue is that it is not actually mandatory at all. Even the UriSigner allows an empty secret and in the Configuration tree of the FrameworkBundle, framework.secret is allowed to be anything. Currently only symfony/flex will automatically generate an APP_SECRET for you when you first set up the project. But that does not help if you either do not setup your project via symfony/flex or when you deploy the website to the live environment.

In any case, will this change also be back ported to Symfony 5.4?

@alexislefebvre
Copy link
Contributor

@fritzmg I can't find the relevant page in the doc but backports are not applied in the Symfony ecosystem. This new way to render is a feature, it can't land on a previous release.

@fritzmg
Copy link
Contributor

fritzmg commented May 4, 2023

But it's a security issue that arbitrary code can be executed via the _fragment controller.

@alexislefebvre
Copy link
Contributor

Fair enough, does it require a CVE issue? 😬

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented May 4, 2023 •
edited
Loading

There is no security issue when one sets the secret as documented.

@fabpot
Copy link
Member

fabpot commented May 4, 2023

Let's merge the fix in 5.4 and 6.2 as well.

@nicolas-grekas nicolas-grekas mentioned this pull request May 4, 2023
Merged
@nicolas-grekas
Copy link
Member Author

See #50238

fabpot added a commit that referenced this pull request May 5, 2023
@fabpot
bug #50238 [HttpKernel] Don't use eval() to render ESI/SSI (nicolas-g…
9af90a8 
…rekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[HttpKernel] Don't use eval() to render ESI/SSI

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Because this might be an important security hardening, this PR is a backport of #50013 for 5.4.

Commits
-------

ea449ca [HttpKernel] Don't use eval() to render ESI/SSI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
HttpKernel Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
6 participants
@nicolas-grekas @fabpot @fritzmg @alexislefebvre @stof @carsonbot