Skip to content

Commit

Permalink
security #cve-2019-10910 [DI] Check service IDs are valid (nicolas-gr…
Browse files Browse the repository at this point in the history 
…ekas)

This PR was merged into the 4.1 branch.

Discussion
----------

[DI] Check service IDs are valid

Based on #87

Commits
-------

c86d27181a [DI] Check service IDs are valid
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Apr 16, 2019
1 parent ceb1eed commit 919a245
Show file tree
Hide file tree
Showing 11 changed files with 251 additions and 40 deletions.
8 changes: 8 additions & 0 deletions ContainerBuilder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,10 @@ public function setAlias($alias, $id)
{
$alias = (string) $alias;

if ('' === $alias || '\\' === $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) {
Expand Down Expand Up @@ -977,6 +981,10 @@ public function setDefinition($id, Definition $definition)

$id = (string) $id;

if ('' === $id || '\\' === $id[-1] || \strlen($id) !== strcspn($id, "\0\r\n'")) {
throw new InvalidArgumentException(sprintf('Invalid service id: "%s"', $id));
}

unset($this->aliasDefinitions[$id], $this->removedIds[$id]);

return $this->definitions[$id] = $definition;
Expand Down
37 changes: 20 additions & 17 deletions Dumper/PhpDumper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ private function addServiceInstance(string $id, Definition $definition, bool $is
$instantiation = '';

if (!$isProxyCandidate && $definition->isShared()) {
$instantiation = sprintf('$this->%s[\'%s\'] = %s', $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates', $id, $isSimpleInstance ? '' : '$instance');
$instantiation = sprintf('$this->%s[%s] = %s', $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates', $this->doExport($id), $isSimpleInstance ? '' : '$instance');
} elseif (!$isSimpleInstance) {
$instantiation = '$instance';
}
Expand Down Expand Up @@ -639,6 +639,9 @@ private function addService(string $id, Definition $definition, string &$file =
* Gets the $public '$id'$shared$autowired service.
*
* $return
EOF;
$code = str_replace('*/', ' ', $code).<<<EOF
*/
protected function {$methodName}($lazyInitialization)
{
Expand All @@ -659,7 +662,7 @@ protected function {$methodName}($lazyInitialization)

if ($this->getProxyDumper()->isProxyCandidate($definition)) {
$factoryCode = $asFile ? "\$this->load('%s.php', false)" : '$this->%s(false)';
$code .= $this->getProxyDumper()->getProxyFactoryCode($definition, $id, sprintf($factoryCode, $methodName));
$code .= $this->getProxyDumper()->getProxyFactoryCode($definition, $id, sprintf($factoryCode, $methodName, $this->doExport($id)));
}

if ($definition->isDeprecated()) {
Expand Down Expand Up @@ -733,14 +736,14 @@ private function addInlineReference(string $id, Definition $definition, string $

$code .= sprintf(<<<'EOTXT'
if (isset($this->%s['%s'])) {
return $this->%1$s['%2$s'];
if (isset($this->%s[%s])) {
return $this->%1$s[%2$s];
}
EOTXT
,
$this->container->getDefinition($id)->isPublic() ? 'services' : 'privates',
$id
$this->doExport($id)
);

return $code;
Expand Down Expand Up @@ -830,7 +833,7 @@ private function generateServiceFiles()
$code = ["\n", $code];
}
$code[1] = implode("\n", array_map(function ($line) { return $line ? ' '.$line : $line; }, explode("\n", $code[1])));
$factory = sprintf('$this->factories%s[\'%s\']', $definition->isPublic() ? '' : "['service_container']", $id);
$factory = sprintf('$this->factories%s[%s]', $definition->isPublic() ? '' : "['service_container']", $this->doExport($id));
$code[1] = sprintf("%s = function () {\n%s};\n\nreturn %1\$s();\n", $factory, $code[1]);
$code = $code[0].$code[1];
}
Expand Down Expand Up @@ -1341,14 +1344,14 @@ private function getServiceConditionals($value): string
if (!$this->container->hasDefinition($service)) {
return 'false';
}
$conditions[] = sprintf("isset(\$this->%s['%s'])", $this->container->getDefinition($service)->isPublic() ? 'services' : 'privates', $service);
$conditions[] = sprintf("isset(\$this->%s[%s])", $this->container->getDefinition($service)->isPublic() ? 'services' : 'privates', $this->doExport($service));
}
foreach (ContainerBuilder::getServiceConditionals($value) as $service) {
if ($this->container->hasDefinition($service) && !$this->container->getDefinition($service)->isPublic()) {
continue;
}

$conditions[] = sprintf("\$this->has('%s')", $service);
$conditions[] = sprintf("\$this->has(%s)", $this->doExport($service));
}

if (!$conditions) {
Expand Down Expand Up @@ -1579,11 +1582,11 @@ private function dumpParameter(string $name): string
}

if (!preg_match("/\\\$this->(?:getEnv\('(?:\w++:)*+\w++'\)|targetDirs\[\d++\])/", $dumpedValue)) {
return sprintf("\$this->parameters['%s']", $name);
return sprintf('$this->parameters[%s]', $this->doExport($name));
}
}

return sprintf("\$this->getParameter('%s')", $name);
return sprintf('$this->getParameter(%s)', $this->doExport($name));
}

private function getServiceCall(string $id, Reference $reference = null): string
Expand All @@ -1598,7 +1601,7 @@ private function getServiceCall(string $id, Reference $reference = null): string

if ($this->container->hasDefinition($id) && $definition = $this->container->getDefinition($id)) {
if ($definition->isSynthetic()) {
$code = sprintf('$this->get(\'%s\'%s)', $id, null !== $reference ? ', '.$reference->getInvalidBehavior() : '');
$code = sprintf('$this->get(%s%s)', $this->doExport($id), null !== $reference ? ', '.$reference->getInvalidBehavior() : '');
} elseif (null !== $reference && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $reference->getInvalidBehavior()) {
$code = 'null';
if (!$definition->isShared()) {
Expand All @@ -1607,20 +1610,20 @@ private function getServiceCall(string $id, Reference $reference = null): string
} elseif ($this->isTrivialInstance($definition)) {
$code = substr($this->addNewInstance($definition, '', '', $id), 8, -2);
if ($definition->isShared()) {
$code = sprintf('$this->%s[\'%s\'] = %s', $definition->isPublic() ? 'services' : 'privates', $id, $code);
$code = sprintf('$this->%s[%s] = %s', $definition->isPublic() ? 'services' : 'privates', $this->doExport($id), $code);
}
$code = "($code)";
} elseif ($this->asFiles && !$this->isHotPath($definition)) {
$code = sprintf("\$this->load('%s.php')", $this->generateMethodName($id));
if (!$definition->isShared()) {
$factory = sprintf('$this->factories%s[\'%s\']', $definition->isPublic() ? '' : "['service_container']", $id);
$factory = sprintf('$this->factories%s[%s]', $definition->isPublic() ? '' : "['service_container']", $this->doExport($id));
$code = sprintf('(isset(%s) ? %1$s() : %s)', $factory, $code);
}
} else {
$code = sprintf('$this->%s()', $this->generateMethodName($id));
}
if ($definition->isShared()) {
$code = sprintf('($this->%s[\'%s\'] ?? %s)', $definition->isPublic() ? 'services' : 'privates', $id, $code);
$code = sprintf('($this->%s[%s] ?? %s)', $definition->isPublic() ? 'services' : 'privates', $this->doExport($id), $code);
}

return $code;
Expand All @@ -1629,12 +1632,12 @@ private function getServiceCall(string $id, Reference $reference = null): string
return 'null';
}
if (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE < $reference->getInvalidBehavior()) {
$code = sprintf('$this->get(\'%s\', /* ContainerInterface::NULL_ON_INVALID_REFERENCE */ %d)', $id, ContainerInterface::NULL_ON_INVALID_REFERENCE);
$code = sprintf('$this->get(%s, /* ContainerInterface::NULL_ON_INVALID_REFERENCE */ %d)', $this->doExport($id), ContainerInterface::NULL_ON_INVALID_REFERENCE);
} else {
$code = sprintf('$this->get(\'%s\')', $id);
$code = sprintf('$this->get(%s)', $this->doExport($id));
}

return sprintf('($this->services[\'%s\'] ?? %s)', $id, $code);
return sprintf('($this->services[%s] ?? %s)', $this->doExport($id), $code);
}

/**
Expand Down
32 changes: 32 additions & 0 deletions Tests/ContainerBuilderTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,38 @@ public function testNonSharedServicesReturnsDifferentInstances()
$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.
Expand Down
8 changes: 7 additions & 1 deletion Tests/Dumper/PhpDumperTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,18 @@ public function testAddServiceIdWithUnsupportedCharacters()
{
$class = 'Symfony_DI_PhpDumper_Test_Unsupported_Characters';
$container = new ContainerBuilder();
$container->setParameter("'", 'oh-no');
$container->register("foo*/oh-no", 'FooClass')->setPublic(true);
$container->register('bar$', 'FooClass')->setPublic(true);
$container->register('bar$!', 'FooClass')->setPublic(true);
$container->compile();
$dumper = new PhpDumper($container);
eval('?>'.$dumper->dump(['class' => $class]));

$this->assertStringEqualsFile(self::$fixturesPath.'/php/services_unsupported_characters.php', $dumper->dump(['class' => $class]));

require_once self::$fixturesPath.'/php/services_unsupported_characters.php';

$this->assertTrue(method_exists($class, 'getFooOhNoService'));
$this->assertTrue(method_exists($class, 'getBarService'));
$this->assertTrue(method_exists($class, 'getBar2Service'));
}
Expand Down
4 changes: 2 additions & 2 deletions Tests/Fixtures/php/services33.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public function getRemovedIds()
*/
protected function getFooService()
{
return $this->services['Bar\Foo'] = new \Bar\Foo();
return $this->services['Bar\\Foo'] = new \Bar\Foo();
}

/**
Expand All @@ -76,6 +76,6 @@ protected function getFooService()
*/
protected function getFoo2Service()
{
return $this->services['Foo\Foo'] = new \Foo\Foo();
return $this->services['Foo\\Foo'] = new \Foo\Foo();
}
}
18 changes: 9 additions & 9 deletions Tests/Fixtures/php/services_adawson.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,11 @@ public function getRemovedIds()
*/
protected function getBusService()
{
$a = ($this->services['App\Db'] ?? $this->getDbService());
$a = ($this->services['App\\Db'] ?? $this->getDbService());

$this->services['App\Bus'] = $instance = new \App\Bus($a);
$this->services['App\\Bus'] = $instance = new \App\Bus($a);

$b = ($this->privates['App\Schema'] ?? $this->getSchemaService());
$b = ($this->privates['App\\Schema'] ?? $this->getSchemaService());
$c = new \App\Registry();
$c->processor = [0 => $a, 1 => $instance];

Expand All @@ -94,9 +94,9 @@ protected function getBusService()
*/
protected function getDbService()
{
$this->services['App\Db'] = $instance = new \App\Db();
$this->services['App\\Db'] = $instance = new \App\Db();

$instance->schema = ($this->privates['App\Schema'] ?? $this->getSchemaService());
$instance->schema = ($this->privates['App\\Schema'] ?? $this->getSchemaService());

return $instance;
}
Expand All @@ -108,12 +108,12 @@ protected function getDbService()
*/
protected function getSchemaService()
{
$a = ($this->services['App\Db'] ?? $this->getDbService());
$a = ($this->services['App\\Db'] ?? $this->getDbService());

if (isset($this->privates['App\Schema'])) {
return $this->privates['App\Schema'];
if (isset($this->privates['App\\Schema'])) {
return $this->privates['App\\Schema'];
}

return $this->privates['App\Schema'] = new \App\Schema($a);
return $this->privates['App\\Schema'] = new \App\Schema($a);
}
}
6 changes: 3 additions & 3 deletions Tests/Fixtures/php/services_inline_requires.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public function getRemovedIds()
*/
protected function getParentNotExistsService()
{
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\ParentNotExists'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\ParentNotExists();
return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\ParentNotExists'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\ParentNotExists();
}

/**
Expand All @@ -91,7 +91,7 @@ protected function getParentNotExistsService()
*/
protected function getC1Service()
{
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C1'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C1();
return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\includes\\HotPath\\C1'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C1();
}

/**
Expand All @@ -104,7 +104,7 @@ protected function getC2Service()
include_once $this->targetDirs[1].'/includes/HotPath/C2.php';
include_once $this->targetDirs[1].'/includes/HotPath/C3.php';

return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2(new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3());
return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\includes\\HotPath\\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2(new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3());
}

public function getParameter($name)
Expand Down
2 changes: 1 addition & 1 deletion Tests/Fixtures/php/services_inline_self_ref.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ protected function getFooService()
$b = new \App\Baz($a);
$b->bar = $a;

$this->services['App\Foo'] = $instance = new \App\Foo($b);
$this->services['App\\Foo'] = $instance = new \App\Foo($b);

$a->foo = $instance;

Expand Down
4 changes: 2 additions & 2 deletions Tests/Fixtures/php/services_rot13_env.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public function getRemovedIds()
*/
protected function getRot13EnvVarProcessorService()
{
return $this->services['Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor'] = new \Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor();
return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Dumper\\Rot13EnvVarProcessor'] = new \Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor();
}

/**
Expand All @@ -79,7 +79,7 @@ protected function getRot13EnvVarProcessorService()
protected function getContainer_EnvVarProcessorsLocatorService()
{
return $this->services['container.env_var_processors_locator'] = new \Symfony\Component\DependencyInjection\ServiceLocator(['rot13' => function () {
return ($this->services['Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor'] ?? ($this->services['Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor'] = new \Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor()));
return ($this->services['Symfony\\Component\\DependencyInjection\\Tests\\Dumper\\Rot13EnvVarProcessor'] ?? ($this->services['Symfony\\Component\\DependencyInjection\\Tests\\Dumper\\Rot13EnvVarProcessor'] = new \Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor()));
}]);
}

Expand Down
Loading

0 comments on commit 919a245

Please sign in to comment.