Task #52668

Install Tool: Remove permission checking and fixing code from "folder structure"

Added by Ernesto Baschny almost 6 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Should have
Category:
Install Tool
Target version:
Start date:
2014-03-26
Due date:
% Done:

100%

TYPO3 Version:
6.2
PHP Version:
Tags:
Complexity:
medium
Sprint Focus:

Description

We've got several complaints and issues with the new "folder permission" checking and fixing - In the Install Tool "Step Installer" and later in the "Folder Structure" menu entry.

So we decided to simply remove this functionality from the install tool for now, and leave the responsability of a "secure" and "working" environment with the administrator / hosting provider. There are just too many constelations and exceptional conditions to consider here.

Default permissions (for new folders / files) should be:

Folders: 2775
Files: 0664

In the "Folder Structure" checking section a warning could be displayed if these are the active settings, pointing to a Wiki page describing different methods on how to "secure your installation". The wiki page could be collaborative effort documenting the best practice setup for different PHP vs Webservers vs OS variants.

Streamline what is considered an "Error", "Warning" and Notice":

Errors are:
  • Default permissions allowing world (other) writing (o+w)
  • Directories that do not exist
  • Directories that are not writeable (no files can be created within)
  • Paths that are files but should be directories
  • Paths that are not files but should be files
  • Symlinks that should be present but do not exist
Warnings are:
  • Default permissions allowing world (other) reading (o+r)
  • Symlink that is not a symlink (i.e. typo3_src)
  • Files that should be present but do not exist
Notices are:
  • Default permissions allowing group (other) r/w (g+rw)
  • Files that cannot be written (index.html, .htaccess)
  • Files which not match the configured default permission
  • Files that do not contain what we expect

chmodTest.php View (897 Bytes) Michiel Roos, 2014-01-12 10:38

chmodTest.php View (1.5 KB) Michiel Roos, 2014-01-13 10:51


Subtasks

Bug #57152: First Install lists directory errors: confusingClosedErnesto Baschny

Task #57354: Default file permissions recommendation schould be 0665 instead of 0660Closed


Related issues

Related to TYPO3 Core - Bug #52578: Install process removes permission Closed 2013-10-07
Related to TYPO3 Core - Bug #53037: Install Tool: Folder structure is too strict Closed 2013-10-22
Related to TYPO3 Core - Task #57055: Wrong unit tests after Installer permission fix Closed 2014-03-18
Related to TYPO3 Core - Bug #52016: wrong recommendations for typo3temp permission Closed 2013-09-15
Related to TYPO3 Core - Bug #58809: Default permissions for new files should be 0664 Closed 2014-05-14

Associated revisions

Revision 18a8f444 (diff)
Added by Ernesto Baschny over 5 years ago

[TASK] Install Tool: Optimize permission checking and fixing

  • Adjust default permissions back to "world readable" (775/664)
  • Do not attempt to fix permissions on an existing folder structure
  • Streamline the separation of Notices, Warnings and Errors
  • Cosmetical optimizations in the "Folder Structure" item
  • Also report if the default permissions are too insecure (i.e.
    world writeable / readable).

Note that the new defaults (775/664) are set to make the system work
by default on almost most scenarios, but the recommended secure setup
is still 770/660. This is on purpose to make the administrator aware
of it but will require manual intervention (i.e. "thinking") if this
setup will work or not (we cannot detect that automatically).

Resolves: #52668
Releases: 6.2
Change-Id: I77c72617fa58fd808ebc5fd818e94b48c2644102
Reviewed-on: https://review.typo3.org/25416
Reviewed-by: Christian Kuhn
Reviewed-by: Wouter Wolters
Reviewed-by: Stefan Neufeind
Reviewed-by: Philipp Gampe
Reviewed-by: Anja Leichsenring
Tested-by: Anja Leichsenring
Reviewed-by: Ernesto Baschny
Tested-by: Ernesto Baschny

History

#1 Updated by Gerrit Code Review almost 6 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

#2 Updated by Gerrit Code Review almost 6 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

#3 Updated by Ernesto Baschny almost 6 years ago

  • Assignee changed from Christian Kuhn to Ernesto Baschny
  • % Done changed from 0 to 90
  • Complexity set to medium

#4 Updated by Gerrit Code Review almost 6 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

#5 Updated by Gerrit Code Review over 5 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

#6 Updated by Michiel Roos over 5 years ago

Would it be possible to use only a umask instead of separate file and dir permissions? The umask could be set once per script invocation (or just outside of the loop doing the file operations). This would save a lot of octdec() calls and chmod() calls.

Just an idea. Have not done any tests yet. The file creation functions respect the current umask.

The PHP man page says:
Avoid using this function in multithreaded webservers. It is better to change the file permissions with chmod() after creating the file. Using umask() can lead to unexpected behavior of concurrently running scripts and the webserver itself because they all use the same umask.

So I guess this would be a nono for most cases. Implementing it as an option could be done, but would introduce unwanted complexity. So I guess we need to live with chmod for a while longer.

We should cache the octdec() calls in the calling classes though.

#7 Updated by Gerrit Code Review over 5 years ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

#8 Updated by Michiel Roos over 5 years ago

Please see this chmod test. All mode values are stored in a variable.

You can see that in the first case, where the mode is stored as an int, and prefixed with a 0, the permissions are set correctly without a call to octdec.

#9 Updated by Michiel Roos over 5 years ago

Here's a little test to figure out how PHP handles this whole chmod situation. We try both string values and plain int values to set the access controls.

It turns out that mkdir does not respect the sticky bit. An extra chmod is needed to add it. Not even ignoring the umask helps. Is this a PHP bug?

http://pastebin.com/RnHcFmJx

If we decide to keep on using the string values for the mode, the octdec() call should happen only once per class instance, when setting the mode property. The default property would have to be the 'dec' value then: 436 or 1533.

    protected function setTargetPermission($permission) {
        $this->targetPermission = octdec($permission);
    }

If we change the mode property to be a plain integer, we can completely do away with the octdec() calls. But the direcotry mode must be prefixed by a 0 for that to work correctly.

#10 Updated by Gerrit Code Review over 5 years ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

#11 Updated by Gerrit Code Review over 5 years ago

Patch set 7 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

#12 Updated by Gerrit Code Review over 5 years ago

Patch set 8 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

#13 Updated by Gerrit Code Review over 5 years ago

Patch set 9 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

#14 Updated by Ernesto Baschny over 5 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 90 to 100

#15 Updated by Jan Radecker over 5 years ago

  • Estimated time set to 0.00 h

The fileCreateMask has been set to 0665 in the changeset, which is wrong.
As stated above 0664 should be the default setting.

#16 Updated by Jan Radecker over 5 years ago

  • Estimated time deleted (0.00 h)

#17 Updated by Riccardo De Contardi almost 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF