From: Nicolas Grekas Date: Tue, 16 Apr 2019 12:49:01 +0200 Subject: security #cve-2019-10910 [DI] Check service IDs are valid (nicolas-grekas) This PR was merged into the 2.8 branch. Discussion ---------- [DI] Check service IDs are valid Based on #87 Commits ------- 7fdfeefdfb [DI] Check service IDs are valid Origin: backport, https://github.com/symfony/symfony/commit/3876c75f858d5d82e2c309698d21af2f1d721afb --- .../LazyProxy/PhpDumper/ProxyDumper.php | 8 ++++-- .../DependencyInjection/ContainerBuilder.php | 8 ++++++ .../DependencyInjection/Dumper/PhpDumper.php | 27 +++++++++++------- .../Tests/ContainerBuilderTest.php | 32 ++++++++++++++++++++++ 4 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php b/src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php index 417d94e..d65c6a0 100644 --- a/src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php +++ b/src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php @@ -67,15 +67,17 @@ class ProxyDumper implements DumperInterface public function getProxyFactoryCode(Definition $definition, $id) { $instantiation = 'return'; + $scope = ''; if ($definition->isShared()) { - $instantiation .= " \$this->services['$id'] ="; + $instantiation .= ' $this->services[%s] ='; - if (defined('Symfony\Component\DependencyInjection\ContainerInterface::SCOPE_CONTAINER') && ContainerInterface::SCOPE_CONTAINER !== $scope = $definition->getScope(false)) { - $instantiation .= " \$this->scopedServices['$scope']['$id'] ="; + if (\defined('Symfony\Component\DependencyInjection\ContainerInterface::SCOPE_CONTAINER') && ContainerInterface::SCOPE_CONTAINER !== $scope = $definition->getScope(false)) { + $instantiation .= ' $this->scopedServices[%s][%1$s] ='; } } + $instantiation = sprintf($instantiation, var_export($id, true), var_export($scope, true)); $methodName = 'get'.Container::camelize($id).'Service'; $proxyClass = $this->getProxyClassName($definition); diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php index 4544f47..bd7f20b 100644 --- a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php +++ b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php @@ -655,6 +655,10 @@ class ContainerBuilder extends Container implements TaggedContainerInterface { $alias = strtolower($alias); + if ('' === $alias || '\\' === substr($alias, -1) || \strlen($alias) !== strcspn($alias, "\0\r\n'")) { + throw new InvalidArgumentException(sprintf('Invalid alias id: "%s"', $alias)); + } + if (is_string($id)) { $id = new Alias($id); } elseif (!$id instanceof Alias) { @@ -789,6 +793,10 @@ class ContainerBuilder extends Container implements TaggedContainerInterface $id = strtolower($id); + if ('' === $id || '\\' === substr($id, -1) || \strlen($id) !== strcspn($id, "\0\r\n'")) { + throw new InvalidArgumentException(sprintf('Invalid service id: "%s"', $id)); + } + unset($this->aliasDefinitions[$id]); return $this->definitions[$id] = $definition; diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php index 9394876..e065fcb 100644 --- a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php +++ b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php @@ -393,9 +393,9 @@ class PhpDumper extends Dumper $instantiation = ''; if (!$isProxyCandidate && $definition->isShared() && ContainerInterface::SCOPE_CONTAINER === $definition->getScope(false)) { - $instantiation = "\$this->services['$id'] = ".($simple ? '' : '$instance'); + $instantiation = sprintf('$this->services[%s] = %s', var_export($id, true), $simple ? '' : '$instance'); } elseif (!$isProxyCandidate && $definition->isShared() && ContainerInterface::SCOPE_PROTOTYPE !== $scope = $definition->getScope(false)) { - $instantiation = "\$this->services['$id'] = \$this->scopedServices['$scope']['$id'] = ".($simple ? '' : '$instance'); + $instantiation = sprintf('$this->services[%s] = $this->scopedServices[%s][%1$s] = %s', var_export($id, true), var_export($scope, true), $simple ? '' : '$instance'); } elseif (!$simple) { $instantiation = '$instance'; } @@ -651,6 +651,9 @@ EOF; * Gets the '$id' service.$doc *$lazyInitializationDoc * $return +EOF; + $code = str_replace('*/', ' ', $code).<<camelize($id)}Service($lazyInitialization) { @@ -662,7 +665,7 @@ EOF; if (!in_array($scope, array(ContainerInterface::SCOPE_CONTAINER, ContainerInterface::SCOPE_PROTOTYPE))) { $code .= <<scopedServices['$scope'])) { - throw new InactiveScopeException('$id', '$scope'); + throw new InactiveScopeException({$this->export($id)}, '$scope'); } @@ -670,7 +673,7 @@ EOF; } if ($definition->isSynthetic()) { - $code .= sprintf(" throw new RuntimeException('You have requested a synthetic service (\"%s\"). The DIC does not know how to construct this service.');\n }\n", $id); + $code .= sprintf(" throw new RuntimeException(%s);\n }\n", var_export("You have requested a synthetic service (\"$id\"). The DIC does not know how to construct this service.", true)); } else { if ($definition->isDeprecated()) { $code .= sprintf(" @trigger_error(%s, E_USER_DEPRECATED);\n\n", var_export($definition->getDeprecationMessage($id), true)); @@ -748,10 +751,11 @@ EOF; $arguments[] = $this->dumpValue($value); } - $call = $this->wrapServiceConditionals($call[1], sprintf("\$this->get('%s')->%s(%s);", $definitionId, $call[0], implode(', ', $arguments))); + $definitionId = var_export($definitionId, true); + $call = $this->wrapServiceConditionals($call[1], sprintf('$this->get(%s)->%s(%s);', $definitionId, $call[0], implode(', ', $arguments))); $code .= <<initialized('$definitionId')) { + if (\$this->initialized($definitionId)) { $call } @@ -1176,7 +1180,7 @@ EOF; $conditions = array(); foreach ($services as $service) { - $conditions[] = sprintf("\$this->has('%s')", $service); + $conditions[] = sprintf('$this->has(%s)', var_export($service, true)); } // re-indent the wrapped code @@ -1449,11 +1453,13 @@ EOF; */ public function dumpParameter($name) { + $name = (string) $name; + if ($this->container->isFrozen() && $this->container->hasParameter($name)) { return $this->dumpValue($this->container->getParameter($name), false); } - return sprintf("\$this->getParameter('%s')", strtolower($name)); + return sprintf('$this->getParameter(%s)', var_export($name, true)); } /** @@ -1484,14 +1490,15 @@ EOF; } if (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $reference->getInvalidBehavior()) { - return sprintf('$this->get(\'%s\', ContainerInterface::NULL_ON_INVALID_REFERENCE)', $id); + return sprintf('$this->get(%s, ContainerInterface::NULL_ON_INVALID_REFERENCE)', var_export($id, true)); } else { if ($this->container->hasAlias($id)) { $id = (string) $this->container->getAlias($id); } - return sprintf('$this->get(\'%s\')', $id); + return sprintf('$this->get(%s)', var_export($id, true)); } + } /** diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php index 8e80c47..fb73c34 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php @@ -140,6 +140,38 @@ class ContainerBuilderTest extends \PHPUnit_Framework_TestCase $this->assertNotSame($builder->get('bar'), $builder->get('bar')); } + /** + * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException + * @dataProvider provideBadId + */ + public function testBadAliasId($id) + { + $builder = new ContainerBuilder(); + $builder->setAlias($id, 'foo'); + } + + /** + * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException + * @dataProvider provideBadId + */ + public function testBadDefinitionId($id) + { + $builder = new ContainerBuilder(); + $builder->setDefinition($id, new Definition('Foo')); + } + + public function provideBadId() + { + return [ + [''], + ["\0"], + ["\r"], + ["\n"], + ["'"], + ['ab\\'], + ]; + } + /** * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException * @expectedExceptionMessage You have requested a synthetic service ("foo"). The DIC does not know how to construct this service.