A senior developer from the company I work for had the following
comments (in support of Tomcat) regarding the very interesting threading
post from Lloyd Chambers last week.
****
This is an interesting post, but not for the reason that the author
intended. I'm sure he's an awesome developer who totally understands
this stuff. And the findings might be serious problems. But if they
are real, *he blew the writeups (to the Tomcat dev list).*
Reread these findings -- they're all theoretical. I guess they all sort
of represent "violation of best practice". If you ever write that it
just means that you can't find an actual problem to report. Every one
of these findings raises obvious questions that the finding should have
answered. *Always answer "who cares"*
· How is SingleSignOn actually instantiated? (my guess is that there
aren't lots of threads at startup).
· Are the "public" get/setRequireReauthentiation() methods actually
called from any different threads? (my guess is that this doesn't make
a bit of difference).
· Are there any classes that extend SingleSignOn? (if not then who
cares about subclasses modifying non-finals).
· What actually calls findSessions() and does it modify the array? (my
guess is not)
The conclusion that SSO is a "disaster waiting to happen" *is the kind
of FUD (fear uncertainty doubt) talk that undermines the whole point.*
The reference to "Java Concurrency in Practice" is probably
well-intentioned, but just comes off snotty.
We face these dangers in our writings (as an Application Security
company) as well. Always try to put yourself in the customer's *(in this
case, the Tomcat dev team) *shoes and really explain more than just the
narrow-scope technical problem. If you're using the words this guy is
using -- something is wrong with your findings:
· "Perhaps"
· "Could"
· "Presumably"
· "What if"
· "Bug-prone if"
--
Jim Manico, Senior Application Security Engineer
[EMAIL PROTECTED] | [EMAIL PROTECTED]
(301) 604-4882 (work)
(808) 652-3805 (cell)
Aspect Security^(TM)
Securing your applications at the source
http://www.aspectsecurity.com
(post from Lloyd Chambers)
First, I'm an experienced developer, and well-versed in Java threading.
My main work is on the Glassfish project at Sun.
I've been looking into the code of
org.apache.catalina.authenticator.SingleSignOn to see how it works, and
I've noticed a number of thread-safety bugs. My understanding is that
Valve SingleSignOn could run on any thread (eg any incoming request).
1. Most of the instance variables are not 'final'. So when
SingleSignOn is instantiated, there is no guarantee that the instance
will be seen correctly by any other thread; it's state is undefined from
the point of view of another thread.
Its immutable variable values (eg the Maps) should be 'final' so as to
benefit from the JVM guarantee of thread safety for 'final' instance
variables. And/or there needs to be an external synchronizing
side-effect that would make the object instance "visible" to other
threads. Perhaps there exists such an external side effect that masks
this bug.
2. The get/set methods are thread-unsafe. For example,
getRequireReauthentication() and setRequireReauthentication() are not
'synchronized', and the variable 'requireReauthentication' is not
'volatile'. As a result, different threads could see different values
and/or never see an updated value. Since these are public methods, and
presumably can be called from any number of different threads, this is a
problem.
3. Variables 'lifecycle' and 'reverse' and 'cache' are not 'final'. See
#1 above, but this also exposes/encourages another thread-safety issue:
what if the variables themselves are changed by a subclass (since
SingleSignOn is not a 'final' class).
4. The Map classes use external synchronization, which is bug-prone if
any modifications are made (and there might be some race conditions here
as well). The java.util.concurrent.ConcurrentHashMap class is much more
scalable and doesn't have these risks.
5. Method deregister() (both variants) contain a race condition whereby
the call sso.findSessions(). Method findSessions() is 'synchronized',
but *returns its internal array 'sessions'*!!! Upon return there is no
further thread-safety; 'sessions' is now exposed and anything looking at
it is now subject to a race condition and/or "visibility" problem.
These are almost certainly not all the issues. From what I can see,
SingleSignOn is a disaster waiting to happen on the wrong hardware,
where caches are not write-through as they are on most of today's
processors. And even on common hardware (eg Intel), there are still
some "windows of opportunity" for failures to occur, especially item #5
above.
The class SingleSignOnEntry is also not thread-safe; for example
updateCredentials() is thread-unsafe.
I recommend "Java Concurrency in Practice" for understanding these issues.
Lloyd Chambers
http://diglloyd.com </exchweb/bin/redir.asp?URL=http://diglloyd.com/>
[Mac OS X 10.5.2 Intel, Tomcat 6.0.16]
