On 17/02/2020 17:31, Carsten Klein wrote: > Hi there, > > finally, I got my first Tomcat enhancement ready. You can view its code > at my Tomcat fork on GitHub: > > https://github.com/cklein05/tomcat/tree/session-manager-persist-authentication > > > Before I'm opening an enhancement in Tomcat's Bugzilla, maybe, Mark and > Christopher (or whoever else is interested), could you please have a > quick look at the code?
I was going to write this at the end, but I decided it would be better up front. Please don't be put off by the number of comments and suggested changes. I think the core idea is sound and meets a valid requirement that some users have. To some extent, the volume of comments reflects that fact I'm responding to a clear proposal and explanation. This is a good thing in my eyes. In the order I thought of them: a) Please don't add author tags. ASF policy is not to add them. The ones you see in the Tomcat codebase pre-date that policy. You will be credited in the changelog. Which brings me on to... b) Please add a changelog entry for this addition. c) Please add this new attribute to the documentation. > 1. The new boolean option 'persistAuthentication' is implemented in > ManagerBase and so can be configured for StandardManager or > PersistentManager (defaults to false). Also added this to > mbeans-descriptor.xml, so it's writable through JMX. +1 > 2. That option is set for any new StandardSession upon creation (in > method ManagerBase.createSession(String)). Once a session got this > option, it's not being changed during the session's lifetime. d) Why do it this way as opposed to looking at the current setting in the Manager when the session is persisted? e) I am very much against using system properties for configuration except as a last resort. I don't see why PRINCIPAL_SERIALIZABLE_CHECK can't be a property on the Manager. Although see g) below first. > 3. In StandardSession, method doWriteObject writes authentication > information (authType and principal) only, if 'persistAuthentication' is > true, the session is authenticated (authType or principal != null) and > principal is serializable. f) Would the code be cleaner if these were always written and null values used if authentication persistence is disabled? > 4. The "is principal serializable" test is, by default, a deep > (expensive?) check, which actually serializes the whole principal object > to an ObjectOutputStream backed by a new NullInputStream, which just > discards all bytes written. This check can be skipped by setting system > property > org.apache.catalina.session.StandardSession.PRINCIPAL_SERIALIZABLE_CHECK > to false (defaults to true, however). If skipped, the principal is > considered serializable if (principal instanceof Serializable). That's > how the session's attribute values are checked for being serializable or > not. > > However, that is odd, if such a serialized object has a child object > which is not serializable. In that case, the ObjectOutputStream is left > in an inconsistent state and no so called fatal exception code is > written to the stream (that is, when reading such a stream, no > WriteAbortedException is not thrown for such an error). g) The instanceof Serializable check should be sufficient. It a class has that and then is not serializable that is a bug but not one I think the Manager needs to protect itself from beyond catching and logging the exception (and inserting null into the object stream). > 5. A Boolean object is used as a tag/marker that determines, whether > authentication information is present id the stream or not. If none of > the above conditions are met, both authType and principal are not > serialized (than, only the initial Boolean false marker has been emitted > to the stream). > > BTW, the Boolean false marker is not even required (if there is no > authentication information in the stream) since the reading code works > fine without any Boolean in the stream. So emitting Boolean false for > signalling "no auth info" is actually optional (we could consider > omitting it). h) See comment f). > 6. When sessions are loaded, ManagerBase provides a > org.apache.catalina.util.CustomObjectInputStream instance to read > sessions from. That instance is configured with the session's > sessionAttributeValueClassNamePattern property. This essentially defines > the classes, session attribute values may consist of. This pattern > defaults to "java\\.lang\\.(?:Boolean|Integer|Long|Number|String)", so > only attributes with these simple types can be loaded from a session. > > That filter pattern is only in effect, if a security manager is active, > or if a pattern has been configured for the manager (e.g. in context.xml). > > Currently, however, all session data (including its base properties like > creationTime, isNew, isValid etc) is loaded with that filter mechanism > in place. Since those so called 'scalar instance properties' actually > only consist of those simple types, that was not a problem. > > However, loading the serialized principal from the object stream is now > subject to that filter mechanism (BTW, HA's DeltaManager and > DeltaSession just do not utilize the CustomObjectInputStream). > > Since, as the name implies, the sessionAttributeValueClassNamePattern > applies to attribute values only, I decided to give the > CustomObjectInputStream a boolean 'filterActive' property, which is set > to true, just before StandardSession starts deserializing attributes. > The initial value of 'filterActive' can be specified though the > constructor, to which both StandardManager and StoreBase pass false > (actually, only StandardManager and PersistenManager (through StoreBase) > do use the CustomObjectInputStream class). i) This needs some careful thought. Use of the CustomObjectInputStream was added in response to CVE-2016-0714. /me goes away to think about this... The short version is the above opens up a security vulnerability in limited circumstances. "Safe" principal classes need to be added to the sessionAttributeValueClassNamePattern. I suggest this patch adds the necessary classes for this to work with Tomcat's built-in authentication and leaves CustomObjectInputStream unchanged. The long version is, well, longer. The scenario where this is a problem is where the Tomcat administrator does not trust the applications that are running on the instance. To limit what those applications can do, Tomcat is run under a SecurityManager. Use of CustomObjectInputStream was added in response to CVE-2016-0714 because the sessions are de-serialized under Tomcat's security context (which is higher privileged) rather than the web application's security context (which is typically limited in this scenario). There is an assumption that an attacker is able to control the contents of the session serialization file. With the change above, an attacker can insert a malicious Principal into the session that, when loaded, will perform some malicious action that would normally be blocked by the SecurityManager but will not in this case. The above scenario is fairly unlikely but the current session deserialization is designed to be safe in that scenario (with appropriate configuration) so this patch needs to retain that. > 7. Conclusion > > - Tomcat still builds with that enhancement in place > - The enhancement obviously works as expected > - Minor decisions can still be made (e.g. pull NullInputStream into a > new file, maybe in ...catalina.util) > - What about the Tomcat documentation? Where to document all this? j) At a minimum, new Manager attributes need to be added here: https://github.com/apache/tomcat/blob/master/webapps/docs/config/manager.xml Thanks for working on this. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org For additional commands, e-mail: users-h...@tomcat.apache.org
