From 40e1e1e59e360d3589747b69157c26b4b3343da7 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sun, 24 Feb 2019 13:57:19 -0500 Subject: [PATCH] Improve SafeCookieData Error Logging 1. Use request.session instead of request.user, since request.user won't necessarily be properly set. 2. Be extra paranoid by putting logging after session cookie deletion, so that even if there is some error related to logging, the important work will complete and the browser won't get left in a broken state. 3. Write out the full contents of the Cookie header (up to 4096 bytes) in the log as a base64 encoded string. This way we can look at broken cookie states and diagnose what's breaking them (the Python parser will just silently skip anything past a corrupted cookie entry). We base64 encode mostly to prevent people from maliciously injecting garbage into our logs. --- .../djangoapps/safe_sessions/middleware.py | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index dc78a21bb1..19a91698ab 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -55,7 +55,7 @@ SSL-protected channel. Otherwise, a session hijacker could copy the entire cookie and use it to impersonate the victim. """ - +from base64 import b64encode from contextlib import contextmanager from hashlib import sha256 from logging import ERROR, getLogger @@ -261,7 +261,6 @@ class SafeSessionMiddleware(SessionMiddleware): final verification before sending the response (in process_response). """ - cookie_data_string = request.COOKIES.get(settings.SESSION_COOKIE_NAME) if cookie_data_string: @@ -459,10 +458,6 @@ def _delete_cookie(request, response): Delete the cookie by setting the expiration to a date in the past, while maintaining the domain, secure, and httponly settings. """ - log.warning( - u"SafeCookieData is deleting session cookie for user %d", - request.user.id - ) response.set_cookie( settings.SESSION_COOKIE_NAME, max_age=0, @@ -472,6 +467,21 @@ def _delete_cookie(request, response): httponly=settings.SESSION_COOKIE_HTTPONLY or None, ) + # Log the cookie, but cap the length and base64 encode to make sure nothing + # malicious gets directly dumped into the log. + cookie_header = request.META.get('HTTP_COOKIE', '')[:4096] + log.warning( + u"Malformed Cookie Header? First 4K, in Base64: %s", + b64encode(cookie_header) + ) + + # Note, there is no request.user attribute at this point. + if hasattr(request, 'session') and hasattr(request.session, 'session_key'): + log.warning( + u"SafeCookieData deleted session cookie for session %s", + request.session.session_key + ) + def _is_from_logout(request): """