- Issue created by @tedbow
🌱
[Meta] Address low risk issues found in security Audit
Active
Cure53 ascertained that the application writes to the shared temporary storage metadata key without initially acquiring a lock. The lock within the temporary storage is only acquired within the stage’s create method, though the metadata is overwritten shortly prior to invocation. The ownership check performed within the getMetadata method only checks if the lock key is indeed held by the caller, but does not perform the same check for the returned metadata value. This could be abused by malicious authenticated administrators to overwrite the metadata value while a stage is currently being processed, resulting in unexpected behaviors or information exposure.
Affected file:
automatic_updates/src/UpdateStage.php
Affected code:
public function begin(array $project_versions, ?int $timeout = 300): string
{
[...]
// Ensure that package versions are available to pre-create event
// subscribers. We can't use ::setMetadata() here because it requires the // stage to be claimed, but that only happens during ::create(). $this->tempStore->set(static::TEMPSTORE_METADATA_KEY, [
'packages' => $package_versions,
]);
return $this->create($timeout); [...]
public function getMetadata(string $key) {
$this->checkOwnership();
$metadata = $this->tempStore->get(static::TEMPSTORE_METADATA_KEY) ?: []; return $metadata[$key] ?? NULL;
}
[...]
final protected function checkOwnership(): void {
i[...]
$stored_lock = $this->tempStore->getIfOwner(static::TEMPSTORE_LOCK_KEY); if ($stored_lock !== $this->lock) {
throw new StageOwnershipException($this, 'Stage is not owned by the
current user or session.');
} }
To mitigate this issue, Cure53 recommends that the metadata key is only configured after the stage is claimed with the assistance of the setMetadata method before the PreCreateEvent is dispatched.
Active
3.0
Code