[#8532] - Fixed logic bug in JPath::isOwner
- Closed
- 19 Jan 2016
- Medium
- Build: staging
- Easy Test
- # 8532
- Diff
- SniperSister:patch-1
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Issue
There's a logic bug in JPath::isOwner causing false results of the ownership check. Let's assume:
- /tmp is unwriteable
- session.save_path is writeable
Expected result: session.save_path is used as $dir
Actual result: As $dir is a string, !$dir becomes a boolean false and the whole check returns false. This causes that the correct $dir is overwritten with a false in line 241.
I found this issue because I was trying to save a configuration.php with chmod 444 on a webspace with an unwriteable /tmp and a writeable session.save_path. Because of the logic bug, isOwner doesn't return true as expected and Joomla fails to execute a chmod 644 which would allow us to update the file.
Solution
I fixed the logic bug and improved the readablity by converting the code into a foreach loop.
Testing
That's a bit harder. You need a server with:
- /tmp unwriteable for PHP (i.e. because it's not included in the open_basedir)
- session.save_path writeable
If you have such a machine. Change the chmod of the configuration.php to 444 and try to save the global configuration. You'll get an errror message.
Apply the patch, try to save the configuration.php again, now it works as expected.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
zero-24
- | Category | ⇒ | Libraries |
| Easy | No | ⇒ | Yes |
@SniperSister I think the current code is flawed that much that George has the same issues understanding it like I had yesterday in Glip 
No I see the issue now. What that code should have been was
$dir = is_writable('/tmp') ? '/tmp' : false;
$dir = (!$dir && is_writable($ssp)) ? $ssp : $dir;
$dir = (!$dir && is_writable($jtp)) ? $jtp : dir;
Now it makes sense why you are changing the order as well - because currently the other two are completely ignored anyhow.
Hi David,
confirm: Actual result: As $dir is a string, !$dir becomes a boolean false and the whole check returns false. This causes that the correct $dir is overwritten with a false in line 241. 74db56f
Confirm integrated deletions/additions to /libraries/joomla/filesystem/path.php and the patch solves it.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8532.
I have tested this item 
successfully on 86faeef
confirm patch successfully.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8532.
I have tested this item successfully on 86faeef
Test OK, configuration.php can be saved and it remains 444
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8532.
Awesome, thanks folks! Setting to RTC as we're having 2 successful tests.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8532.
| Status | Pending | ⇒ | Ready to Commit |
@wilsonge @SniperSister Any idea how it will go on with this PR? It is RTC but has no milestone. If it shall be merged, I suggest to change it to correct the issue in the same way as @wilsonge did for the framework with commit 5ee2159. Or is it solved with the commit on the framework for the CMS, too, means the file libraries/joomla/filesystem/path.php is taken from the framework? In this case this PR here should be closed, or not?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8532.
| Labels |
Added:
?
|
||
| Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-01-19 11:44:44 |
| Closed_By | ⇒ | wilsonge |
| Milestone |
Added: |
||
| Labels |
Removed:
?
|
||
Checking tmp first does make sense. This will do for now.
OK so obvious question maybe but why is
$dira string in the first place? The only way that can happen is ifis_writable('/tmp')returnstruewhich suggests you have bigger problems going on?