Skip to content

Conversation

Naros
Copy link
Member

@Naros Naros commented Jul 11, 2018

https://hibernate.atlassian.net/browse/HHH-12542

@gsmet @raphw - Would you guys be able to check my changes to the ByteBuddy implementation to see if that makes sense for the signed jar issue that you guys worked on as a part of HHH-12618? We need to make sure that the proxy we create uses the ORM protection domain and not the target class so that when reflective methods are invoked, the proper access controls are allowed.

@gbadner You may want to compare this with your PR #2280 if you need it backported to 5.1.

@gsmet
Copy link
Member

gsmet commented Jul 12, 2018

@Naros my issue is more with https://hibernate.atlassian.net/browse/HHH-12614 and the original forum post https://discourse.hibernate.org/t/securityexception-with-bytebuddy-and-signed-application-jars/816 .

The original error was:

Caused by: java.lang.SecurityException: class "com.fred.flintstone.entity.task.TestRunTask$HibernateProxy$yYbBct5p"'s signer information does not match signer information of other classes in the same package
	at java.lang.ClassLoader.checkCerts(Unknown Source)

So if I understand that correctly, it means that we cannot generate classes in a package if the protection domain is not consistent with the other classes of the package, which is why I chose to use the protection domain of the original class.

As for the MethodHandles approach, I suspect the protection domain of the lookup will be used. Thus your issue.

If we want to use the protection domain of an Hibernate class, I suspect we will have to generate the proxies in an Hibernate package.

What's the issue you have exactly when keeping the protection domain of the original class?

@Naros
Copy link
Member Author

Naros commented Jul 12, 2018

@gsmet The error I ran into was this

Caused by: org.hibernate.HibernateException: HHH000142: Bytecode enhancement failed: org.jboss.as.test.integration.hibernate.naturalid.Person
	at org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyFactory.getProxy(ByteBuddyProxyFactory.java:142)
	at org.hibernate.tuple.entity.AbstractEntityTuplizer$1.run(AbstractEntityTuplizer.java:706)
	at java.security.AccessController.doPrivileged(Native Method)
	at org.hibernate.tuple.entity.AbstractEntityTuplizer.createProxy(AbstractEntityTuplizer.java:703)
	at org.hibernate.persister.entity.AbstractEntityPersister.createProxy(AbstractEntityPersister.java:4787)
	at org.hibernate.event.internal.DefaultLoadEventListener.createProxyIfNecessary(DefaultLoadEventListener.java:360)
	at org.hibernate.event.internal.DefaultLoadEventListener.proxyOrLoad(DefaultLoadEventListener.java:275)
	at org.hibernate.event.internal.DefaultLoadEventListener.doOnLoad(DefaultLoadEventListener.java:122)
	at org.hibernate.event.internal.DefaultLoadEventListener.onLoad(DefaultLoadEventListener.java:90)
	at org.hibernate.internal.SessionImpl.fireLoad(SessionImpl.java:1257)
	at org.hibernate.internal.SessionImpl.access$1900(SessionImpl.java:207)
	at org.hibernate.internal.SessionImpl$IdentifierLoadAccessImpl.doGetReference(SessionImpl.java:2814)
	at org.hibernate.internal.SessionImpl$IdentifierLoadAccessImpl.getReference(SessionImpl.java:2793)
	at org.hibernate.internal.SessionImpl$NaturalIdLoadAccessImpl.getReference(SessionImpl.java:3159)
	at org.jboss.as.test.integration.hibernate.naturalid.SFSBHibernateSFNaturalId.getPersonReference(SFSBHibernateSFNaturalId.java:100)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.jboss.as.ee.component.ManagedReferenceMethodInterceptor.processInvocation(ManagedReferenceMethodInterceptor.java:52)
	at org.jboss.invocation.InterceptorContext.proceed(InterceptorContext.java:422)
	at org.jboss.invocation.InterceptorContext$Invocation.proceed(InterceptorContext.java:509)
	at org.jboss.as.weld.interceptors.Jsr299BindingsInterceptor.doMethodInterception(Jsr299BindingsInterceptor.java:90)
	at org.jboss.as.weld.interceptors.Jsr299BindingsInterceptor.processInvocation(Jsr299BindingsInterceptor.java:101)
	at org.jboss.as.ee.component.interceptors.UserInterceptorFactory$1.processInvocation(UserInterceptorFactory.java:63)
	at org.jboss.invocation.InterceptorContext.proceed(InterceptorContext.java:422)
	at org.jboss.as.ejb3.component.invocationmetrics.ExecutionTimeInterceptor.processInvocation(ExecutionTimeInterceptor.java:43)
	at org.jboss.invocation.InterceptorContext.proceed(InterceptorContext.java:422)
	at org.jboss.as.jpa.interceptor.SBInvocationInterceptor.processInvocation(SBInvocationInterceptor.java:47)
	at org.jboss.invocation.InterceptorContext.proceed(InterceptorContext.java:422)
	at org.jboss.as.jpa.interceptor.SFSBInvocationInterceptor.processInvocation(SFSBInvocationInterceptor.java:57)
	at org.jboss.invocation.InterceptorContext.proceed(InterceptorContext.java:422)
	at org.jboss.as.ejb3.tx.StatefulBMTInterceptor.handleInvocation(StatefulBMTInterceptor.java:94)
	... 190 more
Caused by: java.lang.ExceptionInInitializerError
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at java.lang.Class.newInstance(Class.java:442)
	at org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyFactory.getProxy(ByteBuddyProxyFactory.java:135)
	... 222 more
Caused by: java.security.AccessControlException: WFSM000001: Permission check failed (permission "("java.lang.RuntimePermission" "accessDeclaredMembers")" in code source "(vfs:/content/hibernate4naturalid_test.ear/lib/entities.jar <no signer certificates>)" of "ModuleClassLoader for Module "deployment.hibernate4naturalid_test.ear" from Service Module Loader")
	at org.wildfly.security.manager.WildFlySecurityManager.checkPermission(WildFlySecurityManager.java:295)
	at org.wildfly.security.manager.WildFlySecurityManager.checkPermission(WildFlySecurityManager.java:192)
	at java.lang.Class.checkMemberAccess(Class.java:2348)
	at java.lang.Class.getDeclaredMethod(Class.java:2127)
	at org.jboss.as.test.integration.hibernate.naturalid.Person$HibernateProxy$vNZZ9U3E.<clinit>(Unknown Source)

After talking with @dmlloyd he suggested we had a couple options; however, first we needed to ensure that the bytebuddy generated class has a protection domain which is equal to the generating code (Hibernate) rather than the target code. Presently what was happening is exactly the opposite, we were using the target code's protection domain rather than ORM's.

After that we had two options:

  1. Generate a doPrivilegedBlock
  2. Add code that initializes the class immediately after being generated in a privileged block
  3. Do not use getDeclaredMethod inside of the of the generated class

I found that by making sure that the generated class had the same protection domain as ORM that we did not need any further handling from what I saw.

I'm fine taking a different path if someone has any ideas or suggestions.

@gsmet
Copy link
Member

gsmet commented Jul 12, 2018

After talking with @dmlloyd he suggested we had a couple options; however, first we needed to ensure that the bytebuddy generated class has a protection domain which is equal to the generating code (Hibernate) rather than the target code. Presently what was happening is exactly the opposite, we were using the target code's protection domain rather than ORM's.

It might well be the case as I'm not well versed in this subject but I'm not sure it's doable. As mentioned, in the case of a signed jar, we need the protection domain to be consistent with the one of the other classes of the package. So if we generate the proxy in the same package as the entity, we need it to have the same protection domain.

I don't know if it's feasible to generate the proxies in a different package, an Hibernate one, not sure if we need access to protected methods.

@gsmet
Copy link
Member

gsmet commented Jul 13, 2018

@dmlloyd @Naros

So given the constraints of having the protection domain consistent with the package to support signed jars, we have 2 solutions:

  • generate the proxy classes in an Hibernate hosted package and use the protection domain of Hibernate. The consequence of this is that we will lose the access to any package protected method of the entity class (think of a package protected default constructor for instance). If this is a configuration supported by Hibernate then it's a no-no.
  • generate the proxy classes in the entity package and use the protection domain of this package. In this case, the issue is that the user will need to give the related permissions to its user code and not only Hibernate. And we will need a privileged block. That being said @dmlloyd, I understand you had a strong case against using the protection domain of the entity package, what is the issue exactly?

The last solution is to explicitly not support signed jars.

@Naros
Copy link
Member Author

Naros commented Jul 13, 2018

@gsmet

We definitely support package private constructors and accessor methods on entity classes, so I agree that the former option you mention is a no-go. Hopefully @dmlloyd has a chance to comment on the second as I'm not sure what the best path here is.

@dmlloyd
Copy link
Contributor

dmlloyd commented Jul 13, 2018

The case against using the PD of the entity is simply that the user may not wish to grant it reflection permission. If you're okay with that though, then it's probably fine.

TBH I think the "best" solution is probably option 3 above: do not use getDeclaredMethod inside the generated class, and instead pass it in or provide a way for the generated class to acquire it securely.

@dmlloyd
Copy link
Contributor

dmlloyd commented Jul 13, 2018

Two ways to pass things in to a class initializer:

  1. Use a ThreadLocal (ugly but effective)
  2. Keep a registry of generated classes and provide a public method which acquires the calling class and determines if it is allowed to access the privileged data. On Java 8 you use Reflection.getCallerClass() and Java 9 uses StackWalker, so you need JDK-specific (MR JAR) code for this solution.

@gsmet
Copy link
Member

gsmet commented Jul 13, 2018

This is what the ByteBuddy proxy looks like:

public class ProxyTest$SimpleEntity$HibernateProxy$7tVpOdgl
implements ProxyConfiguration {
    private ProxyConfiguration.Interceptor $$_hibernate_interceptor;
    private static final /* synthetic */ Method cachedValue$7i1hAKhy$4cscpe1;
    private static final /* synthetic */ Method cachedValue$7i1hAKhy$5j4bem0;
    private static final /* synthetic */ Method cachedValue$7i1hAKhy$7m9oaq0;
    private static final /* synthetic */ Method cachedValue$7i1hAKhy$9pqdof1;

    public boolean equals(Object object) {
        return (Boolean)ProxyConfiguration.InterceptorDispatcher.intercept((Object)this, (Method)cachedValue$7i1hAKhy$5j4bem0, (Object[])new Object[]{object}, (Object)false, (ProxyConfiguration.Interceptor)this.$$_hibernate_interceptor);
    }

    public String toString() {
        return (String)ProxyConfiguration.InterceptorDispatcher.intercept((Object)this, (Method)cachedValue$7i1hAKhy$4cscpe1, (Object[])new Object[0], (Object)null, (ProxyConfiguration.Interceptor)this.$$_hibernate_interceptor);
    }

    public int hashCode() {
        return (Integer)ProxyConfiguration.InterceptorDispatcher.intercept((Object)this, (Method)cachedValue$7i1hAKhy$9pqdof1, (Object[])new Object[0], (Object)0, (ProxyConfiguration.Interceptor)this.$$_hibernate_interceptor);
    }

    protected Object clone() throws CloneNotSupportedException {
        return ProxyConfiguration.InterceptorDispatcher.intercept((Object)this, (Method)cachedValue$7i1hAKhy$7m9oaq0, (Object[])new Object[0], (Object)null, (ProxyConfiguration.Interceptor)this.$$_hibernate_interceptor);
    }

    public void $$_hibernate_set_interceptor(ProxyConfiguration.Interceptor interceptor) {
        this.$$_hibernate_interceptor = interceptor;
    }

    static {
        cachedValue$7i1hAKhy$4cscpe1 = Object.class.getDeclaredMethod("toString", new Class[0]);
        cachedValue$7i1hAKhy$5j4bem0 = Object.class.getDeclaredMethod("equals", Object.class);
        cachedValue$7i1hAKhy$7m9oaq0 = Object.class.getDeclaredMethod("clone", new Class[0]);
        cachedValue$7i1hAKhy$9pqdof1 = Object.class.getDeclaredMethod("hashCode", new Class[0]);
    }
}

Javassist has the exact same pattern except it runs them as privileged action if the SM is enabled.

I don't know exactly which protection domain is used by Javassist though.

We should probably ask Rafael to protect these calls with privileged actions.

And that lets us with deciding which is better:

  • not supporting signed jars
  • requiring the user to give reflection permissions to his own code - which kinda defeats the purpose of the SM

@dmlloyd
Copy link
Contributor

dmlloyd commented Jul 16, 2018

Is it always just those four methods? Because those could/should easily be cached somewhere. In fact looking them up every time kind of stinks, really. More memory footprint, more CPU consumption on class init.

@gsmet
Copy link
Member

gsmet commented Jul 16, 2018

Nope, all the proxied entity methods will be there.

@gsmet
Copy link
Member

gsmet commented Jul 16, 2018

@dmlloyd FWIW, I created raphw/byte-buddy#493 .

What I would recommend:

  • let's check what is the protection domain with Javassist (Chris was checking that on Friday) and let's try to be consistent in our ByteBuddy implementation;
  • let's commit all the fixes that make it work with Javassist. Chris checked that he was able to run the SM test with Javassist.

Then we can start the discussion of how we will support the SM in WF 14.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added 2 comments and I have one big issue with it: usually, we try to check if the security manager is enabled before doing a privileged access.

Typically, you have a good example here: https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/jpa/internal/util/PersistenceUtilHelper.java#L380 .

I think we should stick to this pattern.

revInfoCfgResult.getRevisionInfoXmlMapping(),
revInfoCfgResult.getRevisionInfoRelationMapping()
);
this.entitiesConfigurations = AccessController.doPrivileged( new PrivilegedAction<EntitiesConfigurations>() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this call be more fine grained? It sounds weird we need to run all the configure code in a privileged block?

}
final Callback[] callbacks = resolveEntityCallbacks( entityXClass, callbackType, reflectionManager );

final Callback[] callbacks = AccessController.doPrivileged( new PrivilegedAction<Callback[]>() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the granularity of this call.

@gsmet
Copy link
Member

gsmet commented Jul 18, 2018

Squashed and merged, thanks!

@gsmet gsmet closed this Jul 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants