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

AccessController, marked deprecated for removal - remove or remain "AccessController.doPrivileged" (based on research of #1263) #1267

Closed
speckyspooky opened this issue Apr 15, 2023 · 4 comments
Assignees
Milestone

Comments

@speckyspooky
Copy link
Contributor

Hello Together,
based on the discussion (#1263) and my reserach I saw an eclipse hint that the usage of "AccessController.doPrivileged()"
is deprecated and marked for removal in an upcoming Java version.
I checked the latest documentation of JDK 20 and the class is given and listed again "only" like deprecated.
Afterwards I check the BIRT-master and we have over 100entries with the usage of doPrivileged()-call,
e.g.:

  • FunctionProviderBaseImpl (2times)
  • SecurityUtil (34times)

The final question is the action according of this fiding:

  1. Nothing/remain and we will wait thill the method is removed in full
  2. Remove the calls without special privileged grant handling
  3. Remove the calls with special privileged grant handling

I haven't any idea of option 3 because currently there is no replacement or newer Verson of the SecurityManager in planning.
So it seems to me that option 1 or 2 would be the way. My favorit would option 2.

Any opinions are welcome to this point of security and the next steps.

Example according point 2.:

	public void setApplicationClassLoader(final ClassLoader appLoader, Context context) {
		if (appLoader == null) {
			return;
		}
		ClassLoader loader = appLoader;
		try {
			appLoader.loadClass("org.mozilla.javascript.Context");
		} catch (ClassNotFoundException e) {
			**loader = (new RhinoClassLoaderDecoration(appLoader, FunctionProviderImpl.class.getClassLoader()));**
		}
		context.setApplicationClassLoader(loader);
	}

Documentation:

Java 20: Deprecated List:
https://docs.oracle.com/en/java/javase/20/docs/api/deprecated-list.html

Java 17: JDK-Ticket
https://bugs.openjdk.org/browse/JDK-8264713

@hvbtup
Copy link
Contributor

hvbtup commented Apr 17, 2023

I don't understand the example. Can you give a bit more context?

Apart from that, I'm +1 for option 2. The idea of "sandboxed Java" is dead. Today, there are other ways to protect programs running on the same machine from each other, e.g. virtual machines, docker containers, ...

@speckyspooky
Copy link
Contributor Author

No problem the example is the corrected version,
here the unchanged part of "FunctionProviderBaseImpl" with the deprecated method "AccessController.doPrivileged".

	public void setApplicationClassLoader(final ClassLoader appLoader, Context context) {
		if (appLoader == null) {
			return;
		}
		ClassLoader loader = appLoader;
		try {
			appLoader.loadClass("org.mozilla.javascript.Context");
		} catch (ClassNotFoundException e) {
			loader = AccessController.doPrivileged(new PrivilegedAction<ClassLoader>() {

				@Override
				public ClassLoader run() {
					return new RhinoClassLoaderDecoration(appLoader, FunctionProviderImpl.class.getClassLoader());
				}
			});
		}
		context.setApplicationClassLoader(loader);
	}

Therefore the idea is onyl to reduce the class loader at the end by removing the "AccessController.doPrivileged"
and the result will be:

loader = new RhinoClassLoaderDecoration(appLoader, FunctionProviderImpl.class.getClassLoader());

So for me is important to talk about the option and the change so that we are fine with it.

@hvbtup
Copy link
Contributor

hvbtup commented Apr 18, 2023

OK, understood.

Can you explain what option 3 would look like? Do you mean something like:
Create a new class org.eclipse.birt.security.AccessController which is a dummy implementation of AccessController and just always allows doPrivileged, and replace the imports of java.security.AccessController with org.eclipe.birt.security.AccessController throughout BIRT's source code?

This would make sense if we think that the future will bring a replacement for AccessController.

But I don't think so, so I'd still choose option 2.

@speckyspooky
Copy link
Contributor Author

The option 3) would be (in theory) a alternate Controller like a replacement, if some one would be able to do this.
The official sides write that there is no replacement from official side in planning.

My favorit would be "option 2)" and remove the current usage of AccessController at all.

speckyspooky added a commit to speckyspooky/birt that referenced this issue May 23, 2023
speckyspooky added a commit that referenced this issue May 25, 2023
…emoval" (#1267) (#1290)

* Removed "AccessController.doPrivileged()" due to marked on Java-side as
"deprecated for removal" (#1267)

* Updated URL class loader and test case class
@speckyspooky speckyspooky added this to the 4.14 milestone May 26, 2023
@speckyspooky speckyspooky self-assigned this May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants