dont execute anything with active `sudo session`

[Flyingmana]

its common that an update of composer happens with sudo, as composer is installed in a place which is only changeable with higher privileges.

Its not uncommon, that an self-update is made directly before an install/updaten command. To avoid easy privilege escalation in case of an evil attack (like a worm spreading trough the composer ecosystem) we should add a check for an active sudo session, and block further execution if found.

Such malware could spread silently as sudo allows sudo -n true which allows for testing for an active session, without triggering the typical interactive password input.

[Seldaek]

If you really need sudo for the self-update, run it with sudo, but running the composer update or install with sudo sounds insane to me in the first place. I doubt many people really do this as it'd likely end up messing up a bunch of permissions in their project.

That is not to say I am against improving safety here, but I don't think it's very high prio.

[Flyingmana]

its not about running the install/update directly as sudo.

But if I did run something with sudo before, every programm afterwards can execute sudo commands as long as the session did not timeout.

example:

sudo composer self-update
composer install

during install now somehow an evil script gets executed. Because of the earlier sudo command, it now can run itself sudo evil-infection.sh without the user getting asked again for a password.

[curry684]

The case is rather real indeed because of the 5 minute standard lease. I made a PR that doesn't only solve this situation but also emits a general warning if running Composer as root.

Would appreciate your review @Flyingmana

[Flyingmana]

@curry684 your PR is good if you did run it directly with sudo, but the SUDO_UID is not set if not directly run with sudo, even if the session is still active. But sudo -K is a good find, it kills the session and you dont even need to run it as root. Actually your PR is wrong there, you kill the sudo session of the sudo user, not your own one.

flyingmana@Magneto:~$ sudo apt-get update
[sudo] password for flyingmana: 
flyingmana@Magneto:~$ env | grep sudo
flyingmana@Magneto:~$ php -r 'var_dump(null);'
NULL
flyingmana@Magneto:~$ php -r 'var_dump(getenv("SUDO_UID"));'
bool(false)
flyingmana@Magneto:~$ sudo -n true
flyingmana@Magneto:~$ sudo -K
flyingmana@Magneto:~$ sudo -n true
sudo: a password is required
flyingmana@Magneto:~$ sudo php -r 'var_dump(getenv("SUDO_UID"));'
[sudo] password for flyingmana: 
string(4) "1000"
flyingmana@Magneto:~$ sudo -n true
flyingmana@Magneto:~$ sudo sudo -K
flyingmana@Magneto:~$ sudo -n true
flyingmana@Magneto:~$ sudo -n true
flyingmana@Magneto:~$ sudo -K
flyingmana@Magneto:~$ sudo -n true

[curry684]

it kills the session and you dont even need to run it as root.

No, it can't be run as root. If you run it as root you clobber root's sudo credentials. That's why I need the env variable to switch back to the invoking user's context.

The SUDO_UID variable is indeed only set when inside a sudo session, but that's our only responsibility - clobbering the session when you used it for composer itself. When you still have a sudo lease from another run you should've just used the -k switch there. We can only detect an open lease by manually looking inside /var/lib/sudo and that's both hacky and out of our scope.

Feel free to validate using the test script I used for prototyping:

<?php

if (function_exists('posix_getuid') && posix_getuid() === 0) {
  echo "Running as root\n\n";
}
$uid = getenv('SUDO_UID');
exec("sudo -u \\#{$uid} sudo -K > /dev/null 2>&1");

Keep running sudo php test.php and it will keep asking for password.

[Flyingmana]

i see, but I created this issue mostly for an active sudo lease from an earlier run.

Yes, its valid to say this is not an issue, as people can just kill a sudo session after doing a sudo command.

But I have also the opinion, that composer is a special case for cli tools. Its used by, even build for people who have not much experience with using the cli in a secure way. And in the same time its a tool which mostly is used to execute un-reviewed stuff from the internet. I really think in this position its better to protect inexperienced users, even if they use things wrong.

[bamarni]

Is it really to Composer to deal with this? The vulnerability described here is valid for basically any other program.

We could instead tweak the documentation here. There is already a recommendation about the -H flag, we could add the -k flag to tackle this security concern, what do you think @Flyingmana?

[curry684]

Is it really to Composer to deal with this?

No, not with the outer sessions. I do think this is a very common scenario however, which may even be scripted or aliased on a lot of dev-machines as they usually have composer installed globally:

sudo composer self-update
composer install

All the PR does is ensure that, if the installer runs malicious plugins or post-install hooks that attempt to sudo rm / -rf --no-preserve-root, a password is requested again instead of silently killing your system. And it shows a fat warning that things like that may happen without a password prompt if you run the entire process as root.

All of Composer's responsibilities end beyond that in my opinion.

There is already a recommendation about the -H flag, we could add the -k flag to tackle this security concern

It doesn't work like that - -k clobbers existing credentials before running the command. So running sudo -Hk composer self-update still leaves an active lease for your current user, you have to run sudo -k afterwards to clobber. Which is essentially what the PR does for you.

i see, but I created this issue mostly for an active sudo lease from an earlier run.

I can add something for that actually.

[curry684]

Added a commit that always clobbers any existing sudo leases for the current user so exec("sudo rm -rf / --no-preserve-root"); can really only work when running as root itself.

[bamarni]

So running sudo -Hk composer self-update still leaves an active lease for your current user

Sounds fine to me, it's just to avoid Composer update command to initiate one, not to clear an existing one.

I suggested to update the documentation for this because hardcoding those things in composer are somehow aggressive. If I have some sudoers configs with a given timeout, I wouldn't expect any tool around to clear it as he wants. Also personally I'm using php through Docker, it runs as root so I'd be prompted with this warning all the time, will I don't think there is anything insecure in my dev setup because of this.

[curry684]

The Docker or in more general VM situation is a bit special like that. We could detect running in docker but that just seems kind of patchy for in general stating that you shouldn't run as root, not even in Docker.

[bamarni]

I don't want to drive this off-topic with Docker, but it's by default a very limited root user as most privileges have to be enabled explicitly. Also, on the dev VM they provide the underlying host is a lightweight Linux distribution of 20MB with basically only Docker installed on it.

Actually, I don't really see what we're trying to solve here. This issue is not even about vulnerable plugins but malicious ones. If such things exist on Packagist, we should ban them. If people are running this kind of plugin we should educate them more with better documentation. Trying to mitigate this by silently calling sudo -K on every composer command seems hacky to me. It wouldn't work when NOPASSWD is enabled and doesn't fix the root cause, malware can also be harmful when it runs as non-root.

[curry684]

If such things exist on Packagist, we should ban them.

That implies a vetting or verification process. There isn't one. Sure, Seldaek will pull any packages or plugins that contain malicious code, but not until after people have notified him of their dev machines (or worse: production servers) being wiped clean. Closing the sudo loophole is a hack, but in my opinion a sensible one - it's a single line of code that costs no measurable performance, has no drawbacks whatsoever, yet completely wipes out an entire attack vector, and one that is highly likely to exist in many real world usages of Composer.

The "you shouldn't be running as root" warning is something that's much more subjective as to whether it's needed or useful. I'd argue yes - the only valid reason to run Composer as root is to self-update a global install. The Docker scenario is also valid, but not valid enough methinks just to say "oh then running as root is suddenly a good idea for everyone".

Perhaps we should add another IGNORE env variable, perhaps we should detect running inside containers, perhaps we should have a global COMPOSER_F*CK_SECURITY env variable that disables this warning, and the secure-http requirements, and some other stuff in the code base. I don't know, those are valid points for discussion.

[curry684]

Trying to mitigate this by silently calling sudo -K on every composer command seems hacky to me.

That isn't the point btw. That is about never, ever, allowing any of our forked code getting access to a silent sudo lease. You could've done sudo apt-get upgrade 4 minutes ago, started your development duties of the day, and not think about the open lease while running composer update in another folder. At that point any loaded package or plugin can kill your system without a single prompt.

Yes, I consider that 'our' responsibility to help prevent, and that's what the sudo -K during startup does - prevent any of our own untrusted PHP code from using sudo without a prompt.

[bamarni]

I must agree with you that the Docker use case is an exception, and that in general composer commands shouldn't be run as root. If the PR is accepted, an env variable like COMPOSER_DISABLE_ROOT_WARN would be nice indeed :+1:

I'm still really sceptical about the main topic of this discussion. Why updating composer itself with sudo before running composer install would be a common use case, why is it needed? And if this is done in production it's most likely during a deployment script, which is a very bad practice imo as until the very recent #5064 it could only grab the latest commit of the development branch, of an alpha tool... People doing this for some time now would have most likely encounter deployment issues and change their mind about this practice.

it's a single line of code that costs no measurable performance, has no drawbacks whatsoever, yet completely wipes out an entire attack vector, and one that is highly likely to exist in many real world usages of Composer.

It doesn't completely wipes out an entire attack vector, again it doesn't work when NOPASSWD is set, and malware can be dangerous even run as a regular user. What is the point of the sudoers timestamp_timeout directive if any tool around decides to expire it? If you have the security concern described in this ticket (with until now no real example about such plugin malwares) you can set it to 0. If my deployment script is just a bash file doing sudo something; composer install; sudo service nginx reload, I should be able to continue running it with only one password prompt, and everyone is happy :smile:, or?

[curry684]

why is it needed? [...] very bad practice

Of course it's very bad practice, and of course it shouldn't be needed. You know that, I know that, many people know that. Composer is however a tool used by millions of programmers, and let's not be all too naive about all of them understanding security and the implications of doing dumb things on their computers. We could take the Darwinistic stance - let them accidentally wipe their machines oft enough and they will stop bothering us. Not something I'd advocate.

Why doesn't any modern OS installer allow you to set empty passwords for root accounts? Why doesn't rm / -rf work anymore without --no-preserve-root flag? Why do most well known network service applications blatantly refuse to run as root whatsoever? Because it makes sense for user friendly products to protect their users from making extremely grave mistakes with severe consequences. Whether we like it or not, many of Composer's users are doing stupid things with it. Let's try to care instead of laughing at the sideline.

It doesn't completely wipes out an entire attack vector

On an out of the box Linux install every possibility of unchecked root privilege escalations is gone.

it doesn't work when NOPASSWD is set

Someone smart enough to consciously set that knows to close his shell, use sudo -K himself, or not to run dangerous programs running unchecked third party code like Composer afterwards. Someone stupid enough to set that without understanding the consequences cannot be protected by anyone.

malware can be dangerous even run as a regular user

Having to retrieve a home directory from backups is nowhere on the same severity scale as rooting a production server.

What is the point of the sudoers timestamp_timeout directive if any tool around decides to expire it?

Not 'any tool', just one that runs unchecked third party code for a living.

with until now no real example about such plugin malwares

And we both know some scriptkiddie will read this thread one day and get his brilliant idea for his minute of worldwide fame and glory.

Please do note that I fundamentally agree with you about that this patch should not be necessary. I'm just sharing my regrettably vast experience that most of the self-proclaimed programmers out there know nothing about programming, and less about security and good system administration practices.

[bamarni]

Composer dependencies are not "third-party unchecked code", it's part of our project even though we don't maitain the code directly. As such goes through the same sanity checks, reviews, tests, ci and staging servers, I fail to see how pure malware can end up in production, even with no workflow at all you'd still have to run it once in dev at least before deploying, so, taking your example of "rm -rf /", how could we deploy something that just wiped out our dev machine?

Anyway, it's trying to get unconstructive, we both gave our point so I'll wait for @Seldaek and other opinions on this matter.

[curry684]

You are missing the part where Composer has a plugin architecture and support for running post-install and post-update scripts. If it were only downloading code that could ever be run within Apache or nginx's protected least-possible-rights security the problem indeed would not exist.

However, many people use the FXP Composer Asset Plugin for example. It is by the very nature of plugins allowed to execute code within Composer's process context.

Same goes for scripts - for example many Symfony projects use hooks to automatically clear caches, update Doctrine bindings and deploy assets automatically whenever composer update is run. While this makes sense, it again means third party code, not rarely right from dev-master, is automatically executed within Composer's process context.

Now just imagine if one the maintainers of those projects, or their many recursive dependencies, went rogue and just pushed malicious code for the hell of it. All it takes is a single autoload-files entry pointing to a oneliner PHP that wipes your machine - it'll always be loaded, no matter if referenced or not.

I'm all but defending my implementation. I think it's a good sane idea to prevent sudo lease abuse altogether. But we may also (or instead) want to disable plugins and hooks when running as root. I think that would be overdoing it but it would definitely make sense from a security purist perspective.

[bamarni]

@Seldaek : would you still accept the environment variable for the warning as discussed here?

@curry684 : sorry for not replying, I just didn't want to turn this into an endless conversation before even hearing other opinions. Unfortunately didn't happen.

[Seldaek]

Yeah sorry I kinda TLDR'd this whole thread :p Anyway yes it sounds reasonable.. I also removed the warning when running self-update as that's a reasonable use case for sudo.

See https://github.com/composer/composer/commit/cb8587cdade3cdbe00b8d6d2488458a2b6c0a592

[curry684]

Hmmm came in just too late for my cents on that part hehe.

Given that you built it so it also disables clobbering (which makes sense) wouldn't it be better to call it COMPOSER_ALLOW_ROOT or something? DISABLE_WARN now doesn't match what it actually does.

[Seldaek]

At least I documented it ;) But yeah it's kinda ALLOW_SUDO as well as allowing root though? Naming is hard. Discuss.

[curry684]

As sudo implies becoming root I consider them identical.

[bamarni]

Thanks @Seldaek for the quick move, seems like a fair compromise to me. Moral of the story, think about it twice before arguing with @curry684 :smiley:

[Seldaek]

Renamed to COMPOSER_ALLOW_SUPERUSER