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

Fixed logic bug in JPath::isOwner #8532

Merged
merged 2 commits into from Jan 19, 2016

Conversation

Projects
None yet
7 participants
@SniperSister
Copy link
Contributor

commented Nov 24, 2015

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.

$dir = (!$dir && is_writable($jtp)) ? $jtp : false;
$dir = false;
foreach (array($jtp, $ssp, '/tmp') as $currentDir)

This comment has been minimized.

Copy link
@wilsonge

wilsonge Nov 25, 2015

Contributor

We now appear to be checking in the reverse order to present. Surely we should be checking /tmp first then $ssp then $jtp.

This comment has been minimized.

Copy link
@SniperSister

SniperSister Nov 25, 2015

Author Contributor

@Bakual suggested this as $jtp is the most likely writeable directory

This comment has been minimized.

Copy link
@Bakual

Bakual Nov 25, 2015

Contributor

We discussed that on Glip and we thought it makes most sense if the temp directory in the Joomla folder is tried (and used) first. With falling back to the session temp folder and then to a (maybe) existing tmp directory in server root.

@wilsonge

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2015

OK so obvious question maybe but why is $dir a string in the first place? The only way that can happen is if is_writable('/tmp') returns true which suggests you have bigger problems going on?

@SniperSister

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2015

I don't get your last comment @wilsonge, can you rephrase it?

@Bakual

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2015

@SniperSister I think the current code is flawed that much that George has the same issues understanding it like I had yesterday in Glip 😄

@wilsonge

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2015

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.

wilsonge added a commit to joomla-framework/filesystem that referenced this pull request Nov 25, 2015

@ChristineWk

This comment has been minimized.

Copy link

commented Nov 25, 2015

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.

@ChristineWk

This comment has been minimized.

Copy link

commented Nov 25, 2015

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.

@anibalsanchez

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2015

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.

@SniperSister

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2015

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.

@richard67

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

@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.

@joomla-cms-bot joomla-cms-bot added the RTC label Jan 19, 2016

wilsonge added a commit that referenced this pull request Jan 19, 2016

Merge pull request #8532 from SniperSister/patch-1
Fixed logic bug in JPath::isOwner

@wilsonge wilsonge merged commit 8419130 into joomla:staging Jan 19, 2016

2 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wilsonge wilsonge added this to the Joomla! 3.5.0 milestone Jan 19, 2016

@joomla-cms-bot joomla-cms-bot removed the RTC label Jan 19, 2016

@wilsonge

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

Checking tmp first does make sense. This will do for now.

mbabker added a commit to joomla-framework/filesystem that referenced this pull request Mar 26, 2016

@SniperSister SniperSister deleted the SniperSister:patch-1 branch May 5, 2019

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