[security-dev 00363]: Re: SecurityException in AnnotationInvocationHandler.getMemberMethods
Joseph D. Darcy
Joe.Darcy at Sun.COM
Thu Oct 23 17:02:01 UTC 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