Open the pull request in your own fork... this link should work: https://github.com/cklein05/tomcat/compare/cklein05:master...cklein05:session-manager-persist-authentication?expand=1
On Tue, Feb 18, 2020 at 6:42 AM Carsten Klein <c.kl...@datagis.com> wrote: > Mark, > > > 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... > > +1 OK > > > > > b) Please add a changelog entry for this addition. > > Where is the changelog? Sorry, didn't find anything so far... > > > > > c) Please add this new attribute to the documentation. > > +1 OK > > > > >> 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? > > That was my first idea, too. However, the StandardSession only knows a > Manager (interface). I could have added the get/setPersistAuthentication > methods to that interface (but didn't want to introduce such > far-reaching changes) or need to test with instanceof for every session > being written. > > Furthermore, some locking mechanism would be a good idea while sessions > are stored, so that the option cannot be changed while a bunch of > sessions is currently persisted (StandardManager synchronizes with > sessions itself, however, PersistentManager does not). Actually a new > lock monitor would be ideal. > > On the other side, one will likely not re-decide whether sessions shall > be persisted or not from one day to another. I guess, that setting will > remain quit constant for a certain installation, so my solution seemed > not much worse. > > However, a simple instanceof ManageBase is also cheap and saves the > setters and getters in StandardSession. Shall I refactor it acoordingly? > > > > > 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. > > No longer needed, if a simple instanceof Serializable is enough. My idea > was, that this option is true by default (a Principal is not a big class > and the dry-run test should be fast enough). So, I did not see that this > option was worth a config option (initially, that was just an internal > constant). > > > > >> 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? > > The idea was to be able to read the old-style object stream as well (a > file created with the version without that enhancement). But, this can > be achieved even without the Boolean tag. I will make that a bit more > straight. > > > > >> 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). > > +1 OK > > > > >> 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). > > +1 OK > > > > >> 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. > > +1 OK > > > I suggest this patch adds the > > necessary classes for this to work with Tomcat's built-in authentication > > and leaves CustomObjectInputStream unchanged. > > There are several places where Tomcat's SerializablePrincipal class name > needs to be added to the pattern: > > 1. ManagerBase (line 213/214) > 2. context.xml of webapps/host-manager > 3. context.xml of webapps/manager > > Shall I add the SerializablePrincipal class to all? > > > Carsten > > --------------------------------------------------------------------- > To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org > For additional commands, e-mail: users-h...@tomcat.apache.org > > -- Jonathan | exabr...@gmail.com Pessimists, see a jar as half empty. Optimists, in contrast, see it as half full. Engineers, of course, understand the glass is twice as big as it needs to be.
