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

[Joe Darcy]

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