[security-dev 00363]: Re: SecurityException in AnnotationInvocationHandler.getMemberMethods

Joseph D. Darcy Joe.Darcy at Sun.COM
Thu Oct 23 10:02:01 PDT 2008


Joseph D. Darcy wrote:
> Martin Buchholz wrote:
>> If I had known when I started how hard it would be,
>> I would never have started writing one...
>>   
>
> That is the case with many endeavors ;-)
>
>> Where are annotations tests supposed to go?
>> They seem under-test-covered.
>> I've put it into a new directory, test/java/lang/annotation
>>   
>
> Recently I moved over the existing closed regression tests for 
> annotations to test/java/lang/annotation:
>    
> http://mail.openjdk.java.net/pipermail/jdk6-dev/2008-October/000231.html
> so I'll find a home for your tests in there.  The newly-opened tests 
> will be in the b13 source drop.

Martin, I've reviewed your test and code and Sean Mullan has reviewed 
the code too.  I verified the test fails with the current build and 
passes with change.  I've putback the code changes and test into OpenJDK 
6 b13.  The test lives at 
test/java/lang/annotations/ParameterAnnotations.java; the other 
annotation tests will go in that directory once I move them over in JDK 
7 too.  I think you're clear to commit this fix to the JDK 7 repository too.

-Joe

>
> Thanks,
>
> -Joe
>
>> Please review.  The following fails with
>> current openjdk, passes with proposed fix.
>>
>> /*
>>  * Copyright 2008 Sun Microsystems, Inc.  All Rights Reserved.
>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>  *
>>  * This code is free software; you can redistribute it and/or modify it
>>  * under the terms of the GNU General Public License version 2 only, as
>>  * published by the Free Software Foundation.
>>  *
>>  * This code is distributed in the hope that it will be useful, but 
>> WITHOUT
>>  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>>  * version 2 for more details (a copy is included in the LICENSE file 
>> that
>>  * accompanied this code).
>>  *
>>  * You should have received a copy of the GNU General Public License 
>> version
>>  * 2 along with this work; if not, write to the Free Software 
>> Foundation,
>>  * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>  *
>>  * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa 
>> Clara,
>>  * CA 95054 USA or visit www.sun.com if you need additional 
>> information or
>>  * have any questions.
>>  */
>>
>> /*
>>  * @test
>>  * @bug 6761678
>>  * @summary Check properties of Annotations returned from
>>  * getParameterAnnotations, including freedom from security
>>  * exceptions.
>>  * @author Martin Buchholz
>>  */
>>
>> import java.lang.annotation.Annotation;
>> import java.lang.annotation.ElementType;
>> import java.lang.annotation.Retention;
>> import java.lang.annotation.RetentionPolicy;
>> import java.lang.annotation.Target;
>> import java.lang.reflect.Method;
>> import java.security.Permission;
>> import java.security.Policy;
>> import java.security.ProtectionDomain;
>>
>> @Retention(RetentionPolicy.RUNTIME)
>> @Target({ ElementType.FIELD, ElementType.PARAMETER })
>> @interface Named {
>>   String value();
>> }
>>
>> public class ParameterAnnotations {
>>
>>     // A security policy that differs from the default only in that it
>>     // allows a security manager to be uninstalled.
>>     static class MyPolicy extends Policy {
>>         final Policy defaultPolicy;
>>         MyPolicy(Policy defaultPolicy) {
>>             this.defaultPolicy = defaultPolicy;
>>         }
>>         public boolean implies(ProtectionDomain pd, Permission p) {
>>             return p.getName().equals("setSecurityManager") ||
>>                 defaultPolicy.implies(pd, p);
>>         }
>>     }
>>
>>     public void nop(@Named("foo") Object foo,
>>                     @Named("bar") Object bar) {
>>     }
>>
>>     void test(String[] args) throws Throwable {
>>         // Test without a security manager
>>         test1();
>>
>>         // Test with a security manager
>>         Policy defaultPolicy = Policy.getPolicy();
>>         Policy.setPolicy(new MyPolicy(defaultPolicy));
>>         System.setSecurityManager(new SecurityManager());
>>         try {
>>             test1();
>>         } finally {
>>             System.setSecurityManager(null);
>>             Policy.setPolicy(defaultPolicy);
>>         }
>>     }
>>
>>     void test1() throws Throwable {
>>         for (Method m : thisClass.getMethods()) {
>>             if (m.getName().equals("nop")) {
>>                 Annotation[][] ann = m.getParameterAnnotations();
>>                 equal(ann.length, 2);
>>                 Annotation foo = ann[0][0];
>>                 Annotation bar = ann[1][0];
>>                 equal(foo.toString(), "@Named(value=foo)");
>>                 equal(bar.toString(), "@Named(value=bar)");
>>                 check(foo.equals(foo));
>>                 check(! foo.equals(bar));
>>             }
>>         }
>>     }
>>
>>     //--------------------- Infrastructure ---------------------------
>>     volatile int passed = 0, failed = 0;
>>     void pass() {passed++;}
>>     void fail() {failed++; Thread.dumpStack();}
>>     void fail(String msg) {System.err.println(msg); fail();}
>>     void unexpected(Throwable t) {failed++; t.printStackTrace();}
>>     void check(boolean cond) {if (cond) pass(); else fail();}
>>     void equal(Object x, Object y) {
>>     if (x == null ? y == null : x.equals(y)) pass();
>>     else fail(x + " not equal to " + y);}
>>     static Class<?> thisClass = new 
>> Object(){}.getClass().getEnclosingClass();
>>     public static void main(String[] args) throws Throwable {
>>         try {thisClass.getMethod("instanceMain",String[].class)
>>                 .invoke(thisClass.newInstance(), (Object) args);}
>>         catch (Throwable e) {throw e.getCause();}}
>>     public void instanceMain(String[] args) throws Throwable {
>>     try {test(args);} catch (Throwable t) {unexpected(t);}
>>     System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed);
>>     if (failed > 0) throw new AssertionError("Some tests failed");}
>> }
>>
>> Martin
>>
>> On Mon, Oct 20, 2008 at 19:01, Joe Darcy <Joe.Darcy at sun.com> wrote:
>>  
>>> PS Martin, I haven't written tests that setup and use a security 
>>> manager,
>>> etc., that should go along with this fix.  Having a stand-alone 
>>> regression
>>> test along those lines would be helpful.
>>>
>>> Thanks,
>>>
>>> -Joe
>>>
>>> On 10/20/08 06:59 PM, Joe Darcy wrote:
>>>    
>>>> Hello.
>>>>
>>>> On 10/16/08 12:46 PM, Martin Buchholz wrote:
>>>>      
>>>>> Hi all,
>>>>>
>>>>> This is a bug report with fix.
>>>>> Joe Darcy, please file a bug and review this change,
>>>>>
>>>>>         
>>>> I've filed 6761678 "(ann) SecurityException in
>>>> AnnotationInvocationHandler.getMemberMethods" for this issue.  The 
>>>> problem
>>>> seems similar to 6370080 "(ann) Method.getAnnotations() sometimes 
>>>> throw
>>>> SecurityException: doPrivileged or javadoc missing?," which was 
>>>> fixed in JDK
>>>> 6 and a JDK 5 update release.
>>>>
>>>> Martin, can you, Toby, and Josh review any other uses of reflection 
>>>> in the
>>>> src/share/classes/sun/reflect/annotation package for similar 
>>>> problems so we
>>>> can address any other such issues now?
>>>>
>>>> I've looked over the change and the use of getMemberMethods in 
>>>> equalsImpl
>>>> and don't see a problem.  However, I'd like the security team to 
>>>> give it a
>>>> once over too; security-dev folk, please take a look at this.
>>>>
>>>> Thanks,
>>>>
>>>> -Joe
>>>>      
>>>>> and perhaps provide a small test case (it is impractical
>>>>> to share the test we have at Google).
>>>>>
>>>>> Description:
>>>>>
>>>>> sun/reflect/annotation/AnnotationInvocationHandler.java.getMemberMethods 
>>>>>
>>>>> might throw if there is a security manager that does not allow
>>>>> getDeclaredMethods.
>>>>>
>>>>> The author of this code (Josh Bloch) confirms that the intent was 
>>>>> for the
>>>>> doPrivileged block in this method to prevent security exceptions.
>>>>> The methods cannot escape to untrusted code.
>>>>>
>>>>> Evaluation:
>>>>>
>>>>> Yes.  Fix provided courtesy of Toby Reyelts and Josh Bloch at Google.
>>>>>
>>>>> # HG changeset patch
>>>>> # User martin
>>>>> # Date 1224185752 25200
>>>>> # Node ID 68730f05449cd4f39ce1cb82adc6c4e57f87554f
>>>>> # Parent  214ebdcf7252d4862449fe0ae295e6c60a127315
>>>>> SecurityException in AnnotationInvocationHandler.getMemberMethods
>>>>> Summary: Move call to getDeclaredMethods inside doPrivileged
>>>>> Reviewed-by:
>>>>> Contributed-by: jjb at google.com
>>>>>
>>>>> diff --git
>>>>> a/src/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java 
>>>>>
>>>>>
>>>>> b/src/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java 
>>>>>
>>>>> ---
>>>>> a/src/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java 
>>>>>
>>>>> +++
>>>>> b/src/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java 
>>>>>
>>>>> @@ -272,14 +272,14 @@
>>>>>      */
>>>>>     private Method[] getMemberMethods() {
>>>>>         if (memberMethods == null) {
>>>>> -            final Method[] mm = type.getDeclaredMethods();
>>>>> -            AccessController.doPrivileged(new 
>>>>> PrivilegedAction<Void>() {
>>>>> -                public Void run() {
>>>>> -                    AccessibleObject.setAccessible(mm, true);
>>>>> -                    return null;
>>>>> -                }
>>>>> -            });
>>>>> -            memberMethods = mm;
>>>>> +            memberMethods = AccessController.doPrivileged(
>>>>> +                new PrivilegedAction<Method[]>() {
>>>>> +                    public Method[] run() {
>>>>> +                        final Method[] mm = 
>>>>> type.getDeclaredMethods();
>>>>> +                        AccessibleObject.setAccessible(mm, true);
>>>>> +                        return mm;
>>>>> +                    }
>>>>> +                });
>>>>>         }
>>>>>         return memberMethods;
>>>>>     }
>>>>>
>>>>>         
>>>     
>




More information about the security-dev mailing list