Re: [drlvm][kernel_classes] ThreadLocal vulnerability
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 local, Object value) {
if (localValues == null) {
localValues = new HashMap, 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
Re: [drlvm][kernel_classes] ThreadLocal vulnerability
Tim Ellison wrote: Thomas Hawtin wrote: 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? Good question. Googling tends to just pick up ThreadLocals assigned to static finals, which aren't so much of an issue. However, a private e-mail from someone well known for threading work contains: "There are web/EE applications that generate massive numbers (up to millions) of ThreadLocals (most are per-instance, not statics), some of which quickly become garbage and must be reclaimed reasonably promptly...". Tom Hawtin
Re: [drlvm][kernel_classes] ThreadLocal vulnerability
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 local, Object value) {
if (localValues == null) {
localValues = new HashMap, 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
--
Tim Ellison ([EMAIL PROTECTED])
IBM Java technology centre, UK.
[drlvm][kernel_classes] ThreadLocal vulnerability
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. 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. 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. Some modern code, such as I believe Spring, creates many ThreadLocal instances, so you may wish to look further at quality of implementation issues. FWIW, I believe early versions of Sun's 1.3 J2SE suffered a similar problem. Tom Hawtin
