8000 Fix clickjacking rules definition to keep the correct order by romainneutron · Pull Request #72 · nelmio/NelmioSecurityBundle · GitHub
[go: up one dir, main page]
Skip to content

Fix clickjacking rules definition to keep the correct order #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix clickjacking rules definition to keep the correct order #72

wants to merge 1 commit into from

Conversation

romainneutron
Copy link
Collaborator

Hello,

At the moment, it's possible to declare multiple paths for the clickjacking, for example

    clickjacking:
        content_types: ['text/html']
        paths:
            '/myroute/embed': ALLOW
            '^/': DENY

However, due to how Symfony Config merges associative arrays, it sorts it by keys, and this result into this inside the listener:

array(2) {
  ["^/"]=>
  array(1) {
    ["header"]=>
    string(4) "DENY"
  }
  ["/myroute/embed"]=>
  array(1) {
    ["header"]=>
    string(5) "ALLOW"
  }
}

First matching rule is applied, everything is denied :( (smiley vraiment triste).

I don't want to patch Symfony, as PHP7 and HHVM do not guarantee that an hashmap order is still conserved.
So I propose this BC patch that introduce new "rules" (inspired by security bundle):

    clickjacking:
        content_types: ['text/html']
        rules:
            - { path:'^/myroute/embed$', header: ALLOW }
            - { path:'^/', header: DENY }

As this array is now indexed, it's not sorted anymore and I got this in the listener:

array(2) {
  [0]=>
  array(2) {
    ["path"]=>
    string(16) "^/myroute/embed$"
    ["header"]=>
    string(5) "ALLOW"
  }
  [1]=>
  array(2) {
    ["path"]=>
    string(2) "^/"
    ["header"]=>
    string(4) "DENY"
  }
}

This patch is fully BC (no need to change any config for upgrading users) and could be drop easily in 2.0 (I commented everywhere it's needed). I can even create the upcoming PR for dropping this.
However, I doubt any user use this feature, because it does not work at the moment :D

I hope it would satisfy everybody.

With love!

@Seldaek
Copy link
Member
Seldaek commented Oct 2, 2015

PHP7 and HHVM don't guarantee the order of hash maps?! I never heard of that, but it sounds like that'd break a ton of stuff.. According to this article, this is not true for PHP7. I don't know about HHVM but it would surprise me that they break PHP compat in such a way. https://nikic.github.io/2014/12/22/PHPs-new-hashtable-implementation.html

Also sounds pretty messed up to me that Symfony Config would sort by key though, that's quite unexpected. Has it always been doing that? I don't really have time to dig in right now.

@romainneutron
Copy link
Collaborator Author

I've to confess that Fabien told me "never rely on the order on a hashmap" and I believed him without checking, my bad. Julien (Pauli) now tells me the order is never altered, and it won't change in PHP7. Let's forget this.

I've dig more and more, and here's what's wrong (it took me some time to figure out!). In my previous example, I obfuscated what I was really doing.

The change in the order of declared paths appears when the key contains a parameter that is interpolated:

Try this:
First, declare in your parameters.yml:

    #...
    uuid_regex: '(\d+|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})'

Then use this parameter in Nelmio Security Bundle:

        paths:
            '/profiles/%uuid_regex%/embed': ALLOW
            '^/': DENY

And patch the security bundle to dump what's coming:

diff --git a/DependencyInjection/NelmioSecurityExtension.php b/DependencyInjection/NelmioSecurityExtension.php
index e93e69e..8cc40c7 100644
--- a/DependencyInjection/NelmioSecurityExtension.php
+++ b/DependencyInjection/NelmioSecurityExtension.php
@@ -65,6 +65,7 @@ class NelmioSecurityExtension extends Extension
                 $rules[] = array('path' => '^/.*', 'header' => 'DENY');
             }

+            var_dump($config['clickjacking']['paths']);exit;
             $container->setParameter('nelmio_security.clickjacking.rules', $rules);
             $container->setParameter('nelmio_security.clickjacking.content_types', $config['clickjacking']['content_types']);
         }

run app/console, and boum:

array(2) {
  ["^/"]=>
  array(1) {
    ["header"]=>
    string(4) "DENY"
  }
  ["/profiles/(\d+|[0_9a_f]{8}_[0_9a_f]{4}_[0_9a_f]{4}_[0_9a_f]{4}_[0_9a_f]{12})/embed"]=>
  array(1) {
    ["header"]=>
    string(5) "ALLOW"
  }
}

if I revert my patch and remove the interpolation:

        paths:
            '/profiles/uuid_regex/embed': ALLOW
            '^/': DENY

then:

array(2) {
  ["/profiles/uuid_regex/embed"]=>
  array(1) {
    ["header"]=>
    string(5) "ALLOW"
  }
  ["^/"]=>
  array(1) {
    ["header"]=>
    string(4) "DENY"
  }
}

@romainneutron
Copy link
Collaborator Author

It smells like a bug in Symfony...

@romainneutron
Copy link
Collaborator Author

This is done here: https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php#L142-L152

I'm not sure there's an easy fix for this, I'm going to dig

@romainneutron
Copy link
Collaborator Author

I think I'm still wrong about the origin of this issue... I'm stopping my monologue until I find a solution

@Seldaek
Copy link
Member
Seldaek commented Oct 9, 2015

Any news @romainneutron?

@romainneutron
Copy link
Collaborator Author

There's something in Symfony that prevents keeping the order when using interpolation as I do in my example. I did not have time to dig where it comes from yet

@sstok
Copy link
sstok commented Oct 19, 2015

Because when the parameter is interpolated the old key is removed and a new one (at a later) position is added ;)

'/profiles/%uuid_regex%/embed' is changed to '/profiles/(\d+|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})/embed' but you can't replace a key without adding a new one.

It is possible by rebuilding the array as described in http://stackoverflow.com/a/10182739
But it will affect performance when this is done at runtime (during a request).

@Seldaek
Copy link
Member
Seldaek commented Oct 24, 2015

Might be a good idea to open an issue on the symfony repo, then maybe someone else has time to look into it and we can get a fix in in time for 2.8? :)

@romainneutron romainneutron force-pushed the clickjacking-rules-order branch from a48b5c9 to 2628385 Compare December 22, 2015 10:44
@romainneutron romainneutron force-pushed the clickjacking-rules-order branch from 2628385 to 4cd9474 Compare December 23, 2015 12:24
@romainneutron
Copy link
Collaborator Author

FYI, a patch in Symfony is pending here symfony/symfony#17129
Let's close this PR , and merry christmas ! :)

@romainneutron romainneutron mentioned this pull request Dec 24, 2015
Merged
@romainneutron romainneutron deleted the clickjacking-rules-order branch December 28, 2015 09:23
@Seldaek
Copy link
Member
Seldaek commented Dec 30, 2015

Great thanks for digging into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@romainneutron @Seldaek @sstok
0