Skip to content

Check whether secrets are empty and mark them all as sensitive #52469

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

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Check whether secrets are empty and mark them all as sensitive #52469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/Symfony/Component/HttpFoundation/UriSigner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
namespace Symfony\Component\HttpFoundation;

/**
* Signs URIs.
*
* @author Fabien Potencier <fabien@symfony.com>
*/
class UriSigner
Expand All @@ -22,11 +20,14 @@ class UriSigner
private string $parameter;

/**
* @param string $secret A secret
* @param string $parameter Query string parameter to use
*/
public function __construct(#[\SensitiveParameter] string $secret, string $parameter = '_hash')
{
if (!$secret) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we consider '0' as an empty secret?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's equally weak

throw new \InvalidArgumentException('A non-empty secret is required.');
}

$this->secret = $secret;
$this->parameter = $parameter;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Mailer/Bridge/Brevo/Webhook/BrevoRequestParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
]);
}

protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
{
$content = $request->toArray();
if (
Expand Down
9 changes: 7 additions & 2 deletions src/Symfony/Component/Mailer/Bridge/Mailgun/Webhook/MailgunRequestParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Symfony\Component\HttpFoundation\RequestMatcher\MethodRequestMatcher;
use Symfony\Component\HttpFoundation\RequestMatcherInterface;
use Symfony\Component\Mailer\Bridge\Mailgun\RemoteEvent\MailgunPayloadConverter;
use Symfony\Component\Mailer\Exception\InvalidArgumentException;
use Symfony\Component\RemoteEvent\Event\Mailer\AbstractMailerEvent;
use Symfony\Component\RemoteEvent\Exception\ParseException;
use Symfony\Component\Webhook\Client\AbstractRequestParser;
Expand All @@ -37,8 +38,12 @@ protected function getRequestMatcher(): RequestMatcherInterface
]);
}

protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
{
if (!$secret) {
throw new InvalidArgumentException('A non-empty secret is required.');
}

$content = $request->toArray();
if (
!isset($content['signature']['timestamp'])
Expand All @@ -60,7 +65,7 @@ protected function doParse(Request $request, string $secret): ?AbstractMailerEve
}
}

private function validateSignature(array $signature, string $secret): void
private function validateSignature(array $signature, #[\SensitiveParameter] string $secret): void
{
// see https://documentation.mailgun.com/en/latest/user_manual.html#webhooks-1
if (!hash_equals($signature['signature'], hash_hmac('sha256', $signature['timestamp'].$signature['token'], $secret))) {
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Mailer/Bridge/Mailjet/Webhook/MailjetRequestParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
]);
}

protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
{
try {
return $this->converter->convert($request->toArray());
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Mailer/Bridge/Postmark/Webhook/PostmarkRequestParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
]);
}

protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
{
$payload = $request->toArray();
if (
Expand Down
13 changes: 7 additions & 6 deletions src/Symfony/Component/Mailer/Bridge/Sendgrid/Webhook/SendgridRequestParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Symfony\Component\HttpFoundation\RequestMatcher\MethodRequestMatcher;
use Symfony\Component\HttpFoundation\RequestMatcherInterface;
use Symfony\Component\Mailer\Bridge\Sendgrid\RemoteEvent\SendgridPayloadConverter;
use Symfony\Component\Mailer\Exception\InvalidArgumentException;
use Symfony\Component\RemoteEvent\Event\Mailer\AbstractMailerEvent;
use Symfony\Component\RemoteEvent\Exception\ParseException;
use Symfony\Component\Webhook\Client\AbstractRequestParser;
Expand Down Expand Up @@ -86,12 +87,12 @@ protected function doParse(Request $request, string $secret): ?AbstractMailerEve
*
* @see https://docs.sendgrid.com/for-developers/tracking-events/getting-started-event-webhook-security-features
*/
private function validateSignature(
string $signature,
string $timestamp,
string $payload,
string $secret,
): void {
private function validateSignature(string $signature, string $timestamp, string $payload, #[\SensitiveParameter] string $secret): void
{
if (!$secret) {
throw new InvalidArgumentException('A non-empty secret is required.');
}

$timestampedPayload = $timestamp.$payload;

// Sendgrid provides the verification key as base64-encoded DER data. Openssl wants a PEM format, which is a multiline version of the base64 data.
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/Mailer/Transport/Smtp/Auth/CramMd5Authenticator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Mailer\Transport\Smtp\Auth;

use Symfony\Component\Mailer\Exception\InvalidArgumentException;
use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;

/**
Expand Down Expand Up @@ -41,6 +42,10 @@ public function authenticate(EsmtpTransport $client): void
*/
private function getResponse(#[\SensitiveParameter] string $secret, string $challenge): string
{
if (!$secret) {
throw new InvalidArgumentException('A non-empty secret is required.');
}

if (\strlen($secret) > 64) {
$secret = pack('H32', md5($secret));
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Notifier/Bridge/Twilio/Webhook/TwilioRequestParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
return new MethodRequestMatcher('POST');
}

protected function doParse(Request $request, string $secret): ?SmsEvent
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?SmsEvent
{
// Statuses: https://www.twilio.com/docs/sms/api/message-resource#message-status-values
// Payload examples: https://www.twilio.com/docs/sms/outbound-message-logging
Expand Down
9 changes: 7 additions & 2 deletions src/Symfony/Component/Notifier/Bridge/Vonage/Webhook/VonageRequestParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Symfony\Component\HttpFoundation\RequestMatcher\IsJsonRequestMatcher;
use Symfony\Component\HttpFoundation\RequestMatcher\MethodRequestMatcher;
use Symfony\Component\HttpFoundation\RequestMatcherInterface;
use Symfony\Component\Notifier\Exception\InvalidArgumentException;
use Symfony\Component\RemoteEvent\Event\Sms\SmsEvent;
use Symfony\Component\Webhook\Client\AbstractRequestParser;
use Symfony\Component\Webhook\Exception\RejectWebhookException;
Expand All @@ -30,8 +31,12 @@ protected function getRequestMatcher(): RequestMatcherInterface
]);
}

protected function doParse(Request $request, string $secret): ?SmsEvent
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?SmsEvent
{
if (!$secret) {
throw new InvalidArgumentException('A non-empty secret is required.');
}

// Signed webhooks: https://developer.vonage.com/en/getting-started/concepts/webhooks#validating-signed-webhooks
if (!$request->headers->has('Authorization')) {
throw new RejectWebhookException(406, 'Missing "Authorization" header.');
Expand Down Expand Up @@ -70,7 +75,7 @@ protected function doParse(Request $request, string $secret): ?SmsEvent
return $event;
}

private function validateSignature(string $jwt, string $secret): void
private function validateSignature(string $jwt, #[\SensitiveParameter] string $secret): void
{
$tokenParts = explode('.', $jwt);
if (3 !== \count($tokenParts)) {
Expand Down
9 changes: 5 additions & 4 deletions src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Security\Core\Authentication\Token;

use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
use Symfony\Component\Security\Core\User\UserInterface;

/**
Expand All @@ -32,12 +33,12 @@ public function __construct(UserInterface $user, string $firewallName, #[\Sensit
{
parent::__construct($user->getRoles());

if (empty($secret)) {
throw new \InvalidArgumentException('$secret must not be empty.');
if (!$secret) {
throw new InvalidArgumentException('A non-empty secret is required.');
}

if ('' === $firewallName) {
throw new \InvalidArgumentException('$firewallName must not be empty.');
if (!$firewallName) {
throw new InvalidArgumentException('$firewallName must not be empty.');
}

$this->firewallName = $firewallName;
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/Security/Core/Signature/SignatureHasher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\Security\Core\Signature;

use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
use Symfony\Component\Security\Core\Signature\Exception\ExpiredSignatureException;
use Symfony\Component\Security\Core\Signature\Exception\InvalidSignatureException;
use Symfony\Component\Security\Core\User\UserInterface;
Expand All @@ -37,6 +38,10 @@ class SignatureHasher
*/
public function __construct(PropertyAccessorInterface $propertyAccessor, array $signatureProperties, #[\SensitiveParameter] string $secret, ExpiredSignatureStorage $expiredSignaturesStorage = null, int $maxUses = null)
{
if (!$secret) {
throw new InvalidArgumentException('A non-empty secret is required.');
}

$this->propertyAccessor = $propertyAccessor;
$this->signatureProperties = $signatureProperties;
$this->secret = $secret;
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/Security/Http/Authenticator/RememberMeAuthenticator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Exception\AuthenticationException;
use Symfony\Component\Security\Core\Exception\CookieTheftException;
use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
use Symfony\Component\Security\Core\Exception\UserNotFoundException;
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
Expand Down Expand Up @@ -51,6 +52,10 @@ class RememberMeAuthenticator implements InteractiveAuthenticatorInterface

public function __construct(RememberMeHandlerInterface $rememberMeHandler, #[\SensitiveParameter] string $secret, TokenStorageInterface $tokenStorage, string $cookieName, LoggerInterface $logger = null)
{
if (!$secret) {
throw new InvalidArgumentException('A non-empty secret is required.');
}

$this->rememberMeHandler = $rememberMeHandler;
$this->secret = $secret;
$this->tokenStorage = $tokenStorage;
Expand Down
7 changes: 4 additions & 3 deletions src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Symfony\Component\HttpFoundation\RateLimiter\AbstractRequestRateLimiter;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\RateLimiter\RateLimiterFactory;
use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
use Symfony\Component\Security\Http\SecurityRequestAttributes;

/**
Expand All @@ -35,10 +36,10 @@ final class DefaultLoginRateLimiter extends AbstractRequestRateLimiter
*/
public function __construct(RateLimiterFactory $globalFactory, RateLimiterFactory $localFactory, #[\SensitiveParameter] string $secret = '')
{
if ('' === $secret) {
trigger_deprecation('symfony/security-http', '6.4', 'Calling "%s()" with an empty secret is deprecated. A non-empty secret will be mandatory in version 7.0.', __METHOD__);
// throw new \Symfony\Component\Security\Core\Exception\InvalidArgumentException('A non-empty secret is required.');
if (!$secret) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change. Both the argument, as the deprecation, have been added in #51434 (@Spomky), which is also 6.4+. Converting the deprecation to an exception means that on upgrade from 6.3 and lower users of this class will immediately run into this exception. (Because it is a new argument it obviously isn't provided yet).

It isn't a big deal to fix, but just wanted to give a heads up about this. Wheteher you decide to revert to a deprecation on 6.4 and keep throwing on 7.0 is up to you. As said, it's not a big issue to fix as an end user, but it's a BC break without previous warning nevertheless.

Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided it's OK to add this exception when security is at risk.
That being said, I'm not sure why we use the secret here to hash the logged info.

Copy link
Contributor

@Spomky Spomky Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'm not sure why we use the secret here to hash the logged info.

This is the pepper for preventing rainbow tables to be used on IP addresses or username/IPs combinations
Edit: article (in French) on bad design where simple hashes are used issued few days after the PR => https://theconversation.com/les-dangers-de-la-cryptographie-non-maitrisee-212881

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in the MR, if you notice this breaking change happening, you had your app severely misconfigured and should send thanks to the core team for pointing it out. 😃

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided it's OK to add this exception when security is at risk.

I do get that argument for the existing $secret arguments, this as it indeed is a security risk to not provide it (/use an empty string). But in this case it isn't an existing argument for which the user (knowingly) provided an unsecure value. It's a new parameter, so to not break BC it does require a default value. But then this default value is considered invalid and still throws an exception.

So if it is decided that the exception must stay, I now think it makes sense to just drop the default value. Then users are at least made aware they're not providing a (now required, for security) parameter. Instead of the currently "vague" exception that they're providing an invalid value (to then discover they don't provide any value at all, the default is "broken", and they must provide a value themselfs).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please send a PR on branch 6.4?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in short / to put it differently:
On version 6.3 one must provide 2 parameters, $globalFactory and $localFactory. But the exact same code on 6.4 leads to an exception, that a non-empty secret must be provided. But I'm not providing a secret at all. So in an ordinary case this would be an "illegal" change as it breaks BC. For the sake of security I do get the "we want to require a secret". But IMO then the error should be about the missing parameter (so just the PHP error), and not a custom exception. I.e.: dropping the default value so PHP gives an error for the invalid method call / missing parameter, and keeping the check it's not empty as well. As IMO the standard PHP error is a better DX because it's a common error, the custom error requires more time to figure out what exactly is going wrong and why (to still come to the conclusion the argument is now required).

And in all(/most?) other cases changed by this PR the $secret parameter already existed. So IMO indeed can be considered a bug / security fix where values which have always been invalid now actually are detected and result in an error. But that's not the case for this specific scenario (as it's a new parameter which could not have been provided beforehand).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Sorry, I apparently missed some comments in between)

Can you please send a PR on branch 6.4?

I'll try to do so, but it will most likely be after the weekend. (I read the blogpost about #51434 on the go, and it reminded me about the fix I had to make yesterday).
And just to be sure: I presume a PR to drop the default value?

As noted in the MR, if you notice this breaking change happening, you had your app severely misconfigured and should send thanks to the core team for pointing it out. 😃

That's not the case here. Because it is a new parameter. And I can't severly misconfigure anything when there was nothing to configure it in the first place. The "configuration" is added in 6.4 and the default is "severely misconfigured".

throw new InvalidArgumentException('A non-empty secret is required.');
}

$this->globalFactory = $globalFactory;
$this->localFactory = $localFactory;
$this->secret = $secret;
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Webhook/Client/AbstractRequestParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
*/
abstract class AbstractRequestParser implements RequestParserInterface
{
public function parse(Request $request, string $secret): ?RemoteEvent
public function parse(Request $request, #[\SensitiveParameter] string $secret): ?RemoteEvent
{
$this->validate($request);

Expand All @@ -41,7 +41,7 @@ public function createRejectedResponse(string $reason): Response

abstract protected function getRequestMatcher(): RequestMatcherInterface;

abstract protected function doParse(Request $request, string $secret): ?RemoteEvent;
abstract protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?RemoteEvent;

protected function validate(Request $request): void
{
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Webhook/Client/RequestParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
]);
}

protected function doParse(Request $request, string $secret): RemoteEvent
protected function doParse(Request $request, #[\SensitiveParameter] string $secret): RemoteEvent
{
$body = $request->toArray();

Expand All @@ -60,7 +60,7 @@ protected function doParse(Request $request, string $secret): RemoteEvent
);
}

private function validateSignature(HeaderBag $headers, string $body, $secret): void
private function validateSignature(HeaderBag $headers, string $body, #[\SensitiveParameter] string $secret): void
{
$signature = $headers->get($this->signatureHeaderName);
$event = $headers->get($this->eventHeaderName);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Webhook/Client/RequestParserInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ interface RequestParserInterface
*
* @throws RejectWebhookException When the payload is rejected (signature issue, parse issue, ...)
*/
public function parse(Request $request, string $secret): ?RemoteEvent;
public function parse(Request $request, #[\SensitiveParameter] string $secret): ?RemoteEvent;

public function createSuccessfulResponse(): Response;

Expand Down
7 changes: 6 additions & 1 deletion src/Symfony/Component/Webhook/Server/HeaderSignatureConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\HttpClient\HttpOptions;
use Symfony\Component\RemoteEvent\RemoteEvent;
use Symfony\Component\Webhook\Exception\InvalidArgumentException;
use Symfony\Component\Webhook\Exception\LogicException;

/**
Expand All @@ -26,8 +27,12 @@ public function __construct(
) {
}

public function configure(RemoteEvent $event, string $secret, HttpOptions $options): void
public function configure(RemoteEvent $event, #[\SensitiveParameter] string $secret, HttpOptions $options): void
{
if (!$secret) {
throw new InvalidArgumentException('A non-empty secret is required.');
}

$opts = $options->toArray();
$headers = $opts['headers'];
if (!isset($opts['body'])) {
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Webhook/Server/HeadersConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function __construct(
) {
}

public function configure(RemoteEvent $event, string $secret, HttpOptions $options): void
public function configure(RemoteEvent $event, #[\SensitiveParameter] string $secret, HttpOptions $options): void
{
$options->setHeaders([
$this->eventHeaderName => $event->getName(),
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Webhook/Server/JsonBodyConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function __construct(
) {
}

public function configure(RemoteEvent $event, string $secret, HttpOptions $options): void
public function configure(RemoteEvent $event, #[\SensitiveParameter] string $secret, HttpOptions $options): void
{
$body = $this->serializer->serialize($event->getPayload(), 'json');
$options->setBody($body);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Webhook/Server/RequestConfiguratorInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@
*/
interface RequestConfiguratorInterface
{
public function configure(RemoteEvent $event, string $secret, HttpOptions $options): void;
public function configure(RemoteEvent $event, #[\SensitiveParameter] string $secret, HttpOptions $options): void;
}
8 changes: 7 additions & 1 deletion src/Symfony/Component/Webhook/Subscriber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,18 @@

namespace Symfony\Component\Webhook;

use Symfony\Component\Webhook\Exception\InvalidArgumentException;

class Subscriber
{
public function __construct(
private readonly string $url,
#[\SensitiveParameter] private readonly string $secret,
#[\SensitiveParameter]
private readonly string $secret,
) {
if (!$secret) {
throw new InvalidArgumentException('A non-empty secret is required.');
}
}

public function getUrl(): string
Expand Down