Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surpress chmod warnings #47

Closed
wants to merge 1 commit into from
Closed

Surpress chmod warnings #47

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/PackageVersions/Installer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ private static function writeVersionClassToFile(string $versionClassSource, Comp
$io->write('<info>ocramius/package-versions:</info> Generating version class...');

file_put_contents($installPath, $versionClassSource);
chmod($installPath, 0664);
@chmod($installPath, 0664);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really be silenced without an error message? It doesn't feel right.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the test would find out. IMO, since this is a CLI script modifying the data, it should probably not be silenced, but rather crash hard.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Hard" as in \RuntimeException and a $io->write('<error>ocramius/package-versions: You ruined my life!!1!</error>') before the exception?

Copy link

@morozov morozov Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't file_put_contents() just rely on what's defined by umask? Like:

↪ umask 
0002

↪ php -r 'file_put_contents("foo", "bar");'

↪ ls -l foo
-rw-rw-r-- 1 morozov morozov 3 Nov 27 12:51 foo

Is there a reason why the installer wants to enforce particular permissions in the first place?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://www.doctrine-project.org/2015/08/31/security_misconfiguration_vulnerability_in_various_doctrine_projects.html

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I set umask 0 and run composer install, then the resulting files look like this:

↪  ls -l vendor/ocramius/package-versions/src/PackageVersions/
total 20
-rw-rw-rw- 1 morozov morozov 2582 Sep  6 08:24 FallbackVersions.php
-rw-rw-rw- 1 morozov morozov 5468 Sep  6 08:24 Installer.php
-rw-rw-r-- 1 morozov morozov 4558 Nov 27 23:06 Versions.php

The auto-generated file is indeed not writable by others but the rest of the files are. The attacker still can write code into other files.


$io->write('<info>ocramius/package-versions:</info> ...done generating version class');
}
Expand Down