Evaluate the permission change for jackson-databind

[ryanbogan]

Coming from #5366 (comment) in order for jackson-databind to be used in the server folder, the SHAs, licenses, and notices had to be removed from all plugins and modules. They still have access to jackson through server, but the discovery-ec2 plugin also had the accessDeclaredMembers and suppressAccessChecks permissions in the build.gradle. For the plugin to function properly with the jackson relocation, these permissions were added to the server build.gradle. We need to evaluate whether there are any unintended consequences of adding these permissions.

Link to jackson relocation PR: #5366

[anasalkouz]

@davidlago, any thoughts?

[dblock]

cc: @peternied

[peternied]

suppressAccessChecks
Warning: Extreme caution should be taken before granting this permission to code, for it provides the ability to access fields and invoke methods in a class. This includes not only public, but protected and private fields and methods as well.

This is dangerous in that information (possibly confidential) and methods normally unavailable would be accessible to malicious code.

From https://docs.oracle.com/javase/7/docs/api/java/lang/reflect/ReflectPermission.html

---

These permissions govern allow for the finding, altering, and executing private, protected, ... etc methods. By allowing these permissions it will be possible for the code in server to bypass access protections. Many serialization libraries rely on this reflection so its usage is not surprising.

Having these permissions creates exposure of the code under server/... via tampering. I'd recommend we remove these permissions since server/... does things like establishing TLS, access control systems, or other security features.

The databind library is used for readFromExtensionsYml(...) in ExtensionsManager. Is this the only place its needed - maybe we can look at an alternative approach? Alternatively, would it be possible to sandbox this usage so the permission doesn't need to be broadly available in code under server/... ?

OpenSearch/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java

Lines 537 to 541 in 57d4485

	private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOException {
	ObjectMapper objectMapper = new ObjectMapper(new YAMLFactory());
	InputStream input = Files.newInputStream(filePath);
	ExtensionsSettings extensionSettings = objectMapper.readValue(input, ExtensionsSettings.class);
	return extensionSettings;

[dblock]

@rmuir @uschindler, since we were just talking about JSM, maybe you have a moment to pitch in your 0.02c here

[uschindler]

You can apply the policy only to a specific jar file, but then you need to make sure that the code there uses AccessController.doPrivilegedvl around the reflection code.

If not you would need to somehow otherwise sandbox it, by delegating all calls to that library sandbox through a proxy that sets the caller domain.

[uschindler]

We had a test in Lucene doing something like this, but this was actually to limit the permissions to run some code without any permission. I have to look it up.

[rmuir]

@rmuir @uschindler, since we were just talking about JSM, maybe you have a moment to pitch in your 0.02c here

As far as the specific permissions, the risk IMO here really involves protection against untrusted code. The vectors for the untrusted code could be things such as scripting features, java serialization, etc. Historically (pre-java 9), if code could just call setAccessible then it is trivial to escape security manager sandbox, just manipulate some private members and you are good to go. But IMO, if code is untrusted and someone wants badly enough to escape the sandbox, they will find a way to do it: one reason we don't run applets in our browsers anymore. So it is best to not be in a situation of executing untrusted code to begin with.

Probably a few things have changed since java 8.x times where we did a lot of cleanup around these permissions:

• vectors seem a lot less scary (e.g. there isn't integrated groovy scripting module any more)
• module system may provide some protection against this kind of stuff (at least for jdk classes) ???

[rmuir]

A couple thoughts when thinking about untrusted code wrt these permissions:

• there is still the untrusted.policy sandbox that has really minimal permissions, for anything considered untrusted code.
• If you have something that is trusted code but still scary, you can sandbox it a bit better with something like the tika sandbox
• maybe adopting java module system (e.g. with plugins and everything) would be a more useful future way forward, to set some boundaries between codebases and prevent things from being able to access private guts.

[dblock]

@rmuir @uschindler For this specific issue, what do you recommend we change in code, if anything?

[uschindler]

The problem here is that you have added the 2 permissions globally to the server/src/main/resources/org/opensearch/bootstrap/security.policy, this is indeed very risky as tis now allows anything in Opensearch to make private members available.

As said before there are 2 variants to overcome this:

• If jackson correctly uses AccessController#doPrivileged() to access those private APIs, then you can do it in a similar way like for lucene-core.jar: grant codeBase "${codebase.lucene-core}" {.
• If Jackson does not use AccessController on relevant code paths (I hope it does), the only workaround is to add a proxy class fro all relevant calls to jackson that do deep reflection and do the doPrivileged in the proxy. This proxy class should go to a separate JAR file and only give this JAR file the permission.

Uwe

[uschindler]

Hi,
I checked this locally, unfortunately, jackson-databind is broken-to-hell™, and it forcefully tries to call setAccessible() on everything except JDK classes. The problem is now that it does NOT warp those calls into AccessController.doPrivileged(). See tho code here: https://github.com/FasterXML/jackson-databind/blob/8db6d1d8e1793bbcb5b8d69ae8cb7fa0de2b25fa/src/main/java/com/fasterxml/jackson/databind/util/ClassUtil.java#L980-L1017 (this does not have any security guards with AcessController#doPrivileged so it inherits top-level permissions and those are limited on server level of elasticsearch.

I added the jackson-databind as a separate policy, but for the reason before I wasn't able to pass org.opensearch.extensions.ExtensionsManagerTests, it still throws ex.

The "correct" way to fix this would be this draft PR: #5767

My recommendation: Avoid Jackson Databind, sorry!

Alternative ways to fix this: Enclose any calls to jackson-databind into a wrapper JAR file that guards everything with AccessController#doPrivileged and that one "owns" the codebase. The Policy files's codebase is always determined from the most inner AccessController#doPrivileged() guard around security senstitive calls. So basically all methods called by Elasticsearch should be guarded and the "ownership" of codebase should be taken by a wrapper JAR file which can get correct permissions. If this is not possible, e.g. in those plugins, those should run the usual way.

Another alternative is to add a plugin like "jackson-databind-plugin" that is loaded in its own classloader like all plugins and that puts jackson ito a sandbox. But I am not sure if this is worth the trouble, my suggestion: kill broken jackson-databind. It is curretly only used in Elasticsearch plugin for special cases but it hsould NOT be added to core.

[uschindler]

FYI, I have no time to work on this, I just wanted to give you some options.

The PR also shows the problem of this relaxing of policy that shortly after adding the additional highly debatable permission to Elasticsearch server's bootstrap policy the first one added a broken test: WriteableSettingTests.java

This test adds SuppressForbidden and then calls setAccessible(). After I have seen that I got disappointed and gave up here. You opened the can of worns by this and now you are also ignoring forbiddenapis that complained already!

[uschindler]

Another option would be to revert the move of jackson to "server" (I would really recommend this) and instead make the ExtensionManager a plugin in its own codebase with a sandbox. This also allows to better control what it is doing.

So I'd suggest to revert #5366.

P.S.: There was a reason why the original code used jackson-databind only in plugin sandboxes and not globally! The current code has now also only one classloader with a single Jackson instance, so when something goes wrong and somebody deserializes a bad class with jackson the hell is open! RCE,.... like with Java serialization!

[rmuir]

if setAccessible() is allowed everywhere, it will prevent eventually migrating to java 9 modules (you will get InaccessibleObjectException), so it isn't just a security manager issue. And as i mentioned above, I really recommend looking into modules because it can provide some barriers around different codebases which is one thing that security manager did.

I will try not to mention how much work it took to remove this stuff from the core before, i hope there is a really good reason for adding the setAccessible stuff back :(

[uschindler]

As it looks like ExtensionsManager is the only one using that shitty jackson-databind, why not write your own hand-written (de)serializer for this single class org.opensearch.extensions.ExtensionsSettings? It isn't so hard to read write json with Elastic- ähm Opensearch's nice XContent classes and populate your classes with settings.

[peternied]

shitty jackson-databind

Haha!

why not write your own hand-written (de)serializer for this single class org.opensearch.extensions.ExtensionsSettings

+1

[ryanbogan]

I removed both permissions from the policy as part of #5768

[uschindler]

Cool. Plain yaml is much better here.
I think it is now possible to revert #5366 and restore the original dependency graph?

[ryanbogan]

I think it is now possible to revert #5366 and restore the original dependency graph?

Ideally, we would like to keep jackson in the server module without using it to centralize versions for plugins. On the PR right now, a few native plugins are failing due to lack of access checks, even though their security policies grant the correct permissions. Is there a way to keep jackson in server and have the native plugins get the permissions from their own policies?

[rmuir]

i don't think its good to keep this library in core: nuke it with a passion. and any plugins depending on it, fix them too. setAccessible is bad news... the code uwe linked to is... (there are no adjectives in any language to properly communicate).

it is not just about security permissions, again java 9+ modules will barf on this stuff too. move away from it... run, don't walk.

[uschindler]

Since Java 9, all serialization libs need to be revised at some point. whenever you serialize stuff in private fields, the correct way to do this is now by using a correct lookup instantiated using MethodHandles.lookup() by the class to serialize itself and that passed to the serializer to use it to access private stuff with same privileges like the caller (method handles instead of reflection). The problem is that all those libs like this libcowboy ähm jackson-databind are very old and should be rewritten for modern java, but the apis would dramatically change.

So only way is to avoid them, because module system breaks with them due to encapsulation.

[dblock]

@uschindler Thank you so much for insisting on this important issue, really appreciate your time.

@ryanbogan since @uschindler said he isn't taking this to its logical conclusion whichever one it is, could you please followup on ^ and do the rightful?

[ryanbogan]

Now that Jackson has been removed from the server module, I'm attempting to find a solution that can mirror the functionality that jackson-databind provides. The other prominent JSON libraries are GSON and jsonsimple, but GSON also uses the reflective permissions and jsonsimple does not have the capability to match all of jackson's functionality. What is the best alternative to jackson?

[uschindler]

In gson you can disable private access:

• https://javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/GsonBuilder.html#disableJdkUnsafe()
• https://javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/GsonBuilder.html#addReflectionAccessFilter(com.google.gson.ReflectionAccessFilter)
• https://javadoc.io/static/com.google.code.gson/gson/2.10.1/com.google.gson/com/google/gson/GsonBuilder.html#excludeFieldsWithModifiers(int...)

[ryanbogan]

Thanks! I'll do some more research into GSON and see if I can get it to work

[ryanbogan]

@dblock @uschindler @rmuir As a quick update on our progress for this issue, I have replaced all instances of jackson-databind in the SQL plugin as a proof of concept. The new solution utilizes GSON with a reflective access filter. All unit tests are passing locally, but we are still looking into integration test failures. The draft PR here (opensearch-project/sql#1411) contains my implementation, and any feedback would be appreciated. We are currently planning to pause work on this issue for a month until April, at which point I will pick it up again. Are there any concerns with this course of action?

[dblock]

Seems like a reasonable approach @ryanbogan. Hope you get to pick this up soon again, or maybe someone else can finish it up before you get back to it.

[minalsha]

Thanks @dblock . We are pausing this work for now and will pick it up in April, 2023. @ryanbogan will work on this in April. Thanks

[dbwiddis]

Hey @ryanbogan @dblock was there ever any final recommendation coming out of this issue? We're building a new plugin and trying to decide how to parse JSON. Any advice (here or on opensearch-project/flow-framework#46)?