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.
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff.txt | 1.51 KB | Wim Leers |
#44 | 2678674-44.patch | 15.73 KB | Wim Leers |
#42 | interdiff.txt | 4.3 KB | Wim Leers |
#42 | 2678674-42.patch | 16.77 KB | Wim Leers |
#33 | interdiff.txt | 761 bytes | Wim Leers |
Comments
Comment #3
alexpottReading the original issue @WimLeers said in #2595695-47: 4xx handling using subrequests: no longer able to vary by URL
Comment #4
mr.baileysComment #5
mlhess CreditAttribution: mlhess as a volunteer commentedFrom 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.
Comment #6
xjmWould 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.
Comment #7
mr.baileysComment #8
mr.baileysYes, 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.
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.
Comment #9
xjmSo my proposal for this issue is:
Comment #10
mr.baileysSetting 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.
Comment #11
catchFor 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.
Comment #12
Wim LeersInitial thoughts
Quoting myself from #2595695-47: 4xx handling using subrequests: no longer able to vary by URL:
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:
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:
If others agree, this issue is no longer a major security bug, but a normal usability task.
Comment #13
Wim LeersComment #14
mr.baileysCouple 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?
Comment #16
xjmAs 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.
Comment #17
xjmDiscussed 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.
Comment #18
xjmComment #19
dawehnerHere is some test coverage.
Comment #20
benjy CreditAttribution: benjy at PreviousNext commentedSetting to NR so we can see the tests fail.
Comment #22
benjy CreditAttribution: benjy at PreviousNext commentedI 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.
Comment #24
benjy CreditAttribution: benjy at PreviousNext commentedFixed the unit test.
Comment #25
Wim LeersFixed. This patch should fail in
PageNotFoundTest
.Unnecessary change, can be reverted easily.
The docblock is not updated.
s/404/403/
This path actually exists, so this test doesn't really make sense. This is testing a 403, not a 404.
Comment #27
Wim LeersThis reroll does that, since #25 successfully failed.
Comment #29
Wim LeersSigh.
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…Comment #30
Wim LeersHere we go.
Comment #31
Wim LeersHm… 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.
This is a very incomplete test. It should be made as complete as the one in
PageNotFoundTest
.Comment #32
benjy CreditAttribution: benjy at PreviousNext commentedTest is disabled.
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?
Comment #33
Wim LeersHah, oops, fixed!
Because:
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 thatinstanceof
check.Comment #34
xjmGreat work @benjy and @Wim Leers!
I need a docblock.
The patch looks RTBCable to me, but I think we should get security review/signoff.
Comment #35
Wim LeersGreat :)
Just have one more remark for now:
Not sure why we need the
node
module here.Comment #36
benjy CreditAttribution: benjy at PreviousNext commented@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.
Theres some irony there i'm sure :)
Comment #37
Wim LeersYep :P
Also, note how it MUST be that exact string, not a boolean. Another bit of painful irony.
Comment #38
dawehnerSure, feel free to actually improve that.
Comment #39
alexpottSome minor nits.
Not used.
Missing $access_manager
Missing full stop.
Missing function doc comment
Comment #40
alexpottOther than #39 the patch looks excellent and has the requisite test coverage.
Comment #41
Wim LeersAddressing #39.
Comment #42
Wim LeersAll done. Note that #39.3 was copy/pasted, so I also fixed it in the original location.
Comment #43
dawehnerCouldn't we just use
_access: 'FALSE'
?Comment #44
Wim Leers#43: hah, yes, we can!
Comment #45
dawehnerCool, thank you
Comment #46
alexpottCommitted 040ce0c and pushed to 8.1.x and 8.2.x. Thanks!
Comment #49
xjmComment #50
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedRetitling since #17 did turn out to be the case - this was not limited to unpublished nodes.