Skip to content

Commit

Permalink
[DCOM-293] Fix security misconfiguration vulnerability allowing local…
Browse files Browse the repository at this point in the history 
… remote arbitrary code execution.
  • Loading branch information
@beberlei
beberlei committed Aug 31, 2015
1 parent 0cf7e0e commit caf30b8
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 6 deletions.
2 changes: 1 addition & 1 deletion lib/Doctrine/ORM/Tools/Console/Command/ConvertMappingCommand.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ protected function execute(InputInterface $input, OutputInterface $output)

// Process destination directory
if ( ! is_dir($destPath = $input->getArgument('dest-path'))) {
mkdir($destPath, 0777, true);
mkdir($destPath, 0775, true);
}
$destPath = realpath($destPath);

Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ORM/Tools/Console/Command/GenerateProxiesCommand.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
}

if ( ! is_dir($destPath)) {
mkdir($destPath, 0777, true);
mkdir($destPath, 0775, true);
}

$destPath = realpath($destPath);
Expand Down
3 changes: 2 additions & 1 deletion lib/Doctrine/ORM/Tools/EntityGenerator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ public function writeEntityClass(ClassMetadataInfo $metadata, $outputDirectory)
$dir = dirname($path);

if ( ! is_dir($dir)) {
mkdir($dir, 0777, true);
mkdir($dir, 0775, true);
}

$this->isNew = !file_exists($path) || (file_exists($path) && $this->regenerateEntityIfExists);
Expand All @@ -365,6 +365,7 @@ public function writeEntityClass(ClassMetadataInfo $metadata, $outputDirectory)
} elseif ( ! $this->isNew && $this->updateEntityIfExists) {
file_put_contents($path, $this->generateUpdatedEntityClass($metadata, $path));
}
chmod($path, 0664);

This comment has been minimized.

Sign in to view

Copy link
@mpdude

mpdude Sep 18, 2015

Contributor

I am not sure whether these chmod() calls should be silenced (see for example doctrine/common#383).

The problem of failing chmod() calls is especially present in Vagrant setups using NFS to share files but seems only to be an issue if the user owning the file ("vagrant" most probably) is not the user running the code ("www-data", for example).

I think we're fine here if this is code that is run by a console user who is also the file owner, but silencing the chmod() might do no harm otoh.

@beberlei What do you think?

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Sep 19, 2015

Member

@mpdude alternatively: is there any issue with having them non-silenced, and therefore making the user aware that there is a cache permissions issue?

This comment has been minimized.

Sign in to view

Copy link
@mpdude

mpdude Sep 19, 2015

Contributor

Marco,

I'd have to make some additional tests to double-check, but if memory serves me right the problem is that when you are for example using Vagrant and the cache directory is exported/mounted over NFS, then the chmod() call may fail. This may even be the case when the permissions are already set to what you're calling for, so it is not necessarily a cache permission issue.

We've been able to work around this in Vagrant by using bindfs and special parameters (ignoring chmod calls, making them always succeed + "mirroring" user permissions), but for less experienced users out of the box setups, this may be a hurdle to take.

I understand that this chmod() call has just recently been added to work around a security issue when the application is running with a umask of 0 (correct?). I understand that you want to make users aware of the potential risk in that situation; so maybe we could conditionally (for umask != 0) silence the chmod?

(Note to self: Follow up on https://github.com/doctrine/cache/pull/93/files#r39917935 later).

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Sep 19, 2015

Member

the problem is that when you are for example using Vagrant and the cache directory is exported/mounted over NFS, then the chmod() call may fail.

Agreed, but in those cases, isn't it better to run apache and vagrant under the same group? (that's what I do, at least).

but for less experienced users out of the box setups, this may be a hurdle to take.

It may be, but sadly the patch isn't aimed at making this friendly. This sort of failure may lead to following quite bad scenarios for more advanced users as well:

  • failed cache warmup in a deployment process

  • pushing caches with wrong rights in a deployment process

    to work around a security issue when the application is running with a umask of 0 (correct?).

Pretty much, yes. The chmod() call is executed right after creating the file, in order to avoid having any malicious interaction from the outside

I understand that you want to make users aware of the potential risk in that situation; so maybe we could conditionally (for umask != 0) silence the chmod?

That... may be a solution, but then we're adding yet another level of complication.

I don't think there is anything wrong with vagrant and apache clashing for chmod() rights in a local environment: this actually makes users aware of the problem, and makes them look for it. Maybe it is a documentation issue instead?

This comment has been minimized.

Sign in to view

Copy link
@mpdude

mpdude Sep 19, 2015

Contributor

Still no good idea how to tackle this, but meanwhile did a little more research:

This comment has been minimized.

Sign in to view

Copy link
@mpdude

mpdude Oct 1, 2015

Contributor

ping @Ocramius and @beberlei

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Oct 1, 2015

Member

I would suggest (for now) to simply rely on vagrant and www-data to be in the same group.

Otherwise, since the problem is indeed team productivity (and can be worked around) then you should ask your co-workers (you are having the issue there, right now) and see which one is the most secure solution.

Another thing that must be clear before proceeding is whether these warnings are currently blocking your work or if they are just noise.

This comment has been minimized.

Sign in to view

Copy link
@mpdude

mpdude Oct 1, 2015

Contributor
  • In the context of using the Symfony, the warnings will lead to "exception pages"; that is, they are show-stoppers.
  • Yes, I can work around this on my side.
  • Others are reporting the same problem, for example in doctrine/common#381.

I have seen many other places (for example in Symfony) where they are just silencing the chmod. Maybe this is good for DX, maybe it's bad for security. I think you guys need to make a decision :).

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Oct 1, 2015

Member

The point is that if it's bad for security (or leads to wrong chmodded files) then this is a no-go. DX has nothing to do with this sort of problem

This comment has been minimized.

Sign in to view

Copy link
@mpdude

mpdude Oct 1, 2015

Contributor

What about changing the umask to make sure "others" don't have write permissions before creating the file and then changing it back again?

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Oct 1, 2015

Member

before creating the file

O_o

This comment has been minimized.

Sign in to view

Copy link
@mpdude

mpdude Oct 1, 2015

Contributor

Was that a dumb suggestion?

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Oct 1, 2015

Member

@mpdude well, how can you chmod something not existing?

This comment has been minimized.

Sign in to view

Copy link
@mpdude

mpdude Oct 1, 2015

Contributor

Not chmod, change the umask and reset it back afterwards

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Oct 1, 2015

Member

Not possible, as umask() is thread-unsafe

This comment has been minimized.

Sign in to view

Copy link
@mpdude

mpdude Dec 3, 2015

Contributor

@Ocramius as you just merged doctrine/common#383, is this something you want to address as well?
}

/**
Expand Down
3 changes: 2 additions & 1 deletion lib/Doctrine/ORM/Tools/EntityRepositoryGenerator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,12 @@ public function writeEntityRepositoryClass($fullClassName, $outputDirectory)
$dir = dirname($path);

if ( ! is_dir($dir)) {
mkdir($dir, 0777, true);
mkdir($dir, 0775, true);
}

if ( ! file_exists($path)) {
file_put_contents($path, $code);
chmod($path, 0664);
}
}
}
5 changes: 3 additions & 2 deletions lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public function setOutputDir($dir)
public function export()
{
if ( ! is_dir($this->_outputDir)) {
mkdir($this->_outputDir, 0777, true);
mkdir($this->_outputDir, 0775, true);
}

foreach ($this->_metadata as $metadata) {
Expand All @@ -139,12 +139,13 @@ public function export()
$path = $this->_generateOutputPath($metadata);
$dir = dirname($path);
if ( ! is_dir($dir)) {
mkdir($dir, 0777, true);
mkdir($dir, 0775, true);
}
if (file_exists($path) && !$this->_overwriteExistingFiles) {
throw ExportException::attemptOverwriteExistingFile($path);
}
file_put_contents($path, $output);
chmod($path, 0664);
}
}
}
Expand Down

0 comments on commit caf30b8

Please sign in to comment.