[#16587] - Prevent serialization of the JApplication object
- Closed
- 22 Aug 2020
- Medium
- Build: master
- # 16587
The problem
The last few weeks I have come across at least one template provider whose templates, due to negligence and/or lack of understanding of how Joomla! works, tries to store a reference to the JApplication object in an object that ends up in the Joomla! cache. As you know, this means that the object gets serialized on cache save and resumed (woken up) on cache read.
That wouldn't be all that bad except for the fact that JApplication holds transient information such as the JInput object (GET, POST and FILES request data) and the plugins event dispatcher. This opens a security hole since a plugin could now be cached under a Super User and resumed under a plain user. This could easily and plausibly be the case of a content plugin, thus leading to information disclosure.
In the unlikely event the site owner is so profoundly lucky that they do not get a security issue, they will most likely get quirky behavior. Even worse, they are likely to blame the plugin developer -not the offending template developer- for the misbehaving plugins. This is understandable. From the user's point of view there's no way to connect the odd behavior with the template.
Proposed solution
JApplication should have a magic __sleep method like this:
public function__sleep()
{
die('Application objects are transient and should never be serialized.');
}Perhaps we could add "Go to your site's administrator backend, System, Global Configuration and set Caching to Off to fix this issue". We could even make this string localizable. Even better, we could like to a Joomla! documentation page explaining the security and behavior aspects of this bug in third party software.
Even though in the short term this could be moderately annoying for the users, in the medium term we are improving the security and reliability of thousands of Joomla! sites and forcing template developers to adopt the same sane security practices that we, core and backend developers, have been following and taking for granted for over a decade.
Tagging @mbabker and @wilsonge since they are qualified to discuss this and we have spoken briefly about it at JaB17. Tagging @PhilETaylor since he knows his security stuff ;) Finally, tagging @rdeutz since he's the release manager of the current branch. Apologies if I forgot someone else, feel free to chip in your opinion.
joomla-cms-bot
- | Labels |
Added:
?
|
||
franz-wohlkoenig
- | Category | ⇒ | com_plugins |
http://php.net/manual/en/language.oop5.magic.php
The intended use of __sleep() is to commit pending data or perform similar cleanup tasks. Also, the function is useful if you have very large objects which do not need to be saved completely.
.
The intended use of __wakeup() is to reestablish any database connections that may have been lost during serialization and perform other reinitialization tasks.
Too much of the application is transient I think to sanely deal with that:
- Static instances container with singletons
JDocumentinstance options which are request specific- System message queue (extracted from the session)
- A
JProfilerinstance when debug is enabled - The web client (device detection, a per-request thing)
- A
JDocumentinstance - A
JSessioninstance - A
JLanguageinstance - A
JInputinstance
I think we're looking at a case where this object, by design, shouldn't be serialized and stored somewhere as a whole, and definitely shouldn't be unserialized and re-used.
Regarding __sleep(), yes, it is possible to use it correctly but much
more complicated (I had already considered that). The problem is that
__sleep() is expected to return an array with the properties which
should be serialized (as opposed to those which shouldn't). This means
we have to go through reflection and apply a blacklist. The blacklist
can be defined in a protected property, allowing us to override it in
descendant classes. Not pretty, but works.
My original proposal was on the grounds that JApplication is not meant
to be serialized. We should make it abundantly clear to developers.
Using magic methods to prevent serialization or direct instantiation is
uncommon but not forbidden. Before abstract classes we'd use a
constructor that threw an error to prevent instantiation of helper
classes, for example.
Anyway, either solution works for me. All I care about is that people's
sites are secure.
Unless there is a strong argument for keeping support, I think this is one class chain where we need to just full stop block serialization. And in general it'd be a good time to review high level parts of the API to sort out where we need to start defining serializable aspects of objects a bit better (i.e. K2 doing something where it serializes a JTable instance which holds the database driver).
| Status | New | ⇒ | Discussion |
Well J4 would be a good opportunity to block the serialization - not that I have a clue how
| Labels |
Added:
?
|
||
| Labels |
Added:
J3 Issue
|
||
| Title |
|
||||||
| Status | Discussion | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-08-22 14:10:24 |
| Closed_By | ⇒ | nikosdion | |
| Labels |
Added:
?
Removed: ? |
||
Closing since nobody cares to do anything about it for two years.
| Labels |
Removed:
?
|
||
Rather than a hard die() in
__sleep, surely the correct way would be to use__sleepmagic method to remove transient/insecure properties, and leave a bare, secure, sleepy object, which can be awoken and reinitialised with__wakeupin the future...That's the whole point of those magic methods.
Although you, and I , cannot think when sleeping JApplication would be a valid technical solution to any problem, doesn't mean we should prevent that from being put to sleep.