Bug #103569
open
Time-of-check vs. time-of-use bug in TYPO3\CMS\Core\FormProtection\FormProtectionFactory
0%
Description
FormProtectionFactory tries to cache form protection instances by storing them in a cache pool. However, after creating a new form projection object in FormProtectionFactory::createForType() it immediately makes a second trip to the cache to fetch the newly stored object. This makes this method vulnerable to a race condition. Instead, it should return the created object itself without an additional cache fetch (which is unnecessary here, btw).
$classNameAndConstructorArguments = $this->getClassNameAndConstructorArguments($type, $GLOBALS['TYPO3_REQUEST'] ?? null);
$this->runtimeCache->set($identifier, $this->createInstance(...$classNameAndConstructorArguments));
return $this->runtimeCache->get($identifier);
should rather be
$classNameAndConstructorArguments = $this->getClassNameAndConstructorArguments($type, $GLOBALS['TYPO3_REQUEST'] ?? null);
$formProtection = $this->createInstance(...$classNameAndConstructorArguments);
$this->runtimeCache->set($identifier, $formProtection);
return $formProtection;
You can verify correct behaviour by switching the cache backend to the NullBackend that will immediately forget the stored object.
Updated by Christian Spoo 8 months ago
Obviously, the same goes for FormProtectionFactory::createFromRequest()
Updated by Stefan Bürk 7 months ago
Configure the runtimeCache to a null backend is weired anyway. Still returning the object will
make other places retriving it from the cache vulneable at all (in the same request).
Updated by Christian Spoo 7 months ago
Why? If the question whether the cache really hits or not changes behaviour in other ways than simply in terms of performance, then this is not a cache but a persistent storage and should be treated as such. In that case we could not even rely on the characteristics of the cache backend behind. My tests with NullBackend are only for testing purposes and were never meant to be used in production. This is only to illustrate the problem. In theory - if all usages of caches in the core are correct - you should be able to replace all cache backends with the NullBackend and TYPO3 should run correctly, although quite slowly, of course. TBH, I never tried that but it would be a good test if TYPO3 works correctly in that regard. Might consider that to be a future side-project. :-)
However, I do not think that the problem above is this large (which is why I marked the issue as 'no-brainer'). The issue is that the cache APIs are misused here. As I said, one simply must never rely on a cache pool to actually carry an object previously stored. There are numerous possible reasons for a cache pool to be unable to store a new item. the pool might be full or temporarily unavailable - I am sure you can think of several additional reasons.
The PSR-16 specification even clearly documents a different case where relying on a item previously stored can introduce race conditions and must be avoided. In the spec for Psr\SimpleCache\CacheInterface::has() it says:
It is recommended that has() is only to be used for cache warming type purposes and not to be used within your live applications operations for get/set, as this method is subject to a race condition where your has() will return true and immediately after, another script can remove it, making the state of your app out of date.
Basically, it's the same problem as with the code above. Between the check (in the form of the set operation) and the use (get operation), a different call might have evicted the object from the cache, for example if the cache pool is nearly full.
That's why two slightly changed SLOC would do the trick and ensure proper usage of the cache in question.