Bug #87733
closed
unwanted side-effect in fileDenyPattern
100%
Description
The newly introduced fileDenyPattern
\.(php[3-7]?|phpsh|phtml|pht|phar|shtml|cgi|pl)(\..*)?$|^\.htaccess$
matches e.g. setup.pl.typoscript what appears to me as a suitable filename in a TypoScript cascade for a polish website. I have only limited knowledge in regex, but the following expression served its purpose in my tests.
\.(php[3-7]?|phpsh|phtml|pht|phar|shtml|cgi|pl)(\.*)?$|^\.htaccess$
Kind regards
Stefan
Updated by Markus Klein almost 6 years ago
Can you please describe what the "unwanted side-effect" is?
What is your usecase, what does not work?
The regex modification of yours does not deny files like .pl.bak anymore, but it would still deny .pl.....
Updated by Markus Klein almost 6 years ago
- Status changed from New to Needs Feedback
Updated by Stefan Frank almost 6 years ago
Markus Klein wrote:
Can you please describe what the "unwanted side-effect" is?
What is your usecase, what does not work?
Thank you for your attention to my request.
It is very reasonable to prevent files with, for example, the file extensions .bak and .sic from being delivered by the server. The .htaccess supplied with TYPO3 prevents the delivery of .bak files, so the fileDenyPattern is a "duplicate" if you use the supplied .htaccess. But I do not want to discuss or doubt your efforts in hardening TYPO3. My use case: At application level, I have stumbled across the fact that TYPO3 rejected the file setup.pl.typoscript. This file contains configuration data for a Polish website.
Updated by Markus Klein almost 6 years ago
Sorry, I ask again. Please describe your setup.
Where do you use this setup.pl.typoscript file? Do you include it somewhere?
Give us instructions how to reproduce the issue.
Thanks
Updated by Stefan Frank over 5 years ago
Markus Klein wrote:
Where do you use this
setup.pl.typoscriptfile? Do you include it somewhere?
In a multi-client system, the file setup.pl.ts is read in the root template of the client. The command is
<INCLUDE_TYPOSCRIPT: source="FILE:MYPATH/templates/ts/setup.ts"><INCLUDE_TYPOSCRIPT: source="FILE:MYPATH/templates/ts/setup.pl.ts">
The file setup.pl.ts overwrites values from setup.ts.
Do you need further data?
Kind regards
Updated by Markus Klein over 5 years ago
Thanks for the info. Will take a look and will discuss, how to solve this problem properly.
Updated by Markus Klein over 5 years ago
- Category set to Security
- Status changed from Needs Feedback to Accepted
- Assignee set to Markus Klein
- Priority changed from Should have to Must have
- Target version set to Candidate for patchlevel
- Complexity set to medium
- Is Regression set to Yes
Updated by Oliver Hader over 5 years ago
The modified file deny pattern would allow files like evil.php.jpg which might be executable depending on which web server and configuration is used.
In the last few years setups e.g. using the following in Apache have been "popular":
AddHandler php-script .php
Apache's approach for multiple file extensions is described here: https://httpd.apache.org/docs/2.4/mod/mod_mime.html#multipleext
Concerning setup.pl.typoscript and having a setup like documented in https://httpd.apache.org/docs/2.4/howto/cgi.html it would have to be considered as RCE vulnerability (that's why it was added to the deny pattern), basically:
AddHandler cgi-script .cgi .pl
Updated by Oliver Hader over 5 years ago
Thus, the the general question is why TypoScript includes are using the fileDeny pattern (in terms of parsing TypoScript configuration). It has been introduced in a security release a couple of years ago:
- https://github.com/TYPO3/TYPO3.CMS/commit/368f57fe2158aff559c4bee2bba5de20d271799e
- https://typo3.org/security/advisory/typo3-sa-2010-022/
These are the aspects that should be tackled, replaced and hardened further.
Updated by Oliver Hader over 5 years ago
- Status changed from Accepted to Needs Feedback
- Priority changed from Must have to Should have
Updated by Oliver Hader over 5 years ago
- Target version deleted (
Candidate for patchlevel)
Updated by Oliver Hader over 5 years ago
- Related to Bug #24221: t3lib_TSparser::checkIncludeLines() does not check files to be included added
Updated by Wolfgang Klinger over 5 years ago
- Priority changed from Should have to Must have
I stumbled upon this too right now …
We have a customer with clients in Poland (PL) and obviously uses filenames like
K.PL.BAN.A.V7.ACH.RES.pdf
and this does not work with the current regular expression in the last TYPO3 (neither 8, nor 9) versions, because ResourceStorage::checkFileExtensionPermission uses GeneralUtility::verifyFilenameAgainstDenyPattern which uses this pattern.
That's a real blocker, we can't tell clients: huh, you know, you can't use certain filenames when using TYPO3 ;-)
I mean it's easily solvable by changing the regular expression to match the extensions only after the last dot,
but that is not so obvious that you immediately think about it when this problem occurs.
Updated by Gerrit Code Review over 5 years ago
- Status changed from Needs Feedback 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/c/Packages/TYPO3.CMS/+/59886
Updated by Gerrit Code Review over 5 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/c/Packages/TYPO3.CMS/+/59886
Updated by Gerrit Code Review over 5 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/c/Packages/TYPO3.CMS/+/59886
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/c/Packages/TYPO3.CMS/+/59886
Updated by Gerrit Code Review over 5 years ago
Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/59913
Updated by Gerrit Code Review over 5 years ago
Patch set 1 for branch TYPO3_8-7 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/59914
Updated by Oliver Hader over 5 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset 399a62a62a6853e72ef694d0ce7b4adb1fe89ba7.
Updated by