Task #52668
Install Tool: Remove permission checking and fixing code from "folder structure"
| Status: | Resolved | Start date: | 2014-03-26 | |
|---|---|---|---|---|
| Priority: | Should have | Due date: | ||
| Assigned To: | Ernesto Baschny | % Done: | 100% |
|
| Category: | Install Tool | Spent time: | - | |
| Target version: | 6.2.0 | |||
| TYPO3 Version: | 6.2 | Complexity: | medium | |
| PHP Version: | 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
- 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
- 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
(897 Bytes)
chmodTest.php
(1.5 kB)
Subtasks
Related issues
Associated revisions
[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 over 1 year 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 over 1 year 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 over 1 year ago
- Assigned To changed from Christian Kuhn to Ernesto Baschny
- % Done changed from 0 to 90
- Complexity set to medium
#4 Updated by Gerrit Code Review over 1 year 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 1 year 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 1 year 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 1 year 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 1 year ago
- File chmodTest.php added
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 1 year ago
- File chmodTest.php added
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?
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 1 year 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 1 year 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 1 year 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 1 year 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 1 year ago
- Status changed from Under Review to Resolved
- % Done changed from 90 to 100
Applied in changeset 18a8f44414593cccd3469ff63afe6d90ebd7cab1.
#15 Updated by Jan Radecker over 1 year ago
- Estimated time set to 0.00
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 1 year ago
- Estimated time deleted (
0.00)