Skip to content
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.

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

Security hardening #6724

Closed

Conversation

@aschempp
Copy link
Contributor

commented Feb 10, 2014

No description provided.

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 10, 2014

👍

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 10, 2014

This is a BC break but it fixes a potential security issue for 99.999% of all extensions that do not use deserialize for object unserialization. So it's only a BC break for those that use it to unserialize an object which is probably a security issue in their case anyway or can easily be fixed by renaming deserialize to unserialize.

However, the solution prevents users from having O: in their content. We'll have to find another solution.

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2014

It only prevents from Having O:35:" which is very unlikely ;-)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 10, 2014

Not sure about register_shutdown_function() but definitely +1 for the hardening of the deserialize() function. In fact, I had the code in my first fix 8c9cb04, but since it implied a BC break, I have removed it. And now we know that we also have to look for S: :)

@contao/workgroup-core @tristanlins @discordier

@tristanlins

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2014

For me, this looks good for the moment, but I need to test it. I have the discussion right now with @psi-4ward in mumble and propose the similar solution :-)

@psi-4ward

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2014

For me @aschempp fix with the register_shutdown_function() looks good.

  • unserialize() object injection is a PHP problem
  • it provides only an attack-vector if theres any attackable _destruct or __wakeup
  • unserialize() an object does not call _construct()

So we need to secure all _destruct and __wakeup. I think using the register_shutdown_function() within the _construct() solves the problem without any API-changes.

Thanks @tristanlins for helping me to understand this stuff.

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 11, 2014

ToDo's for this pull request in my opinion:

  • We call deserialize very often so I'd prefer to have a simple string comparison using strncmp first and only use preg_match if this condition matches so we don't suffer from too much performance loss (might be little but anyway)
  • Verify if we only need to check for O: or if there's anything else
  • Ask those guys that reported the issue and the exploit to verify that the changes fix the issue
  • Rename "Insertion" in comments to "Injection"
  • Add the comment to the deserialize function too
  • Add a comment to UPGRADE.md so developers that really want to unserialize objects use unserialize instead of Contao's deserialize. Also announce this BC break in the release news.
  • Add an issue to make sure, all serialize and unserialize/deserialize calls are replaced by either json_encode and json_decode or any other means such as a a comma-separated list in 4.0

@leofeyer leofeyer added this to the 3.2.6 milestone Feb 11, 2014

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 11, 2014

Add an issue to make sure, all serialize and unserialize/deserialize calls are replaced by either json_encode and json_decode or any other means such as a a comma-separated list in 4.0

What exactly do you mean?

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 11, 2014

What exactly do you mean?

Update all existing database fields and replace the serialized arrays by either a comma-separated list of id's or use json to store the data. We don't need any serialized data imho :)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 11, 2014

Why not? It is not dangerous to store serialized arrays in the database, is it? We should hold ourselves back from overreacting in this case.

@tristanlins

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2014

In fact, Serialization != Serialization!
We could just switch to another serialisation method, like json_encode / json_decode. For more complex data like objects, serialisation libraries like Symfony Serializer or JMS Serializer are realy good solutions to serialize and unserialize data.

@tristanlins

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2014

Why not? It is not dangerous to store serialized arrays in the database, is it?

If you able to inject the exploit into the database, even data from the database is dangerous!

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 11, 2014

If you able to inject the exploit into the database

That's true, but if the deserialize() function only handled arrays, the string would not be unserialized.

Also, changing the serialization type is a massive BC break which we cannot add in a maintenance or minor release. It can only be a long-term change then.

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 11, 2014

Also, changing the serialization type is a massive BC break which we cannot add in a maintenance or minor release. It can only be a long-term change then.

Yes, that's why I said 4.0 :)

That's true, but if the deserialize() function only handled arrays, the string would not be unserialized.

This is fighting the symptoms. We should do it now to fix the immediate threat of security breaches but we should certainly not consider this to be a long-term solution!

@fritzmg

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2014

Also json_encode, json_decode have the added bonus of being faster (and taking up less space, if that's an issue) compared to serialize, unserialize. According to this it's even a lot faster sometimes.

@discordier

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2014

Please also have a look at my PR at #6730 which hardens the Contao Core classes.

@discordier

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2014

On this PR:
I am strongly against pushing everything to register_shutdown_function() calls as it just does not feel right to tackle this issue that way.

Shutdown functions are meant to perform cleanup, correct. But yet we have a pretty fragile Singleton stack in Contao which might collapse once again.
If we really end up having this issue solved via shutdown functions then, at least, let's write it that we only register ONE shutdown function which will then shutdown all Singletons in a predefined order.
To achieve this, we will need something like a public method Singleton::shutdown(); which will be called from within that shutdown function.

On the commit to deserialize to kill O:35:" with an exception, this is clearly a BC break.
I.e in the UNIX environment, we have very often colon separated values (the PATH environment variable being the most prominent example).

Imagine the following (quickly made up) list of Delimiter separated values: LOGO:35:"some other information":"some more information" which will be misunderstood as "serialized value" which it clearly is not.
So we need to alter the regex to at least be a lot more restrictive, hence I doubt we will get it perfect but we can get it "good enough".

It might be fair to add that passing such a DSV through deserialize() should not happen (as it won't understand it anyway) but to have no BC, we simple can not accept such a change in such a widely used function. I mean, every value has been passed through it so far. Have a look at all the onload_callback handling in extensions etc.
All of those are candidates to break now as almost all of them are serializing and deserializing simply because many developers use it to "ensure we have an array" (see second parameter of function) or the like.
One can argue now, this is bad practice (and I strongly agree) but does not allow a BC break, at least not as long as we call the thing 3.2.
For 3.3 this may happen and should be done.

My two cents on the topic.

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2014

Regarding the shutdown functions, I intentionally only applied it to the methods that can and should be executed before objects are destroyed. register_shutdown_function is executed before __destruct of "global" objects like the database, so database connection etc. should always be available. I don't see the problem with the stack, but it should also solve #2236

Regarding #6730, how about I pull these changes into this pull request? Would that make sense? I guess you're aware you can also send pull requests to my pull request? ;)

@discordier

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2014

@aschempp IMO you are wrong with your statement:

Regarding the shutdown functions, I intentionally only applied it to the methods that can and should be executed before objects are destroyed. register_shutdown_function is executed before __destruct of "global" objects like the database, so database connection etc. should always be available. I don't see the problem with the stack, but it should also solve #2236.

You are simply moving the original Problem in #2236 to some earlier state.
Current state:

  • shutdown_functions() => nothing to do as no functions are registered currently.
  • __destruct() => Must be called in a fixed sequence which can be easily corrupted by calling getInstance() in the wrong order.

Your desired state (with this PR):

  • shutdown_functions() => Must be called in a fixed sequence which can be easily corrupted by calling getInstance() in the wrong order.
  • __destruct() => nothing to do as no functions are registered currently.

I really can not see how you want to fix #2236 along using your current state of the PR.
When the shutdown function of the Database is being executed, the ressource/mysqli object is unset from the instance (yet the instance still exists but becomes useless). Therefore the Session will not be able to be saved in either case.
The same was also being observed by @leofeyer in the discussion of #2236. Read the comments from #2236 (comment) on.

Regarding the changes in #6730, I also filed an WIP PR to your repository but I think we should discuss both approaches to the end before merging anything as this is a too severe matter to be lightly hearted merged into anything.

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2014

If you look at my PR, I have not changed __destruct of the Database class, only the safe-to-database methods are adjusted.

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 12, 2014

I've found a very simple but effective solution to harden the deserialize() function without preg_match() or the risk of blocking false positives. I'll present it to you in our call at 1:30 pm.

@discordier

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2014

@leofeyer My proposal is in #6732

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 12, 2014

@discordier You are still using preg_match(), which is not wrong but will cause overhead given the high number of deserialize() calls in the core. Let's wait for the call, shall we? :)

@discordier

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2014

@aschempp, @tristanlins, @Toflar, @leounglaub and me are already all waiting. :)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 12, 2014

I have just released versions 2.11.15 and 3.2.6 with the changes we have discussed in the call. If we want to keep thinking about using register_shutdown_function() instead of the destructors, the best way were to open another issue/PR.

@leofeyer leofeyer closed this Feb 12, 2014

@aschempp aschempp deleted the aschempp:pullrequest/PHP_Object_Injection branch Feb 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.