Patches from drupal.org merge request URLs are dangerous?

[JohnAlbin]

composer-patches allows developers to specify patches to download and use via HTTP/S, which is a super useful feature that we use all the time!

This is totally safe to do when the resource at the end of that URL is static (unchanging). For example, a URL to an actual .patch file on a website is safe to use as long as the website owner is not malicious.

The new drupal.org merge request URLs are not safe, however. For example: https://git.drupalcode.org/project/drupal/-/merge_requests/121.diff

Anyone with commit access to Drupal issues can change the code on that the URL above references. That’s a malicious code injection vector.

The docs on drupal.org describing how to create a patch file from a merge request specifically mention that the URL is static, but the code is not:

WARNING: The URL for a Merge Request patch file will be named after the MR and will not change as more and more commits are being added in the MR. The contents of the patch file will change however.

However, those docs assume that a developer would review the patch before applying it manually. They don't take into this feature of composer-patches that allows you to automatically apply whatever patch is at the end point.

Anyone with commit access to Drupal issues can change the code on that the URL above references.

It wouldn't be that hard for a malicious user to pretend to be an open-source developer. If they knew a particular website was using a merge request URL for production builds, they could target that site. Or, theoretically, they could insert some phone-home code that exploits and then contacts the malicious user afterwards.

Obviously, I haven't tried this yet. Maybe I'm missing something and it's not even theoretically possible. But I'd love to see some eyeballs on this to make sure a scenario like this isn't possible.

[cweagans]

I think you're right -- this probably warrants at least a warning for people using that kind of URL. It may also be a good idea to see if Gitlab has some way to pin a merge request URL to a specific commit or something (so that you can say I've reviewed the MR patch at this specific point in time and that's what I'm going to apply to my codebase). That seems like a really specific feature that Gitlab may not have thought to implement though.

Longer term, this plugin will start storing SHA-256 hashes of all patches in the lock file (if not manually specified, then the file will be hashed when the patch is first applied), so if something changes on the server side that changes that patch contents, there will be an error.

[jonathanjfshaw]

Regardless of the security aspect, this dynamic aspect of gitlab MR patches also breaks the reliability of deployments. Every time you redeploy a site and redownload the patches, you may get different code that is now untested in you project and may not even merge properly.

AFAICT Gitlab does not provide a url that allows you to access the diff of a particular commit in a way we can use, and the Drupal.org team's attitude seems to be "you should be downloading the patches and commiting them anyway, instead of relying on our servers at deployment time".

Can we improve the documentation on how to reference a local patch? The readme examples here all use urls, and I've not managed to get local patches working. See e.g. #315

[phil-s]

Even if one can obtain a GitLab URL for diff between specific commits, that's not useful unless GitLab is permanently maintaining the refs for all the commits ever pushed to a branch, even if they've been replaced, as otherwise git's garbage-collection will delete those commits at some point (typically after 30 days), at which point the URL would stop working.

Patch files with permanent URLs are more useful; and if the various attempts to allow checksums were to reach fruition there would be a way to validate their contents.

• Add checksum to patch definition #19
• Adds optional support for SHA1 checksums for patches #24
• Validate Patch Checksum #163

[mxr576]

If you work for enterprise, it is also a regular security requirement that you only install external libs from signed sources. By using Packagist and Composer you get that. But pulling an unpinned patch state from an external server breaks that requirement 2 times at least - also creates a snowflake without warranty, but that is an other topic.
As a workaround we started to download patches on all projects, store them in VCS and apply them from local fs. The problem is that you cannot do that if you are maintaining an upstream project/distro/install profile, whatever, you a can only use remote references. We have an idea how to combine composer patches with other tools to serve patches from trusted sources, but an OOTB support for this would be better. (or if we would not need to maintain a software with 10+ patches , but that is an another topic 😁)

This issue also explains what I did not want to disclose here, how patching can be a backdoor: https://twitter.com/IEMIXER/status/1435247235613306882?t=Rg5K-iFv37pU7H-VoSuL3A&s=19

[greggles]

Did this:

Longer term, this plugin will start storing SHA-256 hashes of all patches in the lock file (if not manually specified, then the file will be hashed when the patch is first applied), so if something changes on the server side that changes that patch contents, there will be an error.

Ever happen?

[greggles]

Answering my own question: the work on hashes is linked in Phil-s' comment and is still open/in progress.

[ceesgeene]

I've posted a new PR: #388

[mxr576]

First of all, I am thrilled that there is some movement on this topic 🎉 How Composer Patches could be leveraged in supply chain attacks was one of those problems that concerned me the most in the past year(s), but my focus was always shifted so I could not work on these problems. Also, @cweagans made it quite clear on Twitter that no matter how much the PHP community (not just Drupal developers) depends on this package, it is minimally maintained, and there is no problem with that. Although, that made me realize that a community effort and support are needed to provide at least the minimum necessary care to this valuable library. Something similar that was formulated around https://github.com/php-tuf/php-tuf.

Maybe that will happen now, and I would be glad if I could be part of that effort.

[mxr576]

Post a long review on the solution implemented in #388 which may contains ideas how this can be solved differently, so I am leaving a reference here: #388 (comment)

[cweagans]

main now caches patches locally. I still need to add a warning and maybe some kind of automated "download all of my PR/MR-based patches, store them in a patches dir, and change my composer.json/patches.json accordingly" functionality. I've made a note to do that.

[lhridley]

Interesting approach: https://github.com/vaimo/composer-patches

Use: https://github.com/vaimo/composer-patches/tree/148ab02bcc073099ded44baf6ff0fefc65d29edd#usage-json-declaration

Code: https://github.com/vaimo/composer-patches/blob/148ab02bcc073099ded44baf6ff0fefc65d29edd/src/Patch/DefinitionList/LoaderComponents/DownloaderComponent.php#L153