Wait this feature, cause now everyone can spot the framework type and moreover a specific version of symfony. It's security risk.

We've been wondering about the same many times.
The BC aspect is critical, the current PR cannot be merged.
Why do we need the hash at all? Maybe we can just remove it?
Then, we need to find a BC-friendly path forward.

We've been wondering about the same many times. The BC aspect is critical, the current PR cannot be merged. Why do we need the hash at all? Maybe we can just remove it? Then, we need to find a BC-friendly path forward.

I've carefully reviewed the tests and code again. It turns out that I missed some places where the userFqcn is used. I've made the necessary adjustments in this update.

I believe it is important to store some identifier in the cookie to associate it with the user class. Removing the class identifier entirely might lead to a potential security issue if a user with the same identifier but a different class is created. While I can't provide a concrete proof of concept for this, I think adding such an identifier would add an extra layer of safety.

In fact, the check for FQCN is already used in \Symfony\Component\Security\Http\RememberMe\PersistentRememberMeHandler::consumeRememberMeCookie, which further confirms the necessity of handling it properly.

To achieve this, I propose replacing userFqcn with a hash of the FQCN (e.g., a SHA-256 hash). This approach strikes a balance between preserving security (as it doesn't expose the internal structure of the application) and ensuring unique identification of the user class.

The BC aspect is critical, and if anyone relies on the \Symfony\Component\Security\Http\RememberMe\RememberMeDetails::getUserFqcn method for their own purposes, this change could cause issues. So I could implement a transitional solution: in the current version, mark the getUserFqcn method as deprecated and introduce a new getUserFqcnHash method. In future versions, getUserFqcn would be fully replaced by getUserFqcnHash.

What do you think about this approach?

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 7.3 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

I think we don't need to leak anything about user's FQCN, not even a hash:

• for persistent tokens, we store the FQCN in the storage, so we can use it to compare
• for non-persistent, we can add the FQCN to the signature

Do you think that's doable?

Self-analyzing my proposal :D

for non-persistent, we can add the FQCN to the signature

That'd mean not being able to validate the signature before calling the user provider, which means opening a timing-attack vector.

So a hash looks required then, but only for non-persistent ones.
Then, this hash shouldn't be a straight sha256 of the FQCN, because then I could still trivially guess the FQCN by building some rainbow table with precomputed and likely FQCNs (eg containing sha256(App\Entity\User), etc.)
To fix this, we could compute the hash by hashing the signature and the FQCN together. WDYT?

Up to dig this idea @thereisnobugs?

@nicolas-grekas

Sorry for the delayed answer. I've had a lot of work.

I dug deeper and realized that for non-persistent tokens in Symfony, FQCN is not used at all. If we remove this property, nothing will change in terms of signature calculation and verification. The only potential compatibility issues would occur if someone is relying on this data in their implementation.
In that case, we could deprecate this property and introduce a transition period and then remove this.

Persistent tokens are more complicated. Before checking the token, userIdentifier and userFqcn are compared.
I also found that for persistentToken to have an FQCN, $user::class is written into $tokenValue in RememberMeDetails and stored in the cookie.
I believe the idea behind storing FQCN for persistent tokens was for security reasons. Hypothetically, someone could have two different user classes with the same UserIdentifiers. In some cases, it might be possible to create a user with a different FQCN but the same UserIdentifier (e.g., "admin") and gain more privileges than intended.
It seems to me that there are at least two possible solutions:

1. Hash TokenValue and FQCN
   Modify \Symfony\Component\Security\Http\RememberMe\RememberMeDetails::fromPersistentToken:

// \Symfony\Component\Security\Http\RememberMe\RememberMeDetails::fromPersistentToken
public static function fromPersistentToken(PersistentToken $persistentToken, int $expires): self
{
    return new static($persistentToken->getUserIdentifier(), $expires, $persistentToken->getSeries().':'.hash('sha256', $persistentToken->getTokenValue().$persistentToken->getClass()));
}

and change the token verification method:

 public function verifyToken(PersistentTokenInterface $token, #[\SensitiveParameter] string $tokenValue): bool
 {
     if (hash_equals(hash('sha256', $token->getTokenValue().$token->getClass()), $tokenValue)) {
         return true;
     }
     // ...

1. Use a more complex hash in \Symfony\Component\Security\Http\RememberMe\RememberMeDetails::toString
   Instead of storing FQCN as is, store a hash of the concatenation of FQCN and tokenValue and compare this hash as I suggested before, for example:

public function toString(): string
{
    return implode(self::COOKIE_DELIMITER, [hash('sha256', $this->userFqcn, $this->value), strtr(base64_encode($this->userIdentifier), '+/=', '-_~'), $this->expires, $this->value]);
}

I think this should be enough, since the main idea is to make it less obvious from the cookie signature that we are using Symfony and PHP.

I don't think this same-user-identifier/different-fqcn exists. In the end userProvider->loadUserByIdentifier is called, so that there's only one user with said identifier.

What about just deprecating RememberMeDetails::getUserFqcn() and PersistentTokenInterface::getClass()?

Then of course we'll need to find a proper BC layer with support for existing cookies/databases.

WDYT @wouterj maybe?

I don't think this same-user-identifier/different-fqcn exists.

It does, migrating a legacy codebase or merging two projects with users in common for example.

It does, migrating a legacy codebase or merging two projects with users in common for example.

Can you expand? signature-based remember-me don't use the FQCN, so this doesn't apply to them.
Then what's different with persistent tokens?
If one has a remember-me cookie, the user-identifier is the only thing we need to know to reconnect the user.
Why would the internal implementation matter to end users?