From fdbc53b4c338f64cf59d02b714989d7c081898bd Mon Sep 17 00:00:00 2001 From: David Roberts Date: Fri, 27 Oct 2017 10:22:31 +0100 Subject: [PATCH 1/3] Use a private directory for temporary files This change ensures that the contents of the directory where Elasticsearch creates temporary files can only be listed by the user that Elasticsearch is running as. (The temporary files themselves are already read/write for only the user that Elasticsearch is running as. The extra protection this change adds is for the visibility of the file names.) --- .../elasticsearch/bootstrap/Bootstrap.java | 19 +++++ .../org/elasticsearch/bootstrap/Security.java | 2 +- .../org/elasticsearch/env/Environment.java | 48 ++++++++++- .../bootstrap/BootstrapTests.java | 28 ++++++- .../env/EnvironmentEvilTests.java | 81 +++++++++++++++++++ .../bootstrap/BootstrapForTesting.java | 5 +- 6 files changed, 178 insertions(+), 5 deletions(-) create mode 100644 qa/evil-tests/src/test/java/org/elasticsearch/env/EnvironmentEvilTests.java diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index 30b9fb7e28dd0..cf736459eba60 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -56,11 +56,15 @@ import java.io.PrintStream; import java.io.UnsupportedEncodingException; import java.net.URISyntaxException; +import java.nio.file.Files; import java.nio.file.Path; import java.security.NoSuchAlgorithmException; +import java.util.Comparator; import java.util.List; import java.util.Collections; +import java.util.Set; import java.util.concurrent.CountDownLatch; +import java.util.stream.Collectors; /** * Internal startup code. @@ -185,6 +189,7 @@ public void run() { IOUtils.close(node, spawner); LoggerContext context = (LoggerContext) LogManager.getContext(false); Configurator.shutdown(context); + cleanTmpFiles(environment.tmpFile()); } catch (IOException ex) { throw new ElasticsearchException("failed to stop node", ex); } @@ -262,9 +267,17 @@ private void start() throws NodeValidationException { keepAliveThread.start(); } + /** + * This is used for graceful shutdown on Windows when triggered by + * CTRL-C or the service control manager. + */ static void stop() throws IOException { try { IOUtils.close(INSTANCE.node, INSTANCE.spawner); + // TODO: should the logger be shut down here like it is for the shutdown hook? + if (INSTANCE.node != null) { + cleanTmpFiles(INSTANCE.node.getEnvironment().tmpFile()); + } } finally { INSTANCE.keepAliveLatch.countDown(); } @@ -395,4 +408,10 @@ private static void checkLucene() { } } + static void cleanTmpFiles(Path tmpFile) throws IOException { + List toDelete = Files.walk(tmpFile).sorted(Comparator.reverseOrder()).collect(Collectors.toList()); + for (Path path : toDelete) { + Files.delete(path); + } + } } diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java index a1ce20a0e27c8..99058a611478e 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -276,7 +276,7 @@ static void addFilePermissions(Permissions policy, Environment environment) thro addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.pluginsFile(), "read,readlink"); addDirectoryPath(policy, "path.conf'", environment.configFile(), "read,readlink"); // read-write dirs - addDirectoryPath(policy, "java.io.tmpdir", environment.tmpFile(), "read,readlink,write,delete"); + addDirectoryPath(policy, "java.io.tmpdir", environment.systemTmpFile(), "read,readlink,write,delete"); addDirectoryPath(policy, Environment.PATH_LOGS_SETTING.getKey(), environment.logsFile(), "read,readlink,write,delete"); if (environment.sharedDataFile() != null) { addDirectoryPath(policy, Environment.PATH_SHARED_DATA_SETTING.getKey(), environment.sharedDataFile(), diff --git a/core/src/main/java/org/elasticsearch/env/Environment.java b/core/src/main/java/org/elasticsearch/env/Environment.java index 31a67333a810f..c61f5f09d8d1b 100644 --- a/core/src/main/java/org/elasticsearch/env/Environment.java +++ b/core/src/main/java/org/elasticsearch/env/Environment.java @@ -25,6 +25,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.set.Sets; import java.io.IOException; import java.net.MalformedURLException; @@ -33,9 +34,12 @@ import java.nio.file.FileStore; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; import java.util.Collections; import java.util.List; import java.util.Objects; +import java.util.Set; import java.util.function.Function; /** @@ -83,7 +87,38 @@ public class Environment { private final Path pidFile; /** Path to the temporary file directory used by the JDK */ - private final Path tmpFile = PathUtils.get(System.getProperty("java.io.tmpdir")); + private static final Path systemTmpFile = PathUtils.get(System.getProperty("java.io.tmpdir")); + + /** + * Path to the temporary file directory used by Elasticsearch. + * This will be a sub-directory that is created under the JDK temporary directory. + */ + private static final Path tmpFile; + + /** + * Creates a secure temporary directory whose contents can only be seen by the user + * that Elasticsearch is running as. This is static so that only one such directory + * is created, even when many instances of {@link Environment} are created. + */ + static { + Path createdTmpFile; + try { + try { + // On POSIX file systems everyone can see the contents of the /tmp directory, so create a + // sub-directory under it that only the user running Elasticsearch can list the contents of. + Set attrs = Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, + PosixFilePermission.OWNER_EXECUTE); + createdTmpFile = Files.createTempDirectory(systemTmpFile, "elasticsearch", PosixFilePermissions.asFileAttribute(attrs)); + } catch (UnsupportedOperationException e) { + // Assume this isn't a POSIX file system. On Windows each user's %TEMP% directory is visible only to + // them and administrators, so the exact permissions of this sub-directory are less important. + createdTmpFile = Files.createTempDirectory(systemTmpFile, "elasticsearch"); + } + } catch (IOException e) { + throw new IllegalStateException("Unable to create secure temp directory", e); + } + tmpFile = createdTmpFile; + } public Environment(Settings settings) { this(settings, null); @@ -289,6 +324,14 @@ public Path pidFile() { } /** Path to the default temp directory used by the JDK */ + public Path systemTmpFile() { + return systemTmpFile; + } + + /** + * Path to the temporary file directory used by Elasticsearch. + * This will be a sub-directory that is created under the JDK temporary directory. + */ public Path tmpFile() { return tmpFile; } @@ -317,4 +360,7 @@ public static void assertEquivalent(Environment actual, Environment expected) { private static void assertEquals(Object actual, Object expected, String name) { assert Objects.deepEquals(actual, expected) : "actual " + name + " [" + actual + "] is different than [ " + expected + "]"; } + + // does nothing, just easy way to make sure the class is loaded. + public static void ensureClassLoaded() {} } diff --git a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapTests.java b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapTests.java index 6c390a5a8a7d7..3925466be5d43 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapTests.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.common.settings.SecureSettings; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; import org.junit.After; @@ -33,12 +34,15 @@ import java.nio.file.FileSystem; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; import java.util.ArrayList; import java.util.List; +import java.util.Set; public class BootstrapTests extends ESTestCase { - Environment env; - List fileSystems = new ArrayList<>(); + private Environment env; + private List fileSystems = new ArrayList<>(); @After public void closeMockFileSystems() throws IOException { @@ -66,4 +70,24 @@ public void testLoadSecureSettings() throws Exception { assertTrue(Files.exists(configPath.resolve("elasticsearch.keystore"))); } } + + public void testCleanTmpFiles() throws IOException { + Path testBaseDir = createTempDir(); + Path createdTmpFile; + try { + Set attrs = Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, + PosixFilePermission.OWNER_EXECUTE); + createdTmpFile = Files.createTempDirectory(testBaseDir, "BootstrapTests", PosixFilePermissions.asFileAttribute(attrs)); + } catch (UnsupportedOperationException e) { + // Assume this isn't a POSIX file system + createdTmpFile = Files.createTempDirectory(testBaseDir, "BootstrapTests"); + } + assertTrue(Files.isDirectory(createdTmpFile)); + Files.createTempFile(createdTmpFile, "some", "junk"); + Files.createTempFile(createdTmpFile, "more", "junk"); + Path nested = Files.createTempDirectory(createdTmpFile, "nested"); + Files.createTempFile(nested, "nested", "junk"); + Bootstrap.cleanTmpFiles(createdTmpFile); + assertFalse(Files.exists(createdTmpFile)); + } } diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/env/EnvironmentEvilTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/env/EnvironmentEvilTests.java new file mode 100644 index 0000000000000..70cec27b47d5e --- /dev/null +++ b/qa/evil-tests/src/test/java/org/elasticsearch/env/EnvironmentEvilTests.java @@ -0,0 +1,81 @@ +/* + * 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.env; + +import org.apache.lucene.util.LuceneTestCase; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.test.ESTestCase; +import org.junit.BeforeClass; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.attribute.PosixFileAttributeView; +import java.nio.file.attribute.PosixFilePermission; +import java.util.Set; + +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; + +/** + * This has to be an evil test because it checks file permissions, + * and security manager does not allow that. + */ +public class EnvironmentEvilTests extends ESTestCase { + + private static boolean isPosix; + + @BeforeClass + public static void checkPosix() throws IOException { + isPosix = Files.getFileAttributeView(createTempFile(), PosixFileAttributeView.class) != null; + } + + public Environment newEnvironment() throws IOException { + Settings build = Settings.builder() + .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath()) + .putList(Environment.PATH_DATA_SETTING.getKey(), tmpPaths()).build(); + return new Environment(build); + } + + public void testTmpDirIsNotSystemTmpDir() throws IOException { + final Environment environment = newEnvironment(); + assertThat(environment.tmpFile(), not(equalTo(environment.systemTmpFile()))); + } + + public void testTmpDirPerms() throws IOException { + // %TEMP% directories are restricted by default on Windows, so in this case it doesn't + // really matter what permissions the directory we create beneath java.io.tmpdir have. + // And if the user has changed java.io.tmpdir then they should take responsibility for + // ensuring the chosen location is as secure as they require. + assumeTrue("Temp directory permissions not set on Windows", isPosix); + + final Environment environment = newEnvironment(); + + Set perms = Files.getPosixFilePermissions(environment.tmpFile()); + assertThat(perms, + containsInAnyOrder(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_EXECUTE)); + assertThat(perms, not(contains(PosixFilePermission.GROUP_READ))); + assertThat(perms, not(contains(PosixFilePermission.GROUP_WRITE))); + assertThat(perms, not(contains(PosixFilePermission.GROUP_EXECUTE))); + assertThat(perms, not(contains(PosixFilePermission.OTHERS_READ))); + assertThat(perms, not(contains(PosixFilePermission.OTHERS_WRITE))); + assertThat(perms, not(contains(PosixFilePermission.OTHERS_EXECUTE))); + } +} 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 e4ecd02615e91..d2ab8f16b1f66 100644 --- a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java +++ b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -28,10 +28,10 @@ import org.elasticsearch.common.io.FileSystemUtils; import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.network.IfConfig; +import org.elasticsearch.env.Environment; import org.elasticsearch.plugins.PluginInfo; import org.junit.Assert; -import java.io.FilePermission; import java.io.InputStream; import java.net.SocketPermission; import java.net.URL; @@ -72,6 +72,9 @@ public class BootstrapForTesting { "please set ${java.io.tmpdir} in pom.xml")); try { Security.ensureDirectoryExists(javaTmpDir); + // This ensures that the secure temp directory under java.io.tmpdir + // exists before we install any mock file systems + Environment.ensureClassLoaded(); } catch (Exception e) { throw new RuntimeException("unable to create test temp directory", e); } From 04a9f69262e250dea454d56557531f1561784fbb Mon Sep 17 00:00:00 2001 From: David Roberts Date: Mon, 30 Oct 2017 08:51:13 +0000 Subject: [PATCH 2/3] Revert "Use a private directory for temporary files" This reverts commit fdbc53b4c338f64cf59d02b714989d7c081898bd. --- .../elasticsearch/bootstrap/Bootstrap.java | 19 ----- .../org/elasticsearch/bootstrap/Security.java | 2 +- .../org/elasticsearch/env/Environment.java | 48 +---------- .../bootstrap/BootstrapTests.java | 28 +------ .../env/EnvironmentEvilTests.java | 81 ------------------- .../bootstrap/BootstrapForTesting.java | 5 +- 6 files changed, 5 insertions(+), 178 deletions(-) delete mode 100644 qa/evil-tests/src/test/java/org/elasticsearch/env/EnvironmentEvilTests.java diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index cf736459eba60..30b9fb7e28dd0 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -56,15 +56,11 @@ import java.io.PrintStream; import java.io.UnsupportedEncodingException; import java.net.URISyntaxException; -import java.nio.file.Files; import java.nio.file.Path; import java.security.NoSuchAlgorithmException; -import java.util.Comparator; import java.util.List; import java.util.Collections; -import java.util.Set; import java.util.concurrent.CountDownLatch; -import java.util.stream.Collectors; /** * Internal startup code. @@ -189,7 +185,6 @@ public void run() { IOUtils.close(node, spawner); LoggerContext context = (LoggerContext) LogManager.getContext(false); Configurator.shutdown(context); - cleanTmpFiles(environment.tmpFile()); } catch (IOException ex) { throw new ElasticsearchException("failed to stop node", ex); } @@ -267,17 +262,9 @@ private void start() throws NodeValidationException { keepAliveThread.start(); } - /** - * This is used for graceful shutdown on Windows when triggered by - * CTRL-C or the service control manager. - */ static void stop() throws IOException { try { IOUtils.close(INSTANCE.node, INSTANCE.spawner); - // TODO: should the logger be shut down here like it is for the shutdown hook? - if (INSTANCE.node != null) { - cleanTmpFiles(INSTANCE.node.getEnvironment().tmpFile()); - } } finally { INSTANCE.keepAliveLatch.countDown(); } @@ -408,10 +395,4 @@ private static void checkLucene() { } } - static void cleanTmpFiles(Path tmpFile) throws IOException { - List toDelete = Files.walk(tmpFile).sorted(Comparator.reverseOrder()).collect(Collectors.toList()); - for (Path path : toDelete) { - Files.delete(path); - } - } } diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java index 99058a611478e..a1ce20a0e27c8 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -276,7 +276,7 @@ static void addFilePermissions(Permissions policy, Environment environment) thro addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.pluginsFile(), "read,readlink"); addDirectoryPath(policy, "path.conf'", environment.configFile(), "read,readlink"); // read-write dirs - addDirectoryPath(policy, "java.io.tmpdir", environment.systemTmpFile(), "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) { addDirectoryPath(policy, Environment.PATH_SHARED_DATA_SETTING.getKey(), environment.sharedDataFile(), diff --git a/core/src/main/java/org/elasticsearch/env/Environment.java b/core/src/main/java/org/elasticsearch/env/Environment.java index c61f5f09d8d1b..31a67333a810f 100644 --- a/core/src/main/java/org/elasticsearch/env/Environment.java +++ b/core/src/main/java/org/elasticsearch/env/Environment.java @@ -25,7 +25,6 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.util.set.Sets; import java.io.IOException; import java.net.MalformedURLException; @@ -34,12 +33,9 @@ import java.nio.file.FileStore; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.attribute.PosixFilePermission; -import java.nio.file.attribute.PosixFilePermissions; import java.util.Collections; import java.util.List; import java.util.Objects; -import java.util.Set; import java.util.function.Function; /** @@ -87,38 +83,7 @@ public class Environment { private final Path pidFile; /** Path to the temporary file directory used by the JDK */ - private static final Path systemTmpFile = PathUtils.get(System.getProperty("java.io.tmpdir")); - - /** - * Path to the temporary file directory used by Elasticsearch. - * This will be a sub-directory that is created under the JDK temporary directory. - */ - private static final Path tmpFile; - - /** - * Creates a secure temporary directory whose contents can only be seen by the user - * that Elasticsearch is running as. This is static so that only one such directory - * is created, even when many instances of {@link Environment} are created. - */ - static { - Path createdTmpFile; - try { - try { - // On POSIX file systems everyone can see the contents of the /tmp directory, so create a - // sub-directory under it that only the user running Elasticsearch can list the contents of. - Set attrs = Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, - PosixFilePermission.OWNER_EXECUTE); - createdTmpFile = Files.createTempDirectory(systemTmpFile, "elasticsearch", PosixFilePermissions.asFileAttribute(attrs)); - } catch (UnsupportedOperationException e) { - // Assume this isn't a POSIX file system. On Windows each user's %TEMP% directory is visible only to - // them and administrators, so the exact permissions of this sub-directory are less important. - createdTmpFile = Files.createTempDirectory(systemTmpFile, "elasticsearch"); - } - } catch (IOException e) { - throw new IllegalStateException("Unable to create secure temp directory", e); - } - tmpFile = createdTmpFile; - } + private final Path tmpFile = PathUtils.get(System.getProperty("java.io.tmpdir")); public Environment(Settings settings) { this(settings, null); @@ -324,14 +289,6 @@ public Path pidFile() { } /** Path to the default temp directory used by the JDK */ - public Path systemTmpFile() { - return systemTmpFile; - } - - /** - * Path to the temporary file directory used by Elasticsearch. - * This will be a sub-directory that is created under the JDK temporary directory. - */ public Path tmpFile() { return tmpFile; } @@ -360,7 +317,4 @@ public static void assertEquivalent(Environment actual, Environment expected) { private static void assertEquals(Object actual, Object expected, String name) { assert Objects.deepEquals(actual, expected) : "actual " + name + " [" + actual + "] is different than [ " + expected + "]"; } - - // does nothing, just easy way to make sure the class is loaded. - public static void ensureClassLoaded() {} } diff --git a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapTests.java b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapTests.java index 3925466be5d43..6c390a5a8a7d7 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapTests.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapTests.java @@ -24,7 +24,6 @@ import org.elasticsearch.common.settings.SecureSettings; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; import org.junit.After; @@ -34,15 +33,12 @@ import java.nio.file.FileSystem; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.attribute.PosixFilePermission; -import java.nio.file.attribute.PosixFilePermissions; import java.util.ArrayList; import java.util.List; -import java.util.Set; public class BootstrapTests extends ESTestCase { - private Environment env; - private List fileSystems = new ArrayList<>(); + Environment env; + List fileSystems = new ArrayList<>(); @After public void closeMockFileSystems() throws IOException { @@ -70,24 +66,4 @@ public void testLoadSecureSettings() throws Exception { assertTrue(Files.exists(configPath.resolve("elasticsearch.keystore"))); } } - - public void testCleanTmpFiles() throws IOException { - Path testBaseDir = createTempDir(); - Path createdTmpFile; - try { - Set attrs = Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, - PosixFilePermission.OWNER_EXECUTE); - createdTmpFile = Files.createTempDirectory(testBaseDir, "BootstrapTests", PosixFilePermissions.asFileAttribute(attrs)); - } catch (UnsupportedOperationException e) { - // Assume this isn't a POSIX file system - createdTmpFile = Files.createTempDirectory(testBaseDir, "BootstrapTests"); - } - assertTrue(Files.isDirectory(createdTmpFile)); - Files.createTempFile(createdTmpFile, "some", "junk"); - Files.createTempFile(createdTmpFile, "more", "junk"); - Path nested = Files.createTempDirectory(createdTmpFile, "nested"); - Files.createTempFile(nested, "nested", "junk"); - Bootstrap.cleanTmpFiles(createdTmpFile); - assertFalse(Files.exists(createdTmpFile)); - } } diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/env/EnvironmentEvilTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/env/EnvironmentEvilTests.java deleted file mode 100644 index 70cec27b47d5e..0000000000000 --- a/qa/evil-tests/src/test/java/org/elasticsearch/env/EnvironmentEvilTests.java +++ /dev/null @@ -1,81 +0,0 @@ -/* - * 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.env; - -import org.apache.lucene.util.LuceneTestCase; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.test.ESTestCase; -import org.junit.BeforeClass; - -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.attribute.PosixFileAttributeView; -import java.nio.file.attribute.PosixFilePermission; -import java.util.Set; - -import static org.hamcrest.Matchers.contains; -import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.not; - -/** - * This has to be an evil test because it checks file permissions, - * and security manager does not allow that. - */ -public class EnvironmentEvilTests extends ESTestCase { - - private static boolean isPosix; - - @BeforeClass - public static void checkPosix() throws IOException { - isPosix = Files.getFileAttributeView(createTempFile(), PosixFileAttributeView.class) != null; - } - - public Environment newEnvironment() throws IOException { - Settings build = Settings.builder() - .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath()) - .putList(Environment.PATH_DATA_SETTING.getKey(), tmpPaths()).build(); - return new Environment(build); - } - - public void testTmpDirIsNotSystemTmpDir() throws IOException { - final Environment environment = newEnvironment(); - assertThat(environment.tmpFile(), not(equalTo(environment.systemTmpFile()))); - } - - public void testTmpDirPerms() throws IOException { - // %TEMP% directories are restricted by default on Windows, so in this case it doesn't - // really matter what permissions the directory we create beneath java.io.tmpdir have. - // And if the user has changed java.io.tmpdir then they should take responsibility for - // ensuring the chosen location is as secure as they require. - assumeTrue("Temp directory permissions not set on Windows", isPosix); - - final Environment environment = newEnvironment(); - - Set perms = Files.getPosixFilePermissions(environment.tmpFile()); - assertThat(perms, - containsInAnyOrder(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_EXECUTE)); - assertThat(perms, not(contains(PosixFilePermission.GROUP_READ))); - assertThat(perms, not(contains(PosixFilePermission.GROUP_WRITE))); - assertThat(perms, not(contains(PosixFilePermission.GROUP_EXECUTE))); - assertThat(perms, not(contains(PosixFilePermission.OTHERS_READ))); - assertThat(perms, not(contains(PosixFilePermission.OTHERS_WRITE))); - assertThat(perms, not(contains(PosixFilePermission.OTHERS_EXECUTE))); - } -} 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 d2ab8f16b1f66..e4ecd02615e91 100644 --- a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java +++ b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -28,10 +28,10 @@ import org.elasticsearch.common.io.FileSystemUtils; import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.network.IfConfig; -import org.elasticsearch.env.Environment; import org.elasticsearch.plugins.PluginInfo; import org.junit.Assert; +import java.io.FilePermission; import java.io.InputStream; import java.net.SocketPermission; import java.net.URL; @@ -72,9 +72,6 @@ public class BootstrapForTesting { "please set ${java.io.tmpdir} in pom.xml")); try { Security.ensureDirectoryExists(javaTmpDir); - // This ensures that the secure temp directory under java.io.tmpdir - // exists before we install any mock file systems - Environment.ensureClassLoaded(); } catch (Exception e) { throw new RuntimeException("unable to create test temp directory", e); } From a0009f596d7f04043fb0e5900e2e82e987ae1de1 Mon Sep 17 00:00:00 2001 From: David Roberts Date: Mon, 30 Oct 2017 11:34:48 +0000 Subject: [PATCH 3/3] Change approach to pass temp directory to Environment --- .../elasticsearch/bootstrap/Bootstrap.java | 29 +++++++- .../org/elasticsearch/bootstrap/Security.java | 4 +- .../cli/EnvironmentAwareCommand.java | 5 +- .../org/elasticsearch/env/Environment.java | 14 +++- .../node/InternalSettingsPreparer.java | 16 +++-- .../java/org/elasticsearch/node/Node.java | 2 +- .../node/InternalSettingsPreparerTests.java | 4 +- .../bootstrap/EvilBootstrapTests.java | 66 +++++++++++++++++++ .../java/org/elasticsearch/node/MockNode.java | 4 +- 9 files changed, 128 insertions(+), 16 deletions(-) create mode 100644 qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilBootstrapTests.java diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index 30b9fb7e28dd0..6d80abcf9de84 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -35,6 +35,7 @@ import org.elasticsearch.common.PidFile; import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.inject.CreationException; +import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.logging.ESLoggerFactory; import org.elasticsearch.common.logging.LogConfigurator; import org.elasticsearch.common.logging.Loggers; @@ -43,6 +44,7 @@ import org.elasticsearch.common.settings.SecureSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.transport.BoundTransportAddress; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.env.Environment; import org.elasticsearch.monitor.jvm.JvmInfo; import org.elasticsearch.monitor.os.OsProbe; @@ -56,10 +58,14 @@ import java.io.PrintStream; import java.io.UnsupportedEncodingException; import java.net.URISyntaxException; +import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; import java.security.NoSuchAlgorithmException; import java.util.List; import java.util.Collections; +import java.util.Set; import java.util.concurrent.CountDownLatch; /** @@ -239,12 +245,13 @@ static SecureSettings loadSecureSettings(Environment initialEnv) throws Bootstra return keystore; } + @SuppressForbidden(reason = "gets java.io.tmpdir") private static Environment createEnvironment( final boolean foreground, final Path pidFile, final SecureSettings secureSettings, final Settings initialSettings, - final Path configPath) { + final Path configPath) throws IOException { Terminal terminal = foreground ? Terminal.DEFAULT : null; Settings.Builder builder = Settings.builder(); if (pidFile != null) { @@ -254,7 +261,22 @@ private static Environment createEnvironment( if (secureSettings != null) { builder.setSecureSettings(secureSettings); } - return InternalSettingsPreparer.prepareEnvironment(builder.build(), terminal, Collections.emptyMap(), configPath); + return InternalSettingsPreparer.prepareEnvironment(builder.build(), terminal, Collections.emptyMap(), configPath, + makeSecureTmpPath(PathUtils.get(System.getProperty("java.io.tmpdir")))); + } + + static Path makeSecureTmpPath(Path baseDir) throws IOException { + try { + // On POSIX file systems everyone can see the contents of the /tmp directory, so create a + // sub-directory under it that only the user running Elasticsearch can list the contents of. + Set attrs = Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, + PosixFilePermission.OWNER_EXECUTE); + return Files.createTempDirectory(baseDir, "elasticsearch", PosixFilePermissions.asFileAttribute(attrs)); + } catch (UnsupportedOperationException e) { + // Assume this isn't a POSIX file system. On Windows each user's %TEMP% directory is visible only to + // them and administrators, so the exact permissions of this sub-directory are less important. + return Files.createTempDirectory(baseDir, "elasticsearch"); + } } private void start() throws NodeValidationException { @@ -285,8 +307,9 @@ static void init( INSTANCE = new Bootstrap(); final SecureSettings keystore = loadSecureSettings(initialEnv); - final Environment environment = createEnvironment(foreground, pidFile, keystore, initialEnv.settings(), initialEnv.configFile()); + final Environment environment; try { + environment = createEnvironment(foreground, pidFile, keystore, initialEnv.settings(), initialEnv.configFile()); LogConfigurator.configure(environment); } catch (IOException e) { throw new BootstrapException(e); diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java index a1ce20a0e27c8..96c915318a658 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -268,6 +268,7 @@ static void addClasspathPermissions(Permissions policy) throws IOException { /** * Adds access to all configurable paths. */ + @SuppressForbidden(reason = "gets java.io.tmpdir") static void addFilePermissions(Permissions policy, Environment environment) throws IOException { // read-only dirs addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.binFile(), "read,readlink"); @@ -276,7 +277,8 @@ static void addFilePermissions(Permissions policy, Environment environment) thro addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.pluginsFile(), "read,readlink"); addDirectoryPath(policy, "path.conf'", environment.configFile(), "read,readlink"); // read-write dirs - addDirectoryPath(policy, "java.io.tmpdir", environment.tmpFile(), "read,readlink,write,delete"); + addDirectoryPath(policy, "java.io.tmpdir", PathUtils.get(System.getProperty("java.io.tmpdir")), + "read,readlink,write,delete"); addDirectoryPath(policy, Environment.PATH_LOGS_SETTING.getKey(), environment.logsFile(), "read,readlink,write,delete"); if (environment.sharedDataFile() != null) { addDirectoryPath(policy, Environment.PATH_SHARED_DATA_SETTING.getKey(), environment.sharedDataFile(), diff --git a/core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java b/core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java index d9d19a56a2f32..c2e1a6064434d 100644 --- a/core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java +++ b/core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java @@ -23,6 +23,7 @@ import joptsimple.OptionSpec; import joptsimple.util.KeyValuePair; import org.elasticsearch.common.SuppressForbidden; +import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.node.InternalSettingsPreparer; @@ -70,12 +71,14 @@ protected void execute(Terminal terminal, OptionSet options) throws Exception { } /** Create an {@link Environment} for the command to use. Overrideable for tests. */ + @SuppressForbidden(reason = "gets java.io.tmpdir") protected Environment createEnv(final Terminal terminal, final Map settings) throws UserException { final String esPathConf = System.getProperty("es.path.conf"); if (esPathConf == null) { throw new UserException(ExitCodes.CONFIG, "the system property [es.path.conf] must be set"); } - return InternalSettingsPreparer.prepareEnvironment(Settings.EMPTY, terminal, settings, getConfigPath(esPathConf)); + return InternalSettingsPreparer.prepareEnvironment(Settings.EMPTY, terminal, settings, getConfigPath(esPathConf), + PathUtils.get(System.getProperty("java.io.tmpdir"))); } @SuppressForbidden(reason = "need path to construct environment") diff --git a/core/src/main/java/org/elasticsearch/env/Environment.java b/core/src/main/java/org/elasticsearch/env/Environment.java index 31a67333a810f..c4cef04bc0e31 100644 --- a/core/src/main/java/org/elasticsearch/env/Environment.java +++ b/core/src/main/java/org/elasticsearch/env/Environment.java @@ -82,14 +82,20 @@ public class Environment { /** Path to the PID file (can be null if no PID file is configured) **/ private final Path pidFile; - /** Path to the temporary file directory used by the JDK */ - private final Path tmpFile = PathUtils.get(System.getProperty("java.io.tmpdir")); + /** Path to the temporary file directory */ + private final Path tmpFile; + /** Convenience method for tests - do not use in production code */ public Environment(Settings settings) { this(settings, null); } - public Environment(final Settings settings, final Path configPath) { + /** Convenience method for tests - do not use in production code */ + public Environment(Settings settings, Path configPath) { + this(settings, configPath, PathUtils.get(System.getProperty("java.io.tmpdir"))); + } + + public Environment(final Settings settings, final Path configPath, final Path tmpPath) { final Path homeFile; if (PATH_HOME_SETTING.exists(settings)) { homeFile = PathUtils.get(PATH_HOME_SETTING.get(settings)).normalize(); @@ -103,6 +109,8 @@ public Environment(final Settings settings, final Path configPath) { configFile = homeFile.resolve("config"); } + tmpFile = Objects.requireNonNull(tmpPath); + pluginsFile = homeFile.resolve("plugins"); List dataPaths = PATH_DATA_SETTING.get(settings); diff --git a/core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java b/core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java index a2c7663ec9e15..eca45fa181153 100644 --- a/core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java +++ b/core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java @@ -31,7 +31,9 @@ import org.elasticsearch.cli.Terminal; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.env.Environment; @@ -62,8 +64,10 @@ public static Settings prepareSettings(Settings input) { * @param terminal the Terminal to use for input/output * @return the {@link Settings} and {@link Environment} as a {@link Tuple} */ + @SuppressForbidden(reason = "gets java.io.tmpdir") public static Environment prepareEnvironment(Settings input, Terminal terminal) { - return prepareEnvironment(input, terminal, Collections.emptyMap(), null); + return prepareEnvironment(input, terminal, Collections.emptyMap(), null, + PathUtils.get(System.getProperty("java.io.tmpdir"))); } /** @@ -76,13 +80,15 @@ public static Environment prepareEnvironment(Settings input, Terminal terminal) * @param terminal the Terminal to use for input/output * @param properties map of properties key/value pairs (usually from the command-line) * @param configPath path to config directory; (use null to indicate the default) + * @param tmpPath path to use for temporary files * @return the {@link Settings} and {@link Environment} as a {@link Tuple} */ - public static Environment prepareEnvironment(Settings input, Terminal terminal, Map properties, Path configPath) { + public static Environment prepareEnvironment(Settings input, Terminal terminal, Map properties, Path configPath, + Path tmpPath) { // just create enough settings to build the environment, to get the config dir Settings.Builder output = Settings.builder(); initializeSettings(output, input, properties); - Environment environment = new Environment(output.build(), configPath); + Environment environment = new Environment(output.build(), configPath, tmpPath); if (Files.exists(environment.configFile().resolve("elasticsearch.yaml"))) { throw new SettingsException("elasticsearch.yaml was deprecated in 5.5.0 and must be renamed to elasticsearch.yml"); @@ -106,11 +112,11 @@ public static Environment prepareEnvironment(Settings input, Terminal terminal, initializeSettings(output, input, properties); finalizeSettings(output, terminal); - environment = new Environment(output.build(), configPath); + environment = new Environment(output.build(), configPath, tmpPath); // we put back the path.logs so we can use it in the logging configuration file output.put(Environment.PATH_LOGS_SETTING.getKey(), environment.logsFile().toAbsolutePath().normalize().toString()); - return new Environment(output.build(), configPath); + return new Environment(output.build(), configPath, tmpPath); } /** diff --git a/core/src/main/java/org/elasticsearch/node/Node.java b/core/src/main/java/org/elasticsearch/node/Node.java index 0ddc03de8c049..a0b25b64e32b0 100644 --- a/core/src/main/java/org/elasticsearch/node/Node.java +++ b/core/src/main/java/org/elasticsearch/node/Node.java @@ -305,7 +305,7 @@ protected Node(final Environment environment, Collection // create the environment based on the finalized (processed) view of the settings // this is just to makes sure that people get the same settings, no matter where they ask them from - this.environment = new Environment(this.settings, environment.configFile()); + this.environment = new Environment(this.settings, environment.configFile(), environment.tmpFile()); Environment.assertEquivalent(environment, this.environment); final List> executorBuilders = pluginsService.getExecutorBuilders(settings); diff --git a/core/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java b/core/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java index 1ce6ed5779f9c..65261b3530e65 100644 --- a/core/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java +++ b/core/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java @@ -21,6 +21,7 @@ import org.elasticsearch.cli.MockTerminal; import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.settings.MockSecureSettings; import org.elasticsearch.common.settings.SecureSetting; import org.elasticsearch.common.settings.SecureString; @@ -181,7 +182,8 @@ public void testSecureSettings() { public void testDefaultPropertiesDoNothing() throws Exception { Map props = Collections.singletonMap("default.setting", "foo"); - Environment env = InternalSettingsPreparer.prepareEnvironment(baseEnvSettings, null, props, null); + Environment env = InternalSettingsPreparer.prepareEnvironment(baseEnvSettings, null, props, null, + PathUtils.get(System.getProperty("java.io.tmpdir"))); assertEquals("foo", env.settings().get("default.setting")); assertNull(env.settings().get("setting")); } diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilBootstrapTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilBootstrapTests.java new file mode 100644 index 0000000000000..91bfbbafd42a9 --- /dev/null +++ b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilBootstrapTests.java @@ -0,0 +1,66 @@ +/* + * 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.test.ESTestCase; +import org.junit.BeforeClass; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.PosixFileAttributeView; +import java.nio.file.attribute.PosixFilePermission; +import java.util.Set; + +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.not; + +/** + * This has to be an evil test because it checks file permissions, and security manager does not allow that. + */ +public class EvilBootstrapTests extends ESTestCase { + + private static boolean isPosix; + + @BeforeClass + public static void checkPosix() throws IOException { + isPosix = Files.getFileAttributeView(createTempFile(), PosixFileAttributeView.class) != null; + } + + public void testTmpDirPerms() throws IOException { + // %TEMP% directories are restricted by default on Windows, so in this case it doesn't + // really matter what permissions the directory we create beneath java.io.tmpdir have. + // And if the user has changed java.io.tmpdir then they should take responsibility for + // ensuring the chosen location is as secure as they require. + assumeTrue("Temp directory permissions not set on Windows", isPosix); + + final Path tmpPath = Bootstrap.makeSecureTmpPath(createTempDir()); + + Set perms = Files.getPosixFilePermissions(tmpPath); + assertThat(perms, + containsInAnyOrder(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_EXECUTE)); + assertThat(perms, not(contains(PosixFilePermission.GROUP_READ))); + assertThat(perms, not(contains(PosixFilePermission.GROUP_WRITE))); + assertThat(perms, not(contains(PosixFilePermission.GROUP_EXECUTE))); + assertThat(perms, not(contains(PosixFilePermission.OTHERS_READ))); + assertThat(perms, not(contains(PosixFilePermission.OTHERS_WRITE))); + assertThat(perms, not(contains(PosixFilePermission.OTHERS_EXECUTE))); + } +} diff --git a/test/framework/src/main/java/org/elasticsearch/node/MockNode.java b/test/framework/src/main/java/org/elasticsearch/node/MockNode.java index 4255163db7fea..d74682bfd6fe1 100644 --- a/test/framework/src/main/java/org/elasticsearch/node/MockNode.java +++ b/test/framework/src/main/java/org/elasticsearch/node/MockNode.java @@ -25,6 +25,7 @@ import org.elasticsearch.cluster.MockInternalClusterInfoService; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.transport.BoundTransportAddress; @@ -66,7 +67,8 @@ public MockNode(Settings settings, Collection> classpath } public MockNode(Settings settings, Collection> classpathPlugins, Path configPath) { - this(InternalSettingsPreparer.prepareEnvironment(settings, null, Collections.emptyMap(), configPath), classpathPlugins); + this(InternalSettingsPreparer.prepareEnvironment(settings, null, Collections.emptyMap(), configPath, + PathUtils.get(System.getProperty("java.io.tmpdir"))), classpathPlugins); } public MockNode(Environment environment, Collection> classpathPlugins) {