Surpress chmod warnings #47

peterrehm · 2017-09-21T15:38:01Z

This avoids the error chmod(): Operation not permitted when your current user has not created the file but is in the same group.

Ocramius · 2017-09-21T16:42:01Z

This requires testing though

peterrehm · 2017-09-21T16:47:19Z

I expected no other response, but when chmod was introduced with faf066e there was also no test?

How would you like it to be tested?

Ocramius · 2017-09-21T17:36:22Z

I would suggest an end-to-end test that does an explicit `chown` on a a different user.

…

On 21 Sep 2017 18:47, "Peter Rehm" ***@***.***> wrote: I expected no other response, but when chmod was introduced with faf066e <faf066e> there was also no test? How would you like it to be tested? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#47 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakJpQPveeEvoxqVqbaL74qPM2yU2hks5skpMZgaJpZM4Pfg3y> .

Ocramius · 2017-09-21T17:36:48Z

And yeah, my bad for not adding the test in first place, sorry 😥

…

On 21 Sep 2017 19:36, "Marco Pivetta" ***@***.***> wrote: I would suggest an end-to-end test that does an explicit `chown` on a a different user. On 21 Sep 2017 18:47, "Peter Rehm" ***@***.***> wrote: > I expected no other response, but when chmod was introduced with faf066e > <faf066e> > there was also no test? > > How would you like it to be tested? > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#47 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAJakJpQPveeEvoxqVqbaL74qPM2yU2hks5skpMZgaJpZM4Pfg3y> > . >

peterrehm · 2017-09-21T18:27:01Z

Hehe 😄

As this more or less reverts the previous change or at least does not throw an exception when the chmod breaks I think this could be accepted without the overhead of a full E2E test.

This is no functionality just the supression of errors.

What do you think?

Ocramius · 2017-09-21T19:29:42Z

No, sorry, I no longer accept patches without clear verification of the change: each of these changes can lead to further bugs, so I want to have my own free time backed by automated checks.

…

On 21 Sep 2017 20:27, "Peter Rehm" ***@***.***> wrote: Hehe 😄 As this more or less reverts the previous change or at least does not throw an exception when the chmod breaks I think this could be accepted without the overhead of a full E2E test. This is no functionality just the supression of errors. What do you think? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#47 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakEVaSG-8OBSJlKxACsYuLOJj9PJTks5skqp2gaJpZM4Pfg3y> .

peterrehm · 2017-09-21T20:02:50Z

Okay, then feel free to close the PR.

Ocramius · 2017-09-21T20:05:16Z

@peterrehm I'm fine with keeping it open until somebody else comes up with a valid test scenario ;-)

peterrehm · 2017-09-21T20:22:23Z

Btw. another solution would have been deleting the file prior to the chown.

peterrehm · 2017-09-21T20:29:52Z

And this is the reason for the issue: https://unix.stackexchange.com/a/125787

SenseException · 2017-09-21T21:32:08Z

src/PackageVersions/Installer.php

@@ -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);


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

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.

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

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?

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

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.

peterrehm · 2017-09-22T06:19:52Z

It has been fixed thiws way e.g. in Twig:

twigphp/Twig#705

Another option would be to remove the file prior to writing it, a user in the same group can do this and afterwards the file has the current user as the owner and a chmod is permitted.

Prior to introducing the chmod command I had no issues.

Ocramius · 2017-09-22T09:23:05Z

Yep Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

…

On Fri, Sep 22, 2017 at 11:22 AM, Claudio Zizza ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/PackageVersions/Installer.php <#47 (comment)> : > @@ -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); "Hard" as in \RuntimeException and a $io->write('<error>ocramius/package-versions: You ruined my life!!1!</error>') before the exception? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#47 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakJkGym2eJpHYcnbPU6Xxl-d7YE-Sks5sk3xGgaJpZM4Pfg3y> .

Ocramius · 2017-11-28T07:28:38Z

Yes, not every lib applied that patch, including composer. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

…

On Tue, Nov 28, 2017 at 8:15 AM, Sergei Morozov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/PackageVersions/Installer.php <#47 (comment)> : > @@ -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); 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. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#47 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakPsSSiBq3tySRBoVV7PAd2Gy8iCKks5s67MigaJpZM4Pfg3y> .

peterrehm · 2017-11-30T18:49:43Z

I am closing this as it looks like this wont be fixed.

Ocramius · 2017-11-30T18:58:52Z

@peterrehm thanks for the feedback anyway!

Surpress chmod warnings

faa751b

Ocramius added bug help wanted labels Sep 21, 2017

SenseException reviewed Sep 21, 2017

View reviewed changes

peterrehm closed this Nov 30, 2017

Ocramius assigned peterrehm Nov 30, 2017

Ocramius mentioned this pull request Dec 12, 2017

Ignore possible permission error during version class generation #54

Closed

Ocramius mentioned this pull request Mar 29, 2018

Change the chmod call to @chmod #63

Closed

peterrehm mentioned this pull request Jun 14, 2018

Delete class file prior to writing it to ensure permissions #66

Closed

Amunak mentioned this pull request Aug 14, 2018

Check whether the chmod call is needed before calling it #68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Surpress chmod warnings #47

Surpress chmod warnings #47

peterrehm commented Sep 21, 2017

Ocramius commented Sep 21, 2017

peterrehm commented Sep 21, 2017

Ocramius commented Sep 21, 2017 via email

Ocramius commented Sep 21, 2017 via email

peterrehm commented Sep 21, 2017

Ocramius commented Sep 21, 2017 via email

peterrehm commented Sep 21, 2017

Ocramius commented Sep 21, 2017

peterrehm commented Sep 21, 2017

peterrehm commented Sep 21, 2017

SenseException Sep 21, 2017

Ocramius Sep 21, 2017

SenseException Sep 22, 2017

morozov Nov 27, 2017 •

edited

Loading

Ocramius Nov 28, 2017

morozov Nov 28, 2017

morozov Nov 28, 2017

peterrehm commented Sep 22, 2017

Ocramius commented Sep 22, 2017 via email

Ocramius commented Nov 28, 2017 via email

peterrehm commented Nov 30, 2017

Ocramius commented Nov 30, 2017

Surpress chmod warnings #47

Surpress chmod warnings #47

Conversation

peterrehm commented Sep 21, 2017

Ocramius commented Sep 21, 2017

peterrehm commented Sep 21, 2017

Ocramius commented Sep 21, 2017 via email

Ocramius commented Sep 21, 2017 via email

peterrehm commented Sep 21, 2017

Ocramius commented Sep 21, 2017 via email

peterrehm commented Sep 21, 2017

Ocramius commented Sep 21, 2017

peterrehm commented Sep 21, 2017

peterrehm commented Sep 21, 2017

SenseException Sep 21, 2017

Choose a reason for hiding this comment

Ocramius Sep 21, 2017

Choose a reason for hiding this comment

SenseException Sep 22, 2017

Choose a reason for hiding this comment

morozov Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Ocramius Nov 28, 2017

Choose a reason for hiding this comment

morozov Nov 28, 2017

Choose a reason for hiding this comment

morozov Nov 28, 2017

Choose a reason for hiding this comment

peterrehm commented Sep 22, 2017

Ocramius commented Sep 22, 2017 via email

Ocramius commented Nov 28, 2017 via email

peterrehm commented Nov 30, 2017

Ocramius commented Nov 30, 2017

morozov Nov 27, 2017 •

edited

Loading