ImageStyleDownloadController routes do not limit schemes served

Brought this up in slack in the #needs-review-queue-initiative channel and @cmlara talked about rescoping, #8

Downgrading priority as we found a work-around to add override routes no longer making this a contrib blocker (though it is still considered unreliable and could lead to security issues occurring)

Moving issue to 10.1.x and can be backported.

Rebased on 10.1.x

Removed the $public parameter per #8

The last submitted patch, 2: 3298701-limit_schemes_imagestyledownloadcontroler-2.patch, failed testing. View results →

Changing exception message as per suggestions from #20.

The last submitted patch, 21: 3298701-21.patch, failed testing. View results →

Random test failure, retest is green, back to NR.

Think the message is better now.

Though since changing it didn't break anything I wonder if we should add an assertion testing for the message. Will let the committer decide if that's overkill or not.

larowlan → credited effulgentsia → .

Crediting @effulgentia

I've pinged security team folks in slack to ask some questions about this

larowlan → credited benjifisher → .

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -106,7 +108,7 @@ public static function create(ContainerInterface $container) {
+  public function deliver(Request $request, $scheme, ImageStyleInterface $image_style, string $required_derivative_scheme) {

Should we make this optional for now, defaulting to e.g. public - is a BC break otherwise

Crediting @benjifisher from the security team for pointing out the above

Should we make this optional for now, defaulting to e.g. public - is a BC break otherwise

As noted in the IS, this class is @internal (by default) and not subject to the BC policy. See https://www.drupal.org/aboutcore/policies/core-change-policies/bc-policy... → .

I don't believe adding a default of 'public' is likely to fix help much and instead will just allow a security hole to persist.

With a default any class extending the base controller class, or registering a route utilizing the controller would still serve public:// scheme bypassing any new rules placed when a module replaces the existing controller (like s3fs public takeover).

Additionally a default would still prevent 3rd party routes from serving routes because their scheme wouldn't match the 'public' scheme for the destination image still rendering the route broken.

If we are adding a default to prevent a WSOD I think the best we can do is make the value nullable with a NULL default allowing the 403 to be thrown for scheme mismatch, or if we really want to go the extra mile a 403 with a DX targeted make the 403 message referencing the change notice.

@cmlara:

The link in #31 is missing a '/' in 'aboutcore'. If I add that and scroll up the page, I see the paragraph,

The Drupal core deprecation policy → applies to both public and internal APIs: wherever possible, old APIs should be deprecated for removal in the next major version instead of being removed immediately. For public APIs, the deprecation process is a requirement; for internal APIs, we provide BC and deprecation where possible → , but breaking changes may be made in situations where BC is not possible or has negative consequences. (Even for internal APIs, core contributors should always try to follow the deprecation process first, or document in issue discussions why deprecation is not used.)

As you say, the BC layer is not a requirement in this case, but I think the "where possible" part applies.

One thing that bothers me about this issue is that I do not see any discussion of whether this is the simplest/safest approach. For example, could the page controller figure out the expected scheme without being given an extra parameter? Maybe not, but I would like to document that here on the issue.

Note: in my mind a BC layer is 'not possible' if it retains the vulnerability. This is in line with Drupal being a 'secure' cms and fixing vulnerabilities before they can be exploited. If a core committer wants a layer that retains the vulnerability I believe it necessitates an explicit sign-off to do so.

One thing that bothers me about this issue is that I do not see any discussion of whether this is the simplest/safest approach. For example, could the page controller figure out the expected scheme without being given an extra parameter?

I don't think we can get much more simple or secure than the patch as it exists.

The scheme we receive as part of the request does not reflect the routes intended usage and is fully in the hands of an attacker, as such we can not trust that data. We can not depend upon the route name (if we were to do a request to route name lookup) because there is no hard coded rule that the route name must be a specific format. If we were to implement such a check we may be able to make it work for core (because we know the routes) but still break contrib. See last suggestion in this post for relevant scenario.

If we do if ($required_derivative_scheme !== NULL && $required_derivative_scheme !== $derivative_scheme) { we still leave the vulnerability present if a site doesn't register a scheme with the route. This would fix the vulnerability for the most common offender (core). This has impacts on those extending the class as well as those creating/modifying routes.

We possibly could remove the '{scheme}' wildcard from the routes and only register the routes without wildcard and manually set the scheme value as part of the route definition. This still leaves the vulnerability present for code that uses the wildcard (contrib). This could break some read-only streamWrapper that depend upon core to have the existing wildcard route to provide support (the 'default' filesystem is used to store derivatives for read only streamWrappers.) I can think of no way a read-only streamWrapper would be able to reliability determine what route is the 'default' that they should add their scheme onto. This has impacts on any module modifying/creating routes.

We could add a new method "expectedDestinationScheme()" to avoid modifying the existing method, however I believe this may require a new class for each route to define the approved scheme (or some form of lookup that allows defining routes.) This is a scenario where a route name lookup could be trusted because its in control of the class. This has impacts on any module that extends the existing controller or attempts to implement a route based on the controller.

This has impacts on any module that extends the existing controller or attempts to implement a route based on the controller.

I'm not sure between the various options, but just in principle we don't need to provide backwards compatibility for controllers. https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →

So even if that breaks a contrib or custom module, that'd be fine with a change record and release note.

The same with route definitions too to be honest.

We try not to go out of our way to break things like this, but if it's necessary there is nothing stopping us from doing it.

This probably should have been back to NR after #35

As noted by catch the goal is to not break, however if necessary breaking BC is permitted.

As noted in my previous posts, any BC layer here would leave a security vulnerability present and as such I contend meets the definition of "necessary" justifying no BC layer.

Rebased #21 on 11.x

patching file core/modules/image/image.routing.yml
patching file core/modules/image/src/Controller/ImageStyleDownloadController.php
Hunk #2 succeeded at 108 with fuzz 1.
Hunk #3 succeeded at 156 (offset 14 lines).
patching file core/modules/image/src/Routing/ImageStyleRoutes.php
patching file core/modules/image/tests/src/Functional/ImageStyleDownloadAccessControlTest.php

No interdiff as this is a rebase.

So taking a look I see there is now a new parameter that could be used in the service, but the CR doesn't have any code example of that and when to use. Think the CR could be tweaked some to be more clear.

Thanks

I have added an example to the CR showing the required routing changes, and made it more clear the value is required.

CR reads well. Lets see what the committers think about the need for BC or not.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Hiding files and converted #36 to an MR.

Please do not credit me if I did not other wise earn it as all I did was convert this.

Restoring status

Fixed a typo and a few readability issues in the IS.

Also verified all changes from the patch are in the MR. So the re-rtbc is double checked 🙂

I read the issue summary and comments. Thanks for the clear issue summary, it made reading the comments much easier. I did not find any unanswered questions.

@BramDriesen, thank you for confirming the change from patch to MR is correct.

The only think I can think of is that the release note snippet should mention the possible BC break.

Committed fd36154 and pushed to 11.x. Thanks!

Not backportable due to 10.2.x due to the minor BC break. Updated the change record to more explicitly note this is a BC break for anyone using the controller as suggested by @quietone, and published it.

Publish the CR

Automatically closed - issue fixed for 2 weeks with no activity.

Unfortunately, this is a breaking change in 10.3.x, isn't it?

As noted in 🐛 Provide BC for ImageStyleDownloadController Needs review , we need to do a better job of explaining this in its release note (which should also link the CR).

Gave it a shot but not sure best spot to place in

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

Thanks @smustgrave!

Updated the IS with what we eventually settled on here. Thanks!

quietone → closed merge request !5340