From f599c237bdee7624bd5a67cf26828ccac32dbcc7 Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Mon, 27 Apr 2015 20:29:57 -0400 Subject: [PATCH] Security manager cleanups 1. initialize SM after things like mlockall. Their tests currently don't run with securitymanager enabled, and its simpler to just run mlockall etc first. 2. remove redundant test permissions (junit4.childvm.cwd/temp). This is alreay added as java.io.tmpdir. 3. improve tests to load the generated policy with some various settings and assert things about the permissions on configured directories. 4. refactor logic to make it easier to fine-grain the permissions later. for example we currently allow write access to conf/. In the future I think we can improve testing so we are able to make improvements here. --- .../elasticsearch/bootstrap/Bootstrap.java | 3 +- .../org/elasticsearch/bootstrap/Security.java | 81 +++++++----------- .../elasticsearch/bootstrap/security.policy | 6 +- .../bootstrap/SecurityTests.java | 83 +++++++++++++++---- 4 files changed, 101 insertions(+), 72 deletions(-) diff --git a/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index 19bc81b9972..d351a9d4ea2 100644 --- a/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -59,7 +59,6 @@ public class Bootstrap { private static Bootstrap bootstrap; private void setup(boolean addShutdownHook, Settings settings, Environment environment) throws Exception { - setupSecurity(settings, environment); if (settings.getAsBoolean("bootstrap.mlockall", false)) { Natives.tryMlockall(); } @@ -90,6 +89,8 @@ public class Bootstrap { } }); } + // install SM after natives, JNA can require strange permissions + setupSecurity(settings, environment); } /** diff --git a/src/main/java/org/elasticsearch/bootstrap/Security.java b/src/main/java/org/elasticsearch/bootstrap/Security.java index afa8362771e..67ac531f0e7 100644 --- a/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -29,8 +29,6 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; -import java.util.HashSet; -import java.util.Set; /** * Initializes securitymanager with necessary permissions. @@ -68,55 +66,42 @@ class Security { try (InputStream in = new BufferedInputStream(template)) { ByteStreams.copy(in, output); } - - // add permissions for all configured paths. - Set paths = new HashSet<>(); - paths.add(environment.homeFile()); - paths.add(environment.configFile()); - paths.add(environment.logsFile()); - paths.add(environment.pluginsFile()); - for (Path path : environment.dataFiles()) { - paths.add(path); + + // all policy files are UTF-8: + // https://docs.oracle.com/javase/7/docs/technotes/guides/security/PolicyFiles.html + try (Writer writer = new OutputStreamWriter(output, StandardCharsets.UTF_8)) { + writer.write(System.lineSeparator()); + writer.write("grant {"); + writer.write(System.lineSeparator()); + + // add permissions for all configured paths. + // TODO: improve test infra so we can reduce permissions where read/write + // is not really needed... + addPath(writer, environment.homeFile(), "read,readlink,write,delete"); + addPath(writer, environment.configFile(), "read,readlink,write,delete"); + addPath(writer, environment.logsFile(), "read,readlink,write,delete"); + addPath(writer, environment.pluginsFile(), "read,readlink,write,delete"); + for (Path path : environment.dataFiles()) { + addPath(writer, path, "read,readlink,write,delete"); + } + for (Path path : environment.dataWithClusterFiles()) { + addPath(writer, path, "read,readlink,write,delete"); + } + + writer.write("};"); + writer.write(System.lineSeparator()); } - for (Path path : environment.dataWithClusterFiles()) { - paths.add(path); - } - output.write(createPermissions(paths)); } return processed; } - // package private for testing - static byte[] createPermissions(Set paths) throws IOException { - ByteArrayOutputStream stream = new ByteArrayOutputStream(); - - // all policy files are UTF-8: - // https://docs.oracle.com/javase/7/docs/technotes/guides/security/PolicyFiles.html - try (Writer writer = new OutputStreamWriter(stream, StandardCharsets.UTF_8)) { - writer.write(System.lineSeparator()); - writer.write("grant {"); - writer.write(System.lineSeparator()); - for (Path path : paths) { - // data paths actually may not exist yet. - Files.createDirectories(path); - // add each path twice: once for itself, again for files underneath it - addPath(writer, encode(path), "read,readlink,write,delete"); - addRecursivePath(writer, encode(path), "read,readlink,write,delete"); - } - writer.write("};"); - writer.write(System.lineSeparator()); - } - - return stream.toByteArray(); - } - - static void addPath(Writer writer, String path, String permissions) throws IOException { - writer.write("permission java.io.FilePermission \"" + path + "\", \"" + permissions + "\";"); + static void addPath(Writer writer, Path path, String permissions) throws IOException { + // paths may not exist yet + Files.createDirectories(path); + // add each path twice: once for itself, again for files underneath it + writer.write("permission java.io.FilePermission \"" + encode(path) + "\", \"" + permissions + "\";"); writer.write(System.lineSeparator()); - } - - static void addRecursivePath(Writer writer, String path, String permissions) throws IOException { - writer.write("permission java.io.FilePermission \"" + path + "${/}-\", \"" + permissions + "\";"); + writer.write("permission java.io.FilePermission \"" + encode(path) + "${/}-\", \"" + permissions + "\";"); writer.write(System.lineSeparator()); } @@ -124,10 +109,6 @@ class Security { // See "Note Regarding File Path Specifications on Windows Systems". // https://docs.oracle.com/javase/7/docs/technotes/guides/security/PolicyFiles.html static String encode(Path path) { - return encode(path.toString()); - } - - static String encode(String path) { - return path.replace("\\", "\\\\"); + return path.toString().replace("\\", "\\\\"); } } diff --git a/src/main/resources/org/elasticsearch/bootstrap/security.policy b/src/main/resources/org/elasticsearch/bootstrap/security.policy index 44b89c47c58..92056a3ab81 100644 --- a/src/main/resources/org/elasticsearch/bootstrap/security.policy +++ b/src/main/resources/org/elasticsearch/bootstrap/security.policy @@ -37,9 +37,6 @@ grant { permission java.io.FilePermission "${project.basedir}${/}lib/sigar{/}-", "read"; // mvn custom ./m2/repository for dependency jars permission java.io.FilePermission "${m2.repository}${/}-", "read"; - // per-jvm directory - permission java.io.FilePermission "${junit4.childvm.cwd}${/}temp", "read,write"; - permission java.io.FilePermission "${junit4.childvm.cwd}${/}temp${/}-", "read,write,delete"; permission java.nio.file.LinkPermission "symbolic"; permission groovy.security.GroovyCodeSourcePermission "/groovy/script"; @@ -86,7 +83,10 @@ grant { // needed for natives calls permission java.lang.RuntimePermission "loadLibrary.*"; + + // needed for testing access rules etc permission java.lang.RuntimePermission "createSecurityManager"; + permission java.security.SecurityPermission "createPolicy.JavaPolicy"; // reflection hacks: // needed for Striped64 (what is this doing), also enables unmap hack diff --git a/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java b/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java index d3a27f56b1f..4c2ddcd47eb 100644 --- a/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java +++ b/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java @@ -19,30 +19,77 @@ package org.elasticsearch.bootstrap; +import org.elasticsearch.common.settings.ImmutableSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.env.Environment; import org.elasticsearch.test.ElasticsearchTestCase; -import java.nio.charset.StandardCharsets; +import java.io.ByteArrayInputStream; +import java.io.FilePermission; import java.nio.file.Path; -import java.util.Collections; +import java.security.Policy; +import java.security.ProtectionDomain; +import java.security.URIParameter; public class SecurityTests extends ElasticsearchTestCase { - /** backslash escaping (e.g. windows paths) */ - public void testEncode() { - assertEquals("c:\\\\foobar", Security.encode("c:\\foobar")); - } - - /** test template processing */ - public void testTemplateProcessing() throws Exception { + /** test generated permissions */ + public void testGeneratedPermissions() throws Exception { Path path = createTempDir(); - - byte results[] = Security.createPermissions(Collections.singleton(path)); - String unicode = new String(results, StandardCharsets.UTF_8); - // try not to make this test too fragile or useless - assertTrue(unicode.contains("grant {")); - assertTrue(unicode.contains(Security.encode(path))); - assertTrue(unicode.contains("read")); - assertTrue(unicode.contains("write")); + // make a fake ES home and ensure we only grant permissions to that. + Path esHome = path.resolve("esHome"); + ImmutableSettings.Builder settingsBuilder = ImmutableSettings.builder(); + settingsBuilder.put("path.home", esHome.toString()); + Settings settings = settingsBuilder.build(); + + Environment environment = new Environment(settings); + Path policyFile = Security.processTemplate(new ByteArrayInputStream(new byte[0]), environment); + + ProtectionDomain domain = getClass().getProtectionDomain(); + Policy policy = Policy.getInstance("JavaPolicy", new URIParameter(policyFile.toUri())); + // the fake es home + assertTrue(policy.implies(domain, new FilePermission(esHome.toString(), "read"))); + // its parent + assertFalse(policy.implies(domain, new FilePermission(path.toString(), "read"))); + // some other sibling + assertFalse(policy.implies(domain, new FilePermission(path.resolve("other").toString(), "read"))); + } + + /** test generated permissions for all configured paths */ + public void testEnvironmentPaths() throws Exception { + Path path = createTempDir(); + + ImmutableSettings.Builder settingsBuilder = ImmutableSettings.builder(); + settingsBuilder.put("path.home", path.resolve("home").toString()); + settingsBuilder.put("path.conf", path.resolve("conf").toString()); + settingsBuilder.put("path.plugins", path.resolve("plugins").toString()); + settingsBuilder.putArray("path.data", path.resolve("data1").toString(), path.resolve("data2").toString()); + settingsBuilder.put("path.logs", path.resolve("logs").toString()); + Settings settings = settingsBuilder.build(); + + Environment environment = new Environment(settings); + Path policyFile = Security.processTemplate(new ByteArrayInputStream(new byte[0]), environment); + + ProtectionDomain domain = getClass().getProtectionDomain(); + Policy policy = Policy.getInstance("JavaPolicy", new URIParameter(policyFile.toUri())); + + // check that all directories got permissions: + // homefile: this is needed unless we break out rules for "lib" dir. + // TODO: make read-only + assertTrue(policy.implies(domain, new FilePermission(environment.homeFile().toString(), "read,readlink,write,delete"))); + // config file + // TODO: make read-only + assertTrue(policy.implies(domain, new FilePermission(environment.configFile().toString(), "read,readlink,write,delete"))); + // plugins: r/w, TODO: can this be minimized? + assertTrue(policy.implies(domain, new FilePermission(environment.pluginsFile().toString(), "read,readlink,write,delete"))); + // data paths: r/w + for (Path dataPath : environment.dataFiles()) { + assertTrue(policy.implies(domain, new FilePermission(dataPath.toString(), "read,readlink,write,delete"))); + } + for (Path dataPath : environment.dataWithClusterFiles()) { + assertTrue(policy.implies(domain, new FilePermission(dataPath.toString(), "read,readlink,write,delete"))); + } + // logs: r/w + assertTrue(policy.implies(domain, new FilePermission(environment.logsFile().toString(), "read,readlink,write,delete"))); } - }