-
30. Re: Field injection
adrian.brock Mar 17, 2008 9:23 AM (in response to alesj)"adrian@jboss.org" wrote:
"adrian@jboss.org" wrote:
public void setField(Field field)
{
this.field = field;
if (isPublic() == false && field != null)
field.setAccessible(true);
}
Actually its not a security hole, because you have done it properly.
The setAccessible will fail depending upon the who the caller is.
In case its not clear, I'm saying it should be in a privileged block
these objects get cached and shared across threads. -
31. Re: Field injection
alesj Mar 17, 2008 9:23 AM (in response to alesj)"adrian@jboss.org" wrote:
Also I don't see a test in the kernel project that is validating
that you can't use the xml deployment (or programmatic deployment)
to bypass the private field access.
Not yet there. ;-)
Wanted to clear the Signature issue before hacking all the tests.
Will add that to the list of tests. -
32. Re: Field injection
alesj Mar 17, 2008 9:28 AM (in response to alesj)"adrian@jboss.org" wrote:
In case its not clear
Nope, not clear. :-)
OK, I'm glad I didn't introduce security hole :-), but I don't see why your example will fail with the current code?
And you're saying this field.setAcceessible should be in privileged block? -
33. Re: Field injection
adrian.brock Mar 17, 2008 9:43 AM (in response to alesj)"alesj" wrote:
"adrian@jboss.org" wrote:
In case its not clear
Nope, not clear. :-)
OK, I'm glad I didn't introduce security hole :-), but I don't see why your example will fail with the current code?
It won't fail with the current code.
And you're saying this field.setAcceessible should be in privileged block?
Yes, this object gets cached and used across threads. You can't guarantee
that the caller will be able to setAccessible(), but they still want to be able
to use the FieldInfo (even if they can't invoke on it).
Write some tests for the security stuff then you'll understand.
e.g. A caller doesn't have permission to setAccessible()
but still wants to generate a BeanInfo for the class (without the private fields). -
34. Re: Field injection
alesj Mar 17, 2008 9:50 AM (in response to alesj)"adrian@jboss.org" wrote:
"alesj" wrote:
"adrian@jboss.org" wrote:
In case its not clear
Nope, not clear. :-)
OK, I'm glad I didn't introduce security hole :-), but I don't see why your example will fail with the current code?
It won't fail with the current code.
It won't fail, but it's still not a security hole?
Why?
Since the one who already wanted to call setAccessible on the initial field won't be able to do that anyway via RFII, where the caller and the field instance are the same? -
35. Re: Field injection
adrian.brock Mar 17, 2008 10:16 AM (in response to alesj)Write the tests then you'll understand.
It's called the "Suck it and see" principle. :-) -
36. Re: Field injection
alesj Mar 20, 2008 7:24 AM (in response to alesj)"adrian@jboss.org" wrote:
Write the tests then you'll understand.
OK, the test is here, but it doesn't behave as I expect it to behave. :-(public void testFieldAcess() throws Throwable { final FieldsClass tester = new FieldsClass(); final ReflectFieldInfoImpl impl = new ReflectFieldInfoImpl(); SecurityManager sm = suspendSecurity(); try { // e.g. I can't do setAccesible - although you can here, with the current disabled security Field myField = FieldsClass.class.getDeclaredField("privString"); impl.setField(myField); // So I'll use this hole } finally { resumeSecurity(sm); } Field field = impl.getField(); // This should have an access check field.set(tester, "foobar"); assertEquals("foobar", tester.getPrivString()); Runnable runnable = new Runnable() { public void run() { try { Field fi = impl.getField(); // This should have an access check fi.set(tester, "something"); // this should check for caller } catch (Throwable t) { throw new RuntimeException(t); } } }; ErrorHolderThread other = new ErrorHolderThread(runnable); other.start(); other.join(); // we should get an error here - but we don't :-( assertNotNull("Should get access restriction exception.", other.getError()); RuntimeException re = assertInstanceOf(other.getError(), RuntimeException.class); Throwable cause = re.getCause(); assertNotNull(cause); assertInstanceOf(cause, AccessControlException.class, false); }
-
37. Re: Field injection
alesj Mar 20, 2008 8:15 AM (in response to alesj)"alesj" wrote:
"adrian@jboss.org" wrote:
Write the tests then you'll understand.
OK, the test is here, but it doesn't behave as I expect it to behave. :-(
I thought I had it, but nope. :-(
A different test code, similar wondering:public void testFieldAcessFromMain() throws Throwable { final FieldsClass tester = new FieldsClass(); final ReflectFieldInfoImpl impl = new ReflectFieldInfoImpl(); // I can't do setAccesible Field field = FieldsClass.class.getDeclaredField("privString"); // let's try accessible try { field.setAccessible(true); fail("Should not be here."); } catch (Throwable t) { assertInstanceOf(t, AccessControlException.class); } // ok, setAccessible not set, so set should also fail try { field.set(tester, "foobar"); fail("Should not be here."); } catch (Throwable t) { assertInstanceOf(t, IllegalAccessException.class); } impl.setField(field); // So I'll use this hole field = impl.getField(); // This should have an access check // why does this work?!? field.set(tester, "foobar"); assertEquals("foobar", tester.getPrivString());
-
38. Re: Field injection
adrian.brock Mar 20, 2008 9:02 AM (in response to alesj)Don't worry about it, I'll write the tests and fix any problem(s) found.
Just confirm to me that it works besides the potential security hole? :-) -
39. Re: Field injection
alesj Mar 20, 2008 9:10 AM (in response to alesj)"adrian@jboss.org" wrote:
Don't worry about it, I'll write the tests and fix any problem(s) found.
Thanks. :-)"adrian@jboss.org" wrote:
Just confirm to me that it works besides the potential security hole? :-)
So far it looks good.
All of the tests - except for these :-) - behave as expected.
And there is still that issue with AOP proxies and field injection:
- http://www.jboss.org/index.html?module=bb&op=viewtopic&t=132169 -
40. Re: Field injection
adrian.brock Mar 20, 2008 9:32 AM (in response to alesj)"alesj" wrote:
And there is still that issue with AOP proxies and field injection:
- http://www.jboss.org/index.html?module=bb&op=viewtopic&t=132169
Do you or Kabir have a test?
Kabir says it is fixed in latest aop, so it should just be a case of upgrading to that
and testing it.
I'm fine with doing a beta release without the test as long as it is tested and
working by CR1. -
41. Re: Field injection
alesj Mar 20, 2008 9:37 AM (in response to alesj)"adrian@jboss.org" wrote:
"alesj" wrote:
And there is still that issue with AOP proxies and field injection:
- http://www.jboss.org/index.html?module=bb&op=viewtopic&t=132169
Do you or Kabir have a test?
I wrote the test yesterday:
- http://anonsvn.jboss.org/repos/jbossas/projects/microcontainer/trunk/aop-mc-int/src/tests/org/jboss/test/microcontainer/beans/test/FieldAccessTestCase.java -
42. Re: Field injection
adrian.brock Mar 21, 2008 1:20 AM (in response to alesj)I've fixed the security hole problems in the private field access,
although I haven't tested every use case.
I rewrote (deleted one) your tests since they were broken/making incorrect assumptions
about how it should work. The one I deleted I tried to fix but it was like playing a game
of "kaplunk" ;-)
See the FieldsAccessControlTestCase in the controller tests for how to write it
properly. It basically deploys two files (one during bootstrap which isn't
subject to security and one manually that is).
Implementation and a WARNING:
The access to private fields is now correctly controlled inside the
ReflectFieldInfoImpl with final methods that nobody can override.
The only way the MC can get it wrong is by accessing fields when it is not
using the controller context's AccessControlContext. i.e. it is running under
its own privileges instead of whoever tried to register the bean.
So the other use cases still need testing to make sure this does not happen
(either now or in the future). -
43. Re: Field injection
kabirkhan Mar 25, 2008 7:38 AM (in response to alesj)"adrian@jboss.org" wrote:
Kabir says it is fixed in latest aop...
Neither Ales nor I am sure what you mean has been fixed? Field interception will not work, and the proxy + delegate will be two different instances