From 8dd1a6e545c399fb7876a5deb06d3bf39becf2e9 Mon Sep 17 00:00:00 2001 From: Christian Flothmann Date: Fri, 2 Oct 2020 14:16:41 +0200 Subject: [PATCH] prevent hash collisions caused by reused object hashes --- .../Validator/Constraints/FormValidator.php | 13 ++------ src/Symfony/Component/Form/composer.json | 2 +- .../Validator/Context/ExecutionContext.php | 18 +++++++++++ .../Validator/RecursiveValidatorTest.php | 19 ++++++++++++ .../RecursiveContextualValidator.php | 30 +++++++++++++++---- 5 files changed, 64 insertions(+), 18 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php b/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php index f0e10c1d48..72ce53a592 100644 --- a/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php +++ b/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php @@ -25,7 +25,6 @@ use Symfony\Component\Validator\Exception\UnexpectedTypeException; class FormValidator extends ConstraintValidator { private $resolvedGroups; - private $fieldFormConstraints; /** * {@inheritdoc} @@ -68,7 +67,6 @@ class FormValidator extends ConstraintValidator if ($hasChildren && $form->isRoot()) { $this->resolvedGroups = new \SplObjectStorage(); - $this->fieldFormConstraints = []; } if ($groups instanceof GroupSequence) { @@ -93,7 +91,6 @@ class FormValidator extends ConstraintValidator $this->resolvedGroups[$field] = (array) $group; $fieldFormConstraint = new Form(); $fieldFormConstraint->groups = $group; - $this->fieldFormConstraints[] = $fieldFormConstraint; $this->context->setNode($this->context->getValue(), $field, $this->context->getMetadata(), $this->context->getPropertyPath()); $validator->atPath(sprintf('children[%s]', $field->getName()))->validate($field, $fieldFormConstraint, $group); } @@ -139,10 +136,8 @@ class FormValidator extends ConstraintValidator foreach ($form->all() as $field) { if ($field->isSubmitted()) { $this->resolvedGroups[$field] = $groups; - $fieldFormConstraint = new Form(); - $this->fieldFormConstraints[] = $fieldFormConstraint; $this->context->setNode($this->context->getValue(), $field, $this->context->getMetadata(), $this->context->getPropertyPath()); - $validator->atPath(sprintf('children[%s]', $field->getName()))->validate($field, $fieldFormConstraint); + $validator->atPath(sprintf('children[%s]', $field->getName()))->validate($field, $formConstraint); } } } @@ -150,7 +145,6 @@ class FormValidator extends ConstraintValidator if ($hasChildren && $form->isRoot()) { // destroy storage to avoid memory leaks $this->resolvedGroups = new \SplObjectStorage(); - $this->fieldFormConstraints = []; } } elseif (!$form->isSynchronized()) { $childrenSynchronized = true; @@ -159,11 +153,8 @@ class FormValidator extends ConstraintValidator foreach ($form as $child) { if (!$child->isSynchronized()) { $childrenSynchronized = false; - - $fieldFormConstraint = new Form(); - $this->fieldFormConstraints[] = $fieldFormConstraint; $this->context->setNode($this->context->getValue(), $child, $this->context->getMetadata(), $this->context->getPropertyPath()); - $validator->atPath(sprintf('children[%s]', $child->getName()))->validate($child, $fieldFormConstraint); + $validator->atPath(sprintf('children[%s]', $child->getName()))->validate($child, $formConstraint); } } diff --git a/src/Symfony/Component/Form/composer.json b/src/Symfony/Component/Form/composer.json index 2a44a9f68a..b03b63f7c9 100644 --- a/src/Symfony/Component/Form/composer.json +++ b/src/Symfony/Component/Form/composer.json @@ -27,7 +27,7 @@ }, "require-dev": { "doctrine/collections": "~1.0", - "symfony/validator": "^3.4.44|^4.3.4|^5.0", + "symfony/validator": "^4.4.17|^5.1.9", "symfony/dependency-injection": "^3.4|^4.0|^5.0", "symfony/expression-language": "^3.4|^4.0|^5.0", "symfony/config": "^3.4|^4.0|^5.0", diff --git a/src/Symfony/Component/Validator/Context/ExecutionContext.php b/src/Symfony/Component/Validator/Context/ExecutionContext.php index a197168bf7..6acecac152 100644 --- a/src/Symfony/Component/Validator/Context/ExecutionContext.php +++ b/src/Symfony/Component/Validator/Context/ExecutionContext.php @@ -129,6 +129,7 @@ class ExecutionContext implements ExecutionContextInterface * @var array */ private $initializedObjects; + private $cachedObjectsRefs; /** * Creates a new execution context. @@ -153,6 +154,7 @@ class ExecutionContext implements ExecutionContextInterface $this->translator = $translator; $this->translationDomain = $translationDomain; $this->violations = new ConstraintViolationList(); + $this->cachedObjectsRefs = new \SplObjectStorage(); } /** @@ -358,4 +360,20 @@ class ExecutionContext implements ExecutionContextInterface { return isset($this->initializedObjects[$cacheKey]); } + + /** + * @internal + * + * @param object $object + * + * @return string + */ + public function generateCacheKey($object) + { + if (!isset($this->cachedObjectsRefs[$object])) { + $this->cachedObjectsRefs[$object] = spl_object_hash($object); + } + + return $this->cachedObjectsRefs[$object]; + } } diff --git a/src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php b/src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php index f9aa6831ce..17a8914304 100644 --- a/src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php +++ b/src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php @@ -2153,6 +2153,25 @@ class RecursiveValidatorTest extends TestCase $this->assertCount(2, $this->validator->validate($entity, new TestConstraintHashesDoNotCollide())); } + + public function testValidatedConstraintsHashesDoNotCollideWithSameConstraintValidatingDifferentProperties() + { + $value = new \stdClass(); + + $entity = new Entity(); + $entity->firstName = $value; + $entity->setLastName($value); + + $validator = $this->validator->startContext($entity); + + $constraint = new IsNull(); + $validator->atPath('firstName') + ->validate($entity->firstName, $constraint); + $validator->atPath('lastName') + ->validate($entity->getLastName(), $constraint); + + $this->assertCount(2, $validator->getViolations()); + } } final class TestConstraintHashesDoNotCollide extends Constraint diff --git a/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php b/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php index fb321ff838..9e743f44a7 100644 --- a/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php +++ b/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php @@ -108,7 +108,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface $this->validateGenericNode( $value, $previousObject, - \is_object($value) ? spl_object_hash($value) : null, + \is_object($value) ? $this->generateCacheKey($value) : null, $metadata, $this->defaultPropertyPath, $groups, @@ -176,7 +176,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface $propertyMetadatas = $classMetadata->getPropertyMetadata($propertyName); $groups = $groups ? $this->normalizeGroups($groups) : $this->defaultGroups; - $cacheKey = spl_object_hash($object); + $cacheKey = $this->generateCacheKey($object); $propertyPath = PropertyPath::append($this->defaultPropertyPath, $propertyName); $previousValue = $this->context->getValue(); @@ -224,7 +224,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface if (\is_object($objectOrClass)) { $object = $objectOrClass; $class = \get_class($object); - $cacheKey = spl_object_hash($objectOrClass); + $cacheKey = $this->generateCacheKey($objectOrClass); $propertyPath = PropertyPath::append($this->defaultPropertyPath, $propertyName); } else { // $objectOrClass contains a class name @@ -313,7 +313,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface $this->validateClassNode( $object, - spl_object_hash($object), + $this->generateCacheKey($object), $classMetadata, $propertyPath, $groups, @@ -429,7 +429,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface $defaultOverridden = false; // Use the object hash for group sequences - $groupHash = \is_object($group) ? spl_object_hash($group) : $group; + $groupHash = \is_object($group) ? $this->generateCacheKey($group, true) : $group; if ($context->isGroupValidated($cacheKey, $groupHash)) { // Skip this group when validating the properties and when @@ -740,7 +740,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface // Prevent duplicate validation of constraints, in the case // that constraints belong to multiple validated groups if (null !== $cacheKey) { - $constraintHash = spl_object_hash($constraint); + $constraintHash = $this->generateCacheKey($constraint, true); // instanceof Valid: In case of using a Valid constraint with many groups // it makes a reference object get validated by each group if ($constraint instanceof Composite || $constraint instanceof Valid) { @@ -772,4 +772,22 @@ class RecursiveContextualValidator implements ContextualValidatorInterface } } } + + /** + * @param object $object + */ + private function generateCacheKey($object, bool $dependsOnPropertyPath = false): string + { + if ($this->context instanceof ExecutionContext) { + $cacheKey = $this->context->generateCacheKey($object); + } else { + $cacheKey = spl_object_hash($object); + } + + if ($dependsOnPropertyPath) { + $cacheKey .= $this->context->getPropertyPath(); + } + + return $cacheKey; + } }