Skip to content

Conversation

@ldematte
Copy link
Contributor

@ldematte ldematte commented Mar 11, 2025

When we realized we have missed instrumentation of URL.openConnection methods, we tried to see if we could also chase usages of these functions across the codebase, as an alternative to instrument URLConnection classes (the return type). There were many uses, and complex ones, so we ended up doing the URLConnection instrumentation, but we also realized that Elasticsearch still loads the java.desktop module. Collective memory seems to remember we have it for just a couple of encoding/decoding functions; however, since we now "skip" verification for system modules, all calls to file and network operations form java.desktop are permitted, which exposes a huge surface (there might be something exploitable there).

This PR removes the java.desktop module explicitly (with the ability to add more if we need to) from the set of "system modules". We should not need any entitlement for java.desktop (we should barely use it, and not use it for any sensitive operation); however if it turns out that we need let java.desktop perform some sensitive operation, we can explicitly add that back to the server policy, e.g. adding:

new Scope("java.desktop", List.of(new LoadNativeLibrariesEntitlement())),

to serverScopes in EntitlementInitialization.

Testing this with a IT test is difficult/impossible, as we are talking about server policies.
I've tested this manually in the following way:

  • add this code to server (e.g. where we perform self-checks after Entitlement bootstrap):
try {
   var x = ImageIO.read(nodeEnv.configDir().resolve("image.png").toFile());
   logger.info("ImageIO Entitled");
} catch (NotEntitledException e) {
   logger.info("ImageIO Not entitled");
}

(ImageIO is just a "convenient" class I found in java.desktop

  • run it as it is today --> "ImageIO Entitled"
  • add java.desktop to MODULES_EXCLUDED_FROM_SYSTEM_MODULES --> "ImageIO Not entitled"
  • add new Scope("java.desktop", List.of(new LoadNativeLibrariesEntitlement())) to serverScopes in EntitlementInitialization --> "ImageIO Entitled"

I have also:

  • observed with the trace logs that access to config is granted by the basic FileAccessTree policy we have
  • tried to add the entitlement to a modularized plugin: it does not work (it complains that the module is not in the list of this plugin modules, which is correct)
  • tried to add the entitlement to a non-modularized plugin: this of course passes checks
    • but does not grant access (because the module is still considered a (server) module): see next point
  • adding the code above to the plugin makes it fail (this is the only thing we can write a IT test for -- added!)
  • tried to add the entitlement to server: adding the code above to a plugin works correctly (server policy + call to sensitive method from a plugin)

Relates to: ES-11008

@ldematte ldematte added >non-issue auto-backport Automatically create backport pull requests when merged test-entitlements v8.18.1 v8.19.0 v9.0.1 :Core/Infra/Entitlements Entitlements infrastructure labels Mar 11, 2025
@ldematte ldematte marked this pull request as ready for review March 11, 2025 14:17
@ldematte ldematte requested a review from a team as a code owner March 11, 2025 14:17
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 11, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

.stream()
.filter(
m -> systemModulesDescriptors.contains(m.getDescriptor())
&& MODULES_EXCLUDED_FROM_SYSTEM_MODULES.contains(m.getName()) == false
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we filter this out of systemModuleDescriptors instead? Otherwise, that set isn't quite right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand your point; systemModuleDescriptors is a local variable. We can filter it out from systemModuleDescriptors, or from here. Am I missing something? Does it make a difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, there's no functional difference, but it seems odd to create an incorrect value and then account for that incorrectness later.

It's not a huge deal, since the returned value is unaffected, as you say.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, one could argue that the first line is simply collecting whatever ModuleFinder.ofSystem() returns, and it's the second line that's responsible for building the list of system modules that Entitlements needs.

So I'm ok either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, one could argue that the first line is simply collecting whatever ModuleFinder.ofSystem() returns

That's exactly what I was thinking :)

@ldematte ldematte merged commit d844c6a into elastic:main Mar 12, 2025
22 checks passed
@ldematte ldematte deleted the entitlements/exclude-system-modules branch March 12, 2025 07:35
ldematte added a commit to ldematte/elasticsearch that referenced this pull request Mar 12, 2025
…4563)

* exclude java.desktop from system modules

* add IT test
ldematte added a commit to ldematte/elasticsearch that referenced this pull request Mar 12, 2025
…4563)

* exclude java.desktop from system modules

* add IT test
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.18
8.x
9.0

ldematte added a commit to ldematte/elasticsearch that referenced this pull request Mar 12, 2025
…4563)

* exclude java.desktop from system modules

* add IT test
elasticsearchmachine pushed a commit that referenced this pull request Mar 12, 2025
…124620)

* exclude java.desktop from system modules

* add IT test
elasticsearchmachine pushed a commit that referenced this pull request Mar 12, 2025
…124619)

* exclude java.desktop from system modules

* add IT test
elasticsearchmachine pushed a commit that referenced this pull request Mar 12, 2025
…124618)

* exclude java.desktop from system modules

* add IT test
albertzaharovits pushed a commit to albertzaharovits/elasticsearch that referenced this pull request Mar 13, 2025
…4563)

* exclude java.desktop from system modules

* add IT test
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Mar 13, 2025
…4563)

* exclude java.desktop from system modules

* add IT test
prdoyle added a commit to prdoyle/elasticsearch that referenced this pull request Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Entitlements Entitlements infrastructure >non-issue Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants