Support symlinks while not allowing malicious template paths.

[colinmollenhour]

There is too much history to go into behind this ticket but as requested at colinmollenhour/modman#125 here is a PR that includes what I think is the best fix for the "Allow Symlinks" issue. This prevents use of ".." in the template paths and the setScriptPath but does not prevent use of symlinks outright. This is far more secure than enabling "Allow Symlinks" option since it still provides protection against inclusion of malicious files outside of the Magento base "design" directory. As far as I can tell it is just as secure as disabling "Allow Symlinks" option so since it doesn't suffer the downsides I don't see any need for an "option".

Please test this with some of your deployments. It works for mine but I haven't tried it with a wider range of third party extensions. Also if anyone is privy to the vulnerability implementation details and can confirm that this circumvents them that would be great.

[LeeSaferite]

I haven't tested this code so I apologize if this is a stupid question.

Do these changes break using something like modman to keep the external modules outside of the core code directory? Something like /modules/OpenMage_Stuff and /magento where the module is installed using symlinks under /magento. In that setup the docroot would be /magento

[colinmollenhour]

It works with the files outside the Magento root because by default the _viewDir property is set to Mage::getBaseDir('design') to begin with (in renderView just before it is rendered) so this comparison is true. If you use setScriptPath to set a different view directory (I've never once seen a need for this and actually would be all for removing this method or making it private) then the script path has to be within the base design path. If you use '..' in your template paths then it will not work, but then neither would "Allow Symlinks = No" before even if you weren't using symlinks. The important thing is this will be prevented:

{{block type="core/template" template="../../../../some/path/to/malicious.php"}}

But you could create a symlink and reference it as usual with this patch. If an attacker has the capability of writing a symlink they would also be able to write a regular file as well. For example, the "symlink" check could be bypassed by just writing a file that has <?php include __DIR__.'/../../../../some/path/to/malicious.php'; in almost the same number of bytes. This is why it is best to only allow the webserver to write to media and var and all other files/paths should only be readable by the web server.

[Flyingmana]

As far as I can tell it is just as secure as disabling "Allow Symlinks" option so since it doesn't suffer the downsides I don't see any need for an "option".

Iam against merging this without having at least one security expert reviewing this.

[colinmollenhour]

@Flyingmana Please ask a security expert to review. :)

[pocallaghan]

I certainly don't consider myself an expert, but I approve @colinmollenhour's code here. We've been running essentially this code in production for clients for a long time now, mostly because up until the release of SUPEE-10570, it was still possible (through a bypass vulnerability) to re-enable the feature from the admin panel, meaning there was no protection from arbitrary includes in core Magento (given enough priveledges to update config of course).

In my mind, the logic behind this code makes more sense than the original feature ever did, because it allows for symlinks to work inside the directory, but not allow arbitrary inclusion of any file path. As Colin mentioned, if somebody can write the symlink to include a malicious external file, they could likely have written the malicious file directly anyway.

If there is a flaw in this implementation, I've been unable to see it.

[Flyingmana]

If there is a flaw in this implementation, I've been unable to see it.

I keep repeating myself here: all of the attack vectors have not been seen by anyone here before.

We've been running essentially this code in production for clients for a long time now

this does not matter the sightliest for security vulnerabilities. There are some, which survive more then ten years.

[pocallaghan]

this does not matter the sightliest for security vulnerabilities. There are some, which survive more then ten years.

I'm well aware, I've reported a significant number of the ones patched by Magento. But if my opinions on the code don't matter in the slightest, I'll keep them to myself.

[colinmollenhour]

Thanks for the review, @pocallaghan. I've seen your name on many of the vulnerability discoveries for bugs which existed for years and nobody else found so thanks for your continued contributions to Magento's security as well. I'm very happy to see you have some interest in this project, because if you're not a Magento security expert I don't know who is. :)

@Flyingmana wrote:

I keep repeating myself here: all of the attack vectors have not been seen by anyone here before.

When someone who has a proven track record of finding security vulnerabilities looks at an alternative solution for a well known vulnerability speficially in the context of examining security and says they don't see any flaws I think this means a lot more than you're giving credit for. This logic could otherwise be applied to every patch made on this project and so we should not accept any patches since every patch could introduce some new vulnerability.

So, what exactly is your definition of a security expert and how can we determine which code is safe to update and which code is not?

[Flyingmana]

you are right @colinmollenhour , This is a proven track record of finding security vulnerabilities and totally counts.

@pocallaghan
I need to apologize and hope for forgiveness, as I usually dont check the background of participants.
Could you then also add your approval to the PullRequest?

[seansan]

+1 glad you got it sorted And for all contributions!

…

On Thu, 15 Mar 2018 at 17:12, Daniel Fahlke ***@***.***> wrote: you are right @colinmollenhour <https://github.com/colinmollenhour> , This is a proven track record of finding security vulnerabilities and totally counts. @pocallaghan <https://github.com/pocallaghan> I need to apologize and hope for forgiveness, as I usually dont check the background of participants. Could you then also add your approval to the PullRequest? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#442 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAn0a79At6jGbBEdp-HeNN6oc5euVS5qks5tepLvgaJpZM4R0Kv4> .

[pocallaghan]

@Flyingmana no apologies necessarily. As I said, I don't consider myself an expert, but I'm happy to offer my opinions.

[tmotyl]

looks good

[idziakjakub]

I found that there is no way to load non inline CSS files in transactional emails when I'm using symlinks.

magento-lts/app/code/core/Mage/Core/Model/Email/Template/Abstract.php

Lines 238 to 247 in 40720ca

	$filePath = realpath($filePath);
	$positionSkinDirectory = strpos($filePath, Mage::getBaseDir('skin'));
	$validator = new Zend_Validate_File_Extension('css');
	if ($validator->isValid($filePath) && $positionSkinDirectory !== false && is_readable($filePath)) {
	return (string) file_get_contents($filePath);
	}
	// If file can't be found, return empty string
	return '';

magento-lts/app/code/core/Mage/Core/Model/Email/Template/Abstract.php

Line 239 in 40720ca

$positionSkinDirectory = strpos($filePath, Mage::getBaseDir('skin'));

I think "$positionSkinDirectory" should be refactored too.

[colinmollenhour]

LGTM. Can we get a couple reviewers?

I just added an exception to be thrown for all fallback-based file paths if they contain '..' which should fix the issue observed by @idziakjakub while also reducing the chances of there being new or unknown vulnerabilities.

[ollyboy]

Would be great to see this completed. Currently, the settings ( below ) mess up our installation as we use composer to install and manage all modules. Setting this to "0" breaks lots of templates in Amasty Nav modules and other modules

The way we get around this is to use this command in our go-live scripts

n98-magerun config:set dev/template/allow_symlink 1

[colinmollenhour]

Any objections to merging this? We're operating off of a fork right now but it would be nice to get back to the mainline. Setting allow_symlink to 0 disables the security vulnerability patch and this issue removes the flag in favor of fixing the vulnerability in a way that doesn't need to be disabled.

[Flyingmana]

in case someone needs a module to check symlink functionality, I found one which requires them in their install inscructions.
https://github.com/wirecard/magento-ee/wiki/Installation

Did not look in detail into it myself.

The base branch was changed.

[colinmollenhour]

I changed based from 1.9.3.x to 1.9.4.x so all reviews are stale.. I would like to merge this pretty soon if there are no objections so please speak now or forever hold your peace. :)

[colinmollenhour]

Rebased on latest. Removed symlink notification since it is no longer relevant.

Proof that this fixes a security vulnerability when allow_symlink is enabled:

• Created PHP file at var/malicious.php with contents <?php die('pwned');
• Allowed symlinks by adding <default><dev><template><allow_symlink>1</allow_symlink></template></dev></default> to app/etc/local.xml
• Created CMS page with block references to malicious file:
  
• Verified malicious file is loaded:
  
• Checked out support-symlinks branch
• Verified malicious file is no longer loaded: