Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 4c80c34

Browse filesBrowse files
nicolas-grekasnicolas-grekas
committed
security #cve-2019-10910 [DI] Check service IDs are valid (nicolas-grekas)
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
1 parent c8a98df commit 4c80c34
Copy full SHA for 4c80c34

12 files changed

+252
-41
lines changed

‎src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public function getProxyFactoryCode(Definition $definition, $id, $factoryCode =
5454
$instantiation = 'return';
5555

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

6060
if (null === $factoryCode) {

‎src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/ContainerBuilder.php
+8Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -824,6 +824,10 @@ public function setAlias($alias, $id)
824824
{
825825
$alias = (string) $alias;
826826

827+
if ('' === $alias || '\\' === $alias[-1] || \strlen($alias) !== strcspn($alias, "\0\r\n'")) {
828+
throw new InvalidArgumentException(sprintf('Invalid alias id: "%s"', $alias));
829+
}
830+
827831
if (\is_string($id)) {
828832
$id = new Alias($id);
829833
} elseif (!$id instanceof Alias) {
@@ -977,6 +981,10 @@ public function setDefinition($id, Definition $definition)
977981

978982
$id = (string) $id;
979983

984+
if ('' === $id || '\\' === $id[-1] || \strlen($id) !== strcspn($id, "\0\r\n'")) {
985+
throw new InvalidArgumentException(sprintf('Invalid service id: "%s"', $id));
986+
}
987+
980988
unset($this->aliasDefinitions[$id], $this->removedIds[$id]);
981989

982990
return $this->definitions[$id] = $definition;

‎src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
+20-17Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ private function addServiceInstance(string $id, Definition $definition, bool $is
476476
$instantiation = '';
477477

478478
if (!$isProxyCandidate && $definition->isShared()) {
479-
$instantiation = sprintf('$this->%s[\'%s\'] = %s', $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates', $id, $isSimpleInstance ? '' : '$instance');
479+
$instantiation = sprintf('$this->%s[%s] = %s', $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates', $this->doExport($id), $isSimpleInstance ? '' : '$instance');
480480
} elseif (!$isSimpleInstance) {
481481
$instantiation = '$instance';
482482
}
@@ -639,6 +639,9 @@ private function addService(string $id, Definition $definition, string &$file =
639639
* Gets the $public '$id'$shared$autowired service.
640640
*
641641
* $return
642+
EOF;
643+
$code = str_replace('*/', ' ', $code).<<<EOF
644+
642645
*/
643646
protected function {$methodName}($lazyInitialization)
644647
{
@@ -659,7 +662,7 @@ protected function {$methodName}($lazyInitialization)
659662

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

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

734737
$code .= sprintf(<<<'EOTXT'
735738
736-
if (isset($this->%s['%s'])) {
737-
return $this->%1$s['%2$s'];
739+
if (isset($this->%s[%s])) {
740+
return $this->%1$s[%2$s];
738741
}
739742

740743
EOTXT
741744
,
742745
$this->container->getDefinition($id)->isPublic() ? 'services' : 'privates',
743-
$id
746+
$this->doExport($id)
744747
);
745748

746749
return $code;
@@ -830,7 +833,7 @@ private function generateServiceFiles()
830833
$code = ["\n", $code];
831834
}
832835
$code[1] = implode("\n", array_map(function ($line) { return $line ? ' '.$line : $line; }, explode("\n", $code[1])));
833-
$factory = sprintf('$this->factories%s[\'%s\']', $definition->isPublic() ? '' : "['service_container']", $id);
836+
$factory = sprintf('$this->factories%s[%s]', $definition->isPublic() ? '' : "['service_container']", $this->doExport($id));
834837
$code[1] = sprintf("%s = function () {\n%s};\n\nreturn %1\$s();\n", $factory, $code[1]);
835838
$code = $code[0].$code[1];
836839
}
@@ -1341,14 +1344,14 @@ private function getServiceConditionals($value): string
13411344
if (!$this->container->hasDefinition($service)) {
13421345
return 'false';
13431346
}
1344-
$conditions[] = sprintf("isset(\$this->%s['%s'])", $this->container->getDefinition($service)->isPublic() ? 'services' : 'privates', $service);
1347+
$conditions[] = sprintf("isset(\$this->%s[%s])", $this->container->getDefinition($service)->isPublic() ? 'services' : 'privates', $this->doExport($service));
13451348
}
13461349
foreach (ContainerBuilder::getServiceConditionals($value) as $service) {
13471350
if ($this->container->hasDefinition($service) && !$this->container->getDefinition($service)->isPublic()) {
13481351
continue;
13491352
}
13501353

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

13541357
if (!$conditions) {
@@ -1579,11 +1582,11 @@ private function dumpParameter(string $name): string
15791582
}
15801583

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

1586-
return sprintf("\$this->getParameter('%s')", $name);
1589+
return sprintf('$this->getParameter(%s)', $this->doExport($name));
15871590
}
15881591

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

15991602
if ($this->container->hasDefinition($id) && $definition = $this->container->getDefinition($id)) {
16001603
if ($definition->isSynthetic()) {
1601-
$code = sprintf('$this->get(\'%s\'%s)', $id, null !== $reference ? ', '.$reference->getInvalidBehavior() : '');
1604+
$code = sprintf('$this->get(%s%s)', $this->doExport($id), null !== $reference ? ', '.$reference->getInvalidBehavior() : '');
16021605
} elseif (null !== $reference && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $reference->getInvalidBehavior()) {
16031606
$code = 'null';
16041607
if (!$definition->isShared()) {
@@ -1607,20 +1610,20 @@ private function getServiceCall(string $id, Reference $reference = null): string
16071610
} elseif ($this->isTrivialInstance($definition)) {
16081611
$code = substr($this->addNewInstance($definition, '', '', $id), 8, -2);
16091612
if ($definition->isShared()) {
1610-
$code = sprintf('$this->%s[\'%s\'] = %s', $definition->isPublic() ? 'services' : 'privates', $id, $code);
1613+
$code = sprintf('$this->%s[%s] = %s', $definition->isPublic() ? 'services' : 'privates', $this->doExport($id), $code);
16111614
}
16121615
$code = "($code)";
16131616
} elseif ($this->asFiles && !$this->isHotPath($definition)) {
16141617
$code = sprintf("\$this->load('%s.php')", $this->generateMethodName($id));
16151618
if (!$definition->isShared()) {
1616-
$factory = sprintf('$this->factories%s[\'%s\']', $definition->isPublic() ? '' : "['service_container']", $id);
1619+
$factory = sprintf('$this->factories%s[%s]', $definition->isPublic() ? '' : "['service_container']", $this->doExport($id));
16171620
$code = sprintf('(isset(%s) ? %1$s() : %s)', $factory, $code);
16181621
}
16191622
} else {
16201623
$code = sprintf('$this->%s()', $this->generateMethodName($id));
16211624
}
16221625
if ($definition->isShared()) {
1623-
$code = sprintf('($this->%s[\'%s\'] ?? %s)', $definition->isPublic() ? 'services' : 'privates', $id, $code);
1626+
$code = sprintf('($this->%s[%s] ?? %s)', $definition->isPublic() ? 'services' : 'privates', $this->doExport($id), $code);
16241627
}
16251628

16261629
return $code;
@@ -1629,12 +1632,12 @@ private function getServiceCall(string $id, Reference $reference = null): string
16291632
return 'null';
16301633
}
16311634
if (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE < $reference->getInvalidBehavior()) {
1632-
$code = sprintf('$this->get(\'%s\', /* ContainerInterface::NULL_ON_INVALID_REFERENCE */ %d)', $id, ContainerInterface::NULL_ON_INVALID_REFERENCE);
1635+
$code = sprintf('$this->get(%s, /* ContainerInterface::NULL_ON_INVALID_REFERENCE */ %d)', $this->doExport($id), ContainerInterface::NULL_ON_INVALID_REFERENCE);
16331636
} else {
1634-
$code = sprintf('$this->get(\'%s\')', $id);
1637+
$code = sprintf('$this->get(%s)', $this->doExport($id));
16351638
}
16361639

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

16401643
/**

‎src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
+32Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,38 @@ public function testNonSharedServicesReturnsDifferentInstances()
195195
$this->assertNotSame($builder->get('bar'), $builder->get('bar'));
196196
}
197197

198+
/**
199+
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
200+
* @dataProvider provideBadId
201+
*/
202+
public function testBadAliasId($id)
203+
{
204+
$builder = new ContainerBuilder();
205+
$builder->setAlias($id, 'foo');
206+
}
207+
208+
/**
209+
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
210+
* @dataProvider provideBadId
211+
*/
212+
public function testBadDefinitionId($id)
213+
{
214+
$builder = new ContainerBuilder();
215+
$builder->setDefinition($id, new Definition('Foo'));
216+
}
217+
218+
public function provideBadId()
219+
{
220+
return [
221+
[''],
222+
["\0"],
223+
["\r"],
224+
["\n"],
225+
["'"],
226+
['ab\\'],
227+
];
228+
}
229+
198230
/**
199231
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
200232
* @expectedExceptionMessage You have requested a synthetic service ("foo"). The DIC does not know how to construct this service.

‎src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
+7-1Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,12 +236,18 @@ public function testAddServiceIdWithUnsupportedCharacters()
236236
{
237237
$class = 'Symfony_DI_PhpDumper_Test_Unsupported_Characters';
238238
$container = new ContainerBuilder();
239+
$container->setParameter("'", 'oh-no');
240+
$container->register("foo*/oh-no", 'FooClass')->setPublic(true);
239241
$container->register('bar$', 'FooClass')->setPublic(true);
240242
$container->register('bar$!', 'FooClass')->setPublic(true);
241243
$container->compile();
242244
$dumper = new PhpDumper($container);
243-
eval('?>'.$dumper->dump(['class' => $class]));
244245

246+
$this->assertStringEqualsFile(self::$fixturesPath.'/php/services_unsupported_characters.php', $dumper->dump(['class' => $class]));
247+
248+
require_once self::$fixturesPath.'/php/services_unsupported_characters.php';
249+
250+
$this->assertTrue(method_exists($class, 'getFooOhNoService'));
245251
$this->assertTrue(method_exists($class, 'getBarService'));
246252
$this->assertTrue(method_exists($class, 'getBar2Service'));
247253
}

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services33.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services33.php
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public function getRemovedIds()
6666
*/
6767
protected function getFooService()
6868
{
69-
return $this->services['Bar\Foo'] = new \Bar\Foo();
69+
return $this->services['Bar\\Foo'] = new \Bar\Foo();
7070
}
7171

7272
/**
@@ -76,6 +76,6 @@ protected function getFooService()
7676
*/
7777
protected function getFoo2Service()
7878
{
79-
return $this->services['Foo\Foo'] = new \Foo\Foo();
79+
return $this->services['Foo\\Foo'] = new \Foo\Foo();
8080
}
8181
}

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_adawson.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_adawson.php
+9-9Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -71,11 +71,11 @@ public function getRemovedIds()
7171
*/
7272
protected function getBusService()
7373
{
74-
$a = ($this->services['App\Db'] ?? $this->getDbService());
74+
$a = ($this->services['App\\Db'] ?? $this->getDbService());
7575

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

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

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

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

101101
return $instance;
102102
}
@@ -108,12 +108,12 @@ protected function getDbService()
108108
*/
109109
protected function getSchemaService()
110110
{
111-
$a = ($this->services['App\Db'] ?? $this->getDbService());
111+
$a = ($this->services['App\\Db'] ?? $this->getDbService());
112112

113-
if (isset($this->privates['App\Schema'])) {
114-
return $this->privates['App\Schema'];
113+
if (isset($this->privates['App\\Schema'])) {
114+
return $this->privates['App\\Schema'];
115115
}
116116

117-
return $this->privates['App\Schema'] = new \App\Schema($a);
117+
return $this->privates['App\\Schema'] = new \App\Schema($a);
118118
}
119119
}

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_requires.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_requires.php
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ public function getRemovedIds()
8181
*/
8282
protected function getParentNotExistsService()
8383
{
84-
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\ParentNotExists'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\ParentNotExists();
84+
return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\ParentNotExists'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\ParentNotExists();
8585
}
8686

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

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

107-
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());
107+
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());
108108
}
109109

110110
public function getParameter($name)

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_self_ref.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_self_ref.php
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ protected function getFooService()
7070
$b = new \App\Baz($a);
7171
$b->bar = $a;
7272

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

7575
$a->foo = $instance;
7676

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_rot13_env.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_rot13_env.php
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public function getRemovedIds()
6868
*/
6969
protected function getRot13EnvVarProcessorService()
7070
{
71-
return $this->services['Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor'] = new \Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor();
71+
return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Dumper\\Rot13EnvVarProcessor'] = new \Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor();
7272
}
7373

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

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.