harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Geir Magnusson Jr." <g...@pobox.com>
Subject Re: [drlvm][kernel_classes] ThreadLocal vulnerability
Date Fri, 17 Nov 2006 13:56:38 GMT
I grok this.  I have no problem.

geir


Tim Ellison wrote:
> Thomas Hawtin wrote:
>> I had a quick browse through the Harmony SVN and spotted what appears to
>> be a vulnerability in the java.lang.ThreadLocal implementation. I have
>> briefly discussed this with Tim Ellison and Geir Magnusson Jr., off list
>> before posting here.
> 
> Yep, and I'll say again publicly, thank you for looking through the code
> so carefully, and taking the time to report your findings.
> 
>> Harmony uses a per Thread HashMap (WeakHashMap in classlibadapter) to
>> map ThreadLocals onto values. HashMaps (should) check for equality with
>> Object.equals and Object.hashCode instead of == and
>> System.identityHashCode.
> 
> As you correctly point out, Harmony's implementation of
> java.lang.ThreadLocal [2] delegates storing the thread local values to
> the instance of java.lang.Thread to which they apply.
> 
> In Harmony the implementation of Thread is a kernel type, provided
> separately by each VM.
> 
> The code in classlibadapter [1] was an experiment to map between the
> Harmony kernel types and GNU Classpath's VM interface types.  That work
> is not in current usage, and is not being actively maintained.
> 
> The current active kernel code in Harmony is the DRLVM.  Looking at
> DRLVM's version of Thread here [3], it uses a (non-weak) HashMap like this:
> 
>     void setThreadLocal(ThreadLocal<Object> local, Object value) {
>         if (localValues == null) {
>             localValues = new HashMap<ThreadLocal<Object>, Object>();
>         }
>         localValues.put(local, value);
>     }
> 
> [1]
> http://svn.apache.org/viewvc/incubator/harmony/enhanced/classlibadapter/trunk/modules/kernel/src/main/java/java/lang/Thread.java?view=markup
> [2]
> http://svn.apache.org/viewvc/incubator/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/lang/ThreadLocal.java?revision=451251&view=markup
> [3]
> http://svn.apache.org/viewvc/incubator/harmony/enhanced/drlvm/trunk/vm/vmcore/src/kernel_classes/javasrc/java/lang/Thread.java?revision=471005&view=markup
> 
>> Malicious subclasses of ThreadLocal can override hashCode to run through
>> all possible hash codes, extracting all the ThreadLocals present in the
>> current thread through an overridden equals. Some of these ThreadLocals
>> may contain sensitive values. Even if Harmony generates identity hash
>> codes entirely at random, the process should be completable in the order
>> of a few minutes of CPU time.
>>
>> Tim Ellison suggests replacing the HashMap with an IdentityHashMap. I
>> agree that this would fix the security vulnerability.
> 
> Anyone else care to comment on this as a proposed fix?
> 
>> Some modern code,
>> such as I believe Spring, creates many ThreadLocal instances, so you may
>> wish to look further at quality of implementation issues.
> 
> Ack -- thanks.  What do you call "many"?   100's? 1,000s? more?
> 
>> FWIW, I believe early versions of Sun's 1.3 J2SE suffered a similar
>> problem.
> 
> Regards,
> Tim
> 

Mime
View raw message