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)

* di-sec-34:
  [DI] Check service IDs are valid
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Apr 16, 2019
2 parents 4585a41 + d2fb589 commit 47cd029
Show file tree
Hide file tree
Showing 12 changed files with 283 additions and 54 deletions.
2 changes: 1 addition & 1 deletion src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public function getProxyFactoryCode(Definition $definition, $id, $factoryCode =
$instantiation = 'return';

if ($definition->isShared()) {
$instantiation .= sprintf(' $this->%s[\'%s\'] =', \method_exists(ContainerBuilder::class, 'addClassResource') || ($definition->isPublic() && !$definition->isPrivate()) ? 'services' : 'privates', $id);
$instantiation .= sprintf(' $this->%s[%s] =', \method_exists(ContainerBuilder::class, 'addClassResource') || ($definition->isPublic() && !$definition->isPrivate()) ? 'services' : 'privates', var_export($id, true));
}

if (null === $factoryCode) {
Expand Down
8 changes: 8 additions & 0 deletions src/Symfony/Component/DependencyInjection/ContainerBuilder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,10 @@ public function setAlias($alias, $id)
{
$alias = $this->normalizeId($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($this->normalizeId($id));
} elseif (!$id instanceof Alias) {
Expand Down Expand Up @@ -1021,6 +1025,10 @@ public function setDefinition($id, Definition $definition)

$id = $this->normalizeId($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], $this->removedIds[$id]);

return $this->definitions[$id] = $definition;
Expand Down
33 changes: 19 additions & 14 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ private function addServiceInstance($id, Definition $definition, $isSimpleInstan
$instantiation = '';

if (!$isProxyCandidate && $definition->isShared()) {
$instantiation = "\$this->services['$id'] = ".($isSimpleInstance ? '' : '$instance');
$instantiation = sprintf('$this->services[%s] = %s', $this->doExport($id), $isSimpleInstance ? '' : '$instance');
} elseif (!$isSimpleInstance) {
$instantiation = '$instance';
}
Expand Down Expand Up @@ -679,6 +679,9 @@ private function addService($id, Definition $definition, &$file = null)
* Gets the $public '$id'$shared$autowired service.
*
* $return
EOF;
$code = str_replace('*/', ' ', $code).<<<EOF
*/
protected function {$methodName}($lazyInitialization)
{
Expand All @@ -693,7 +696,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 @@ -767,14 +770,14 @@ private function addInlineReference($id, Definition $definition, $targetId, $for

$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
,
'services',
$id
$this->doExport($id)
);

return $code;
Expand Down Expand Up @@ -1530,14 +1533,14 @@ private function getServiceConditionals($value)
if (!$this->container->hasDefinition($service)) {
return 'false';
}
$conditions[] = sprintf("isset(\$this->services['%s'])", $service);
$conditions[] = sprintf('isset($this->services[%s])', $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 @@ -1776,6 +1779,8 @@ private function dumpLiteralClass($class)
*/
private function dumpParameter($name)
{
$name = (string) $name;

if ($this->container->isCompiled() && $this->container->hasParameter($name)) {
$value = $this->container->getParameter($name);
$dumpedValue = $this->dumpValue($value, false);
Expand All @@ -1785,11 +1790,11 @@ private function dumpParameter($name)
}

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));
}

/**
Expand All @@ -1813,7 +1818,7 @@ private function getServiceCall($id, Reference $reference = null)

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 @@ -1822,7 +1827,7 @@ private function getServiceCall($id, Reference $reference = null)
} elseif ($this->isTrivialInstance($definition)) {
$code = substr($this->addNewInstance($definition, '', '', $id), 8, -2);
if ($definition->isShared()) {
$code = sprintf('$this->services[\'%s\'] = %s', $id, $code);
$code = sprintf('$this->services[%s] = %s', $this->doExport($id), $code);
}
$code = "($code)";
} elseif ($this->asFiles && $definition->isShared() && !$this->isHotPath($definition)) {
Expand All @@ -1833,14 +1838,14 @@ private function getServiceCall($id, Reference $reference = null)
} elseif (null !== $reference && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $reference->getInvalidBehavior()) {
return 'null';
} elseif (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));
}

// The following is PHP 5.5 syntax for what could be written as "(\$this->services['$id'] ?? $code)" on PHP>=7.0

return "\${(\$_ = isset(\$this->services['$id']) ? \$this->services['$id'] : $code) && false ?: '_'}";
return sprintf("\${(\$_ = isset(\$this->services[%s]) ? \$this->services[%1\$s] : %s) && false ?: '_'}", $this->doExport($id), $code);
}

/**
Expand Down
32 changes: 32 additions & 0 deletions src/Symfony/Component/DependencyInjection/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 src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,12 +234,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 src/Symfony/Component/DependencyInjection/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 isFrozen()
*/
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();
}
}
46 changes: 23 additions & 23 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_adawson.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,10 @@ public function isFrozen()
*/
protected function getBusService()
{
$this->services['App\Bus'] = $instance = new \App\Bus(${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'});
$this->services['App\\Bus'] = $instance = new \App\Bus(${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'});

$instance->handler1 = ${($_ = isset($this->services['App\Handler1']) ? $this->services['App\Handler1'] : $this->getHandler1Service()) && false ?: '_'};
$instance->handler2 = ${($_ = isset($this->services['App\Handler2']) ? $this->services['App\Handler2'] : $this->getHandler2Service()) && false ?: '_'};
$instance->handler1 = ${($_ = isset($this->services['App\\Handler1']) ? $this->services['App\\Handler1'] : $this->getHandler1Service()) && false ?: '_'};
$instance->handler2 = ${($_ = isset($this->services['App\\Handler2']) ? $this->services['App\\Handler2'] : $this->getHandler2Service()) && false ?: '_'};

return $instance;
}
Expand All @@ -103,9 +103,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 = ${($_ = isset($this->services['App\Schema']) ? $this->services['App\Schema'] : $this->getSchemaService()) && false ?: '_'};
$instance->schema = ${($_ = isset($this->services['App\\Schema']) ? $this->services['App\\Schema'] : $this->getSchemaService()) && false ?: '_'};

return $instance;
}
Expand All @@ -117,13 +117,13 @@ protected function getDbService()
*/
protected function getHandler1Service()
{
$a = ${($_ = isset($this->services['App\Processor']) ? $this->services['App\Processor'] : $this->getProcessorService()) && false ?: '_'};
$a = ${($_ = isset($this->services['App\\Processor']) ? $this->services['App\\Processor'] : $this->getProcessorService()) && false ?: '_'};

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

return $this->services['App\Handler1'] = new \App\Handler1(${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\Schema']) ? $this->services['App\Schema'] : $this->getSchemaService()) && false ?: '_'}, $a);
return $this->services['App\\Handler1'] = new \App\Handler1(${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\\Schema']) ? $this->services['App\\Schema'] : $this->getSchemaService()) && false ?: '_'}, $a);
}

/**
Expand All @@ -133,13 +133,13 @@ protected function getHandler1Service()
*/
protected function getHandler2Service()
{
$a = ${($_ = isset($this->services['App\Processor']) ? $this->services['App\Processor'] : $this->getProcessorService()) && false ?: '_'};
$a = ${($_ = isset($this->services['App\\Processor']) ? $this->services['App\\Processor'] : $this->getProcessorService()) && false ?: '_'};

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

return $this->services['App\Handler2'] = new \App\Handler2(${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\Schema']) ? $this->services['App\Schema'] : $this->getSchemaService()) && false ?: '_'}, $a);
return $this->services['App\\Handler2'] = new \App\Handler2(${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\\Schema']) ? $this->services['App\\Schema'] : $this->getSchemaService()) && false ?: '_'}, $a);
}

/**
Expand All @@ -149,13 +149,13 @@ protected function getHandler2Service()
*/
protected function getProcessorService()
{
$a = ${($_ = isset($this->services['App\Registry']) ? $this->services['App\Registry'] : $this->getRegistryService()) && false ?: '_'};
$a = ${($_ = isset($this->services['App\\Registry']) ? $this->services['App\\Registry'] : $this->getRegistryService()) && false ?: '_'};

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

return $this->services['App\Processor'] = new \App\Processor($a, ${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'});
return $this->services['App\\Processor'] = new \App\Processor($a, ${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'});
}

/**
Expand All @@ -165,9 +165,9 @@ protected function getProcessorService()
*/
protected function getRegistryService()
{
$this->services['App\Registry'] = $instance = new \App\Registry();
$this->services['App\\Registry'] = $instance = new \App\Registry();

$instance->processor = [0 => ${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'}, 1 => ${($_ = isset($this->services['App\Bus']) ? $this->services['App\Bus'] : $this->getBusService()) && false ?: '_'}];
$instance->processor = [0 => ${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'}, 1 => ${($_ = isset($this->services['App\\Bus']) ? $this->services['App\\Bus'] : $this->getBusService()) && false ?: '_'}];

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

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

return $this->services['App\Schema'] = new \App\Schema($a);
return $this->services['App\\Schema'] = new \App\Schema($a);
}
}

0 comments on commit 47cd029

Please sign in to comment.