[#26995] - Invalid fix for security issue 20191002
- Closed
- 5 Nov 2019
- Medium
- Build: staging
- # 26995
Someone thought it was a good idea to add the defined or die check to a bunch of proxy files without any consideration that this path was originally third party library code.
Granted, the JPATH_LIBRARIES constant should have never been used in the first place and things should have just stuck with native PHP filesystem functions and constants (my bad), but as this path was originally third party code it should not be expected that the Joomla application be fully bootstrapped in order to include these files (and yes, for all you purists out there, people can and do execute PHP files that aren't index.php or administrator/index.php), otherwise you need to fork all third party library code and add the defined or die check. There is a reason the files all check for the presence of the core logging class instead of just blindly trying to write to logs and triggering a fatal error.
The defined or die checks should be removed and the use of Joomla's path constants replaced. Unless it can be proven that the inclusion of the phputf8 library files causes unwarranted side effects, it should not be mandatory for the Joomla application to be bootstrapped to include these proxy files for a third party library that was moved in the filesystem.
joomla-cms-bot
- | Labels |
Added:
?
|
||
No!
I'm saying that it was wrong by me in the original PR to use the Joomla path constants, and it is wrong by the JSST to introduce more Joomla constants into these files. The right fix is to just use native PHP __DIR__ for everything and to not have any uses of Joomla path constants in these files.
As said in the original PR, libraries/phputf8 was all third party code that had to be manually included and the proxy logic was set up to keep from running into errors with redeclaring functions after the Framework package started autoloading the phputf8 library.
As third party code, that means it does not require Joomla to be bootstrapped, and the intent was to keep it possible to include those files without bootstrapping Joomla. But I screwed up by using a Joomla path constant instead of sticking to native functions and constants. Didn't catch it then, only caught it today looking at the release diff.
Please respect that this path was originally third party code and not force Joomla to be bootstrapped to include it. That would basically be like saying "we should add defined or die to the Symfony packages because it's shipped in Joomla" (which people have seriously suggested in the past).
I don't really see any reason why a CMS proxy file needs to not depend on the CMS. Just my opinion
The fix is valid.
Is it the best way to solve the path disclosure? Perhaps.
People should not think there site is insecure with this
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-11-05 19:00:56 |
| Closed_By | ⇒ | mbabker |
Nevermind. Clearly I'm not communicating the issue here in a way people will understand, or people will just choose to ignore it anyway.
You are right checking for JPATH_LIBRARIES would be less problematic. But JPATH_LIBRARIES is a core constants that is set by the cms includes.
So this was already wrong in the first place.
Anyway we will change the check to JPATH_LIBRARIES instead of _JEXEC.