- Issue created by @phenaproxima
- πΊπΈUnited States phenaproxima Massachusetts
After pondering the vendor hardening plugin, I think what might make the most sense is...to special-case
composer/composer
. We should still remove its binaries, so thatvendor/bin/composer
doesn't exist...butvendor/composer/composer/bin/composer
should keep existing, and have its execute bit removed.In other words, Composer's linked binaries won't exist anymore, but its original binaries will, without their execute bits.
I think that's the right balance of making Package Manager easier to use and not having to do an extensive refactor of the entire vendor hardening plugin and its configuration.
- πΊπΈUnited States phenaproxima Massachusetts
Discussed the above with @catch and Slack and the feeling seems to be that I should go ahead and create that MR, then let the security team and release managers consider it.
- πΊπΈUnited States phenaproxima Massachusetts
Tagging for security review to confirm that this solution doesn't introduce any security flaws, and release manager review to confirm that this change is non-disruptive and, hopefully, eligible for backport to 11.2.3.
Also tagging to point out that Drupal CMS needs this.
The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- πΊπΈUnited States phenaproxima Massachusetts
The bot is wrong and stupid.
The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- π¬π§United Kingdom longwave UK
Is there a case for making this configurable, at least with an on/off flag, for sites that will never use automatic updates and want to retain the existing behaviour?
- π¬π§United Kingdom longwave UK
Also the bot doing this has a point:
Checking core/tests/Drupal/Tests/Composer/fixtures/fake-composer/bin/composer check failed: file core/tests/Drupal/Tests/Composer/fixtures/fake-composer/bin/composer should not be executable
The core commit check script ensures all files are not executable, so we need an exception or some other way to set this up.
- πΊπΈUnited States phenaproxima Massachusetts
The core commit check script ensures all files are not executable, so we need an exception or some other way to set this up.
Okay, refactored the test a bit in the hopes that it will work on CI now.
- πΊπΈUnited States phenaproxima Massachusetts
Is there a case for making this configurable, at least with an on/off flag, for sites that will never use automatic updates and want to retain the existing behaviour?
I think this is already configurable (by putting
composer/composer: ['bin']
into theextra.drupal-vendor-hardening
section of composer.json), but I should add a test to prove it. - πΊπΈUnited States xjm
I confirmed with @phenaproxima that in HEAD, the binaries are removed entirely, so the change proposed by this issue is adding an internal configuration to make the composer binaries retained and read-only at the given path.
Composer is only a dev dependency for core, not a production one, so this mainly becomes important for sites that have Package Manager enabled (i.e. Composer is a production dependency). If this passes security review, then it's essentially a feature addition of a single internal constant and the binaries.
However, other sites could be affected depending on the sophistication of their deployment workflows. Ideally sites' production builds will be deployed with
--no-dev
, but we can't be sure this is the case everywhere, so it's a bit dangerous to suddenly have these binaries popping up everywhere in unpredictable situations.I asked @phenaproxima to consider instead making this an optional configuration that is off by default, but that Drupal CMS (and perhaps later Package Manager itself) enables. That way, it could be backported safely with very little API surface or risk to existing sites, but Drupal CMS could also get access to the configuration immediately. This would also be a security hardening: sites or applications would need to opt into making the binaries available, and they would remain unavailable in the default case for deployments where they are not needed.
This will also need a CR and a release note.
Thanks!
- πΊπΈUnited States phenaproxima Massachusetts
Submitted for your consideration: a way that keeps the status quo very much intact, but gives Drupal CMS an out.
Right now, there is no way to stop a package from being cleaned. The way the plugin is written utterly precludes that. This MR provides such a way: set a package's cleanup paths to
false
:"extra": { "drupal-core-vendor-hardening": { "composer/composer": false } }
If a project sets this up, then
composer/composer
will not be touched. This leaves the default configuration unchanged -- the package will be cleaned as it already is. The test confirms both scenarios.In other words, existing sites will continue working exactly as they do now with no configuration change required. Drupal CMS, however, will be able to keep Composer's binary and will be responsible for taking whatever additional steps are necessary to make it read-only.
This is the most minimal change I could come up with that should (I hope) be eligible for backport, without changing what will happen for new or existing sites.
- πΊπΈUnited States phenaproxima Massachusetts
Change record drafted: https://www.drupal.org/node/3536166 β
- π¬π§United Kingdom longwave UK
From a security point of view I think this is fine, given it's new behaviour that is explicitly opt-in, so existing sites don't have to change anything. Site owners can do slightly more dangerous things with this feature but that's up to them to configure it and it's not up to us to prevent that.
I don't see any release management issues either given there is little disruption here. The only possible minor disruption is the addition of the File constraint which there is a chance is already used in the wild somewhere, though given this just uses the Symfony constraint it is likely they have made the same implementation anyway. We should add a change record for this just in case.
We should also document this new capability in
composer/Plugin/VendorHardening/README.txt
, marking NW for that and the CR.Also removing the bot tag as I think the bot was correct earlier when we had that executable file in the patch, that is gone so the bot should be happy.
- πΊπΈUnited States phenaproxima Massachusetts
Documentation and change record added.
- π¬π§United Kingdom longwave UK
No further comments, this is as small a change as can be to get this feature in. Marking RTBC for another committer to double check everything.
- πΊπΈUnited States phenaproxima Massachusetts
Committers, please backport this to 11.2.x. π
Doing so will allow this security improvement to come to existing Drupal CMS projects, and it will greatly help new Drupal CMS users get set up with Package Manager (without needing to wait until 11.3.0). The change set is as small as possible to facilitate a clean backport, and I have been explicit about this request since I started talking about it with the release managers. :)
- πΊπΈUnited States xjm
When this is committed, remember to tag the issue for the release notes of the correct 11.2.x patch release (which is 11.2.3 as of now).
- π¬π§United Kingdom catch
I think this looks good and it will help Drupal CMS site a lot.
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!