Browse Source

Ensure all Security configuration is covered by enhanced file protections (#112408) (#114054)

This commit the work started in #108767 to other security config, adding additional protections to the other on-disk files used by the Security module. With this change, non-Security modules will be prevented from accessing Security's on-disk configuration files.

(cherry picked from commit 8fed8e63b0d05dbe975dbbcc391f3863563a2334)
Athena Brown 1 year ago
parent
commit
e3a040ac47

+ 14 - 3
server/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java

@@ -16,6 +16,7 @@ import java.io.FilePermission;
 import java.io.IOException;
 import java.net.SocketPermission;
 import java.net.URL;
+import java.security.AllPermission;
 import java.security.CodeSource;
 import java.security.Permission;
 import java.security.PermissionCollection;
@@ -39,6 +40,7 @@ final class ESPolicy extends Policy {
     static final String UNTRUSTED_RESOURCE = "untrusted.policy";
 
     private static final String ALL_FILE_MASK = "read,readlink,write,delete,execute";
+    private static final AllPermission ALL_PERMISSION = new AllPermission();
 
     final Policy template;
     final Policy untrusted;
@@ -124,7 +126,7 @@ final class ESPolicy extends Policy {
              * It's helpful to use the infrastructure around FilePermission here to do the directory structure check with implies
              * so we use ALL_FILE_MASK mask to check if we can do something with this file, whatever the actual operation we're requesting
              */
-            return canAccessSecuredFile(location, new FilePermission(permission.getName(), ALL_FILE_MASK));
+            return canAccessSecuredFile(domain, new FilePermission(permission.getName(), ALL_FILE_MASK));
         }
 
         if (location != null) {
@@ -157,15 +159,24 @@ final class ESPolicy extends Policy {
     }
 
     @SuppressForbidden(reason = "We get given an URL by the security infrastructure")
-    private boolean canAccessSecuredFile(URL location, FilePermission permission) {
-        if (location == null) {
+    private boolean canAccessSecuredFile(ProtectionDomain domain, FilePermission permission) {
+        if (domain == null || domain.getCodeSource() == null || domain.getCodeSource().getLocation() == null) {
             return false;
         }
 
+        // If the domain in question has AllPermission - only true of sources built into the JDK, as we prevent AllPermission from being
+        // configured in Elasticsearch - then it has access to this file.
+
+        if (system.implies(domain, ALL_PERMISSION)) {
+            return true;
+        }
+        URL location = domain.getCodeSource().getLocation();
+
         // check the source
         Set<URL> accessibleSources = securedFiles.get(permission);
         if (accessibleSources != null) {
             // simple case - single-file referenced directly
+
             return accessibleSources.contains(location);
         } else {
             // there's a directory reference in there somewhere

+ 18 - 9
server/src/main/java/org/elasticsearch/bootstrap/Security.java

@@ -47,6 +47,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Consumer;
@@ -236,17 +237,25 @@ final class Security {
             for (Map.Entry<Pattern, Set<URL>> ps : settingPatterns) {
                 if (ps.getKey().matcher(setting).matches()) {
                     // add the setting value to the secured files for these codebase URLs
-                    Path file = environment.configFile().resolve(environment.settings().get(setting));
-                    if (file.startsWith(environment.configFile()) == false) {
-                        throw new IllegalStateException(ps.getValue() + " tried to grant access to file outside config directory " + file);
-                    }
-                    if (logger.isDebugEnabled()) {
-                        ps.getValue()
-                            .forEach(
-                                url -> logger.debug("Jar {} securing access to config file {} through setting {}", url, file, setting)
+                    String settingValue = environment.settings().get(setting);
+                    // Some settings can also be an HTTPS URL in addition to a file path; if that's the case just skip this one.
+                    // If the setting shouldn't be an HTTPS URL, that'll be caught by that setting's validation later in the process.
+                    // HTTP (no S) URLs are not supported.
+                    if (settingValue.toLowerCase(Locale.ROOT).startsWith("https://") == false) {
+                        Path file = environment.configFile().resolve(settingValue);
+                        if (file.startsWith(environment.configFile()) == false) {
+                            throw new IllegalStateException(
+                                ps.getValue() + " tried to grant access to file outside config directory " + file
                             );
+                        }
+                        if (logger.isDebugEnabled()) {
+                            ps.getValue()
+                                .forEach(
+                                    url -> logger.debug("Jar {} securing access to config file {} through setting {}", url, file, setting)
+                                );
+                        }
+                        securedConfigFiles.computeIfAbsent(file.toString(), k -> new HashSet<>()).addAll(ps.getValue());
                     }
-                    securedConfigFiles.computeIfAbsent(file.toString(), k -> new HashSet<>()).addAll(ps.getValue());
                 }
             }
         }

+ 2 - 1
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtUtil.java

@@ -228,7 +228,8 @@ public class JwtUtil {
         throws SettingsException {
         try {
             final Path path = JwtUtil.resolvePath(environment, jwkSetPathPkc);
-            return Files.readAllBytes(path);
+            byte[] bytes = AccessController.doPrivileged((PrivilegedExceptionAction<byte[]>) () -> Files.readAllBytes(path));
+            return bytes;
         } catch (Exception e) {
             throw new SettingsException(
                 "Failed to read contents for setting [" + jwkSetConfigKeyPkc + "] value [" + jwkSetPathPkc + "].",

+ 16 - 7
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosRealm.java

@@ -29,6 +29,8 @@ import org.ietf.jgss.GSSException;
 
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -101,19 +103,26 @@ public final class KerberosRealm extends Realm implements CachingRealm {
         this.threadPool = threadPool;
         this.keytabPath = config.env().configFile().resolve(config.getSetting(KerberosRealmSettings.HTTP_SERVICE_KEYTAB_PATH));
 
-        if (Files.exists(keytabPath) == false) {
+        validateKeytab(this.keytabPath);
+
+        this.enableKerberosDebug = config.getSetting(KerberosRealmSettings.SETTING_KRB_DEBUG_ENABLE);
+        this.removeRealmName = config.getSetting(KerberosRealmSettings.SETTING_REMOVE_REALM_NAME);
+        this.delegatedRealms = null;
+    }
+
+    private static void validateKeytab(Path keytabPath) {
+        boolean fileExists = AccessController.doPrivileged((PrivilegedAction<Boolean>) () -> Files.exists(keytabPath));
+        if (fileExists == false) {
             throw new IllegalArgumentException("configured service key tab file [" + keytabPath + "] does not exist");
         }
-        if (Files.isDirectory(keytabPath)) {
+        boolean pathIsDir = AccessController.doPrivileged((PrivilegedAction<Boolean>) () -> Files.isDirectory(keytabPath));
+        if (pathIsDir) {
             throw new IllegalArgumentException("configured service key tab file [" + keytabPath + "] is a directory");
         }
-        if (Files.isReadable(keytabPath) == false) {
+        boolean isReadable = AccessController.doPrivileged((PrivilegedAction<Boolean>) () -> Files.isReadable(keytabPath));
+        if (isReadable == false) {
             throw new IllegalArgumentException("configured service key tab file [" + keytabPath + "] must have read permission");
         }
-
-        this.enableKerberosDebug = config.getSetting(KerberosRealmSettings.SETTING_KRB_DEBUG_ENABLE);
-        this.removeRealmName = config.getSetting(KerberosRealmSettings.SETTING_REMOVE_REALM_NAME);
-        this.delegatedRealms = null;
     }
 
     @Override

+ 10 - 3
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java

@@ -93,6 +93,7 @@ import org.elasticsearch.xpack.core.security.authc.RealmConfig;
 import org.elasticsearch.xpack.core.security.authc.RealmSettings;
 import org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings;
 import org.elasticsearch.xpack.core.ssl.SSLService;
+import org.elasticsearch.xpack.security.PrivilegedFileWatcher;
 import org.elasticsearch.xpack.security.authc.jwt.JwtUtil;
 
 import java.io.IOException;
@@ -366,8 +367,14 @@ public class OpenIdConnectAuthenticator {
     private JWKSet readJwkSetFromFile(String jwkSetPath) throws IOException, ParseException {
         final Path path = realmConfig.env().configFile().resolve(jwkSetPath);
         // avoid using JWKSet.loadFile() as it does not close FileInputStream internally
-        String jwkSet = Files.readString(path, StandardCharsets.UTF_8);
-        return JWKSet.parse(jwkSet);
+        try {
+            String jwkSet = AccessController.doPrivileged(
+                (PrivilegedExceptionAction<String>) () -> Files.readString(path, StandardCharsets.UTF_8)
+            );
+            return JWKSet.parse(jwkSet);
+        } catch (PrivilegedActionException ex) {
+            throw (IOException) ex.getException();
+        }
     }
 
     /**
@@ -808,7 +815,7 @@ public class OpenIdConnectAuthenticator {
 
     private void setMetadataFileWatcher(String jwkSetPath) throws IOException {
         final Path path = realmConfig.env().configFile().resolve(jwkSetPath);
-        FileWatcher watcher = new FileWatcher(path);
+        FileWatcher watcher = new PrivilegedFileWatcher(path);
         watcher.addListener(new FileListener(LOGGER, () -> this.idTokenValidator.set(createIdTokenValidator(false))));
         watcherService.add(watcher, ResourceWatcherService.Frequency.MEDIUM);
     }

+ 7 - 2
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java

@@ -49,6 +49,7 @@ import org.elasticsearch.xpack.core.security.authc.support.UserRoleMapper;
 import org.elasticsearch.xpack.core.security.user.User;
 import org.elasticsearch.xpack.core.ssl.CertParsingUtils;
 import org.elasticsearch.xpack.core.ssl.SSLService;
+import org.elasticsearch.xpack.security.PrivilegedFileWatcher;
 import org.elasticsearch.xpack.security.authc.Realms;
 import org.elasticsearch.xpack.security.authc.TokenService;
 import org.elasticsearch.xpack.security.authc.support.DelegatedAuthorizationSupport;
@@ -774,7 +775,11 @@ public final class SamlRealm extends Realm implements Releasable {
         @Override
         protected byte[] fetchMetadata() throws ResolverException {
             assert assertNotTransportThread("fetching SAML metadata from a file");
-            return super.fetchMetadata();
+            try {
+                return AccessController.doPrivileged((PrivilegedExceptionAction<byte[]>) () -> super.fetchMetadata());
+            } catch (PrivilegedActionException e) {
+                throw (ResolverException) e.getException();
+            }
         }
     }
 
@@ -806,7 +811,7 @@ public final class SamlRealm extends Realm implements Releasable {
         resolver.setMaxRefreshDelay(oneDayMs);
         initialiseResolver(resolver, config);
 
-        FileWatcher watcher = new FileWatcher(path);
+        FileWatcher watcher = new PrivilegedFileWatcher(path);
         watcher.addListener(new FileListener(logger, resolver::refresh));
         watcherService.add(watcher, ResourceWatcherService.Frequency.MEDIUM);
         return new Tuple<>(resolver, () -> resolveEntityDescriptor(resolver, entityId, path.toString(), true));

+ 4 - 0
x-pack/plugin/security/src/main/plugin-metadata/plugin-security.policy

@@ -6,6 +6,10 @@ grant {
   permission org.elasticsearch.SecuredConfigFileAccessPermission "x-pack/users";
   // other security files specified by settings
   permission org.elasticsearch.SecuredConfigFileSettingAccessPermission "xpack.security.authc.realms.ldap.*.files.role_mapping";
+  permission org.elasticsearch.SecuredConfigFileSettingAccessPermission "xpack.security.authc.realms.pki.*.files.role_mapping";
+  permission org.elasticsearch.SecuredConfigFileSettingAccessPermission "xpack.security.authc.realms.jwt.*.pkc_jwkset_path";
+  permission org.elasticsearch.SecuredConfigFileSettingAccessPermission "xpack.security.authc.realms.saml.*.idp.metadata.path";
+  permission org.elasticsearch.SecuredConfigFileSettingAccessPermission "xpack.security.authc.realms.kerberos.*.keytab.path";
 
   // needed for SAML
   permission java.util.PropertyPermission "org.apache.xml.security.ignoreLineBreaks", "read,write";