TASK: precompose hash to prevent timing attacks

[reflexxion]

Precomposing a hash on cache warmup will make sure that the used hash to prevent timing attack always reflects the current configuration.

Previously there was a bcrypt password with the cost of 16 hard coded in but the configuration was set to 14 as default.

Fixes neos/neos-development-collection#3908 reported by @Benjamin-K

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked wit !!! and have upgrade-instructions

[Benjamin-K]

Looks fine by reading. But the additional request for the first account could cost some small amount of time, too. But way better than it was...

[kitsunet]

Good point and cool idea to just use the first found user. I feel that might somehow create potential new issues in combination with entity policies... And as Benjamin-K said the timing of the DB request. Depending on infrastructure I fear that might open a new vector for timing attacks...

[reflexxion]

Should I rewrite it, that it'll use a configuration value for the "test hash"?
The default will then reflect all other defaults (bcrypt, cost=>14).

[Benjamin-K]

Isn't the last part of the actual hash irrelevant - at least for bcrypt - and we could simply use bcrypt=>$2a$ROUNDS$etc as Fallback and replace ROUNDS from config? Don't know how to solve this for other hashing algorithms, but would be the best we can get for now I think.

[reflexxion]

Indeed that was our first thought.
IMO having a configurable hash in the configuration would be the better solution, because it can then reflect all algorithms.

[Benjamin-K]

Sounds good. But we need to write about it somewhere, so everyone with adjusted settings knows, where and how to set the Fallback hash.

[kdambekalns]

Isn't the last part of the actual hash irrelevant

It must at least be valid, see #1495 – been there, done that. 😎

[kdambekalns]

I don't really like fact we now use an actual account for the check (plus DB request and so forth). 🤔

My favourite solution would be to have a hash used here that is created based on the algorithm and it's settings actually configured, see:

flow-development-collection/Neos.Flow/Configuration/Settings.Security.yaml

Lines 101 to 125 in 34ef462

	hashingStrategies:
	# The default strategy will be used to hash or validate passwords if no specific strategy is given
	default: bcrypt
	pbkdf2: Neos\Flow\Security\Cryptography\Pbkdf2HashingStrategy
	bcrypt: Neos\Flow\Security\Cryptography\BCryptHashingStrategy
	Pbkdf2HashingStrategy:
	# Length of the random, dynamic salt that will be stored with the hashed password
	dynamicSaltLength: 8
	# Iteration count, high enough to make brute-force attacks unfeasible, use a custom value!
	iterationCount: 10000
	# Length of the derived key (hashed password) in bytes
	derivedKeyLength: 64
	# Hash function to use for PBKDF2
	algorithm: sha256
	BCryptHashingStrategy:
	# Cost of a BCrypt operation, can be between 4 and 31
	# The faster your machine is, the higher this number should be
	cost: 14

Since caclulating that hash on the fly would take time, too, it must be calculated before being used, maybe during ache warmup, and stored somewhere.

Asking the user to set a dummy hash in the settings sounds like something to be "overlooked by default". Then again, if we have that setting next to the existing hashing settings, it might work fine.

[kdambekalns]

Added this to the related issue, putting it here, too:

---

Hm, the cost of 14 was the default since this was added, see 3343e59#diff-4360c37e41d3456986dee193b75552ea7dfa3cb03b0c4161a07fdcbe8e990f07

But looking at #1495 shows that's when the cost of 16 for the dummy was added, so it was me who broke it with the fix. 🙈

---

Now, couldn't we stick to that dummy and really just adjust the cost inside to what is the current configuration? And on top of that provide dummies for the other algorithms, for completeness' sake?

[Benjamin-K]

I like the idea of autogenerating the hash when warming up caches.
The solution with a fallback hash for each hashing algorithm with the Neos defaults is also ok, but will possibly open up this issue whenever someone changes the defaults.

So either autogenerate the hash when warming up caches or set the fallback in configuration with a comment that warns other developers, that they need to update the fallback hash whenever they change default settings of the used hashing algorithm.

[albe]

I also like the idea to pregenerate a hash as a compile step.
But, hear me out, I have another (maybe crazy) idea:

So from a security PoV the following code block should execute exactly the same no matter if a non-existing username or invalid password is picked (so the validatePassword = false code path):

flow-development-collection/Neos.Flow/Classes/Security/Authentication/Provider/PersistedUsernamePasswordProvider.php

Lines 95 to 117 in 4f841ee

	$this->securityContext->withoutAuthorizationChecks(function () use ($username, &$account, $providerName) {
	$account = $this->accountRepository->findActiveByAccountIdentifierAndAuthenticationProviderName($username, $providerName);
	});
	$authenticationToken->setAuthenticationStatus(TokenInterface::WRONG_CREDENTIALS);
	if ($account === null) {
	// validate the account anyways (with the first users salt) in order to prevent timing attacks on this provider
	/** @var Account $testAccount */
	$testAccount = $this->accountRepository->createQuery()->setLimit(1)->execute()->getFirst();
	$this->hashService->validatePassword($password, $testAccount ? $testAccount->getCredentialsSource() : 'bcrypt=>$2a$14$mYqRRlg5V2yUDy1bd9vt3Oq8Fa9d508WWazFWE5tcpTGn3G145RAm');
	return;
	}
	if ($this->hashService->validatePassword($password, $account->getCredentialsSource())) {
	$account->authenticationAttempted(TokenInterface::AUTHENTICATION_SUCCESSFUL);
	$authenticationToken->setAuthenticationStatus(TokenInterface::AUTHENTICATION_SUCCESSFUL);
	$authenticationToken->setAccount($account);
	} else {
	$account->authenticationAttempted(TokenInterface::WRONG_CREDENTIALS);
	}
	$this->accountRepository->update($account);
	$this->persistenceManager->whitelistObject($account);

Now, no matter how accurately we generate a hash matching the (default) settings, we still do an early return when there is no account.

        if ($account === null) {
            // validate the account anyways (with the first users salt) in order to prevent timing attacks on this provider
            /** @var Account $testAccount */
            $testAccount = $this->accountRepository->createQuery()->setLimit(1)->execute()->getFirst();
            $this->hashService->validatePassword($password, $testAccount ? $testAccount->getCredentialsSource() : 'bcrypt=>$2a$14$mYqRRlg5V2yUDy1bd9vt3Oq8Fa9d508WWazFWE5tcpTGn3G145RAm');
            return; // <- this here is a potential timing attack to information disclosure risk
        }

So what we maybe really should do is have a dummy account that is returned by $accountRepository->findActiveByAccountIdentifierAndAuthenticationProviderName() directly from the SQL. Something like

where (accountIdentifier = :accountIdentifier OR accountIdentifier = 'hardcodeddummyaccountIdentifier') AND authenticationProviderName = :authenticationProviderName AND (expirationDate IS NULL OR expirationDate > NOW())

(Disclaimer: I'm not 100% sure if the OR on the accountIdentifier guarantees that whenever the first case matches, that result be the first in the result set)
Then this account should have a credentialSource that will never ever validate correctly and that has a hashing config with current defaults (so should be set in a bootstrap/offline).
That way we could effectively just strip the complete if ($account == null) check block and the rest of the code is free of timing attack vectors (though OTOH without rate limiting it opens up for spamming updates of the dummy account and potentially DOS the database?).

[Benjamin-K]

This will allow users to login if they took the password used for the hardcodeddummyaccount by chance even if the used username does not exist, doesn't it? Or we need to check, if accountIdentifier is the hardcoded one and set the password to sth definitely not matching the hash.

Additionally, if brute-force prevention is used (like a simple one in AE.BruteForce), this will possibly block the hardcoded account as soon as the limit of invalid logins is reached (which will always the case for the hardcoded account).

[albe]

This will allow users to login if they took the password used for the hardcodeddummyaccount by chance even if the used username does not exist, doesn't it?

Yeah, that's why I said "a credentialSource that will never ever validate correctly". If the password chosen is long enough at least the chance to enter exactly that goes towards 0.

Additionally, if brute-force prevention is used (like a simple one in AE.BruteForce), this will possibly block the hardcoded account as soon as the limit of invalid logins is reached (which will always the case for the hardcoded account).

Argh. Good point. So the query for the account needs to become:

where (accountIdentifier = :accountIdentifier AND authenticationProviderName = :authenticationProviderName AND (expirationDate IS NULL OR expirationDate > NOW()) OR accountIdentifier = 'hardcodeddummyaccountIdentifier'

That way it bypasses the expiration of the account (as well as any authenticationProviderName, which makes sense since the "unauthenticatedUserAccount" should be independent of the provider).

[reflexxion]

Nice idea @albe

But IMO they are more security related features than a security fix?
Using your variant brings in some other problems like hiding the user everywhere else, generation (and regeneration) of that user, ...

An updated commit is no ready to be reviewed.
Each strategy now can be configured with an timingAttackFallbackHash that can be regenerated with ./flow security:generatehashedpassword.

[sorenmalling]

This will solve the issue for the shipped PersistedUsernamePasswordProvider.
So if you implement a own provider (I do in all projects, to avoid the persistenceManager and persistence of the Account model at the end), you are not better of.

So should this validation concept, be moved to somewhere else, that is easy extendable from a providers implementation, point of view?

Another point

Argh. Good point. So the query for the account needs to become:
where (accountIdentifier = :accountIdentifier AND authenticationProviderName = :authenticationProviderName AND (expirationDate IS NULL OR expirationDate > NOW()) OR accountIdentifier = 'hardcodeddummyaccountIdentifier'

means that the concept of "being active", is taken into account - though we don't really have a API concept, of what a active account means. With #1077 we still try to solve this including a talk about what a "authenticable user" should look like.

[robertlemke]

By reading this thread, my gut feeling says: this is going to be complicated and by that potentially error-prone. If it's all about the timing, I would really try to avoid solutions like dummy accounts and the like. Also, as @sorenmalling pointed out, problems may arise in custom implementations.

If I understand it correctly, the problem is, that we must make sure to not reveal information about the existence of a user or the correctness of a password by the observed duration of a response.

Given that, we should either make sure that, during authentication, the system's response always takes the same amount of time or that the timing is randomized, so that you cannot determine any conclusions from it.

One potential solution for a stable timing:

• figure out the maximum time the system needs for authentication, for example 1050ms
• round up that time, for example to 1500 ms
• during authentication:
  • record the current time
  • authenticate
  • run PHP's time_sleep_until() function to sleep until $recordedTime + $maximumTime

The "maximum time" could also be determined automatically and stored in a cache.

A solution for a random timing could also make use of time_sleep_until() but using more randomized parameters.

Would that help?

[Benjamin-K]

• figure out the maximum time the system needs for authentication, for example 1050ms

I think this part is hard to get as this might vary a lot depending on the used server / hosting.

IMO we should replace the used hardcoded hash with a hash that complies with the default settings for the next patch release.

The new method getTimingAttackFallback() can be added in a new major release or for 8.3 (as it is a breaking change). Until then we should decide, how to generate and store that hash.

• Generate a fallback hash when warming up hashes using the current configuration?
• Generate a fallback hash through a flow command?

[reflexxion]

I think this part is hard to get as this might vary a lot depending on the used server / hosting.

@Benjamin-K I think that's the same effort as

Generate a fallback hash when warming up hashes using the current configuration?

So both of them are ways we can go without breaking.

[robertlemke]

FWIW, I would not introduce a CLI command for generating a hash as long as we can somehow solve this automatically, for example by creating a hash or determine timing during the initial cache warmup.

[kitsunet]

Another try.

On flow:cache:warmup a token will be generated and the required time will be saved into cache. (with a precision and buffer configuration).

This time will be used in PersistedUsernamePasswordProvider for calling time_sleep_until.

Why wouldn't we then just create an actual hash with the current configuration and use that in our comparison instead of the hardcoded one?

[reflexxion]

Why wouldn't we then just create an actual hash with the current configuration and use that in our comparison instead of the hardcoded one?

Both ways are nearly the same I think.
But using the precomposed hash may be better, because it will also calculate "current CPU speed" in.

I'll add it in another commit (this time really a new commit) so both ways stays visible.

[Benjamin-K]

Didn't expect this to grow that much :)
I'd prefer the precomposed hash, too. The calculated time might change if the server is under big load and therefore might become unsafe.

[reflexxion]

Didn't expect this to grow that much :)

Me too :-D

And just in time the other variant is now pushed ;-)

[Benjamin-K]

Thanks for all the effort. Looks good to me now.

[kdambekalns]

Simple and efficient – thanks you had the patience and drive for all those iterations! 👏

The one thing I am not happy with is the class name TimingAttack. That class "is not a timing attack". PrecomposedHashProvider is already better IMHO, but probably we can find something even better…

[reflexxion]

The one thing I am not happy with is the class name TimingAttack. That class "is not a timing attack". PrecomposedHashProvider is already better IMHO, but probably we can find something even better…

I'm unhappy about that too. And hoped, that I'll get some nice naming proposals in here.

IMO PrecomposedHashProvider feels good – some other opinions?
I'll go for that proposal until someone else gives a better one ;-)

[reflexxion]

Alright, I made them non-lazy. Thank you!

[kdambekalns]

Two doc nitpicks, but good to go!

[kdambekalns]

More tweaks

[kdambekalns]

I'd give it one more review, considering the discussion and scope.

[Benjamin-K]

Ready for merge? As this is security relevant, i think this should go into the next patch release.