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

[Sean Mullan]

I looked at the code changes and it seems fine to me.

--Sean

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;
>>      }
>>   
>