Bug #87733

unwanted side-effect in fileDenyPattern

Added by Stefan Frank 3 months ago. Updated 19 days ago.

Status:
Closed
Priority:
Must have
Assignee:
-
Category:
Security
Target version:
-
Start date:
2019-02-18
Due date:
% Done:

100%

TYPO3 Version:
8
PHP Version:
Tags:
Complexity:
medium
Is Regression:
No
Sprint Focus:

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


Related issues

Related to TYPO3 Core - Bug #24221: t3lib_TSparser::checkIncludeLines() does not check files to be included Closed 2010-11-28

Associated revisions

Revision 399a62a6 (diff)
Added by Oliver Hader 3 months ago

[BUGFIX] Reduce strictness for .pl files in fileDenyPattern

Files like "file.pl.txt" cannot be uploaded anymore since ".pl" is
considered as executable Perl file. In multilingual scenarios "pl"
is used as reference for Polish content. Since required modules
mod_perl or mod_cgid are not enabled by default, and limited to
be executable only when invoked in location "/cgi-bin/", now only
files ending with ".pl" (e.g. "file.pl") are denied.

Resolves: #87733
Releases: master, 9.5, 8.7
Change-Id: Ib9a69fd3ec04f51653857d2f7309e30b78932653
Reviewed-on: https://review.typo3.org/c/59886
Tested-by: TYPO3com <>
Tested-by: Anja Leichsenring <>
Tested-by: Susanne Moog <>
Tested-by: Markus Klein <>
Reviewed-by: Wolfgang Klinger <>
Reviewed-by: Anja Leichsenring <>
Reviewed-by: Susanne Moog <>
Reviewed-by: Markus Klein <>

Revision 8a63c209 (diff)
Added by Oliver Hader 3 months ago

[BUGFIX] Reduce strictness for .pl files in fileDenyPattern

Files like "file.pl.txt" cannot be uploaded anymore since ".pl" is
considered as executable Perl file. In multilingual scenarios "pl"
is used as reference for Polish content. Since required modules
mod_perl or mod_cgid are not enabled by default, and limited to
be executable only when invoked in location "/cgi-bin/", now only
files ending with ".pl" (e.g. "file.pl") are denied.

Resolves: #87733
Releases: master, 9.5, 8.7
Change-Id: Ib9a69fd3ec04f51653857d2f7309e30b78932653
Reviewed-on: https://review.typo3.org/c/59913
Tested-by: TYPO3com <>
Tested-by: Markus Klein <>
Reviewed-by: Markus Klein <>

Revision 0ae09f7d (diff)
Added by Oliver Hader 3 months ago

[BUGFIX] Reduce strictness for .pl files in fileDenyPattern

Files like "file.pl.txt" cannot be uploaded anymore since ".pl" is
considered as executable Perl file. In multilingual scenarios "pl"
is used as reference for Polish content. Since required modules
mod_perl or mod_cgid are not enabled by default, and limited to
be executable only when invoked in location "/cgi-bin/", now only
files ending with ".pl" (e.g. "file.pl") are denied.

Resolves: #87733
Releases: master, 9.5, 8.7
Change-Id: Ib9a69fd3ec04f51653857d2f7309e30b78932653
Reviewed-on: https://review.typo3.org/c/59914
Tested-by: TYPO3com <>
Tested-by: Markus Klein <>
Reviewed-by: Markus Klein <>

History

#1 Updated by Markus Klein 3 months 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.....

#2 Updated by Markus Klein 3 months ago

  • Status changed from New to Needs Feedback

#3 Updated by Stefan Frank 3 months 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.

#4 Updated by Markus Klein 3 months 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

#5 Updated by Stefan Frank 3 months ago

Markus Klein wrote:

Where do you use this setup.pl.typoscript file? 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

#6 Updated by Markus Klein 3 months ago

Thanks for the info. Will take a look and will discuss, how to solve this problem properly.

#7 Updated by Markus Klein 3 months 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

#9 Updated by Oliver Hader 3 months 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

#10 Updated by Oliver Hader 3 months 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:

These are the aspects that should be tackled, replaced and hardened further.

#11 Updated by Oliver Hader 3 months ago

  • Is Regression changed from Yes to No

#12 Updated by Oliver Hader 3 months ago

  • Status changed from Accepted to Needs Feedback
  • Priority changed from Must have to Should have

#13 Updated by Oliver Hader 3 months ago

  • Target version deleted (Candidate for patchlevel)

#14 Updated by Oliver Hader 3 months ago

  • Related to Bug #24221: t3lib_TSparser::checkIncludeLines() does not check files to be included added

#15 Updated by Markus Klein 3 months ago

  • Assignee deleted (Markus Klein)

#16 Updated by Wolfgang Klinger 3 months 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.

#17 Updated by Gerrit Code Review 3 months 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

#18 Updated by Gerrit Code Review 3 months 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

#19 Updated by Gerrit Code Review 3 months 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

#20 Updated by Gerrit Code Review 3 months 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

#21 Updated by Gerrit Code Review 3 months 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

#22 Updated by Gerrit Code Review 3 months 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

#23 Updated by Oliver Hader 3 months ago

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

#24 Updated by Benni Mack 19 days ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF