Problem

If a user has an active/valid session with SimpleSAMLphp, but not with Drupal, the simplesamlphp_auth module will attempt to re-authenticate the user with Drupal (using user_external_login_register in hook_init). During this process a cache entry may be incorrectly inserted into the anonymous page cache, which can expose the authenticated user's data to anonymous users. For example, an anonymous user may see the Drupal toolbar for an authenticated user...

Similar issues have been either reported and/or worked around in other single sign-on modules, such as cosign:

https://www.drupal.org/node/2059581

There are linked examples in that issue of other modules that guard against this in hook_boot or hook_init

Steps to Reproduce

Verifying this issue assumes you have some way of identifying an authenticated page vs an unauthenticated page, such as the Drupal toolbar or any other user-specific data.

  1. Open 2 separate browsers
  2. Authenticate using SimpleSAMLphp in browser 1
  3. Navigate to a test page
  4. Clear the page cache
  5. Manually delete or expire Drupal's session cookie in browser 1 (NOT your SimpleSAMLphp session cookie)
  6. Reload the test page, you Drupal session cookie will be returned in the response after simplesamlphp_auth re-authenticates
  7. If you check the cache_page table now, you will have an entry for the test page...but why? Authenticated requests should never be cached...
  8. Load the test page as an unauthenticated user in browser 2 - you will see the authenticated user's data. Yikes!

Proposed resolution

Actively prevent page caching in simplesamlphp_auth's hook_init in circumstances where Drupal/SAML session parity is re-established.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mdeltito’s picture

Status: Needs work » Needs review
FileSize
1.04 KB

This patch simply adds drupal_page_is_cacheable(FALSE) to prevent page caching in the condition where simplesamlphp_auth re-establishes a Drupal session (in hook_init). This fixes the observed behavior, and seems fairly safe as a solution to this issue.

mdeltito’s picture

Bonus patch against 7.x-2.0-alpha-2

djdevin’s picture

Confirmed this issue, we are seeing the same thing.

We have a "logged in as " bit in our header menu, and it gets cached for some authenticated users. So it appears that the user is logged in however there's no session, so navigating to another page is fine.

gaards’s picture

Is this something that affects the 7.x-3.x-version as well? I see that this logic has been moved to a separate simplesamlphp_auth.inc file (hook_init in the .module file includes another separate file simplesamlphp_auth_pages.inc, which in turn includes simplesamlphp_auth.inc). In simplesamlphp_auth.inc the function user_external_login_register() is found under the function _simplesaml_auth_user_register.

If it indeed affects the 7.x-3.x-version, where should the drupal_page_is_cacheable(FALSE); be inserted?

mdeltito’s picture

I can't say for sure if this affects the 7.x-3.x branch, but after a quick look it seems like much of the logic in _simplesaml_auth_login_register()is the same as the equivalent in the 2.x branch.

If you can confirm this issue exists by following the test-case in the description, you can then test the fix by adding the drupal_page_is_cacheable(FALSE) call at line 32

gaards’s picture

I followed the steps to reproduce this problem in 3.x, but step 6. doesn't create a new session cookie in our environment (SimpleSAMLphp+ADFS), removing the session cookie (SSESS...) will force me to click the log in button again to access an authenticated page / see the admin toolbar. Couldn't reproduce step 7. and 8. because of that.

It would be great if someone else on the 3.x branch could test this issue as well to see if they can reproduce this issue.

snufkin’s picture

Status: Needs review » Needs work

I can confirm that this behaviour is present on 2.x. The patch does make things a bit better, but on my test setup it ends up caching the anonymous version of that page and then sticking with it, which is not good either. I don't really understand why that would happen.

Steps to recreate:
1. Log in via SSO, get redirected back to drupal
2. Remove the local Drupal session
3. Refresh the page

I've ended up on the anonymous version of that page, and I do not get the Drupal session rebuild. Whats worse, I can also see the /user page for this account (although can't edit at least).

djdevin’s picture

Whenever this happens to us (we do other integrations besides SAML as well) it's an issue with the anonymous page cache not being disabled in time.

If you hit /saml_login it will probably work fine as that page can never be cached. But for any other page we might have to integrate with https://www.drupal.org/project/dynamic_cache which can disable the anonymous page cache early enough so that the SSO login routines can happen.

svendecabooter’s picture

FileSize
855 bytes

For the branch 7.x-3.x i can confirm the behaviour mentioned by DMaester.
This issue cannot be reproduced, because in branch 7.x-3.x there is no logic to automatically authenticate through SimpleSAMLphp on a page refresh.
I haven't checked the 7.x-2.x branch in detail, but it seems that functionality is possible there.

In 7.x-3.x the following code is in hook_init:

function simplesamlphp_auth_init() {
  if (user_is_anonymous()) {
    return FALSE;
  }
  (...)
}

This will prevent reproduction step 6 from being able to happen.

Attached is a patch for 7.x-3.x based on my tests that:
1/ adds the functionality to automatically login into Drupal if a valid SAML session exists, but no Drupal session. That would give feature parity with the 7.x-2.x branch if I understand things well (haven't used 2.x myself)
2/ after functionality 1 is added, the problem can be reproduced by following the steps in the original report by mdeltito. This is also fixed in the patch by adding drupal_page_is_cacheable(FALSE); to prevent the page from being cached.

This could probably also be added to the 8.x-3.x branch, since this works similarly as the 7.x-3.x branch.

svendecabooter’s picture

That last patch will set cookies on each page, making it impossible to cache with Varnish for example.
So probably not be a good idea anyway.
See also #2020009: Session cookie being set, breaking through Varnish cache..