Skip to content

Commit

Permalink
bug #32115 [SecurityBundle] don't validate IP addresses from env var …
Browse files Browse the repository at this point in the history
…placeholders (xabbuh)

This PR was merged into the 4.3 branch.

Discussion
----------

[SecurityBundle] don't validate IP addresses from env var placeholders

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32108
| License       | MIT
| Doc PR        |

Commits
-------

f23a7f6 don't validate IP addresses from env var placeholders
  • Loading branch information
fabpot committed Jun 20, 2019
2 parents 0a1a885 + f23a7f6 commit 030396a
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 40 deletions.
Expand Up @@ -143,15 +143,6 @@ private function addAccessControlSection(ArrayNodeDefinition $rootNode)
->integerNode('port')->defaultNull()->end()
->arrayNode('ips')
->beforeNormalization()->ifString()->then(function ($v) { return [$v]; })->end()
->beforeNormalization()->always()->then(function ($v) {
foreach ($v as $ip) {
if (false === $this->isValidIp($ip)) {
throw new \LogicException(sprintf('The given "%s" value in the "access_control" config option is not a valid IP address.', $ip));
}
}

return $v;
})->end()
->prototype('scalar')->end()
->end()
->arrayNode('methods')
Expand Down Expand Up @@ -432,30 +423,4 @@ private function addEncodersSection(ArrayNodeDefinition $rootNode)
->end()
;
}

private function isValidIp(string $cidr): bool
{
$cidrParts = explode('/', $cidr);

if (1 === \count($cidrParts)) {
return false !== filter_var($cidrParts[0], FILTER_VALIDATE_IP);
}

$ip = $cidrParts[0];
$netmask = $cidrParts[1];

if (!ctype_digit($netmask)) {
return false;
}

if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) {
return $netmask <= 32;
}

if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) {
return $netmask <= 128;
}

return false;
}
}
Expand Up @@ -731,20 +731,32 @@ private function createExpression($container, $expression)
return $this->expressions[$id] = new Reference($id);
}

private function createRequestMatcher($container, $path = null, $host = null, int $port = null, $methods = [], $ip = null, array $attributes = [])
private function createRequestMatcher(ContainerBuilder $container, $path = null, $host = null, int $port = null, $methods = [], array $ips = null, array $attributes = [])
{
if ($methods) {
$methods = array_map('strtoupper', (array) $methods);
}

$id = '.security.request_matcher.'.ContainerBuilder::hash([$path, $host, $port, $methods, $ip, $attributes]);
if (null !== $ips) {
foreach ($ips as $ip) {
$container->resolveEnvPlaceholders($ip, null, $usedEnvs);

if (!$usedEnvs && !$this->isValidIp($ip)) {
throw new \LogicException(sprintf('The given value "%s" in the "security.access_control" config option is not a valid IP address.', $ip));
}

$usedEnvs = null;
}
}

$id = '.security.request_matcher.'.ContainerBuilder::hash([$path, $host, $port, $methods, $ips, $attributes]);

if (isset($this->requestMatchers[$id])) {
return $this->requestMatchers[$id];
}

// only add arguments that are necessary
$arguments = [$path, $host, $methods, $ip, $attributes, null, $port];
$arguments = [$path, $host, $methods, $ips, $attributes, null, $port];
while (\count($arguments) > 0 && !end($arguments)) {
array_pop($arguments);
}
Expand Down Expand Up @@ -788,4 +800,30 @@ public function getConfiguration(array $config, ContainerBuilder $container)
// first assemble the factories
return new MainConfiguration($this->factories, $this->userProviderFactories);
}

private function isValidIp(string $cidr): bool
{
$cidrParts = explode('/', $cidr);

if (1 === \count($cidrParts)) {
return false !== filter_var($cidrParts[0], FILTER_VALIDATE_IP);
}

$ip = $cidrParts[0];
$netmask = $cidrParts[1];

if (!ctype_digit($netmask)) {
return false;
}

if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) {
return $netmask <= 32;
}

if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) {
return $netmask <= 128;
}

return false;
}
}
Expand Up @@ -40,6 +40,12 @@ secured-by-one-real-ip-with-mask:
secured-by-one-real-ipv6:
path: /secured-by-one-real-ipv6

secured-by-one-env-placeholder:
path: /secured-by-one-env-placeholder

secured-by-one-env-placeholder-and-one-real-ip:
path: /secured-by-one-env-placeholder-and-one-real-ip

form_logout:
path: /logout_path

Expand Down
Expand Up @@ -109,7 +109,7 @@ public function testSecurityConfigurationForExpression($config)
public function testInvalidIpsInAccessControl()
{
$this->expectException(\LogicException::class);
$this->expectExceptionMessage('The given "256.357.458.559" value in the "access_control" config option is not a valid IP address.');
$this->expectExceptionMessage('The given value "256.357.458.559" in the "security.access_control" config option is not a valid IP address.');

$client = $this->createClient(['test_case' => 'StandardFormLogin', 'root_config' => 'invalid_ip_access_control.yml']);
$client->request('GET', '/unprotected_resource');
Expand Down
@@ -1,6 +1,9 @@
imports:
- { resource: ./../config/default.yml }

parameters:
env(APP_IP): '127.0.0.1'

security:
encoders:
Symfony\Component\Security\Core\User\User: plaintext
Expand Down Expand Up @@ -43,6 +46,8 @@ security:
- { path: ^/secured-by-one-real-ip$, ips: 198.51.100.0, roles: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/secured-by-one-real-ip-with-mask$, ips: '203.0.113.0/24', roles: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/secured-by-one-real-ipv6$, ips: 0:0:0:0:0:ffff:c633:6400, roles: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/secured-by-one-env-placeholder$, ips: '%env(APP_IP)%', roles: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/secured-by-one-env-placeholder-and-one-real-ip$, ips: ['%env(APP_IP)%', 198.51.100.0], roles: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/highly_protected_resource$, roles: IS_ADMIN }
- { path: ^/protected-via-expression$, allow_if: "(is_anonymous() and request.headers.get('user-agent') matches '/Firefox/i') or is_granted('ROLE_USER')" }
- { path: .*, roles: IS_AUTHENTICATED_FULLY }
Expand Up @@ -19,4 +19,4 @@ security:

access_control:
# the '256.357.458.559' IP is wrong on purpose, to check invalid IP errors
- { path: ^/unprotected_resource$, ips: [1.1.1.1, 256.357.458.559], roles: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/unprotected_resource$, ips: [1.1.1.1, '%env(APP_IP)%', 256.357.458.559], roles: IS_AUTHENTICATED_ANONYMOUSLY }

0 comments on commit 030396a

Please sign in to comment.