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
