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

[11.x] Mark sensitive params with SensitiveParameter attribute #51940

Merged
merged 6 commits into from
Jun 28, 2024

Conversation

onlime
Copy link
Contributor

@onlime onlime commented Jun 27, 2024

Since Laravel 11 requires PHP 8.2+, we should make use of the SensitiveParameter attribute wherever it makes sense, to prevent potential leaking of sensitive information in logs or stack traces.

Is there some special reason, Laravel so far does not use #[SensitiveParameter] anywhere?

This PR is not feature complete, it's just a start and covers the most crucial spots where we can assume that at least 99.87% of the values are sensitive.

@driesvints
Copy link
Member

Thanks @onlime. @valorin could you maybe give your 2cents here? This PR seems okay to me but would love your opinion on it 🙏

@valorin
Copy link
Contributor

valorin commented Jun 28, 2024

I really like this idea of doing this for anything that could be sensitive. 👍

I'm pretty sure zend.exception_ignore_args=on is enabled by default anyway (it is default on Forge at least), so the risk is low, but this is a nice proactive measure to ensure nothing slips through if you do zend.exception_ignore_args disabled.

I know you said the PR isn't exhaustive, but I noticed the Encrypter should probably have $iv and $key marked too:

hash($iv, #[SensitiveParameter] $value, $key)

I wonder if it would make sense to add this for anywhere the app encryption key is used, in addition to passwords and encryption plaintext?

Another spot to check would be the password rehashing code, since from memory it passes around the plaintext too.

@onlime
Copy link
Contributor Author

onlime commented Jun 28, 2024

I know you said the PR isn't exhaustive, but I noticed the Encrypter should probably have $iv and $key marked too:

hash($iv, #[SensitiveParameter] $value, $key)

I wonder if it would make sense to add this for anywhere the app encryption key is used, in addition to passwords and encryption plaintext?

Another spot to check would be the password rehashing code, since from memory it passes around the plaintext too.

nice catch! I added it to a lot more Illuminate\Auth spots in 2f61388 – feel free to propose more!

This should (hopefully) not be a BC break, as the added #[SensitiveParameter] on an interface method prop is not enforced on its implementation.

@onlime onlime changed the title [11.x] Mark sensitive params with SensitiveParameter attribute [11.x] Mark sensitive params with SensitiveParameter attribute Jun 28, 2024
@crynobone
Copy link
Member

Hi there,

For global attribute we just declare it as #[\SensitiveParameter] instead of importing the class. Just like all other PHP attributes.

@onlime
Copy link
Contributor Author

onlime commented Jun 28, 2024

For global attribute we just declare it as #[\SensitiveParameter] instead of importing the class. Just like all other PHP attributes.

fixed in d2501a7

@taylorotwell taylorotwell merged commit 7b78fd3 into laravel:11.x Jun 28, 2024
28 checks passed
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.

None yet

5 participants