From 4fca5f734a4aac9a4d1e52639282a9ad6b79f22d Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Mon, 18 Apr 2016 15:31:28 -0600 Subject: [PATCH] Explicitly set packaging permissions This changes our packaging to be explicit about the permissions of files and directories in the tar.gz, rpm, and deb packages. This is to protect against a user having an incorrectly set umask when installing. Additionally, plugins that are installed now have their permissions set by the plugin installation so that plugins that may have been packaged with incorrect permissions are secured. Resolves #17634 --- .../plugins/InstallPluginCommand.java | 74 ++++++++++++------- distribution/build.gradle | 7 ++ distribution/deb/build.gradle | 1 + distribution/rpm/build.gradle | 2 + .../src/main/packaging/scripts/postinst | 6 ++ distribution/tar/build.gradle | 2 + .../plugins/InstallPluginCommandTests.java | 6 +- .../scripts/packaging_test_utils.bash | 38 ++++++---- .../resources/packaging/scripts/plugins.bash | 12 +-- .../test/resources/packaging/scripts/tar.bash | 2 +- 10 files changed, 95 insertions(+), 55 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java b/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java index 4afe6b57e7e..2493c71b7ed 100644 --- a/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java +++ b/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java @@ -51,6 +51,7 @@ import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; @@ -134,6 +135,30 @@ class InstallPluginCommand extends Command { private final OptionSpec batchOption; private final OptionSpec arguments; + public static final Set DIR_AND_EXECUTABLE_PERMS; + public static final Set FILE_PERMS; + + static { + Set dirAndExecutablePerms = new HashSet<>(7); + // Directories and executables get chmod 755 + dirAndExecutablePerms.add(PosixFilePermission.OWNER_EXECUTE); + dirAndExecutablePerms.add(PosixFilePermission.OWNER_READ); + dirAndExecutablePerms.add(PosixFilePermission.OWNER_WRITE); + dirAndExecutablePerms.add(PosixFilePermission.GROUP_EXECUTE); + dirAndExecutablePerms.add(PosixFilePermission.GROUP_READ); + dirAndExecutablePerms.add(PosixFilePermission.OTHERS_READ); + dirAndExecutablePerms.add(PosixFilePermission.OTHERS_EXECUTE); + DIR_AND_EXECUTABLE_PERMS = Collections.unmodifiableSet(dirAndExecutablePerms); + + Set filePerms = new HashSet<>(4); + // Files get chmod 644 + filePerms.add(PosixFilePermission.OWNER_READ); + filePerms.add(PosixFilePermission.OWNER_WRITE); + filePerms.add(PosixFilePermission.GROUP_READ); + filePerms.add(PosixFilePermission.OTHERS_READ); + FILE_PERMS = Collections.unmodifiableSet(filePerms); + } + InstallPluginCommand(Environment env) { super("Install a plugin"); this.env = env; @@ -298,15 +323,7 @@ class InstallPluginCommand extends Command { private Path stagingDirectory(Path pluginsDir) throws IOException { try { - Set perms = new HashSet<>(); - perms.add(PosixFilePermission.OWNER_EXECUTE); - perms.add(PosixFilePermission.OWNER_READ); - perms.add(PosixFilePermission.OWNER_WRITE); - perms.add(PosixFilePermission.GROUP_READ); - perms.add(PosixFilePermission.GROUP_EXECUTE); - perms.add(PosixFilePermission.OTHERS_READ); - perms.add(PosixFilePermission.OTHERS_EXECUTE); - return Files.createTempDirectory(pluginsDir, ".installing-", PosixFilePermissions.asFileAttribute(perms)); + return Files.createTempDirectory(pluginsDir, ".installing-", PosixFilePermissions.asFileAttribute(DIR_AND_EXECUTABLE_PERMS)); } catch (IllegalArgumentException e) { // Jimfs throws an IAE where it should throw an UOE // remove when google/jimfs#30 is integrated into Jimfs @@ -409,6 +426,15 @@ class InstallPluginCommand extends Command { } Files.move(tmpRoot, destination, StandardCopyOption.ATOMIC_MOVE); + try (DirectoryStream stream = Files.newDirectoryStream(destination)) { + for (Path pluginFile : stream) { + if (Files.isDirectory(pluginFile)) { + setFileAttributes(pluginFile, DIR_AND_EXECUTABLE_PERMS); + } else { + setFileAttributes(pluginFile, FILE_PERMS); + } + } + } terminal.println("-> Installed " + info.getName()); } catch (Exception installProblem) { @@ -427,17 +453,7 @@ class InstallPluginCommand extends Command { throw new UserError(ExitCodes.IO_ERROR, "bin in plugin " + info.getName() + " is not a directory"); } Files.createDirectory(destBinDir); - - // setup file attributes for the installed files to those of the parent dir - final Set perms = new HashSet<>(); - final PosixFileAttributeView binAttributeView = Files.getFileAttributeView(destBinDir.getParent(), PosixFileAttributeView.class); - if (binAttributeView != null) { - perms.addAll(binAttributeView.readAttributes().permissions()); - // setting execute bits, since this just means "the file is executable", and actual execution requires read - perms.add(PosixFilePermission.OWNER_EXECUTE); - perms.add(PosixFilePermission.GROUP_EXECUTE); - perms.add(PosixFilePermission.OTHERS_EXECUTE); - } + setFileAttributes(destBinDir, DIR_AND_EXECUTABLE_PERMS); try (DirectoryStream stream = Files.newDirectoryStream(tmpBinDir)) { for (Path srcFile : stream) { @@ -449,11 +465,7 @@ class InstallPluginCommand extends Command { Path destFile = destBinDir.resolve(tmpBinDir.relativize(srcFile)); Files.copy(srcFile, destFile); - - final PosixFileAttributeView view = Files.getFileAttributeView(destFile, PosixFileAttributeView.class); - if (view != null) { - view.setPermissions(perms); - } + setFileAttributes(destFile, DIR_AND_EXECUTABLE_PERMS); } } IOUtils.rm(tmpBinDir); // clean up what we just copied @@ -468,8 +480,8 @@ class InstallPluginCommand extends Command { throw new UserError(ExitCodes.IO_ERROR, "config in plugin " + info.getName() + " is not a directory"); } - // create the plugin's config dir "if necessary" Files.createDirectories(destConfigDir); + setFileAttributes(destConfigDir, DIR_AND_EXECUTABLE_PERMS); final PosixFileAttributeView destConfigDirAttributesView = Files.getFileAttributeView(destConfigDir.getParent(), PosixFileAttributeView.class); final PosixFileAttributes destConfigDirAttributes = @@ -487,6 +499,7 @@ class InstallPluginCommand extends Command { Path destFile = destConfigDir.resolve(tmpConfigDir.relativize(srcFile)); if (Files.exists(destFile) == false) { Files.copy(srcFile, destFile); + setFileAttributes(destFile, FILE_PERMS); if (destConfigDirAttributes != null) { setOwnerGroup(destFile, destConfigDirAttributes); } @@ -504,4 +517,13 @@ class InstallPluginCommand extends Command { fileAttributeView.setGroup(attributes.group()); } + /** + * Sets the attributes for a path iff posix attributes are supported + */ + private static void setFileAttributes(final Path path, final Set permissions) throws IOException { + PosixFileAttributeView fileAttributeView = Files.getFileAttributeView(path, PosixFileAttributeView.class); + if (fileAttributeView != null) { + Files.setPosixFilePermissions(path, permissions); + } + } } diff --git a/distribution/build.gradle b/distribution/build.gradle index 4050f9ccd02..09050db2159 100644 --- a/distribution/build.gradle +++ b/distribution/build.gradle @@ -239,6 +239,8 @@ configure(subprojects.findAll { ['deb', 'rpm'].contains(it.name) }) { dependsOn createEtc, createEtcScripts with configFiles into "${packagingFiles}/etc/elasticsearch" + fileMode 0640 + dirMode 0750 /* Explicitly declare the output files so this task doesn't consider itself up to date when the directory is created, which it would by default. And that'll happen when createEtc runs. */ @@ -303,6 +305,8 @@ configure(subprojects.findAll { ['deb', 'rpm'].contains(it.name) }) { postUninstall file("${scripts}/postrm") into '/usr/share/elasticsearch' + fileMode 0644 + dirMode 0755 user 'root' permissionGroup 'root' with libFiles @@ -344,6 +348,8 @@ configure(subprojects.findAll { ['deb', 'rpm'].contains(it.name) }) { configurationFile project.expansions['path.env'] into(new File(project.expansions['path.env']).getParent()) { from "${project.packagingFiles}/env/elasticsearch" + fileMode 0644 + dirMode 0755 } /** @@ -351,6 +357,7 @@ configure(subprojects.findAll { ['deb', 'rpm'].contains(it.name) }) { */ Closure suckUpEmptyDirectories = { path, u, g -> into(path) { + fileMode 0755 from "${packagingFiles}/${path}" includeEmptyDirs true createDirectoryEntry true diff --git a/distribution/deb/build.gradle b/distribution/deb/build.gradle index d9bd8447ab9..a3d42a2c042 100644 --- a/distribution/deb/build.gradle +++ b/distribution/deb/build.gradle @@ -31,6 +31,7 @@ task buildDeb(type: Deb) { } into('/usr/share/doc/elasticsearch') { from "${project.packagingFiles}/copyright" + fileMode 0644 } } diff --git a/distribution/rpm/build.gradle b/distribution/rpm/build.gradle index a3777d39029..0d9f658f488 100644 --- a/distribution/rpm/build.gradle +++ b/distribution/rpm/build.gradle @@ -38,6 +38,8 @@ task buildRpm(type: Rpm) { license '2009' distribution 'Elasticsearch' vendor 'Elasticsearch' + dirMode 0755 + fileMode 0644 // TODO ospackage doesn't support icon but we used to have one } diff --git a/distribution/src/main/packaging/scripts/postinst b/distribution/src/main/packaging/scripts/postinst index 4dd5bbf5288..451ec3457d3 100644 --- a/distribution/src/main/packaging/scripts/postinst +++ b/distribution/src/main/packaging/scripts/postinst @@ -99,5 +99,11 @@ fi chown -R $ES_USER:$ES_GROUP /var/lib/elasticsearch chown -R $ES_USER:$ES_GROUP /var/log/elasticsearch chown -R root:$ES_GROUP /etc/elasticsearch +chmod 0750 /etc/elasticsearch +chmod 0750 /etc/elasticsearch/scripts + +if [ -f /etc/sysconfig/elasticsearch ]; then + chmod 0644 /etc/sysconfig/elasticsearch +fi ${scripts.footer} diff --git a/distribution/tar/build.gradle b/distribution/tar/build.gradle index 624f5b57022..9edba6c11a2 100644 --- a/distribution/tar/build.gradle +++ b/distribution/tar/build.gradle @@ -22,6 +22,8 @@ task buildTar(type: Tar) { extension = 'tar.gz' with archivesFiles compression = Compression.GZIP + dirMode 0755 + fileMode 0644 } artifacts { diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java index 106385578e9..af36d96f442 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java @@ -236,11 +236,7 @@ public class InstallPluginCommandTests extends ESTestCase { assertFalse("not a dir", Files.isDirectory(file)); if (isPosix) { PosixFileAttributes attributes = Files.readAttributes(file, PosixFileAttributes.class); - Set expectedPermissions = new HashSet<>(binAttributes.permissions()); - expectedPermissions.add(PosixFilePermission.OWNER_EXECUTE); - expectedPermissions.add(PosixFilePermission.GROUP_EXECUTE); - expectedPermissions.add(PosixFilePermission.OTHERS_EXECUTE); - assertEquals(expectedPermissions, attributes.permissions()); + assertEquals(InstallPluginCommand.DIR_AND_EXECUTABLE_PERMS, attributes.permissions()); } } } diff --git a/qa/vagrant/src/test/resources/packaging/scripts/packaging_test_utils.bash b/qa/vagrant/src/test/resources/packaging/scripts/packaging_test_utils.bash index 13f73c288a2..09d0190695e 100644 --- a/qa/vagrant/src/test/resources/packaging/scripts/packaging_test_utils.bash +++ b/qa/vagrant/src/test/resources/packaging/scripts/packaging_test_utils.bash @@ -134,14 +134,18 @@ skip_not_zip() { assert_file_exist() { local file="$1" - echo "Should exist: ${file}" + if [ ! -e "$file" ]; then + echo "Should exist: ${file} but does not" + fi local file=$(readlink -m "${file}") [ -e "$file" ] } assert_file_not_exist() { local file="$1" - echo "Should not exist: ${file}" + if [ -e "$file" ]; then + echo "Should not exist: ${file} but does" + fi local file=$(readlink -m "${file}") [ ! -e "$file" ] } @@ -156,28 +160,38 @@ assert_file() { assert_file_exist "$file" if [ "$type" = "d" ]; then - echo "And be a directory...." + if [ ! -d "$file" ]; then + echo "[$file] should be a directory but is not" + fi [ -d "$file" ] else - echo "And be a regular file...." + if [ ! -f "$file" ]; then + echo "[$file] should be a regular file but is not" + fi [ -f "$file" ] fi if [ "x$user" != "x" ]; then realuser=$(find "$file" -maxdepth 0 -printf "%u") - echo "Expected user: $user, found $realuser" + if [ "$realuser" != "$user" ]; then + echo "Expected user: $user, found $realuser [$file]" + fi [ "$realuser" = "$user" ] fi if [ "x$group" != "x" ]; then realgroup=$(find "$file" -maxdepth 0 -printf "%g") - echo "Expected group: $group, found $realgroup" + if [ "$realgroup" != "$group" ]; then + echo "Expected group: $group, found $realgroup [$file]" + fi [ "$realgroup" = "$group" ] fi if [ "x$privileges" != "x" ]; then realprivileges=$(find "$file" -maxdepth 0 -printf "%m") - echo "Expected privileges: $privileges, found $realprivileges" + if [ "$realprivileges" != "$privileges" ]; then + echo "Expected privileges: $privileges, found $realprivileges [$file]" + fi [ "$realprivileges" = "$privileges" ] fi } @@ -190,10 +204,8 @@ assert_module_or_plugin_directory() { #just make sure that everything is the same as $CONFIG_DIR, which was properly set up during install config_user=$(find "$ESHOME" -maxdepth 0 -printf "%u") config_owner=$(find "$ESHOME" -maxdepth 0 -printf "%g") - # directories should use the user file-creation mask - config_privileges=$(executable_privileges_for_user_from_umask $ESPLUGIN_COMMAND_USER) - assert_file $directory d $config_user $config_owner $(printf "%o" $config_privileges) + assert_file $directory d $config_user $config_owner 755 } assert_module_or_plugin_file() { @@ -201,11 +213,7 @@ assert_module_or_plugin_file() { shift assert_file_exist "$(readlink -m $file)" - - # config files should not be executable and otherwise use the user - # file-creation mask - expected_file_privileges=$(file_privileges_for_user_from_umask $ESPLUGIN_COMMAND_USER) - assert_file $file f $config_user $config_owner $(printf "%o" $expected_file_privileges) + assert_file $file f $config_user $config_owner 644 } assert_output() { diff --git a/qa/vagrant/src/test/resources/packaging/scripts/plugins.bash b/qa/vagrant/src/test/resources/packaging/scripts/plugins.bash index 925beaade09..4f1e574b905 100644 --- a/qa/vagrant/src/test/resources/packaging/scripts/plugins.bash +++ b/qa/vagrant/src/test/resources/packaging/scripts/plugins.bash @@ -84,20 +84,16 @@ install_jvm_example() { bin_user=$(find "$ESHOME/bin" -maxdepth 0 -printf "%u") bin_owner=$(find "$ESHOME/bin" -maxdepth 0 -printf "%g") bin_privileges=$(find "$ESHOME/bin" -maxdepth 0 -printf "%m") - assert_file "$ESHOME/bin/jvm-example" d $bin_user $bin_owner $bin_privileges - assert_file "$ESHOME/bin/jvm-example/test" f $bin_user $bin_owner $bin_privileges + assert_file "$ESHOME/bin/jvm-example" d $bin_user $bin_owner 755 + assert_file "$ESHOME/bin/jvm-example/test" f $bin_user $bin_owner 755 #owner group and permissions vary depending on how es was installed #just make sure that everything is the same as $CONFIG_DIR, which was properly set up during install config_user=$(find "$ESCONFIG" -maxdepth 0 -printf "%u") config_owner=$(find "$ESCONFIG" -maxdepth 0 -printf "%g") # directories should user the user file-creation mask - config_privileges=$(executable_privileges_for_user_from_umask $ESPLUGIN_COMMAND_USER) - assert_file "$ESCONFIG/jvm-example" d $config_user $config_owner $(printf "%o" $config_privileges) - # config files should not be executable and otherwise use the user - # file-creation mask - expected_file_privileges=$(file_privileges_for_user_from_umask $ESPLUGIN_COMMAND_USER) - assert_file "$ESCONFIG/jvm-example/example.yaml" f $config_user $config_owner $(printf "%o" $expected_file_privileges) + assert_file "$ESCONFIG/jvm-example" d $config_user $config_owner 755 + assert_file "$ESCONFIG/jvm-example/example.yaml" f $config_user $config_owner 644 echo "Running jvm-example's bin script...." "$ESHOME/bin/jvm-example/test" | grep test diff --git a/qa/vagrant/src/test/resources/packaging/scripts/tar.bash b/qa/vagrant/src/test/resources/packaging/scripts/tar.bash index 56b162cdefe..277eee60f1a 100644 --- a/qa/vagrant/src/test/resources/packaging/scripts/tar.bash +++ b/qa/vagrant/src/test/resources/packaging/scripts/tar.bash @@ -33,7 +33,7 @@ install_archive() { echo "Unpacking tarball to $ESHOME" rm -rf /tmp/untar mkdir -p /tmp/untar - tar -xzf elasticsearch*.tar.gz -C /tmp/untar + tar -xzpf elasticsearch*.tar.gz -C /tmp/untar find /tmp/untar -depth -type d -name 'elasticsearch*' -exec mv {} "$ESHOME" \; > /dev/null