Skip to content

Commit 3d8feff

Browse files
ywelschywelsch
authored
Use Java 9 FilePermission model (#26302)
This commit makes the security code aware of the Java 9 FilePermission changes (see #21534) and allows us to remove the `jdk.io.permissionsUseCanonicalPath` system property.
1 parent bdefcbd commit 3d8feff

File tree

9 files changed

+146
-68
lines changed

9 files changed

+146
-68
lines changed

‎buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -549,8 +549,6 @@ class BuildPlugin implements Plugin<Project> {
549549
systemProperty 'tests.artifact', project.name
550550
systemProperty 'tests.task', path
551551
systemProperty 'tests.security.manager', 'true'
552-
// Breaking change in JDK-9, revert to JDK-8 behavior for now, see https://github.com/elastic/elasticsearch/issues/21534
553-
systemProperty 'jdk.io.permissionsUseCanonicalPath', 'true'
554552
systemProperty 'jna.nosys', 'true'
555553
// default test sysprop values
556554
systemProperty 'tests.ifNoTests', 'fail'

‎buildSrc/src/main/resources/forbidden/jdk-signatures.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,3 +107,6 @@ java.util.Collections#EMPTY_MAP
107107
java.util.Collections#EMPTY_SET
108108

109109
java.util.Collections#shuffle(java.util.List) @ Use java.util.Collections#shuffle(java.util.List, java.util.Random) with a reproducible source of randomness
110+
111+
@defaultMessage Avoid creating FilePermission objects directly, but use FilePermissionUtils instead
112+
java.io.FilePermission#<init>(java.lang.String,java.lang.String)

‎core/src/main/java/org/elasticsearch/bootstrap/FilePermissionUtils.java

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.bootstrap;
21+
22+
import org.elasticsearch.common.SuppressForbidden;
23+
24+
import java.io.FilePermission;
25+
import java.io.IOException;
26+
import java.nio.file.Files;
27+
import java.nio.file.Path;
28+
import java.security.Permissions;
29+
30+
public class FilePermissionUtils {
31+
32+
/** no instantiation */
33+
private FilePermissionUtils() {}
34+
35+
private static final boolean VERSION_IS_AT_LEAST_JAVA_9 = JavaVersion.current().compareTo(JavaVersion.parse("9")) >= 0;
36+
37+
/**
38+
* Add access to single file path
39+
* @param policy current policy to add permissions to
40+
* @param path the path itself
41+
* @param permissions set of file permissions to grant to the path
42+
*/
43+
@SuppressForbidden(reason = "only place where creating Java-9 compatible FilePermission objects is possible")
44+
public static void addSingleFilePath(Permissions policy, Path path, String permissions) throws IOException {
45+
policy.add(new FilePermission(path.toString(), permissions));
46+
if (VERSION_IS_AT_LEAST_JAVA_9 && Files.exists(path)) {
47+
// Java 9 FilePermission model requires this due to the removal of pathname canonicalization,
48+
// see also https://github.com/elastic/elasticsearch/issues/21534
49+
Path realPath = path.toRealPath();
50+
if (path.toString().equals(realPath.toString()) == false) {
51+
policy.add(new FilePermission(realPath.toString(), permissions));
52+
}
53+
}
54+
}
55+
56+
/**
57+
* Add access to path (and all files underneath it); this also creates the directory if it does not exist.
58+
*
59+
* @param policy current policy to add permissions to
60+
* @param configurationName the configuration name associated with the path (for error messages only)
61+
* @param path the path itself
62+
* @param permissions set of file permissions to grant to the path
63+
*/
64+
@SuppressForbidden(reason = "only place where creating Java-9 compatible FilePermission objects is possible")
65+
public static void addDirectoryPath(Permissions policy, String configurationName, Path path, String permissions) throws IOException {
66+
// paths may not exist yet, this also checks accessibility
67+
try {
68+
Security.ensureDirectoryExists(path);
69+
} catch (IOException e) {
70+
throw new IllegalStateException("Unable to access '" + configurationName + "' (" + path + ")", e);
71+
}
72+
73+
// add each path twice: once for itself, again for files underneath it
74+
policy.add(new FilePermission(path.toString(), permissions));
75+
policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", permissions));
76+
if (VERSION_IS_AT_LEAST_JAVA_9) {
77+
// Java 9 FilePermission model requires this due to the removal of pathname canonicalization,
78+
// see also https://github.com/elastic/elasticsearch/issues/21534
79+
Path realPath = path.toRealPath();
80+
if (path.toString().equals(realPath.toString()) == false) {
81+
policy.add(new FilePermission(realPath.toString(), permissions));
82+
policy.add(new FilePermission(realPath.toString() + realPath.getFileSystem().getSeparator() + "-", permissions));
83+
}
84+
}
85+
}
86+
}

‎core/src/main/java/org/elasticsearch/bootstrap/Security.java

Lines changed: 19 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,12 @@
2323
import org.elasticsearch.common.SuppressForbidden;
2424
import org.elasticsearch.common.io.PathUtils;
2525
import org.elasticsearch.common.network.NetworkModule;
26-
import org.elasticsearch.common.settings.Setting;
2726
import org.elasticsearch.common.settings.Settings;
2827
import org.elasticsearch.env.Environment;
2928
import org.elasticsearch.http.HttpTransportSettings;
3029
import org.elasticsearch.plugins.PluginInfo;
3130
import org.elasticsearch.transport.TcpTransport;
3231

33-
import java.io.FilePermission;
3432
import java.io.IOException;
3533
import java.net.SocketPermission;
3634
import java.net.URISyntaxException;
@@ -52,6 +50,9 @@
5250
import java.util.Map;
5351
import java.util.Set;
5452

53+
import static org.elasticsearch.bootstrap.FilePermissionUtils.addDirectoryPath;
54+
import static org.elasticsearch.bootstrap.FilePermissionUtils.addSingleFilePath;
55+
5556
/**
5657
* Initializes SecurityManager with necessary permissions.
5758
* <br>
@@ -240,33 +241,34 @@ static void addClasspathPermissions(Permissions policy) throws IOException {
240241
throw new RuntimeException(e);
241242
}
242243
// resource itself
243-
policy.add(new FilePermission(path.toString(), "read,readlink"));
244-
// classes underneath
245244
if (Files.isDirectory(path)) {
246-
policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", "read,readlink"));
245+
addDirectoryPath(policy, "class.path", path, "read,readlink");
246+
} else {
247+
addSingleFilePath(policy, path, "read,readlink");
247248
}
248249
}
249250
}
250251

251252
/**
252253
* Adds access to all configurable paths.
253254
*/
254-
static void addFilePermissions(Permissions policy, Environment environment) {
255+
static void addFilePermissions(Permissions policy, Environment environment) throws IOException {
255256
// read-only dirs
256-
addPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.binFile(), "read,readlink");
257-
addPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.libFile(), "read,readlink");
258-
addPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.modulesFile(), "read,readlink");
259-
addPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.pluginsFile(), "read,readlink");
260-
addPath(policy, "path.conf'", environment.configFile(), "read,readlink");
257+
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.binFile(), "read,readlink");
258+
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.libFile(), "read,readlink");
259+
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.modulesFile(), "read,readlink");
260+
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.pluginsFile(), "read,readlink");
261+
addDirectoryPath(policy, "path.conf'", environment.configFile(), "read,readlink");
261262
// read-write dirs
262-
addPath(policy, "java.io.tmpdir", environment.tmpFile(), "read,readlink,write,delete");
263-
addPath(policy, Environment.PATH_LOGS_SETTING.getKey(), environment.logsFile(), "read,readlink,write,delete");
263+
addDirectoryPath(policy, "java.io.tmpdir", environment.tmpFile(), "read,readlink,write,delete");
264+
addDirectoryPath(policy, Environment.PATH_LOGS_SETTING.getKey(), environment.logsFile(), "read,readlink,write,delete");
264265
if (environment.sharedDataFile() != null) {
265-
addPath(policy, Environment.PATH_SHARED_DATA_SETTING.getKey(), environment.sharedDataFile(), "read,readlink,write,delete");
266+
addDirectoryPath(policy, Environment.PATH_SHARED_DATA_SETTING.getKey(), environment.sharedDataFile(),
267+
"read,readlink,write,delete");
266268
}
267269
final Set<Path> dataFilesPaths = new HashSet<>();
268270
for (Path path : environment.dataFiles()) {
269-
addPath(policy, Environment.PATH_DATA_SETTING.getKey(), path, "read,readlink,write,delete");
271+
addDirectoryPath(policy, Environment.PATH_DATA_SETTING.getKey(), path, "read,readlink,write,delete");
270272
/*
271273
* We have to do this after adding the path because a side effect of that is that the directory is created; the Path#toRealPath
272274
* invocation will fail if the directory does not already exist. We use Path#toRealPath to follow symlinks and handle issues
@@ -282,11 +284,11 @@ static void addFilePermissions(Permissions policy, Environment environment) {
282284
}
283285
}
284286
for (Path path : environment.repoFiles()) {
285-
addPath(policy, Environment.PATH_REPO_SETTING.getKey(), path, "read,readlink,write,delete");
287+
addDirectoryPath(policy, Environment.PATH_REPO_SETTING.getKey(), path, "read,readlink,write,delete");
286288
}
287289
if (environment.pidFile() != null) {
288290
// we just need permission to remove the file if its elsewhere.
289-
policy.add(new FilePermission(environment.pidFile().toString(), "delete"));
291+
addSingleFilePath(policy, environment.pidFile(), "delete");
290292
}
291293
}
292294

@@ -367,27 +369,6 @@ private static void addSocketPermissionForPortRange(final Permissions policy, fi
367369
policy.add(new SocketPermission("*:" + portRange, "listen,resolve"));
368370
}
369371

370-
/**
371-
* Add access to path (and all files underneath it); this also creates the directory if it does not exist.
372-
*
373-
* @param policy current policy to add permissions to
374-
* @param configurationName the configuration name associated with the path (for error messages only)
375-
* @param path the path itself
376-
* @param permissions set of file permissions to grant to the path
377-
*/
378-
static void addPath(Permissions policy, String configurationName, Path path, String permissions) {
379-
// paths may not exist yet, this also checks accessibility
380-
try {
381-
ensureDirectoryExists(path);
382-
} catch (IOException e) {
383-
throw new IllegalStateException("Unable to access '" + configurationName + "' (" + path + ")", e);
384-
}
385-
386-
// add each path twice: once for itself, again for files underneath it
387-
policy.add(new FilePermission(path.toString(), permissions));
388-
policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", permissions));
389-
}
390-
391372
/**
392373
* Ensures configured directory {@code path} exists.
393374
* @throws IOException if {@code path} exists, but is not a directory, not accessible, or broken symbolic link.

‎distribution/src/main/resources/config/jvm.options

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,6 @@
6363
# exceptions because stack traces are important for debugging
6464
-XX:-OmitStackTraceInFastThrow
6565

66-
# use old-style file permissions on JDK9
67-
-Djdk.io.permissionsUseCanonicalPath=true
68-
6966
# flags to configure Netty
7067
-Dio.netty.noUnsafe=true
7168
-Dio.netty.noKeySetOptimization=true

‎plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -27,17 +27,19 @@
2727
import org.apache.tika.parser.Parser;
2828
import org.apache.tika.parser.ParserDecorator;
2929
import org.elasticsearch.SpecialPermission;
30+
import org.elasticsearch.bootstrap.FilePermissionUtils;
3031
import org.elasticsearch.bootstrap.JarHell;
3132
import org.elasticsearch.common.SuppressForbidden;
3233
import org.elasticsearch.common.io.PathUtils;
3334

3435
import java.io.ByteArrayInputStream;
35-
import java.io.FilePermission;
3636
import java.io.IOException;
37+
import java.io.UncheckedIOException;
3738
import java.lang.reflect.ReflectPermission;
3839
import java.net.URISyntaxException;
3940
import java.net.URL;
4041
import java.net.URLClassLoader;
42+
import java.nio.file.Files;
4143
import java.nio.file.Path;
4244
import java.security.AccessControlContext;
4345
import java.security.AccessController;
@@ -119,27 +121,32 @@ static String parse(final byte content[], final Metadata metadata, final int lim
119121

120122
// compute some minimal permissions for parsers. they only get r/w access to the java temp directory,
121123
// the ability to load some resources from JARs, and read sysprops
124+
@SuppressForbidden(reason = "adds access to tmp directory")
122125
static PermissionCollection getRestrictedPermissions() {
123126
Permissions perms = new Permissions();
124127
// property/env access needed for parsing
125128
perms.add(new PropertyPermission("*", "read"));
126129
perms.add(new RuntimePermission("getenv.TIKA_CONFIG"));
127130

128-
// add permissions for resource access:
129-
// classpath
130-
addReadPermissions(perms, JarHell.parseClassPath());
131-
// plugin jars
132-
if (TikaImpl.class.getClassLoader() instanceof URLClassLoader) {
133-
URL[] urls = ((URLClassLoader)TikaImpl.class.getClassLoader()).getURLs();
134-
Set<URL> set = new LinkedHashSet<>(Arrays.asList(urls));
135-
if (set.size() != urls.length) {
136-
throw new AssertionError("duplicate jars: " + Arrays.toString(urls));
131+
try {
132+
// add permissions for resource access:
133+
// classpath
134+
addReadPermissions(perms, JarHell.parseClassPath());
135+
// plugin jars
136+
if (TikaImpl.class.getClassLoader() instanceof URLClassLoader) {
137+
URL[] urls = ((URLClassLoader)TikaImpl.class.getClassLoader()).getURLs();
138+
Set<URL> set = new LinkedHashSet<>(Arrays.asList(urls));
139+
if (set.size() != urls.length) {
140+
throw new AssertionError("duplicate jars: " + Arrays.toString(urls));
141+
}
142+
addReadPermissions(perms, set);
137143
}
138-
addReadPermissions(perms, set);
144+
// jvm's java.io.tmpdir (needs read/write)
145+
FilePermissionUtils.addDirectoryPath(perms, "java.io.tmpdir",
146+
PathUtils.get(System.getProperty("java.io.tmpdir")), "read,readlink,write,delete");
147+
} catch (IOException e) {
148+
throw new UncheckedIOException(e);
139149
}
140-
// jvm's java.io.tmpdir (needs read/write)
141-
perms.add(new FilePermission(System.getProperty("java.io.tmpdir") + System.getProperty("file.separator") + "-",
142-
"read,readlink,write,delete"));
143150
// current hacks needed for POI/PDFbox issues:
144151
perms.add(new SecurityPermission("putProviderProperty.BC"));
145152
perms.add(new SecurityPermission("insertProvider"));
@@ -152,14 +159,15 @@ static PermissionCollection getRestrictedPermissions() {
152159

153160
// add resources to (what is typically) a jar, but might not be (e.g. in tests/IDE)
154161
@SuppressForbidden(reason = "adds access to jar resources")
155-
static void addReadPermissions(Permissions perms, Set<URL> resources) {
162+
static void addReadPermissions(Permissions perms, Set<URL> resources) throws IOException {
156163
try {
157164
for (URL url : resources) {
158165
Path path = PathUtils.get(url.toURI());
159-
// resource itself
160-
perms.add(new FilePermission(path.toString(), "read,readlink"));
161-
// classes underneath
162-
perms.add(new FilePermission(path.toString() + System.getProperty("file.separator") + "-", "read,readlink"));
166+
if (Files.isDirectory(path)) {
167+
FilePermissionUtils.addDirectoryPath(perms, "class.path", path, "read,readlink");
168+
} else {
169+
FilePermissionUtils.addSingleFilePath(perms, path, "read,readlink");
170+
}
163171
}
164172
} catch (URISyntaxException bogus) {
165173
throw new RuntimeException(bogus);

‎qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
package org.elasticsearch.bootstrap;
2121

22+
import org.elasticsearch.common.SuppressForbidden;
2223
import org.elasticsearch.test.ESTestCase;
2324

2425
import java.io.FilePermission;
@@ -44,6 +45,7 @@ public class ESPolicyUnitTests extends ESTestCase {
4445
* even though ProtectionDomain's ctor javadocs might make you think
4546
* that the policy won't be consulted.
4647
*/
48+
@SuppressForbidden(reason = "to create FilePermission object")
4749
public void testNullCodeSource() throws Exception {
4850
assumeTrue("test cannot run with security manager", System.getSecurityManager() == null);
4951
// create a policy with AllPermission
@@ -61,6 +63,7 @@ public void testNullCodeSource() throws Exception {
6163
* <p>
6264
* its unclear when/if this happens, see https://bugs.openjdk.java.net/browse/JDK-8129972
6365
*/
66+
@SuppressForbidden(reason = "to create FilePermission object")
6467
public void testNullLocation() throws Exception {
6568
assumeTrue("test cannot run with security manager", System.getSecurityManager() == null);
6669
PermissionCollection noPermissions = new Permissions();

‎qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilSecurityTests.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import org.apache.lucene.util.Constants;
2323
import org.elasticsearch.common.SuppressForbidden;
2424
import org.elasticsearch.common.io.PathUtils;
25-
import org.elasticsearch.common.settings.Setting;
2625
import org.elasticsearch.common.settings.Settings;
2726
import org.elasticsearch.env.Environment;
2827
import org.elasticsearch.test.ESTestCase;
@@ -73,6 +72,7 @@ public void testGeneratedPermissions() throws Exception {
7372

7473
/** test generated permissions for all configured paths */
7574
@SuppressWarnings("deprecation") // needs to check settings for deprecated path
75+
@SuppressForbidden(reason = "to create FilePermission object")
7676
public void testEnvironmentPaths() throws Exception {
7777
Path path = createTempDir();
7878
// make a fake ES home and ensure we only grant permissions to that.
@@ -217,7 +217,7 @@ public void testSymlinkPermissions() throws IOException {
217217
assumeNoException("test cannot create symbolic links with security manager enabled", e);
218218
}
219219
Permissions permissions = new Permissions();
220-
Security.addPath(permissions, "testing", link, "read");
220+
FilePermissionUtils.addDirectoryPath(permissions, "testing", link, "read");
221221
assertExactPermissions(new FilePermission(link.toString(), "read"), permissions);
222222
assertExactPermissions(new FilePermission(link.resolve("foo").toString(), "read"), permissions);
223223
assertExactPermissions(new FilePermission(target.toString(), "read"), permissions);
@@ -227,6 +227,7 @@ public void testSymlinkPermissions() throws IOException {
227227
/**
228228
* checks exact file permissions, meaning those and only those for that path.
229229
*/
230+
@SuppressForbidden(reason = "to create FilePermission object")
230231
static void assertExactPermissions(FilePermission expected, PermissionCollection actual) {
231232
String target = expected.getName(); // see javadocs
232233
Set<String> permissionSet = asSet(expected.getActions().split(","));
@@ -246,6 +247,7 @@ static void assertExactPermissions(FilePermission expected, PermissionCollection
246247
/**
247248
* checks that this path has no permissions
248249
*/
250+
@SuppressForbidden(reason = "to create FilePermission object")
249251
static void assertNoPermissions(Path path, PermissionCollection actual) {
250252
String target = path.toString();
251253
assertFalse(actual.implies(new FilePermission(target, "read")));

0 commit comments

Comments
 (0)