Skip to content
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.

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

[LiveComponent] Allow specifying a live-component specific secret which defaults to kernel.secret #2453

Closed
dkarlovi opened this issue Dec 19, 2024 · 6 comments · Fixed by #2462

Comments

@dkarlovi
Copy link
Contributor

dkarlovi commented Dec 19, 2024

Allowing to specify a LiveComponent specific secret which, (if not specified, defaults to kernel.secret) would increase the security of the feature.

Namely, one use case we had is we wanted a Node app to be able to render correct live component blocks. To do that, it needs to share the key with Symfony's LiveComponent implementation to generate the signatures which get verified by Symfony for it to work. We'd prefer to not have to share the kernel.secret with the node app (even though it's internal and built by us, trusted) because that provides ways to attack the Symfony app in other ways (login links, RCE via fragments) assuming it leaks.

Simply making this secret configurable would improve the security overall because the secret would then have a tiny scope (only that one specific thing) and be intended to be shared with the Node collaborator.

A similar thing was done somewhere on Symfony main repo by @nicolas-grekas recently (login links?)

@WebMamba
Copy link
Contributor

I think this is already implemented in see: #2251

@dkarlovi
Copy link
Contributor Author

AFAIK this is unrelated to CSRF, it's https://github.com/symfony/ux/blob/2.x/src/LiveComponent/src/Util/FingerprintCalculator.php and

private function calculateChecksum(array $dehydratedPropsData): ?string
{
// sort so it is always consistent (frontend could have re-ordered data)
$this->recursiveKeySort($dehydratedPropsData);
return base64_encode(hash_hmac('sha256', json_encode($dehydratedPropsData), $this->secret, true));
}

@smnandre
Copy link
Member

o do that, it needs to share the key with Symfony's LiveComponent implementation to generate the signatures which get verified by Symfony for it to wor

Warning: keep in mind all this is internal, and we won't support actively anything external. Especially related to encryption.

It's enough to maintain code here, we cannot officially support external usage / extension point like this. So you should not take anything about encryption / secrets / etc in the codebase for granted.

It can (and it will) change.

Simply making this secret configurable would improve the security overall because the secret would then have a tiny scope (only that one specific thing)

Can you find the change you mentioned in the core ? It's an interseting idea

@dkarlovi
Copy link
Contributor Author

It can (and it will) change.

I'm aware, we have a small test which is checking the value calculated by Symfony matches the one calculated by Node.

Can you find the change you mentioned in the core ?

symfony/symfony#56840

@smnandre
Copy link
Member

Ok! 👌

smnandre added a commit to smnandre/ux that referenced this issue Dec 21, 2024
…ameter]

Improve security before we allow secret customization for LiveComponents (cf symfony#2453)

I consider this a fix as passing an empty string for secret produce the same hash as passing null... which is deprecated for obvious reasons.
@smnandre
Copy link
Member

smnandre commented Dec 21, 2024

Thank you @dkarlovi

First step here #2461

Second one here #2462

:)

smnandre added a commit to smnandre/ux that referenced this issue Dec 21, 2024
Allow to configure a dedicated secret (used in FingerprintCalculator and LiveComonentHydrator)

Suggested by symfony#2453
Inspired by #56840
smnandre added a commit that referenced this issue Dec 24, 2024
…arameter] (smnandre)

This PR was merged into the 2.x branch.

Discussion
----------

[LiveComponent] Check secret is not empty + add [SensitiveParameter]

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Issues        | Fix #...
| License       | MIT

Improve security before we allow secret customization for LiveComponents (cf #2453)

I consider this a fix as passing an empty string for secret produces the same hash as passing null... which is deprecated for obvious reasons.

Commits
-------

3c3d097 [LiveComponent] Check secret is not empty + add missing [SensitiveParameter]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants