From 3d8feff66e5ae1925cba164bdcec52d274d48700 Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Tue, 22 Aug 2017 11:22:00 +0930 Subject: [PATCH] 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. --- .../elasticsearch/gradle/BuildPlugin.groovy | 2 - .../resources/forbidden/jdk-signatures.txt | 3 + .../bootstrap/FilePermissionUtils.java | 86 +++++++++++++++++++ .../org/elasticsearch/bootstrap/Security.java | 57 ++++-------- .../src/main/resources/config/jvm.options | 3 - .../ingest/attachment/TikaImpl.java | 46 ++++++---- .../bootstrap/ESPolicyUnitTests.java | 3 + .../bootstrap/EvilSecurityTests.java | 6 +- .../bootstrap/BootstrapForTesting.java | 8 +- 9 files changed, 146 insertions(+), 68 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/bootstrap/FilePermissionUtils.java diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy index 906ebf4ce6f..868c612cace 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy @@ -549,8 +549,6 @@ class BuildPlugin implements Plugin { systemProperty 'tests.artifact', project.name systemProperty 'tests.task', path systemProperty 'tests.security.manager', 'true' - // Breaking change in JDK-9, revert to JDK-8 behavior for now, see https://github.com/elastic/elasticsearch/issues/21534 - systemProperty 'jdk.io.permissionsUseCanonicalPath', 'true' systemProperty 'jna.nosys', 'true' // default test sysprop values systemProperty 'tests.ifNoTests', 'fail' diff --git a/buildSrc/src/main/resources/forbidden/jdk-signatures.txt b/buildSrc/src/main/resources/forbidden/jdk-signatures.txt index 52b28ee0726..b17495db6bf 100644 --- a/buildSrc/src/main/resources/forbidden/jdk-signatures.txt +++ b/buildSrc/src/main/resources/forbidden/jdk-signatures.txt @@ -107,3 +107,6 @@ java.util.Collections#EMPTY_MAP java.util.Collections#EMPTY_SET java.util.Collections#shuffle(java.util.List) @ Use java.util.Collections#shuffle(java.util.List, java.util.Random) with a reproducible source of randomness + +@defaultMessage Avoid creating FilePermission objects directly, but use FilePermissionUtils instead +java.io.FilePermission#(java.lang.String,java.lang.String) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/FilePermissionUtils.java b/core/src/main/java/org/elasticsearch/bootstrap/FilePermissionUtils.java new file mode 100644 index 00000000000..5355ffb455e --- /dev/null +++ b/core/src/main/java/org/elasticsearch/bootstrap/FilePermissionUtils.java @@ -0,0 +1,86 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.bootstrap; + +import org.elasticsearch.common.SuppressForbidden; + +import java.io.FilePermission; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.Permissions; + +public class FilePermissionUtils { + + /** no instantiation */ + private FilePermissionUtils() {} + + private static final boolean VERSION_IS_AT_LEAST_JAVA_9 = JavaVersion.current().compareTo(JavaVersion.parse("9")) >= 0; + + /** + * Add access to single file path + * @param policy current policy to add permissions to + * @param path the path itself + * @param permissions set of file permissions to grant to the path + */ + @SuppressForbidden(reason = "only place where creating Java-9 compatible FilePermission objects is possible") + public static void addSingleFilePath(Permissions policy, Path path, String permissions) throws IOException { + policy.add(new FilePermission(path.toString(), permissions)); + if (VERSION_IS_AT_LEAST_JAVA_9 && Files.exists(path)) { + // Java 9 FilePermission model requires this due to the removal of pathname canonicalization, + // see also https://github.com/elastic/elasticsearch/issues/21534 + Path realPath = path.toRealPath(); + if (path.toString().equals(realPath.toString()) == false) { + policy.add(new FilePermission(realPath.toString(), permissions)); + } + } + } + + /** + * Add access to path (and all files underneath it); this also creates the directory if it does not exist. + * + * @param policy current policy to add permissions to + * @param configurationName the configuration name associated with the path (for error messages only) + * @param path the path itself + * @param permissions set of file permissions to grant to the path + */ + @SuppressForbidden(reason = "only place where creating Java-9 compatible FilePermission objects is possible") + public static void addDirectoryPath(Permissions policy, String configurationName, Path path, String permissions) throws IOException { + // paths may not exist yet, this also checks accessibility + try { + Security.ensureDirectoryExists(path); + } catch (IOException e) { + throw new IllegalStateException("Unable to access '" + configurationName + "' (" + path + ")", e); + } + + // add each path twice: once for itself, again for files underneath it + policy.add(new FilePermission(path.toString(), permissions)); + policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", permissions)); + if (VERSION_IS_AT_LEAST_JAVA_9) { + // Java 9 FilePermission model requires this due to the removal of pathname canonicalization, + // see also https://github.com/elastic/elasticsearch/issues/21534 + Path realPath = path.toRealPath(); + if (path.toString().equals(realPath.toString()) == false) { + policy.add(new FilePermission(realPath.toString(), permissions)); + policy.add(new FilePermission(realPath.toString() + realPath.getFileSystem().getSeparator() + "-", permissions)); + } + } + } +} diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java index 2b0812c5577..e5c326b8ee1 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -23,14 +23,12 @@ import org.elasticsearch.SecureSM; import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.network.NetworkModule; -import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.http.HttpTransportSettings; import org.elasticsearch.plugins.PluginInfo; import org.elasticsearch.transport.TcpTransport; -import java.io.FilePermission; import java.io.IOException; import java.net.SocketPermission; import java.net.URISyntaxException; @@ -52,6 +50,9 @@ import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; +import static org.elasticsearch.bootstrap.FilePermissionUtils.addDirectoryPath; +import static org.elasticsearch.bootstrap.FilePermissionUtils.addSingleFilePath; + /** * Initializes SecurityManager with necessary permissions. *
@@ -240,10 +241,10 @@ final class Security { throw new RuntimeException(e); } // resource itself - policy.add(new FilePermission(path.toString(), "read,readlink")); - // classes underneath if (Files.isDirectory(path)) { - policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", "read,readlink")); + addDirectoryPath(policy, "class.path", path, "read,readlink"); + } else { + addSingleFilePath(policy, path, "read,readlink"); } } } @@ -251,22 +252,23 @@ final class Security { /** * Adds access to all configurable paths. */ - static void addFilePermissions(Permissions policy, Environment environment) { + static void addFilePermissions(Permissions policy, Environment environment) throws IOException { // read-only dirs - addPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.binFile(), "read,readlink"); - addPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.libFile(), "read,readlink"); - addPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.modulesFile(), "read,readlink"); - addPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.pluginsFile(), "read,readlink"); - addPath(policy, "path.conf'", environment.configFile(), "read,readlink"); + addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.binFile(), "read,readlink"); + addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.libFile(), "read,readlink"); + addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.modulesFile(), "read,readlink"); + addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.pluginsFile(), "read,readlink"); + addDirectoryPath(policy, "path.conf'", environment.configFile(), "read,readlink"); // read-write dirs - addPath(policy, "java.io.tmpdir", environment.tmpFile(), "read,readlink,write,delete"); - addPath(policy, Environment.PATH_LOGS_SETTING.getKey(), environment.logsFile(), "read,readlink,write,delete"); + addDirectoryPath(policy, "java.io.tmpdir", environment.tmpFile(), "read,readlink,write,delete"); + addDirectoryPath(policy, Environment.PATH_LOGS_SETTING.getKey(), environment.logsFile(), "read,readlink,write,delete"); if (environment.sharedDataFile() != null) { - addPath(policy, Environment.PATH_SHARED_DATA_SETTING.getKey(), environment.sharedDataFile(), "read,readlink,write,delete"); + addDirectoryPath(policy, Environment.PATH_SHARED_DATA_SETTING.getKey(), environment.sharedDataFile(), + "read,readlink,write,delete"); } final Set dataFilesPaths = new HashSet<>(); for (Path path : environment.dataFiles()) { - addPath(policy, Environment.PATH_DATA_SETTING.getKey(), path, "read,readlink,write,delete"); + addDirectoryPath(policy, Environment.PATH_DATA_SETTING.getKey(), path, "read,readlink,write,delete"); /* * We have to do this after adding the path because a side effect of that is that the directory is created; the Path#toRealPath * invocation will fail if the directory does not already exist. We use Path#toRealPath to follow symlinks and handle issues @@ -282,11 +284,11 @@ final class Security { } } for (Path path : environment.repoFiles()) { - addPath(policy, Environment.PATH_REPO_SETTING.getKey(), path, "read,readlink,write,delete"); + addDirectoryPath(policy, Environment.PATH_REPO_SETTING.getKey(), path, "read,readlink,write,delete"); } if (environment.pidFile() != null) { // we just need permission to remove the file if its elsewhere. - policy.add(new FilePermission(environment.pidFile().toString(), "delete")); + addSingleFilePath(policy, environment.pidFile(), "delete"); } } @@ -367,27 +369,6 @@ final class Security { policy.add(new SocketPermission("*:" + portRange, "listen,resolve")); } - /** - * Add access to path (and all files underneath it); this also creates the directory if it does not exist. - * - * @param policy current policy to add permissions to - * @param configurationName the configuration name associated with the path (for error messages only) - * @param path the path itself - * @param permissions set of file permissions to grant to the path - */ - static void addPath(Permissions policy, String configurationName, Path path, String permissions) { - // paths may not exist yet, this also checks accessibility - try { - ensureDirectoryExists(path); - } catch (IOException e) { - throw new IllegalStateException("Unable to access '" + configurationName + "' (" + path + ")", e); - } - - // add each path twice: once for itself, again for files underneath it - policy.add(new FilePermission(path.toString(), permissions)); - policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", permissions)); - } - /** * Ensures configured directory {@code path} exists. * @throws IOException if {@code path} exists, but is not a directory, not accessible, or broken symbolic link. diff --git a/distribution/src/main/resources/config/jvm.options b/distribution/src/main/resources/config/jvm.options index e6f3c5e29f4..3efa8e055fc 100644 --- a/distribution/src/main/resources/config/jvm.options +++ b/distribution/src/main/resources/config/jvm.options @@ -63,9 +63,6 @@ # exceptions because stack traces are important for debugging -XX:-OmitStackTraceInFastThrow -# use old-style file permissions on JDK9 --Djdk.io.permissionsUseCanonicalPath=true - # flags to configure Netty -Dio.netty.noUnsafe=true -Dio.netty.noKeySetOptimization=true diff --git a/plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java b/plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java index 3c0f3b0433c..4b8189449cf 100644 --- a/plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java +++ b/plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java @@ -27,17 +27,19 @@ import org.apache.tika.parser.AutoDetectParser; import org.apache.tika.parser.Parser; import org.apache.tika.parser.ParserDecorator; import org.elasticsearch.SpecialPermission; +import org.elasticsearch.bootstrap.FilePermissionUtils; import org.elasticsearch.bootstrap.JarHell; import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.io.PathUtils; import java.io.ByteArrayInputStream; -import java.io.FilePermission; import java.io.IOException; +import java.io.UncheckedIOException; import java.lang.reflect.ReflectPermission; import java.net.URISyntaxException; import java.net.URL; import java.net.URLClassLoader; +import java.nio.file.Files; import java.nio.file.Path; import java.security.AccessControlContext; import java.security.AccessController; @@ -119,27 +121,32 @@ final class TikaImpl { // compute some minimal permissions for parsers. they only get r/w access to the java temp directory, // the ability to load some resources from JARs, and read sysprops + @SuppressForbidden(reason = "adds access to tmp directory") static PermissionCollection getRestrictedPermissions() { Permissions perms = new Permissions(); // property/env access needed for parsing perms.add(new PropertyPermission("*", "read")); perms.add(new RuntimePermission("getenv.TIKA_CONFIG")); - // add permissions for resource access: - // classpath - addReadPermissions(perms, JarHell.parseClassPath()); - // plugin jars - if (TikaImpl.class.getClassLoader() instanceof URLClassLoader) { - URL[] urls = ((URLClassLoader)TikaImpl.class.getClassLoader()).getURLs(); - Set set = new LinkedHashSet<>(Arrays.asList(urls)); - if (set.size() != urls.length) { - throw new AssertionError("duplicate jars: " + Arrays.toString(urls)); + try { + // add permissions for resource access: + // classpath + addReadPermissions(perms, JarHell.parseClassPath()); + // plugin jars + if (TikaImpl.class.getClassLoader() instanceof URLClassLoader) { + URL[] urls = ((URLClassLoader)TikaImpl.class.getClassLoader()).getURLs(); + Set set = new LinkedHashSet<>(Arrays.asList(urls)); + if (set.size() != urls.length) { + throw new AssertionError("duplicate jars: " + Arrays.toString(urls)); + } + addReadPermissions(perms, set); } - addReadPermissions(perms, set); + // jvm's java.io.tmpdir (needs read/write) + FilePermissionUtils.addDirectoryPath(perms, "java.io.tmpdir", + PathUtils.get(System.getProperty("java.io.tmpdir")), "read,readlink,write,delete"); + } catch (IOException e) { + throw new UncheckedIOException(e); } - // jvm's java.io.tmpdir (needs read/write) - perms.add(new FilePermission(System.getProperty("java.io.tmpdir") + System.getProperty("file.separator") + "-", - "read,readlink,write,delete")); // current hacks needed for POI/PDFbox issues: perms.add(new SecurityPermission("putProviderProperty.BC")); perms.add(new SecurityPermission("insertProvider")); @@ -152,14 +159,15 @@ final class TikaImpl { // add resources to (what is typically) a jar, but might not be (e.g. in tests/IDE) @SuppressForbidden(reason = "adds access to jar resources") - static void addReadPermissions(Permissions perms, Set resources) { + static void addReadPermissions(Permissions perms, Set resources) throws IOException { try { for (URL url : resources) { Path path = PathUtils.get(url.toURI()); - // resource itself - perms.add(new FilePermission(path.toString(), "read,readlink")); - // classes underneath - perms.add(new FilePermission(path.toString() + System.getProperty("file.separator") + "-", "read,readlink")); + if (Files.isDirectory(path)) { + FilePermissionUtils.addDirectoryPath(perms, "class.path", path, "read,readlink"); + } else { + FilePermissionUtils.addSingleFilePath(perms, path, "read,readlink"); + } } } catch (URISyntaxException bogus) { throw new RuntimeException(bogus); diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java index ed71934c72a..f9931e9a182 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.bootstrap; +import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.test.ESTestCase; import java.io.FilePermission; @@ -44,6 +45,7 @@ public class ESPolicyUnitTests extends ESTestCase { * even though ProtectionDomain's ctor javadocs might make you think * that the policy won't be consulted. */ + @SuppressForbidden(reason = "to create FilePermission object") public void testNullCodeSource() throws Exception { assumeTrue("test cannot run with security manager", System.getSecurityManager() == null); // create a policy with AllPermission @@ -61,6 +63,7 @@ public class ESPolicyUnitTests extends ESTestCase { *

* its unclear when/if this happens, see https://bugs.openjdk.java.net/browse/JDK-8129972 */ + @SuppressForbidden(reason = "to create FilePermission object") public void testNullLocation() throws Exception { assumeTrue("test cannot run with security manager", System.getSecurityManager() == null); PermissionCollection noPermissions = new Permissions(); diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilSecurityTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilSecurityTests.java index c54cab44a01..50a9f3426ac 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilSecurityTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilSecurityTests.java @@ -22,7 +22,6 @@ package org.elasticsearch.bootstrap; import org.apache.lucene.util.Constants; import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.io.PathUtils; -import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; @@ -73,6 +72,7 @@ public class EvilSecurityTests extends ESTestCase { /** test generated permissions for all configured paths */ @SuppressWarnings("deprecation") // needs to check settings for deprecated path + @SuppressForbidden(reason = "to create FilePermission object") public void testEnvironmentPaths() throws Exception { Path path = createTempDir(); // make a fake ES home and ensure we only grant permissions to that. @@ -217,7 +217,7 @@ public class EvilSecurityTests extends ESTestCase { assumeNoException("test cannot create symbolic links with security manager enabled", e); } Permissions permissions = new Permissions(); - Security.addPath(permissions, "testing", link, "read"); + FilePermissionUtils.addDirectoryPath(permissions, "testing", link, "read"); assertExactPermissions(new FilePermission(link.toString(), "read"), permissions); assertExactPermissions(new FilePermission(link.resolve("foo").toString(), "read"), permissions); assertExactPermissions(new FilePermission(target.toString(), "read"), permissions); @@ -227,6 +227,7 @@ public class EvilSecurityTests extends ESTestCase { /** * checks exact file permissions, meaning those and only those for that path. */ + @SuppressForbidden(reason = "to create FilePermission object") static void assertExactPermissions(FilePermission expected, PermissionCollection actual) { String target = expected.getName(); // see javadocs Set permissionSet = asSet(expected.getActions().split(",")); @@ -246,6 +247,7 @@ public class EvilSecurityTests extends ESTestCase { /** * checks that this path has no permissions */ + @SuppressForbidden(reason = "to create FilePermission object") static void assertNoPermissions(Path path, PermissionCollection actual) { String target = path.toString(); assertFalse(actual.implies(new FilePermission(target, "read"))); diff --git a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java index 62bf89f6d31..e4ecd02615e 100644 --- a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java +++ b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -102,19 +102,19 @@ public class BootstrapForTesting { Permissions perms = new Permissions(); Security.addClasspathPermissions(perms); // java.io.tmpdir - Security.addPath(perms, "java.io.tmpdir", javaTmpDir, "read,readlink,write,delete"); + FilePermissionUtils.addDirectoryPath(perms, "java.io.tmpdir", javaTmpDir, "read,readlink,write,delete"); // custom test config file if (Strings.hasLength(System.getProperty("tests.config"))) { - perms.add(new FilePermission(System.getProperty("tests.config"), "read,readlink")); + FilePermissionUtils.addSingleFilePath(perms, PathUtils.get(System.getProperty("tests.config")), "read,readlink"); } // jacoco coverage output file final boolean testsCoverage = Booleans.parseBoolean(System.getProperty("tests.coverage", "false")); if (testsCoverage) { Path coverageDir = PathUtils.get(System.getProperty("tests.coverage.dir")); - perms.add(new FilePermission(coverageDir.resolve("jacoco.exec").toString(), "read,write")); + FilePermissionUtils.addSingleFilePath(perms, coverageDir.resolve("jacoco.exec"), "read,write"); // in case we get fancy and use the -integration goals later: - perms.add(new FilePermission(coverageDir.resolve("jacoco-it.exec").toString(), "read,write")); + FilePermissionUtils.addSingleFilePath(perms, coverageDir.resolve("jacoco-it.exec"), "read,write"); } // intellij hack: intellij test runner wants setIO and will // screw up all test logging without it!