Problem/Motivation
A critical security issue was discovered by Dom. in which a basic authentication session that was initialized in a request would persist on subsequent requests, even if they were not using basic authentication any more. See the original report below for the steps that could replicate this issue.
The critical security bug has been fixed in the meanwhile thanks to #2228393: Decouple session from cookie based user authentication. But we still need test coverage to ensure that this issue does not resurface in the future.
A failing test has been posted in comment #60 which included the reverted patch from #2228393: Decouple session from cookie based user authentication to prove that the test correctly detects the security issue. The current test is green to prove that the problem is fixed in HEAD.
Proposed resolution
Test that if a user authenticates using basic authentication and then does a subsequent request _without_ basic authentication the user is no longer authenticated.
Remaining tasks
Review the patch.
User interface changes
None.
API changes
None.
Original report by Dom.
Say you have a drupal 8 website with an admin account and page not accessible by anonymous, for instance /admin/reports.
- Drupal 8 HEAD
- Standard profile install
- Everything to out-of-the-box, thus using Bartik theme and Seven per admin pages.
- Activate HTTP Basic Authentication core module (or use you own module with an Authentication Provider)
- Disconnect from your site
- Visit forbidden page for anonymous using your browser, such as /admin/reports => you should have a 403 forbidden using Bartik theme.
- Perform a GET request as anonymous. You can use any poster for this. For my test, I made it with Postman for Chrome, allowing various authentication method. => You willl get "/admin/reports" and get the expected 403 error using Bartik theme
- Perform a GET request with basic auth registering as admin. => You willl get "/admin/reports", but as a 403 error using Seven theme, this is not expected for two reasons :
- change of theme used
- you provided authentication so you should be logged and see the page
- Refresh the page "/admin/reports" on your browser : you are now logged in. => This is wrong :
- you should not be logged in the browser (although I'm not sure on this point). - Do the GET to page "/admin/reports" again with your poster with the basic authentication : still 403. => but we saw using the browser that we are logged in !
- Do the GET to page "/admin/reports" again with your poster without authentication. => now you see the report page, although you did not provide authentication. This is wrong. The session from the browser should not affect what is done using the poster.
Beta phase evaluation
Issue category | Task because we are adding missing test coverage. |
---|---|
Issue priority | Major since this is part of a critical meta issue, and is providing test coverage for a critical security vulnerability. |
Unfrozen changes | Unfrozen because it only changes test coverage. |
Prioritized changes | The main goal of this issue is enhancing security by providing test coverage for an access restriction bypass vulnerability. |
Disruption | Not disruptive because it is adding test coverage for a problem that is already fixed in HEAD. |
Comment | File | Size | Author |
---|---|---|---|
#78 | interdiff.txt | 2.13 KB | pfrenssen |
#78 | 2468873-78.patch | 6.51 KB | pfrenssen |
#17 | logoff.png | 26.89 KB | Dom. |
#17 | 403-logoff-no_auth.png | 43.97 KB | Dom. |
#17 | 403-logoff-basic_auth.png | 34.1 KB | Dom. |
Comments
Comment #1
xjm#2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session might be related?
Comment #2
dawehnerWell, there seems to be a small misconception of what the module actually does.
Let's read hook_help():
This means, it doesn't enable basic auth for all routes, but really just for the ones using REST module. Technically it is an opt in feature for specific URLs,
rather what you expect for all of them. The 403 behaviour then is right, ... for the HTML route we don't have basic_auth as authentication method enabled.
If you do the same with hal_json accept headers and rest/hal_json enabled, you actually get back the data, as expected, in case you specify those headers.
What you then see is that the somehow a session is saved, started from core/lib/Drupal/Core/StackMiddleware/Session.php:65
After that you have the cookie and you continue on.
Comment #3
xjmSo I can reproduce steps 1-4, but not step 5. Does "Visit" mean in a browser (i.e. using cookie authentication?)
I agree that step 4 seems like not the expected behavior.
Comment #4
xjm@dawehner, what I did is:
curl --request GET --user myuser:mypassword --header 'Accept: application/hal+json' http://localhost:8888/d8git/node/1
I get the 403 in step 4 then. What am I missing?
Comment #5
dawehnerI can reproduce exactly what is happening.
I'm sorry, but it is the expected behaviour. basic_auth is just enabled for a specific subset of routes ... and if you haven't enabled it for HTML (you could do that via code)
its not enabled for that route, which means that you authenticate via a wrong authentication mechanism, which result in a 403.
But yeah, step 5 is IMHO wrong behaviour,
\Drupal\Core\EventSubscriber\AuthenticationSubscriber::onKernelRequestFilterProvider
should reset the authenticated user, if authentication failed.
Comment #6
BerdirYeah, fun.
I found the same problem in #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session, but I didn't notice that you get logged in even when access is denied, that's weird. Maybe if you already had a session before?
Note that except step 5, everything is working as expected. You are supposed to get an access denied, because those routes don't allow basic_auth. You can change the default providers by changing the service definition or hacking it into AuthenticationManager to test basic auth on normal pages.
#2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session is also related.
Comment #7
xjmSo let's update the summary with more detailed steps and explicit steps to reproduce this (including the exact profile to install, modules to enable, etc. and the client used for the requests and exactly how). @dawehner said he reproduced it using the postman Chrome extension to perform the request for step 4 within Chrome. Thanks!
Comment #8
dawehnerWe have to reevaluate that part of the beta evaluation:
... you could have always logged in manually via the login form anyway.
Comment #9
Dom. CreditAttribution: Dom. commentedFor n°5, yes, I mean visit in browser.
@dawehner regarding #1 : OK I understand what you explain regarding the expected behavior. It makes sense. However, considering I have made my own authentication provider (as per explained here : https://www.drupal.org/node/2031999)
My purpose is to be able to log a user from having a token in URL : url?HTTP_W3C_VALIDATOR_TOKEN=xxxxxxxxx
Thus I will be able to have w3c validate all my pages (even /admin... as I need to characterize #2467827: [META] W3C validation for Drupal Core but that another story).
So basically I want all pages being authorized by my authentication_provider, what is expected for me according to the description there.
Also I don't want step 5, which is I expect to still be NOT LOGGED in my browser (this is in response to #8 also)
Comment #10
dawehner@Dom
So, if you would really want to do that you could do something similar as what is done in https://www.drupal.org/files/issues/2283637-36.patch
... You could write a ServiceProvider class, and set the argument of the authentication_manager explicitly.
In your case you would remove the default cookie provider and just always use your authentication mechanism.
I'll work on some basic test coverage so we at least define what exactly should happen.
Comment #11
larowlanAdding needs triage tag
Comment #12
dawehnerMh, I have a little bit of a trouble to reproduce exactly what I as able to see via the browser.
Comment #13
znerol CreditAttribution: znerol commentedThis happens because at step 4 the user is authenticated (via basic auth) and a session is started (because current user is not anonymous) and the user id of the current user is written into the uid attribute of the new session record.
This will be resolved when #2228393-64: Decouple session from cookie based user authentication.1 is implemented. The
uid
column of the session table should not be populated with the current user id, but with auid
attribute stored in the session. That attribute will only be altered upon login/logout. This will turn theuid
column into some sort of secondary index into the session table.Comment #14
almaudoh CreditAttribution: almaudoh commented#9 and #10: There is an open issue to make it easier to override the default auth providers at #2432585: Improve authentication manager service construction to support custom global service providers
Step 5 would be addressed by #2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session
Comment #15
xjmThanks @znerol and @almaudoh. Do you think we can mark this as a duplicate of #2228393: Decouple session from cookie based user authentication?
Comment #16
BerdirDidn't check the other patch yet, but just storing the uid might not be enough, we possibly want to explicitly store the used provider?
Comment #17
Dom. CreditAttribution: Dom. commented@xjm for #7 : Updated the description for reproduction, adding more "weird things" and screenshots.
@dawehner for #8: Yes probably. I just assumed that requesting a page with a poster (not browser) would not create a session. Please see updated description for this.
According to theses steps, it looks like one of the given issues in comment is related indeed.
- Session should not be started using basic auth : #2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session
- I hope that giving a new authentication provider would let me authenticate that way to any pages of the site.
*
I don't know yet if this is related : #2228393: Decouple session from cookie based user authentication
Next is :
- I will try the test given in this issue and see if this can be a validation test.
- I will try the patch given in link issue and see if it affects the behavior in anykind.
Comment #18
Dom. CreditAttribution: Dom. commentedComment #19
YesCT CreditAttribution: YesCT commentedshould there be an assert true that the user is logged in?
should there be an assert that the response included something that would have required permission/access cause they were authenticated ok?
what is session-test/set/test?
it is part of the session_test module, but what would we learn differently than the one above?
Comment #20
Dom. CreditAttribution: Dom. commented@YesCT:
1. I think there should be a test to validate you actually don't receive a 403 error
2. I think we could reproduce the scenario without the use of module session_test but targetting /admin/reports url for instance. I will work on that for a test proposition.
@almaudoh for #14:
I must confess I don't know specifically about #2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session but #2228393: Decouple session from cookie based user authentication does (see below)
@xjm for #15
#2228393: Decouple session from cookie based user authentication
Manually testing with reproduction scenario, I confirm that the patch #67 actually fixes the point 6 : after refresh of the browser, it does no log user in.
However, regarding other point :
- issue point 5 : Perform a GET request with basic auth registering as admin. => You will 403 error using Seven theme.
- issue point 7 : Do the GET to page "/admin/reports" again with your poster with the basic authentication : still 403.
- issue point 8: Do the GET to page "/admin/reports" again with your poster without authentication.
still exists. But because the session is not propagated to browser (at 6), then it gets better. But still, I expect basic auth not create a session since for me, from one request to another, you should still have to pass your credential. Can someone confirm this point ?
[edit: added point description in case issue summary change]
Comment #21
znerol CreditAttribution: znerol commentedRe #16
I think storing
uid
is enough. It just meansuid>0
= authenticated by cookie provider, otherwise authenticated by another provider or anonymous.Comment #22
Dom. CreditAttribution: Dom. commentedI was going to upload a test-only patch but it #12 is already. I add made a patch reproducing the scenario with admin/reports, but it's wrong for two reasons:
- should include the fix in WebTestBase for curl options as per #12
- should not rely on /admin/reports page in case it changes URL
Thus I will do the same with a node created for the test and remove the "view published content" permission to end up with a self-contained test. Still removing the use of extra session_test module.
@znerol : hum... don't know what to say here, but I think this would replace patch from #2228393: Decouple session from cookie based user authentication and fix 6 ("after refresh of the browser, it does no log user in.") But still using poster the user get's answer 403 + a session is created (as per point 5)
Comment #23
almaudoh CreditAttribution: almaudoh commented#15 @xjm: #2228393: Decouple session from cookie based user authentication fixes some of the issues mentioned here, but I think we should at least do a test-only patch to confirm that that issue fixes things.
However, we may have to close #2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session as a duplicate and fix that issue here, since this issue already has a patch (albeit passing).
Comment #24
xjm@almaudoh, the patch in this issue is only tests, which are not failing, so that is two problems. We need to do that the other way around. :) Writing a test that does fail is something we should add to the other issue.
Comment #25
Dom. CreditAttribution: Dom. commentedHere is a patch failing. On last test, anonymous user should still be unauthorized to access restricted page.
Also confirm that #70 here solves problem 5 on manual testing, but does not validates this test though, still trying to understand why.
Comment #27
Dom. CreditAttribution: Dom. commentedI can confirm patch #70 from #2228393-70: Decouple session from cookie based user authentication solves this (with manual testing) so closing as duplicate because this issue is newer.
Also copying failing test from #25 to that other issue.
Comment #28
xjmExcellent work, thanks @Dom.!
Comment #29
znerol CreditAttribution: znerol commentedLooked at what's going on with wireshark and noticed that the
Authorization
header is still present in the lastGET
request. This means that basic authentication credentials are leaking from the former request.I reopen this issue such that we can fix the test independently of the ongoing work in the other one. We can merge them as soon as the test is working properly.
Comment #30
xjmThanks @znerol. Based on #29 I think whichever issue we end up with should be critical.
Comment #31
xjmComment #32
xjmCan we add the test from #25 to #2228393: Decouple session from cookie based user authentication now (including with a test-only patch to expose the fail)? It looks like it does provide coverage for #29, correct?
Comment #33
znerol CreditAttribution: znerol commentedNot yet. The problem is that the current test is still broken. The screenshot from #29 shows requests generated by the test, it does not expose the problem we are fixing in #2228393: Decouple session from cookie based user authentication.
Comment #34
Dom. CreditAttribution: Dom. commentedI do not succeed in fixing it right now. The problems comes from 937-944 I guess. Simpletest actually has a configuration by default at "Basic Auth" at "admin/config/development/testing/settings". That is at least the closest I am to understand why now.
Comment #35
pfrenssenI'll have a look.
Comment #36
pfrenssenThere is a very big overlap between this issue and #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session.
WebTestBase::drupalGet()
, and the other by providing its own request method.MultipleAuthenticationSessionTest
and have borrowed code from eachother.In order to avoid duplicate work and merge conflicts I will postpone #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session on this issue since this is a critical issue.
Looking at how
WebTestBase::curlExec()
performs its requests it seems like we can use$this->additionalCurlOptions
to provide our basic authentication credentials. If this works then this means that we can simplify the approach an don't need to hackdrupalGet()
, nor need to provide an alternative method to perform requests with basic authentication.Removing tags about updating the issue summary and adding replication steps since I was able to replicate the issue perfectly by following the provided steps using the Postman extension for Chromium.
Comment #37
pfrenssenI rewrote the test:
router_test
module.setUp
so it is reusable in future tests from #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session.$this->additionalCurlOptions
to set basic authentication options rather than reinventing$this->drupalGet()
.The test fails correctly on the last step, but the patch from #2228393: Decouple session from cookie based user authentication does not solve the problem. I'm not sure whether the bug is actually still present, or if
curl_exec()
still somehow reuses the basic authentication even after the option is unset. Does anyone know if you can inspect the request headers during a test run to see if basic authentication is used?Comment #39
dawehnerIt feels a little bit weak to just test the http response code but not care about the result body at all.
Comment #40
pfrenssenJust sat with @znerol to discuss the next steps to take. He analyzed the HTTP requests and responses that are done during the test using tcpdump and wireshark. The requests are now all good, it works well to control the basic authentication with
additionalCurlOptions
. Tests were done with the patch from #2228393: Decouple session from cookie based user authentication.UnauthorizedHttpException
instead of a human readable error message. This is out of scope for this issue.Next steps to take:
@dawehner, do you think it is really useful to inspect the HTTP response body of a request that returns a 401? Do you want to see that the correct error message is visible?
Comment #41
Dom. CreditAttribution: Dom. commented@pfrenssen, @dawehner regarding last point #40 :
I would +1 this, because @zneroll can confirm we had seen error 401 (or 403 I'm not sure) replied while the page was actually visible because replied by cache.
Comment #42
Dom. CreditAttribution: Dom. commentedI was just working on #41 about patch #37 but then realized : the page tested is actually only authorized throught basic_auth.
Maybe stupid, but that seems just as one problem to me. Should not we test also what initial issue was with page accessible both with cookie and basic_auth, so that the session does leak from basic_auth to cookie ?
Comment #43
pfrenssenCreated followup for the
UnauthorizedHttpException
: #2472371: Exception shown on 401 Unauthorized.Comment #44
znerol CreditAttribution: znerol commented@pfrenssen: How I do
tcpdump
:sudo tcpdump -s0 -i lo -w /tmp/auth-leak.cap
Then you can open just open the
.cap
file in wireshark.Comment #45
pfrenssenAfter an epic struggle with trying to disable basic authentication in Curl, I finally tackled this thanks to the great help from @znerol. I've also attached a patch that includes the fix from #2228393: Decouple session from cookie based user authentication to see if it actually detects the bug correctly.
Comment #47
znerol CreditAttribution: znerol commented@pfrenssen: Fantastic! Do you think we should merge the test into #2228393: Decouple session from cookie based user authentication or should we first add the use-case from #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session?
Comment #48
pfrenssenThese 3 issues are already very interrelated, I would not make it even more confusing now by merging the tests. Everything hinges now on #2228393: Decouple session from cookie based user authentication, so we should focus on that first. Once that is in the two other issues can be picked up again.
Postponing on #2228393: Decouple session from cookie based user authentication.
Comment #49
pfrenssenForgot a line of documentation.
Comment #50
almaudoh CreditAttribution: almaudoh commented#2228393: Decouple session from cookie based user authentication is in now
Comment #51
pfrenssenUpdating issue title to indicate that this is only providing test coverage.
Comment #52
pfrenssenRenamed the test, this was the original name of the test class of #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session but this is not actually using multiple authentication methods.
Comment #53
xjmDiscussed with @pfrenssen. Now that the actual bug is fixed (and we have demonstration of the coverage in #45, we can downgrade this to a major task.
Thanks a ton @pfrenssen for all the work clarifying this!
Comment #54
pfrenssenJuggling the issue links so that it's clear this is part of the session / authentication meta.
Comment #55
znerol CreditAttribution: znerol commentedUse
[]
syntax for arrays like throughout the rest of the class.We normally do not do that but instead rely on the fact that the commit message includes the issue number. We expect that people know how to use
git blame
andgit log
.This method might be useful in other tests as well. What about moving it to
WebTestBase
. Then pass username and password by parameters and add the rest of (optional) params fordrupalGet
. Might as well be a follow-up.The
Accept
header should not be necessary here.In order to have parity with the other
no-auth
route used in the same test, why not addbasic-auth
to the route name / path.Comment #56
znerol CreditAttribution: znerol commentedOr perhaps move it into a trait in the
basic_auth
module, that might actually be a better idea.Comment #57
pfrenssenThanks for the excellent review! Will update the patch.
Comment #58
pfrenssenAddressed the remarks.
Comment #59
pfrenssenWas a bit too hasty, the trait method was still referencing the protected properties of the test class, made them into arguments as was suggested by @znerol. This should be much more useful.
Comment #60
pfrenssenThis reverts #2228393: Decouple session from cookie based user authentication and includes the test from #59 as proof that the test works. This should fail.
Comment #62
pfrenssenBack to needs review. The patch to review is the one from #59.
Comment #63
pfrenssenComment #64
pfrenssenSmall cleanup.
Comment #65
pfrenssenWoops! Still had the reverted patch from #2228393: Decouple session from cookie based user authentication in my branch. This should be clean & green.
Comment #73
znerol CreditAttribution: znerol commentedTest failures look like random ones #2474909: Allow Simpletest to use the same APC user cache prefix so that tests can share the classmap and other cache objects.
Comment #75
znerol CreditAttribution: znerol commentedMandatory arguments cannot really go after optional ones. I.e. that renders the optional one mandatory also. I propose the following method signature:
protected function basicAuthGet($path, $username, $password, array $options = [], array $headers = []);
The test case itself is spot-on, very well done.
Comment #76
pfrenssenThanks for linking that issue, I was hesitant to pick this back up because of these weird failures.
Comment #77
pfrenssenSetting to NW as per #75.
Comment #78
pfrenssenFixed the ordering of the arguments as suggested in #75.
Comment #81
pfrenssenComment #82
znerol CreditAttribution: znerol commentedThe issue summary should point out that the bug was fixed in #2228393: Decouple session from cookie based user authentication and that this issue now just adds the necessary regression test. Also the beta-evaluation should be adapted accordingly.
Comment #83
pfrenssenUpdated issue summary and beta evaluation.
Comment #84
znerol CreditAttribution: znerol commentedThanks!
Comment #87
znerol CreditAttribution: znerol commentedComment #88
xjmWaiting on a test run right now, but meanwhile, just a note regarding:
I too prefer the
[]
syntax, and it's fine to use it in new code, but while #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 is unresolved, either is acceptable, and we should not push back on contributors using one vs. the other in new lines of code. (Also, as a reminder, patches should not change existingarray()
to[]
as it makes them harder to review.)Just wanted to clarify that; the patch in this issue is fine coding standards-wise.
Comment #90
xjmSo, the test looks great. I confirmed that it is providing the expected coverage as well. Thanks for the beta evaluation and clear summary.
And then:
A note regarding the test failures earlier on the issue. When a patch fails and we retest it, we should document what the fail was on the issue. Sometimes it's testbot issues, but other times it's the test introducing a random failure. And the two APC patches were reverted, but we still saw more fails. I didn't see any of the usual suspects in this test though, so going to go ahead and commit it.
This issue only improves test coverage for what was a critical bug, and is a followup for a critical issue, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x
Comment #91
znerol CreditAttribution: znerol commentedThanks @xjm.
Small follow-up: #2479515: Use BasicAuthTestTrait in BasicAuthTest