Logged in cookies not deleted on logout over HTTPS, not reproducible on edx.org

Hi. This is mainly to vent out, as it took some hours to debug and I’m not sure whose problem is it; and as a heads-up to @regis .

This is an issue not of tutor, but of the edx-platform, so I didn’t see fitting to raise an issue in your repo. However, it is not reproducible on courses.edx.org, which has led to me being 100% sure they are not running the codebase at edx/edx-platform/master, which they report they do.

Long story short. When logging in over HTTPS and using django-cookies-samesite config value DCS_SESSION_COOKIE_SAMESITE = "None" (which tutor is using and edx.org very much seems to be), login cookies (edx-logged-in, edx-jwt-cookie-signature, etc) are set with SameSite=None and Secure, as seen in openedx/core/djangoapps/user_authn/cookies.py:

def standard_cookie_settings(request):
    ...
    cookie_settings['secure'] = request.is_secure()

However, when logging out, those cookies are deleted using Django’s response.delete_cookie(), which does not set the Secure attribute:

for cookie_name in ALL_LOGGED_IN_COOKIE_NAMES:
        response.delete_cookie(
            cookie_name,
            path='/',
            domain=settings.SESSION_COOKIE_DOMAIN
        )

So, at least using a recent version of Chrome, the logged in cookies are not deleted, because they have SameSite=None but not the Secure attribute (including, again, the JWT cookies):

That means that any MFA using the JWT cookies to authenticate (looking at the edx/frontend-platform code, that would mean all of the official ones) will remain logged in when the user logs out from the LMS. That’s not only a functional issue, but potentially a security one.

HOWEVER, trying to reproduce this in edx.org, it’s clear that they set the Secure attribute on the log out cookies, so this issue does not arise:

I’ve reviewed many branches, tags, docs and Jira issues trying to find if/when this was reported/fixed, with no avail.

In the end, this was very easy to fix once found, just changing a few lines in openedx/core/djangoapps/user_authn/cookies.py to match the behaviour at log in:

def delete_logged_in_cookies(response, request):
    for cookie_name in ALL_LOGGED_IN_COOKIE_NAMES:
        response.set_cookie(
            cookie_name,
            '',
            path='/',
            domain=settings.SESSION_COOKIE_DOMAIN,
            secure=request.is_secure(),
            max_age=0,
            expires=datetime.datetime.utcfromtimestamp(0)
        )

    return response

Again, I’m aware this is not an issue with tutor but with edx-platform. However, seeing that it has been shadowly fixed in the production edx.org deployment, and that work is being done with MFAs by @regis (https://github.com/overhangio/tutor-gradebook), I hope at least to save a headache to some poor developer if they face this issue and somehow end up here.

Anyway, if anyone knows what’s the right thing to do in a situation like this, I’m all ears.

Cheers.

Hey @moraleja39, thanks for the thorough investigation and the detailed report, this is much appreciated. I am not going to be in front of a computer for a couple days, so I’m going to ask you to do a few more things:

  1. Report your findings to security@edx.org. Add Ned Batchelder and me in cc and link to this post.
  2. Open a pull request on edx-platform to resolve this issue
  3. Open a pull request on the Tutor repo where you cherry pick your patch.

Can you do that?

1 Like

Hey Regis. Thanks for your guidance. Sure thing, I’ll take care of it tomorrow morning.

@moraleja39 thanks for the post, it’s really useful.

1 Like

I know this is a bit late in the conversation, but we recently tackled this issue and this was a useful reference.

We ended up fixing this issue with a solution that requires no code changes, just updating a few variables to rename the cookies while deploying servers. I’m not sure how it applies to Tutor since we’re not using it for our deployments, but here are the variables we changed to fix the issue in a production environment (these are ansible variables):

# Secure cookie configuration - with __Secure- prefix so logouts work properly
COMMON_JWT_AUTH_COOKIE_HEADER_PAYLOAD: '__Secure-edx-jwt-cookie-header-payload'
COMMON_JWT_AUTH_COOKIE_SIGNATURE: '__Secure-edx-jwt-cookie-signature'
COMMON_JWT_AUTH_REFRESH_COOKIE: '__Secure-edx-jwt-refresh-cookie'
 
EDXAPP_LMS_ENV_EXTRA:
  EDXMKTG_LOGGED_IN_COOKIE_NAME: '__Secure-edxloggedin'
  EDXMKTG_USER_INFO_COOKIE_NAME: '__Secure-edx-user-info' 

I hope this helps :slight_smile:

1 Like

@giovanni are you saying that this issue is still occurring in Lilac? I am not able to reproduce it. Here are the steps I followed:

  1. Enable the MFE plugin on the https://demo.openedx.overhang.io website
  2. Login: Sign in or Register | Open edX Demo Site
  3. Access the gradebook: Gradebook | Open edX Demo Site
  4. Logout from the LMS (incidentally, the edx-jwt-cookie* were all deleted successfully)
  5. Try to load the gradebook again: I am redirected to the login screen.

I did my testing both with Firefox and Chromium.

The solution you are suggesting is to add a __Secure prefix to the logged-in cookie names. If I understand correctly, this means that these cookies will only be accessible in HTTPS (source). Thus, this is not a valid solution for HTTP sites.

:thinking: I understand that your solution comes from the actual implementation of response.delete_cookie in Django 2.2.24:

def delete_cookie(self, key, path='/', domain=None, samesite=None):
    # Most browsers ignore the Set-Cookie header if the cookie name starts
    # with __Host- or __Secure- and the cookie doesn't use the secure flag.
    secure = key.startswith(('__Secure-', '__Host-'))
    self.set_cookie(
        key, max_age=0, path=path, domain=domain, secure=secure,
        expires='Thu, 01 Jan 1970 00:00:00 GMT', samesite=samesite,
    )

Prefixing the cookie name with __Secure guarantees that it is properly deleted (because secure=True).

Note that the newer implementation in Django 3.2.7 is better, as it takes into account the samesite argument:

def delete_cookie(self, key, path='/', domain=None, samesite=None):
    # Browsers can ignore the Set-Cookie header if the cookie doesn't use
    # the secure flag and:
    # - the cookie name starts with "__Host-" or "__Secure-", or
    # - the samesite is "none".
    secure = (
        key.startswith(('__Secure-', '__Host-')) or
        (samesite and samesite.lower() == 'none')
    )
    self.set_cookie(
        key, max_age=0, path=path, domain=domain, secure=secure,
        expires='Thu, 01 Jan 1970 00:00:00 GMT', samesite=samesite,
    )

tl;dr: I like your solution @giovanni, but I do not understand whether Lilac is still affected by this issue. @moraleja39 @giovanni can you please try to reproduce the issue in Lilac?

Hi @regis, sorry for the delay in the reply!

are you saying that this issue is still occurring in Lilac? I am not able to reproduce it.

Lilac is still not using Django 3.2, so the issue still happens in that release, but only on HTTPS sites.

This issue happens with the IDAs (ecommerce, discovery), not with the MFEs afaik. Try:

  1. Set up an instance using HTTPS.
  2. Login the LMS.
  3. Go to ecommerce and log out from there.
  4. You’ll be logged out of the LMS, but not from the IDA, because the cookies were not properly deleted.

The solution you are suggesting is to add a __Secure prefix to the logged-in cookie names. If I understand correctly, this means that these cookies will only be accessible in HTTPS. Thus, this is not a valid solution for HTTP sites.

Yes you’re correct, this is only a fix for production sites using HTTPS. We’ve initially found this issue while setting a client’s production instance and using the configuration above resolved the issues.

tl;dr: I like your solution giovanni, but I do not understand whether Lilac is still affected by this issue. can you please try to reproduce the issue in Lilac?

It probably is still there, given the Django version in that release, but we haven’t upgraded our clients to Lilac yet. It should be fixed in Maple though (Django 3.2 upgrade).

Thanks for your answer @giovanni. Still, I am not able to reproduce the issue. Here are the steps I followed on the https://demo.openedx.overhang.io website:

  1. Enable the right plugins:

    tutor plugins enable ecommerce discovery mfe
    
  2. Initialize the platform:

    tutor local quickstart
    
  3. Create an admin user:

    tutor local createuser --superuser --staff --password=admin admin admin@overhang.io
    
  4. Log in as “admin”: Sign in or Register | Open edX Demo Site

  5. Open ecommerce and verify that the dashboard loads correctly: https://ecommerce.demo.openedx.overhang.io/

  6. Hit “sign out”. The login cookies are properly removed:

    set-cookie: edxloggedin=""; Domain=.demo.openedx.overhang.io; expires=Thu, 01 Jan 1970 00:00:00 GMT; Max-Age=0; Path=/; SameSite=None; Secure
    
  7. Attempt to open https://ecommerce.demo.openedx.overhang.io/ again: I am redirected to Sign in or Register | Open edX Demo Site

I observe this behaviour on Chromium 93.0.4577.63 and the latest Firefox, with tutor v12.0.4.

I’m still able to see the issue when using the Open edX native deployment method (openedx_native.yml playbook). Maybe Tutor’s ingress controller (Caddy) applies some changes to cookies/headers before sending them to the client?

@giovanni thanks for the info. Since the issue no longer occurs on Tutor I took the liberty to close your PR. Feel free to re-open if the bug occurs again.