Hey @Anomie, I did an initial review of the core patch, but can you point me to any design / architecture docs describing your conceptual model for how this all works? On the initial reading, I'm having a hard time understanding the distinction you make between the responsibilities of the manager, backend, and session, and the expected use cases for mutable/immutable.

On the security side, I was hoping to clearly see pretty clearly where the countermeasures from https://www.owasp.org/index.php/Session_Management_Cheat_Sheet were done. It feels like I have to trace through a lot of classes to figure out what's going on. But maybe with some naming changes or comments, we can make those easier to find.

In T116413#1770590, @csteipp wrote:

Hey @Anomie, I did an initial review of the core patch, but can you point me to any design / architecture docs describing your conceptual model for how this all works?

Not really. The first iteration was the code written for https://www.mediawiki.org/wiki/Requests_for_comment/AuthManager#Session_management. Then we decided to split the SessionManager bit out from AuthManager to reduce the complexity of the latter, and I took advantage of the situation to revisit some of the design decisions that had turned out to be a bit problematic in the original implementation.

On the initial reading, I'm having a hard time understanding the distinction you make between the responsibilities of the manager, backend, and session, and the expected use cases for mutable/immutable.

Externally, the Manager is responsible for collecting the vary-headers and vary-cookies and for letting external code find a Session when the external code doesn't go through WebRequest::getSession().

Internally, the Manager constructs the Providers and the Backends, manages access to the Providers for stuff like User::newSystemUser(), manages the list of all active Backends so it can tell all of them to save when the request is shutting down, manages the list of "SessionId" objects for session ID resetting, and holds a few other general-purpose functions like "validateSessionId()" and "generateSessionId()". In the patch as it stands it also has code for auto-creating a user since that code has to be somewhere, but the AuthManager patch moves it all into AuthManager where it more properly belongs.

The Session class itself does almost nothing. Some of the properties of the session depend on the WebRequest used to access it, so Session is a go-between. Session also has user-friendly methods for data access, and once we drop PHP 5.3.3 support it'll have the implementation of ArrayAccess (so $session['foo'] works like $session->get( 'foo' )).

The Backend is really the implementation of the session. It tracks the data and metadata for a particular session, stores the data and metadata to the backend data storage, and calls the Provider to get cookies or whatever set.

The Provider does two main things: (1) it looks at an incoming WebRequest and identifies a user-session from it, and (2) it sets cookies or whatever in the WebResponse so following WebRequests will be able to be identified as belonging to the same user-session.

The PHPSessionHandler class makes PHP's session handling (session_start(), session_write_close(), $_SESSION, and so on) into accessing the same data that the Session accessed via SessionManager::getGlobalSession() accesses. It gets a little help from the Manager and from the Backend for keeping things in sync.

SessionInfo is the other slightly-complicated class. Its main purpose is to represent a candidate session from an individual Provider for the Manager to choose from, but it also contains the code to load the metadata from the backend store and to sanity-check the data from the provider against the backend store.

On the security side, I was hoping to clearly see pretty clearly where the countermeasures from https://www.owasp.org/index.php/Session_Management_Cheat_Sheet were done. It feels like I have to trace through a lot of classes to figure out what's going on. But maybe with some naming changes or comments, we can make those easier to find.

Unfortunately this "cheat sheet" doesn't clearly lay out these countermeasures for me to go over. It seems like more of a white paper than a cheat sheet, really.

Looking through their section headers,

• "Session ID Name Fingerprinting" is the same as in current MediaWiki, in that the code uses the same methods to come up with the same cookie names that MediaWiki currently uses.
• "Session ID length": SessionManager::validateSessionId() insists on at least 32 characters (likely 128 or 160 bits of data). And the only way to create a session is via a SessionInfo, which calls this in its constructor.
• "Session ID entropy"
  • For Providers that can handle arbitrary session IDs, it's taken care of by SessionManager::generateSessionId() which uses MWCryptRand to generate 160-bit session IDs.
  • Some of the Providers might not be able to handle an arbitrary session ID, though. For these, SessionProvider::hashToSessionId() allows them to specify the request-data they do have to identify the user-session and will hmac it with $wgSecretKey to get a 160-bit session ID (or 128-bit if for some reason the only available hmac hash is md5).
• "Built-in Session Management Implementations", sorry, we can't because the whole point is to do something more flexible than what PHP's cookie-based implementation provides.
• "Used vs. Accepted Session ID Exchange Mechanisms" OTOH, since we wrote this ourself we know exactly what mechanisms are accepted. The whole point of a SessionProvider is to implement a "Session ID Exchange Mechanism".
• "Transport Layer Security" is mainly up to MediaWiki, although we provide for the forceHTTPS flag as session metadata.
• "Cookies" are done in the same way they're currently done in MediaWiki, no change there.
• "Session ID Generation and Verification: Permissive and Strict Session Management" was probably be "permissive" in that it wouldn't immediately reset the ID that was passed in from the user if it wasn't known to be safe. I'm fixing that now.
  • I'm defining "safe" as (1) we just generated it, (2) we found it in the backend store, or (3) the provider being used doesn't handle arbitrary session IDs.
• "Manage Session ID as Any Other User Input" SessionManager::validateSessionId() is called from SessionInfo's constructor, and you need a SessionInfo to create a SessionBackend.
• "Renew the Session ID After Any Privilege Level Change" That's up to MediaWiki. wfResetSessionID() still exists, although $session->resetId() is the preferred way to do it.
• "Considerations When Using Multiple Cookies", the thing they describe as "common" doesn't seem common to me. But we have validation of the User and Token cookies (or more specifically, the user that results from processing those cookies) versus the session metadata baked into SessionInfo as part of the sanity checks.
• "Session Expiration" is a section that seems horribly confused. We just do the same thing we've always done: set an expiry when storing the session data into the backend BagOStuff, set the session cookie's expiration to "at end of session", and refresh both as the user does stuff.
• "Manual Session Expiration", yeah, MediaWiki has a logout button. I'm not touching T37220 here. One tricky bit is that we no longer pretend Special:Logout works if you're using something like OAuth.
• "Web Content Caching" is up to whatever MediaWiki currently does, with the exception that I explicitly have the SessionProvider say which cookies and other headers need varying on.
• "Initial Login Timeout" is up to whatever MediaWiki currently does.
• "Force Session Logout On Web Browser Window Close Events" no, I'm not going to screw around with crazy JS trying to do this.
• "Disable Web Browser Cross-Tab Sessions" is a stupid idea for Wikipedia.
• "Automatic Client Logout" is another crazy JS thing I'm not going to mess with.
• "Session ID Guessing and Brute Force Detection" doesn't seem to be something we do now, and I'm not about to try to add it.
• "Detecting Session ID Anomalies" seems like "Hey, use our product!"
• "Binding the Session ID to Other User Properties" if we wanted to do this we'd have deployed SecureSessions, or merged it into core. SecureSessions can still work, there's even some new hooks to probably make it easier.
• "Logging Sessions Life Cycle" I'm happy to add more logger calls or adjust the levels wherever is necessary.
• "Simultaneous Session Logons" Up to other MediaWiki code.

Hope that helps, or at least gives us somewhere to start in our meeting tomorrow.