Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EclipseLink is missing doPrivileged for setAccessible when parsing JPQL query with EXTRACT #25182

Closed
njr-11 opened this issue May 2, 2023 · 7 comments · Fixed by #26608 or #26720
Closed
Labels

Comments

@njr-11
Copy link
Contributor

njr-11 commented May 2, 2023

JPQL that includes EXTRACT, such as,
... WHERE EXTRACT (HOUR FROM o.start) BETWEEN ?1 AND ?2 AND EXTRACT (MINUTE FROM o.start)=?3
fails when Java 2 security is enabled because EclipseLink attempts method.setAccessible without a doPrivileged,

java.security.AccessControlException: Access denied ("java.lang.reflect.ReflectPermission" "suppressAccessChecks")java.base/java.security.AccessController.throwACE(AccessController.java:177)
java.base/java.security.AccessController.checkPermissionHelper(AccessController.java:239)
java.base/java.security.AccessController.checkPermission(AccessController.java:386)
java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:416)
com.ibm.ws.kernel.launch.internal.MissingDoPrivDetectionSecurityManager.checkPermission(MissingDoPrivDetectionSecurityManager.java:47)
java.base/java.lang.reflect.AccessibleObject.checkPermission(AccessibleObject.java:91)
java.base/java.lang.reflect.Method.setAccessible(Method.java:192)
org.eclipse.persistence.jpa.jpql.parser.AbstractExpression.acceptUnknownVisitor(AbstractExpression.java:247)
org.eclipse.persistence.jpa.jpql.parser.AbstractExpression.acceptUnknownVisitor(AbstractExpression.java:192)
org.eclipse.persistence.jpa.jpql.parser.ExtractExpression.accept(ExtractExpression.java:72)
org.eclipse.persistence.jpa.jpql.parser.BetweenExpression.acceptChildren(BetweenExpression.java:104)
org.eclipse.persistence.jpa.jpql.AbstractValidator.visit(AbstractValidator.java:743)
org.eclipse.persistence.jpa.jpql.parser.AnonymousExpressionVisitor.visit(AnonymousExpressionVisitor.java:81)
org.eclipse.persistence.jpa.jpql.AbstractGrammarValidator.visit(AbstractGrammarValidator.java:2640)
org.eclipse.persistence.jpa.jpql.parser.BetweenExpression.accept(BetweenExpression.java:99)
org.eclipse.persistence.jpa.jpql.AbstractGrammarValidator.validateCompoundExpression(AbstractGrammarValidator.java:2250)
org.eclipse.persistence.jpa.jpql.AbstractGrammarValidator.validateLogicalExpression(AbstractGrammarValidator.java:2472)
org.eclipse.persistence.jpa.jpql.AbstractGrammarValidator.visit(AbstractGrammarValidator.java:2554)
org.eclipse.persistence.jpa.jpql.parser.AndExpression.accept(AndExpression.java:59)
org.eclipse.persistence.jpa.jpql.parser.AbstractSingleEncapsulatedExpression.acceptChildren(AbstractSingleEncapsulatedExpression.java:53)
org.eclipse.persistence.jpa.jpql.AbstractValidator.visit(AbstractValidator.java:743)
org.eclipse.persistence.jpa.jpql.parser.AnonymousExpressionVisitor.visit(AnonymousExpressionVisitor.java:439)
org.eclipse.persistence.jpa.jpql.AbstractGrammarValidator.visit(AbstractGrammarValidator.java:4054)
org.eclipse.persistence.jpa.jpql.parser.SubExpression.accept(SubExpression.java:49)
org.eclipse.persistence.jpa.jpql.AbstractGrammarValidator.validateAbstractConditionalClause(AbstractGrammarValidator.java:1676)
org.eclipse.persistence.jpa.jpql.AbstractGrammarValidator.visit(AbstractGrammarValidator.java:4333)
org.eclipse.persistence.jpa.jpql.parser.WhereClause.accept(WhereClause.java:56)
org.eclipse.persistence.jpa.jpql.parser.AbstractSelectStatement.acceptChildren(AbstractSelectStatement.java:94)
org.eclipse.persistence.jpa.jpql.parser.SelectStatement.acceptChildren(SelectStatement.java:105)
org.eclipse.persistence.jpa.jpql.AbstractValidator.visit(AbstractValidator.java:743)
org.eclipse.persistence.jpa.jpql.parser.AnonymousExpressionVisitor.visit(AnonymousExpressionVisitor.java:399)
org.eclipse.persistence.jpa.jpql.AbstractGrammarValidator.visit(AbstractGrammarValidator.java:3993)
org.eclipse.persistence.jpa.jpql.parser.SelectStatement.accept(SelectStatement.java:100)
org.eclipse.persistence.jpa.jpql.AbstractGrammarValidator.visit(AbstractGrammarValidator.java:3604)
org.eclipse.persistence.jpa.jpql.parser.JPQLExpression.accept(JPQLExpression.java:135)
org.eclipse.persistence.internal.jpa.jpql.HermesParser.validate(HermesParser.java:321)
org.eclipse.persistence.internal.jpa.jpql.HermesParser.populateQueryImp(HermesParser.java:271)
org.eclipse.persistence.internal.jpa.jpql.HermesParser.buildQuery(HermesParser.java:164)
org.eclipse.persistence.internal.jpa.EJBQueryImpl.buildEJBQLDatabaseQuery(EJBQueryImpl.java:141)
org.eclipse.persistence.internal.jpa.EJBQueryImpl.buildEJBQLDatabaseQuery(EJBQueryImpl.java:117)
org.eclipse.persistence.internal.jpa.EJBQueryImpl.<init>(EJBQueryImpl.java:104)
org.eclipse.persistence.internal.jpa.EJBQueryImpl.<init>(EJBQueryImpl.java:88)
org.eclipse.persistence.internal.jpa.EntityManagerImpl.createQuery(EntityManagerImpl.java:1726)
org.eclipse.persistence.internal.jpa.EntityManagerImpl.createQuery(EntityManagerImpl.java:1749)
@njr-11 njr-11 added the in:JPA label May 2, 2023
@jgrassel
Copy link
Contributor

jgrassel commented May 10, 2023

Can you reproduce the problem using one of my jpa projects that I put on the internal github in JPA/Support (or something named similarly to that, can't quite remember the name.), and put it up on github? @tkburroughs can point you to the exact repo I'm talking about.

@njr-11
Copy link
Contributor Author

njr-11 commented May 10, 2023

Yes we can easily reproduce this (just by disabling the hack that adds doPriv from Jakarta Data code and running the Jakarta Data test bucket). Given how simple the fix should be (just adding a doPriv in the right place), I thought it would be an excellent initial commit for whomever picks up EclipseLink work once it is figured out who that will be.

@tkburroughs
Copy link
Member

EclipseLinke AbstractExpression.acceptUnknownVisitor is failing here:

            Method visitMethod = type.getDeclaredMethod("visit", parameterType);
            visitMethod.setAccessible(true);
            visitMethod.invoke(visitor, this);

The setAccessible call needs to be in a doPriv. EclipseLink has helper methods for this; probably could use PrivilegedAccessHelper.invokeMethod to accomplish this; wrapping that call in a doPriv, something like this:

        PrivilegedAccessHelper.callDoPrivilegedWithException(
                () -> PrivilegedAccessHelper.invokeMethod(visitMethod, visitor, this)
            );

This would replace both the setAccessible as well as the invokeMethod, and is nice because it only really uses a doPriv if needed, and also only calls setAccessible if needed.

@tkburroughs
Copy link
Member

This failure can be seen by running the following FAT : io.openliberty.data.internal_fat

However, you must first remove the doPriv that was added to workaround this issue in

/io.openliberty.data.internal.persistence/src/io/openliberty/data/internal/persistence/RepositoryImpl.java

Look for the following TODO:

    // TODO remove doPriv once switched to Java 21 only or EclipseLink bug is fixed

@tkburroughs
Copy link
Member

A fix may be tested in open-liberty by adding an overlay of the fixed code in io.openliberty.persistence.3.1.thirdparty.
Add the file, and update bnd.bnd to include the package under Include-Resource.

@tkburroughs
Copy link
Member

Turns out the org.eclise.persistence.jpa.jpql EclipseLink project does not have visibility to PrivilegedAccessHelper that is in org.eclipse.persistence.core.... so instead I think we should go with:

            Method visitMethod = type.getDeclaredMethod("visit", parameterType);
            if (!visitMethod.isAccessible()) {
            	AccessController.doPrivileged((PrivilegedAction<Void>) () -> {visitMethod.setAccessible(true); return null;});
            }
            visitMethod.invoke(visitor, this);

Generally I think isAccessible will be true, thus avoiding the overhead of the doPriv, except when really needed.

@tkburroughs
Copy link
Member

This has also been fixed in EcliseLink 2.7.14, which will be included in Liberty here:
Update to EclipseLink 2.7.14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants