On Sep 9, 2013, at 12:57 PM, Peter Levart wrote:
>
> On 09/09/2013 07:02 PM, David Chase wrote:
>> Take this lightly informed suggestion with a grain of salt, but why not, for
>> purposes of performance and security,
>> change the logging-specific getCallerClass methods so that their "class"
>> references are instead wrapped in some sort of proxy object that only
>> forwards certain operations quickly without a security check? For example,
>> equals, hashcode, and toString are probably not security-sensitive.
>>
>> i.e.
>> class SafeClass {
>> private final Class clazz;
>> public SafeClass(Class clazz) { this.clazz = class; }
>> public String toString() { return clazz.toString(); }
>> public int hashCode() { return clazz.hashCode(); }
>> public boolean equals(Object o) { return clazz.equals(o); }
>> public Class maybeWeLetYouLookAtTheRealClass() { ... a bunch of security
>> checks ... }
>> }
>>
>> If necessary, do the same for classloaders.
>> And them, no security checks needed, as long as the "safe" methods are
>> enough to get the job done.
>
> Hi,
>
> This is a good idea. It got me thinking that there are a bunch of methods in
> j.l.Class that are not security-sensitive. So instead of proxy-ing those in a
> wrapper SafeClass, let's just identify "unsafe" methods 1st. If the security
> checks that are planned for obtaining j.l.Class instances from call-stack are
> transferred to those unsafe methods instead, then holding a reference to a
> j.l.Class instance becomes a security-nonissue.
Yes! This is what I have been saying. If there are unsafe methods in Class<?>
not protected from restricted access, then security checks need to be added to
*those* methods. Otherwise, once malicious code gets a Class<?> all bets are
off. Don't restrict access to the Class<?>, just to its unsafe methods
(instantiation, invoking Constructors, invoking Methods, etc.).
> Just a quick example - presumably the "unsafe" methods of j.l.Class are:
> get(Declared)Method(s), get(Declared)Field(s) and get(Declared)Constructor(s)
I would actually argue that those aren't unsafe. The *invoke* methods on
Constructors/Methods are what are unsafe.
> , because they enable you to call/access public methods/fields/constructors
> of a class represented by j.l.Class object. If this class is from a
> restricted package (say sun.misc) then you could get access to restricted
> methods/fields/instances. Now if the security checks for obtaining reflective
> object would include checking the "class visibility/restrictability", then
> j.l.Class object of say sun.misc.Unsafe class would not represent any
> security issue. It's just about ******delaying******* the security check.
> What do you think?
(asterisks mine) Yes. Please. Let's do this. :-)
> Are there any other security-sensitive j.l.Class methods? Are there any
> public methods in JDK that take j.l.Class instances and delegate to internal
> logic assuming that the caller can only pass-in security-pre-checked
> j.l.Class instances?
A legitimate concern that would need to be researched to identify if this will
be a problem.
> Regards, Peter
>
>> David
>>
>> On 2013-09-09, at 10:54 AM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>
>>> Nick,
>>>
>>> Do you have any information to some of the questions I asked below that can
>>> help the API discussion?
>>>
>>> We need to decide on the permission check and also the hotspot change has
>>> to be integrated and the jdk change will need to wait until it shows in a
>>> promoted build. Schedule-wise, to make JDK 8, we should finalize the API
>>> this week so that you can update the patch soon.
>>>
>>> Mandy
>>>
>>> On 9/3/2013 5:02 PM, Mandy Chung wrote:
>>>> Nick,
>>>>
>>>> I skimmed through the changes. Congratulations for your first patch
>>>> making changes in both hotspot and jdk code.
>>>>
>>>> In my mind, the Log4J use case accessing Class instance to obtain
>>>> additional information for diagnosability is different than the use case
>>>> of obtaining the caller's class / loader to lookup resources. While the
>>>> new APIs might be in the same class, I will discuss them individually and
>>>> keep us focus one at a time.
>>>>
>>>> Ralph has pointed out [1] that Log4j also needs the ability to find an
>>>> appropriate ClassLoader which is used for logging separation (thank you
>>>> Ralph). That will be the caller-sensitivity (i.e. obtain caller's
>>>> class/loader) discussion.
>>>>
>>>> There are a couple of RFEs:
>>>> https://bugs.openjdk.java.net/browse/JDK-4942697
>>>> https://bugs.openjdk.java.net/browse/JDK-6349551
>>>>
>>>> Performance is critical for Log4j to traverse the stack trace and obtain
>>>> Class information. I like your patch to speed up the generation of
>>>> StackTraceElement[] (and StackTraceFrame[] - essentially same code with
>>>> different type). java.util.logging.LogRecord has workaround the
>>>> performance overhead and go to a specific frame directly and avoid the
>>>> cost of generating the entire array. JDK-6349551 requests to lazily
>>>> instantiate the StackTraceElement object unless that frame is requested.
>>>> Does Log4J always walk all frames and log all information? Do you just
>>>> log only some max number of frames rather than the entire stack trace?
>>>>
>>>> Class<?> getDeclaringClass() method is the key method you need to enhance
>>>> the diagnosability. This method opens up a new way to access a Class
>>>> instance that untrusted code wouldn't be able in the past. A permission
>>>> check is needed as Alan points out early. Performance as well as logging
>>>> framework can't access all class loaders are two factors to be considered
>>>> when defining the permission check.
>>>>
>>>> I saw your other comment about permission check (cut-n-paste below). It
>>>> seems to me that you don't agree a permission check is needed for the
>>>> getDeclaringClass() method (regardless of which class it belongs to) and
>>>> if that's the case, no point to continue.
>>>>
>>>> I want to make it very clear that I have agreed to take this on and
>>>> provide a replacement of sun.reflect.Reflection.getCallerClass(int) in JDK
>>>> 8 to address the use cases. It will take time for the API and security
>>>> discussion and please be prepared for that (also I am working on other
>>>> things at the same time).
>>>>
>>>> The second comment on your patch is that there are lot of duplicated code
>>>> in hotspot in order to support two similar but different types
>>>> (StackTraceFrame and StackTraceElement). Serialization is the reason
>>>> leading you to have a new StackTraceFrame class. Maybe some refactoring
>>>> can help but this is the cost of having the VM directly creating the
>>>> instance. One other option, as suggested in the previous thread, is to
>>>> keep the declaring class in the StackTraceElement as a transient field.
>>>> If we add the getDeclaringClass method in the StackTraceElement class, it
>>>> would need to specify to be optional that it only returns the Class
>>>> instance when it's available.
>>>>
>>>> There are currently three different ways to get a stack trace:
>>>> 1. Throwable.getStackTrace()
>>>> 2. Thread.getStackTrace() and Thread.getAllStackTraces()
>>>> 3. java.lang.management.ThreadMXBean.getThreadInfo(long id, int
>>>> maxDepth).getStackTrace() and multiple thread IDs version
>>>> (a) local (b) remote
>>>>
>>>> Since it's a new StackTraceFrame class, you have to provide a new method
>>>> replacing #1 and #2. I don't see any need to provide a new API equivalent
>>>> to Thread.getAllStackTraces() and java.lang.management.
>>>>
>>>> The proposal I originally have last week was to keep declaring class as
>>>> transient and add a method in StackTraceElement with a permission check at
>>>> every call. Tom raises a good question about the cost of permission check.
>>>> Would that be a concern to Log4j? Is log4j on bootclasspath or extension
>>>> directory? I assume not. So for log4j to work with security manager
>>>> installed, it would have torequire the application to grant certain
>>>> permissions - can you confirm? For example it calls
>>>> sun.reflect.Reflection.getCallerClass method that will require
>>>> RuntimePermission("accessClassInPackage.sun.reflect") permission. Calling
>>>> Class.getProtectionDomain and Class.getClassLoader() requires
>>>> RuntimePermission("getProtectionDomain") and
>>>> RuntimePermission("getClassLoader") respectively. That gives me an
>>>> impression that permission check on individual stack frame might be a
>>>> non-issue?
>>>>
>>>> Mandy
>>>> [1]
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-September/020525.html
>>>>
>>>> On 9/3/13 5:24 AM, Nick Williams wrote:
>>>>> On Sep 3, 2013, at 4:07 AM, Alan Bateman wrote:
>>>>>>> I'm not voicing any objection to any kind of security/permissions
>>>>>>> checks in these methods. Before I can pass judgement one way or
>>>>>>> another, I'd want to know 1) specifically what type of permissions
>>>>>>> check you are looking for, and 2) what you're looking to achieve with
>>>>>>> said permissions check.
>>>>>> I would say this is TBD and start by asking the question as to whether
>>>>>> there are concerns about leaking reference to Class objects that
>>>>>> untrusted code wouldn't normally be able to get a reference to. Tom
>>>>>> brings up the cost of the permission check and also whether any API
>>>>>> should be tied to class loader. There are clearly discussion points here
>>>>>> that could potentially influence the API.
>>>>> As I have said before, there are MANY ways to get a Class object that
>>>>> aren't security checked. It's when you try to USE that class object to
>>>>> impersonate it or invoke methods that security checks begin to happen. As
>>>>> they should!
>>>>>
>>>>> Nick
>>>> On 9/1/13 1:16 AM, Nick Williams wrote:
>>>>> I have completed and am proposing a patch for replacing
>>>>> sun.reflect.Reflection#getCallerClass(...) with a public API in Java 8. I
>>>>> saw no point in duplicating an issue, even though it's over 10 years old,
>>>>> so I'm indicating that this is a patch for 4851444
>>>>> (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4851444).
>>>>>
>>>>> I welcome and solicit support/+1s and a sponsor. Someone about a month
>>>>> ago had mentioned that they would be willing to be a sponsor if the patch
>>>>> looked good, but I can't remember who it was and I can't find the email.
>>>>> I want to say it was someone with RedHat, but my memory could be faulty,
>>>>> so please don't hold it against me if I'm wrong.
>>>>>
>>>>> *Summary of Changes*
>>>>> Added the new class java.lang.StackTraceFrame. It's a virtual mirror of
>>>>> StackTraceElement, except that it contains a Class<?> declaringClass
>>>>> property instead of a String className property. Since the list members
>>>>> expressed reluctance to add methods to Thread and Throwable,
>>>>> StackTraceFrame also contains several static methods for retrieving
>>>>> Classes and StackTraceFrames from the stack. These methods are as follows:
>>>>>
>>>>> Class<?> getCallerClass(): Retrieves the class of the caller of the
>>>>> method calling getCallerClass(). This is identical to the new
>>>>> Reflection#getCallerClass() added in Java 7u25/8.
>>>>>
>>>>> Class<?> getCallerClass(int): Retrieves the class of the caller n frames
>>>>> down the stack. This is identical to the old
>>>>> Reflection#getCallerClass(int) that was deprecated in Java 7u25 and
>>>>> removed in Java 8.
>>>>>
>>>>> StackTraceFrame getCallerFrame(): Retrieves the StackTraceFrame of the
>>>>> line of code that called the method calling getCallerClass(). This is
>>>>> similar to the new Reflection#getCallerClass() added in Java 7u25/8,
>>>>> except that it returns a StackTraceFrame.
>>>>>
>>>>> StackTraceFrame getCallerFrame(int): Retrieves the StackTraceFrame of the
>>>>> caller n frames down the stack. This is similar to the old
>>>>> Reflection#getCallerClass(int), except that it returns a StackTraceFrame.
>>>>>
>>>>> StackTraceFrame[] getCurrentStackTrace(): Functionally equivalent to
>>>>> Thread#getStackTrace(), except that it returns an array of
>>>>> StackTraceFrames.
>>>>>
>>>>> StackTraceFrame[] getStackTrace(Throwable throwable): Functionally
>>>>> equivalent to Throwable#getStackTrace(), except that it returns an array
>>>>> of StackTraceFrames. It uses the same save point (backtrace) created when
>>>>> the Throwable is created that Throwable#getStackTrace() uses when it's
>>>>> first called. It caches the array of StackTraceFrames in the Throwable
>>>>> just like the array of StackTraceElements are cached, so that multiple
>>>>> calls for the same Throwable are fast.
>>>>>
>>>>> As a result of this change, sun.reflect.CallerSensitive has been moved to
>>>>> java.lang.CallerSensitive.
>>>>>
>>>>> I spent considerable time reviewing, revising, considering, and testing
>>>>> these changes. I included several unit tests that establish the proper
>>>>> behavior. I also spent considerable time benchmarking the changes. While
>>>>> benchmarking, I noticed some things. First, getCallerClass() and
>>>>> getCallerClass(int) are as fast as their counterparts in
>>>>> sun.reflect.Reflection, and they easily inline when appropriate. Second,
>>>>> getCallerFrame() and getCallerFrame(int) are /almost/ as fast as the
>>>>> Class versions, but there is some additional overhead for the
>>>>> construction of the StackTraceFrame. This is minuscule (1,000,000
>>>>> invocations consume around 500 ms total on my machine). At this point,
>>>>> all of the benchmarking was as expected.
>>>>>
>>>>> However, I then encountered a surprise. The getCurrentStackTrace() and
>>>>> getStackTrace(Throwable) methods executed (again, 1,000,000 times) in
>>>>> about 70% of the time that Thread#getStackTrace() and
>>>>> Throwable#getStackTrace() did, respectively. Theoretically, they should
>>>>> have executed in the same amount of time, not faster. After extensive
>>>>> analysis, I discovered (what I considered to be) a serious flaw in how
>>>>> the stack trace is filled in within Throwable (which also affects how
>>>>> Thread#getStackTrace() works).
>>>>>
>>>>> Instead of simply iterating over the entire save point and filling in the
>>>>> Throwable stack trace in native code (which is what I did when I
>>>>> implemented the StackTraceFrame methods), the Java code in Throwable
>>>>> first called a native method to figure out how deep the stack was, then
>>>>> called another native method once for every frame in the stack to
>>>>> retrieve each element individually. This native method that is called
>>>>> repeatedly iterates over the entire backtrace once for each call,
>>>>> stopping only when it finds the matching element (so it's O(1) for the
>>>>> first call, O(2) for the second call, O(3) for the third call, and so
>>>>> on). While my StackTraceFrame methods were iterating over the backtrace
>>>>> exactly 1 time (O(n)), the Throwable methods were iterating over the
>>>>> backtrace 1+(n/2) times (worse than O(nlogn) but not as bad as O(n^2)).
>>>>> This problem would not have been extremely apparent over small stack
>>>>> traces (the 30% improvement was a stack trace of 6 elements), but over a
>>>>> large (200+ elements) stack traces the performance difference would be
>>>>> huge and noticeable. Seeing a room for improvement, I refactored the code
>>>>> that fills in the stack trace for Throwable, improving its performance
>>>>> accordingly to match the performance of the StackTraceFrame methods.
>>>>>
>>>>> I'm very pleased with how this turned out, and both the unit tests and my
>>>>> extensive functional testing show that the new class and its methods are
>>>>> working great. I just need someone willing to review and sponsor my patch!
>>>>>
>>>>> *The Code Changes*
>>>>> I couldn't get WebRev to run without all kinds of errors. So I ran `hg
>>>>> diff -g` on every repository in the forest with changes. Here are the
>>>>> four patch files for the four repositories that had changes (no other
>>>>> repositories had changes):
>>>>>
>>>>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8.patch
>>>>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_jdk.patch
>>>>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_hotspot.patch
>>>>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_nashorn.patch
>>>>>
>>>>> I believe I have followed all of the procedures as closely as possible. I
>>>>> await feedback and hope for some support on this, so that we can get a
>>>>> public replacement for this method in Java 8. Let me know if you have any
>>>>> questions.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Nick
>
