Problem/Motivation

#2595695: 4xx handling using subrequests: no longer able to vary by URL introduced a less critical security vulnerability (access bypass): when setting a custom 403 or 404 page pointing to a node that is (or becomes) unpublished, visiting /page-does-not-exist shows the unpublished node, even when the active user does not have permission to view unpublished content. This is a regression against D8.0.x, where the fallback 403/404 was shown if the custom 403/404 was not accessible, and no unpublished content was shown.

The issue might be more theoretical that real, since it would require a site admin to unpublish a node that is used as 40x-page without updating the custom error pages configuration. Still, we should never show unpublished content to users without the required permissions, regardless of circumstances.

Issue was discovered while reviewing https://www.drupal.org/project/m4032404 and trying to discover how that module would react to a 404 that 403's.

Proposed resolution

One option would be to perform the access checking prior to making the subrequest, and in case of no access, fall back to the default 403/404.

Remaining tasks

* Review the patch
* Write tests

User interface changes

None.

API changes

PathValidator service added to constructor for CustomPageExceptionHtmlSubscriber.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mr.baileys created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, core-access-check-4xx.patch, failed testing.

alexpott’s picture

Reading the original issue @WimLeers said in #2595695-47: 4xx handling using subrequests: no longer able to vary by URL

I copy/pasted the naming and comments from \Drupal\Core\Path\PathValidator. I'm not sure it's such a bad idea to keep calling it "access-unaware router" etc. Because we are doing routing. And we don't want to do access checking, because these are just exception responses, and we inherit the access result from the original master request.

mr.baileys’s picture

Issue summary: View changes
mlhess’s picture

From the security team: This can be public as it is only in 8.1.x and not in 8.0.X and 8.1.x does not have a stable release.

xjm’s picture

From the security team: This can be public as it is only in 8.1.x and not in 8.0.X and 8.1.x does not have a stable release.

Would this be considered a private issue if it were in 8.0.x? If so, I think we need to revert the original commit and everything dependent on it. If not, it's fine as a public issue.

Personally I don't see an actual sec issue with this.

mr.baileys’s picture

Issue summary: View changes
mr.baileys’s picture

Would this be considered a private issue if it were in 8.0.x?

Yes, it would have been reported to security.drupal.org. After discussion, it could be moved to the public tracker if it was deemed not a security issue.

Personally I don't see an actual sec issue with this.

The (low) risk I see is that as a site admin, I expect that unpublished content is hidden from users without the required permissions at all times. D8.1.x currently violates this expectation for unpublished content set as 40x custom error pages.

xjm’s picture

So my proposal for this issue is:

  1. Timebox patching it to 1 week. Release beta1 with it disclosed publicly.
  2. If it is not fixed within 1 week, get confirmation with whether it is okay for public disclosure in a stable release. Timebox that discussion to 1 week also.
  3. If it is not okay for public disclosure in a stable release, revert the original issue and everything that depends on it, because we need to keep our scheduled release date.
  4. Release a beta2 once it is fixed, whether through a patch or a revert.
mr.baileys’s picture

Priority: Normal » Major

Setting to major since #2595695: 4xx handling using subrequests: no longer able to vary by URL, which could be reverted as a result of this issue, is major.

catch’s picture

For me this doesn't feel like a security issue or possibly even not a bug, more of an undocumented behaviour.

The configuration option determines which page should be used for the custom 404/403 pages, so to have those not available, the configuration can just be changed.

When developing sites I've sometimes been redirected to the custom 403 or 404 pages themselves (situations like trying to log out from user/login or similar), and it's confusing that for example example.com/404 or example.com/403 is actually a 200. So I could see a case for unpublishing those nodes so they're not available to browse to directly, won't get search indexed etc. while still having them used for the 404/403 handling itself.

Wim Leers’s picture

Initial thoughts

Quoting myself from #2595695-47: 4xx handling using subrequests: no longer able to vary by URL:

Because we are doing routing. And we don't want to do access checking, because these are just exception responses, and we inherit the access result from the original master request.

If we don't do that, we could have recursive access checking.

Rollback

All in all, this feels like an extreme edge case: 99% of the time, the 403/404 page you configure is accessible to all users. So I'd be very, very sad/confused if #2595695: 4xx handling using subrequests: no longer able to vary by URL got reverted for this.

Proposed resolution

Upon reading the issue further, I noticed the proposed solution:

One option would be to perform the access checking prior to making the subrequest, and in case of no access, fall back to the default 403/404.

That makes sense.

However, @catch makes a good point in #11: it probably should even be a best practice to unpublish the node that contains the 4xx response, so that you cannot access them directly, and they're only accessible/visible by actually experiencing a 403/404. That also implies this is not a bug.

Therefore I propose that we:

  1. Transform what catch wrote in #11 into documentation.
  2. Write test coverage asserting that that is how it works.

If others agree, this issue is no longer a major security bug, but a normal usability task.

Wim Leers’s picture

Issue summary: View changes
mr.baileys’s picture

Issue summary: View changes
FileSize
76.93 KB

Couple of concerns if we go for the solution proposed in #13

a) we are showing the unpublished node with the .node--unpublished class, meaning it's visually different from regular content:

b) this would set a precedent in that this would be the first and only time we actually show unpublished content to end users.
c) might be an issue if the 40x pages are under workflow moderation, or in combination with another contrib module that relies on the distinction between published and unpublished?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Version: 8.2.x-dev » 8.1.x-dev

As a public (possible) security issue that needs to be addressed in some way before 8.1.0, this one should be against 8.1.x.

xjm’s picture

Priority: Major » Critical

Discussed with @mlhess and @David_Rothstein who both considered that this could actually be a security issue for other scenarios -- for example, if an entirely private site has a custom, internal 404 page with private information on it, could this bug result in that information being disclosed to anonymous users? While that might not be an architecture we'd recommend, it would still be a regression in 8.1.0 for such a site if the bug applies to that scenario.

We decided to promote this issue to critical for more visibility prior to 8.1.0-rc1 which is scheduled for April 6-8. Either releasing with a known security regression or reverting mass amounts of work are both bad options, so the best thing to do would be to try to get a fix here in the public queue since it is already disclosed.

xjm’s picture

Issue tags: +beta target, +rc target
dawehner’s picture

Here is some test coverage.

benjy’s picture

Status: Needs work » Needs review

Setting to NR so we can see the tests fail.

Status: Needs review » Needs work

The last submitted patch, 19: 2678674-19.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
5.87 KB
4.12 KB

I added an access check for the 403 before the sub request. The 404 test needs fixing as it didn't fail in #19 and then if we're happy with the approach can update the 404 custom page as well.

Status: Needs review » Needs work

The last submitted patch, 22: 2678674-22.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
7.29 KB
1.42 KB

Fixed the unit test.

Wim Leers’s picture

I added an access check for the 403 before the sub request. The 404 test needs fixing as it didn't fail in #19 and then if we're happy with the approach can update the 404 custom page as well.

Fixed. This patch should fail in PageNotFoundTest.

  1. +++ b/core/lib/Drupal/Core/Access/AccessManagerInterface.php
    @@ -83,6 +83,6 @@ public function checkRequest(Request $request, AccountInterface $account = NULL,
    -  public function check(RouteMatchInterface $route_match, AccountInterface $account = NULL, Request $request = NULL, $return_as_object = FALSE);
    +  public function   check(RouteMatchInterface $route_match, AccountInterface $account = NULL, Request $request = NULL, $return_as_object = FALSE);
    

    Unnecessary change, can be reverted easily.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
    @@ -41,9 +50,10 @@ class CustomPageExceptionHtmlSubscriber extends DefaultExceptionHtmlSubscriber {
    -  public function __construct(ConfigFactoryInterface $config_factory, HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination, UrlMatcherInterface $access_unaware_router) {
    +  public function __construct(ConfigFactoryInterface $config_factory, HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination, UrlMatcherInterface $access_unaware_router, AccessManagerInterface $access_manager) {
    

    The docblock is not updated.

  3. +++ b/core/modules/system/src/Tests/System/AccessDeniedTest.php
    @@ -109,4 +109,14 @@ function testAccessDenied() {
    +    // Sets up a 404 page not accessible by the anonymous user.
    

    s/404/403/

  4. +++ b/core/modules/system/src/Tests/System/PageNotFoundTest.php
    @@ -50,4 +50,14 @@ function testPageNotFound() {
    +    $this->drupalGet('/admin');
    

    This path actually exists, so this test doesn't really make sense. This is testing a 403, not a 404.

Status: Needs review » Needs work

The last submitted patch, 25: 2678674-25.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.54 KB
1.09 KB

then if we're happy with the approach can update the 404 custom page as well.

This reroll does that, since #25 successfully failed.

Status: Needs review » Needs work

The last submitted patch, 27: 2678674-27.patch, failed testing.

Wim Leers’s picture

Sigh. CustomPageExceptionHtmlSubscriberTest tests with non-existing routes (which is fine, because it's a unit test) but then that ends up calling \Drupal\Core\Url::fromInternalUri(), which calls \Drupal::pathValidator(), which uses global state rather than dependency injection, and boom, these two failures. Let's see if I can work around that…

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.39 KB
1.96 KB

Here we go.

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
16.09 KB
13.69 KB
+++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
@@ -59,7 +69,10 @@ protected static function getPriority() {
+      if ($url->isRouted() && $this->accessManager->checkNamedRoute($url->getRouteName(), $url->getRouteParameters())) {

Hm… this is omitting the cacheability metadata of the access check. That seems wrong. It means that even users who can access this custom 403 would still get the default 403. See #2458349: Route's access result's cacheability not applied to the response's cacheability.


+++ b/core/modules/system/src/Tests/System/AccessDeniedTest.php
@@ -109,4 +109,14 @@ function testAccessDenied() {
+  public function testAccessDeniedCustomPageWithAccessDenied() {
+    // Sets up a 403 page not accessible by the anonymous user.
+    $this->config('system.site')->set('page.403', '/admin/index')->save();
+
+    $this->drupalGet('/admin');
+    $this->assertNoText('Administration');
+    $this->assertResponse(403);
+  }

This is a very incomplete test. It should be made as complete as the one in PageNotFoundTest.

benjy’s picture

+++ b/core/modules/system/src/Tests/System/AccessDeniedTest.php
@@ -34,12 +34,14 @@ protected function setUp() {
-  function testAccessDenied() {
+  function atestAccessDenied() {

Test is disabled.

+++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
@@ -69,8 +84,45 @@ public function on403(GetResponseForExceptionEvent $event) {
+        $existing_access_result = $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT);
+        if ($existing_access_result instanceof RefinableCacheableDependencyInterface) {

What else could $existing_access_result be other than an instanceof RefineableCacheableDependencyInterface? In that instance, we don't add our access result into the mix?

Wim Leers’s picture

Test is disabled.

Hah, oops, fixed!

What else could $existing_access_result be other than an instanceof RefineableCacheableDependencyInterface? In that instance, we don't add our access result into the mix?

Because:

interface AccessResultInterface { … }
abstract class AccessResult implements AccessResultInterface, RefinableCacheableDependencyInterface { … }

i.e. not every AccessResultInterface implementation has cacheability metadata. In Drupal core, it does. I wish this weren't the case, I advocated heavily against this, but this is what consensus landed on. Hence the need for that instanceof check.

xjm’s picture

Issue tags: +Needs security review

Great work @benjy and @Wim Leers!

+++ b/core/modules/system/src/Tests/System/PageNotFoundTest.php
@@ -50,4 +60,25 @@ function testPageNotFound() {
+  public function testPageNotFoundCustomPageWithAccessDenied() {

I need a docblock.

The patch looks RTBCable to me, but I think we should get security review/signoff.

Wim Leers’s picture

Great :)

Just have one more remark for now:

+++ b/core/modules/system/src/Tests/System/AccessDeniedTest.php
@@ -23,7 +23,7 @@ class AccessDeniedTest extends WebTestBase {
+  public static $modules = ['block', 'node', 'system_test'];

Not sure why we need the node module here.

benjy’s picture

@Wim, yes it was the instanceof check that made me ask, often a sign of code smell but it sounds like that was a different discussion

I manually tested and everything works as expected. The rest of the patch looks good to me.

+++ b/core/modules/system/tests/modules/system_test/system_test.routing.yml
@@ -159,3 +159,19 @@ system_test.date:
+system_test.always_denied:
...
+    _access: 'TRUE'

Theres some irony there i'm sure :)

Wim Leers’s picture

Theres some irony there i'm sure :)

Yep :P

Also, note how it MUST be that exact string, not a boolean. Another bit of painful irony.

dawehner’s picture

Also, note how it MUST be that exact string, not a boolean. Another bit of painful irony.

Sure, feel free to actually improve that.

alexpott’s picture

Some minor nits.

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
    @@ -7,8 +7,13 @@
    +use Drupal\Core\Cache\CacheableDependencyInterface;
    

    Not used.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
    @@ -40,10 +52,13 @@ class CustomPageExceptionHtmlSubscriber extends DefaultExceptionHtmlSubscriber {
    +   * @param \Drupal\Core\Access\AccessManagerInterface
    

    Missing $access_manager

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
    @@ -69,8 +84,45 @@ public function on403(GetResponseForExceptionEvent $event) {
    +   *   The event to process
    

    Missing full stop.

  4. +++ b/core/modules/system/src/Tests/System/AccessDeniedTest.php
    @@ -109,4 +111,24 @@ function testAccessDenied() {
    +
    +  public function testAccessDeniedCustomPageWithAccessDenied() {
    
    +++ b/core/modules/system/src/Tests/System/PageNotFoundTest.php
    @@ -50,4 +60,25 @@ function testPageNotFound() {
    +
    +  public function testPageNotFoundCustomPageWithAccessDenied() {
    

    Missing function doc comment

alexpott’s picture

Other than #39 the patch looks excellent and has the requisite test coverage.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Addressing #39.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
16.77 KB
4.3 KB

All done. Note that #39.3 was copy/pasted, so I also fixed it in the original location.

dawehner’s picture

+++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
@@ -353,4 +354,13 @@ public function getCurrentDate() {
+  public function alwaysDenied() {
+    throw new AccessDeniedHttpException();
+  }

+++ b/core/modules/system/tests/modules/system_test/system_test.routing.yml
@@ -159,3 +159,19 @@ system_test.date:
+    _access: 'TRUE'

Couldn't we just use _access: 'FALSE'?

Wim Leers’s picture

#43: hah, yes, we can!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool, thank you

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 040ce0c and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed ba7a18d on 8.2.x
    Issue #2678674 by Wim Leers, benjy, mr.baileys, dawehner, xjm, mlhess:...

  • alexpott committed 040ce0c on 8.1.x
    Issue #2678674 by Wim Leers, benjy, mr.baileys, dawehner, xjm, mlhess:...
xjm’s picture

Status: Needs review » Fixed
David_Rothstein’s picture

Title: Access bypass to unpublished custom error pages. » Access bypass for custom error pages

Retitling since #17 did turn out to be the case - this was not limited to unpublished nodes.

Status: Fixed » Closed (fixed)

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