Copilot commented on code in PR #4471: URL: https://github.com/apache/solr/pull/4471#discussion_r3317328475
########## solr/agent-sm/src/java/org/apache/solr/security/agent/ApprovedCallSite.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF 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.apache.solr.security.agent; + +/** + * A class (or class-name prefix pattern) that is approved to perform a restricted operation such as + * calling {@code System.exit()} or spawning a child process via {@code ProcessBuilder}. + * + * <p>Approved call sites are loaded from the security policy at startup and are immutable. Entries + * use either an exact fully-qualified class name or a prefix ending in {@code .*} (e.g. {@code + * org.apache.solr.cli.*}). + * + * <p>Default approved EXIT callers: + * + * <ul> + * <li>{@code org.apache.solr.cli.SolrCLI} + * <li>{@code org.apache.solr.servlet.SolrDispatchFilter} + * </ul> + * + * <p>Default approved EXEC callers: none (empty list in production policy). Review Comment: The Javadoc states "Default approved EXIT callers: org.apache.solr.cli.SolrCLI, org.apache.solr.servlet.SolrDispatchFilter" and "Default approved EXEC callers: none." However, there is no code anywhere that seeds these defaults — the approved-callers lists are populated solely from `RuntimePermission "exitVM"`/`"exec"` grants parsed out of the policy files. `agent-security.policy` does not contain any such grants, so in practice `approvedExitCallers` and `approvedExecCallers` will be empty. In enforce mode this means SolrCLI/SolrDispatchFilter would themselves be blocked from calling `System.exit()`. Either add the grants to `agent-security.policy` (the documented and intended behavior), or correct the Javadoc to reflect that the defaults are empty. ########## solr/agent-sm/src/java/org/apache/solr/security/agent/PolicyLoader.java: ########## @@ -0,0 +1,309 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF 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.apache.solr.security.agent; + +import java.io.IOException; +import java.io.StringReader; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; + +/** + * Reads and parses JDK-style {@code .policy} files, performing Solr-specific variable substitution, + * and produces a {@link AgentPolicy} ready for enforcement. + * + * <h2>Variable substitution</h2> + * + * {@code ${property}} placeholders in permission targets and {@code codeBase} URLs are expanded + * per-token by {@link PolicyPropertyExpander} using system properties. Any unresolved placeholder + * causes startup to fail immediately (fail-fast). The only built-in default is {@code + * ${solr.zk.port}}, which falls back to {@code solr.port + 1000} when not explicitly set. + * + * <h2>Two-file merge</h2> + * + * Two files are loaded: the mandatory default policy and an optional operator extension file. The + * extra-policy file path is resolved from system property {@code solr.security.agent.extra.policy}, + * falling back to {@code ${server.dir}/etc/agent-security-extra.policy}. An absent extra-policy + * file is silently skipped. Each entry carries a {@link PermittedPath#source() source} tag of + * either {@link PolicySource#DEFAULT} or {@link PolicySource#OPERATOR}. + * + * <p>A missing or unparseable <em>default</em> policy causes an {@link IllegalStateException} at + * startup. + */ +public class PolicyLoader { + + /** + * Loads and merges the default policy file and the optional operator extension file. + * + * @param defaultPolicyPath absolute path to the default {@code agent-security.policy} file + * @return a fully initialized {@link AgentPolicy} + * @throws IllegalStateException if the default policy file is absent or cannot be parsed + */ + public AgentPolicy load(Path defaultPolicyPath) { + if (!Files.exists(defaultPolicyPath)) { + throw new IllegalStateException( + "Security agent default policy not found: " + + defaultPolicyPath + + ". Solr cannot start without a valid security policy. " + + "Check that agent-security.policy is present in server/etc/."); + } + + String defaultContent; + try { + defaultContent = Files.readString(defaultPolicyPath, StandardCharsets.UTF_8); + } catch (IOException e) { + throw new IllegalStateException( + "Failed to read security agent default policy: " + defaultPolicyPath, e); + } + + List<GrantBlock> grants = new ArrayList<>(); + parsePolicy(defaultContent, PolicySource.DEFAULT, grants); + if (grants.isEmpty()) { + throw new IllegalStateException( + "Security agent default policy contains no grant blocks: " + + defaultPolicyPath + + ". The default policy must define at least one grant."); + } + + // Resolve extra-policy path: system property → fallback to ${server.dir}/etc/... + Path extraPolicyPath = resolveExtraPolicyPath(); + if (extraPolicyPath != null && Files.exists(extraPolicyPath)) { + String extraContent; + try { + extraContent = Files.readString(extraPolicyPath, StandardCharsets.UTF_8); + } catch (IOException e) { + throw new IllegalStateException( + "Failed to read operator security policy extension: " + extraPolicyPath, e); + } + int beforeCount = grants.size(); + parsePolicy(extraContent, PolicySource.OPERATOR, grants); + if (grants.size() == beforeCount) { + agentOut( + "[Solr SecurityAgent] Operator extension policy is empty (no grant blocks): " + + extraPolicyPath); + } + } + + return buildPolicy(grants); + } + + /** + * Resolves the extra-policy file path from system property {@code + * solr.security.agent.extra.policy}, falling back to {@code + * ${server.dir}/etc/agent-security-extra.policy}. Returns {@code null} if no fallback is + * available. + */ + static Path resolveExtraPolicyPath() { + String explicitPath = + PolicyPropertyExpander.getPropertyOrEnv("solr.security.agent.extra.policy"); + if (explicitPath != null && !explicitPath.isBlank()) { + return Path.of(explicitPath); + } + String serverDir = System.getProperty("jetty.home", System.getProperty("server.dir")); + if (serverDir != null && !serverDir.isBlank()) { + return Path.of(serverDir, "etc", "agent-security-extra.policy"); + } + return null; + } + + /** + * Parses a policy file and appends the resulting {@link GrantBlock} entries — tagged with the + * given {@code source} — to {@code out}. Variable substitution is performed per-token by {@link + * PolicyPropertyExpander}; any unresolved {@code ${variable}} causes an {@link + * IllegalStateException}. + */ + static void parsePolicy(String content, PolicySource source, List<GrantBlock> out) { + parsePolicyBlocks(content, source, out); + } + + /** + * Parses grant blocks from the given (already variable-substituted) policy text. Only the + * permission types used by the Solr agent are recognised: + * + * <ul> + * <li>{@code java.io.FilePermission} → {@link PermittedPath} + * <li>{@code java.net.SocketPermission} → {@link PermittedEndpoint} + * <li>{@code java.lang.RuntimePermission "exitVM"} → {@link ApprovedCallSite} EXIT + * <li>{@code java.lang.RuntimePermission "exec"} → {@link ApprovedCallSite} EXEC + * </ul> + * + * <p>Parsing uses {@link PolicyFileParser} (backed by {@link java.io.StreamTokenizer}) which + * natively handles {@code //} and {@code /* *\/} comments and quoted strings — no regex. + */ + static void parsePolicyBlocks(String text, PolicySource source, List<GrantBlock> out) { + List<PolicyFileParser.GrantEntry> grantEntries; + try { + grantEntries = PolicyFileParser.read(new StringReader(text)); + } catch (PolicyFileParser.ParsingException | IOException e) { + throw new IllegalStateException("Failed to parse security policy: " + e.getMessage(), e); + } + for (PolicyFileParser.GrantEntry ge : grantEntries) { + GrantBlock block = new GrantBlock(ge.codeBase(), source); + for (PolicyFileParser.PermEntry pe : ge.permissions()) { + addPermission(pe, block); + } + out.add(block); + } + } + + private static void addPermission(PolicyFileParser.PermEntry pe, GrantBlock block) { + String permClass = pe.permission(); + String target = pe.name(); + String actions = pe.action(); + switch (permClass) { + case "java.io.FilePermission": + if (target != null) { + block.filePaths.add( + new RawFilePermission(target, actions != null ? actions : "read", block.source)); + } + break; + case "java.net.SocketPermission": + if (target != null) { + block.socketPerms.add( + new RawSocketPermission( + target, + actions != null ? actions : "connect,resolve", + block.codeBase, + block.source)); + } + break; + case "java.lang.RuntimePermission": + if ("exitVM".equals(target) || (target != null && target.startsWith("exitVM."))) { + block.runtimePerms.add(new RawRuntimePermission("exitVM", block.codeBase, block.source)); + } else if ("exec".equals(target)) { + block.runtimePerms.add(new RawRuntimePermission("exec", block.codeBase, block.source)); + } + break; + default: + // Unrecognised permission types are ignored (e.g. PropertyPermission in legacy policy) + break; + } + } + + /** Converts raw parsed grant blocks into an immutable {@link AgentPolicy}. */ + private AgentPolicy buildPolicy(List<GrantBlock> grants) { + List<PermittedPath> paths = new ArrayList<>(); + List<PermittedEndpoint> endpoints = new ArrayList<>(); + List<ApprovedCallSite> exitCallers = new ArrayList<>(); + List<ApprovedCallSite> execCallers = new ArrayList<>(); + + for (GrantBlock block : grants) { + for (RawFilePermission fp : block.filePaths) { + boolean recursive = fp.target.endsWith("/-") || fp.target.endsWith("\\-"); + String basePath = recursive ? fp.target.substring(0, fp.target.length() - 2) : fp.target; + paths.add(new PermittedPath(basePath, fp.actions, recursive, fp.source)); + } + for (RawSocketPermission sp : block.socketPerms) { + endpoints.add(new PermittedEndpoint(sp.hostPort, sp.actions, sp.codeBase, sp.source)); + } + for (RawRuntimePermission rp : block.runtimePerms) { + if ("exitVM".equals(rp.type)) { + // codeBase-scoped exitVM grants map to approved exit callers + String pattern = rp.codeBase != null ? rp.codeBase : "*"; + exitCallers.add( + new ApprovedCallSite(pattern, ApprovedCallSite.Operation.EXIT, rp.source)); + } else if ("exec".equals(rp.type)) { + String pattern = rp.codeBase != null ? rp.codeBase : "*"; + execCallers.add( + new ApprovedCallSite(pattern, ApprovedCallSite.Operation.EXEC, rp.source)); + } Review Comment: codeBase-scoped exit and exec grants are effectively broken. When parsing `grant codeBase "file:/path/-" { permission java.lang.RuntimePermission "exitVM"; };`, this code stores `rp.codeBase` (e.g. `"file:/opt/solr/modules/foo/-"`) as the `ApprovedCallSite.classNamePattern`. `ApprovedCallSite.matches` then compares this URL-style string against fully-qualified class names — which will never match (it is neither `"*"`, nor ends in `".*"`, nor will it ever exact-equal a class name). The result is that any operator who writes a codeBase-scoped `exitVM`/`exec` grant will get a silently no-op rule. By contrast, codeBase handling for `SocketPermission` correctly resolves to code-source URLs via `SocketChannelInterceptor.isCallerFromCodeBase`. The exit/exec path needs analogous codeBase-vs-code-source matching, or the documentation must clarify that codeBase scoping is unsupported for exitVM/exec grants and this branch should warn loudly. ########## solr/agent-sm/src/java/org/apache/solr/security/agent/SocketChannelInterceptor.java: ########## @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF 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.apache.solr.security.agent; + +import java.lang.reflect.Method; +import java.net.InetSocketAddress; +import java.net.URL; +import java.net.UnixDomainSocketAddress; +import java.security.CodeSource; +import java.util.Collection; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.asm.Advice.Origin; + +/** + * ByteBuddy {@link Advice} interceptor for outbound socket connections. + * + * <p>This file was derived from the OpenSearch project and modified. See {@code NOTICE.txt} for + * attribution. + */ +public class SocketChannelInterceptor { + + /** SocketChannelInterceptor */ + public SocketChannelInterceptor() {} + + /** + * Interceptors + * + * @param args arguments + * @param method method + * @throws Exception exceptions + */ + @Advice.OnMethodEnter + public static void intercept(@Advice.AllArguments Object[] args, @Origin Method method) + throws Exception { + if (!AgentPolicy.isInitialized()) return; + final AgentPolicy policy = AgentPolicy.getInstance(); + + final StackWalker walker = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); + final String caller = walker.getCallerClass().getName(); + + if (args[0] instanceof InetSocketAddress address) { + if (!policy.trustedHosts().contains(address.getHostString())) { + enforceNetworkAccess(policy, address.getHostString(), address.getPort(), caller); + } + } else if (args[0] instanceof UnixDomainSocketAddress) { + // Unix domain socket — local IPC, always allow + } else if (args[0] != null) { + // Unknown SocketAddress subclass — fail closed (host/port unknown, cannot consult policy) + final String target = args[0].toString(); + ViolationMetricsReporter.incrementNetwork(); + SecurityViolationLogger.log( + SecurityViolationLogger.ViolationType.NETWORK_CONNECT, + target, + caller, + policy.enforcementMode()); + if (policy.enforcementMode() == AgentPolicy.EnforcementMode.ENFORCE) { + throw new SecurityException( + "Outbound network connection denied (unknown address type): " + target); + } + } + } Review Comment: The `@Origin Method method` parameter is bound by ByteBuddy advice but never used in this method body. Either drop the parameter or use `method.getName()` instead of relying on the wrapping advice site — currently the binding adds reflective lookup overhead on every intercepted `connect()` call with no benefit. ########## solr/server/etc/agent-security.policy: ########## @@ -0,0 +1,118 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF 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. + */ + +// Solr Security Agent — default production policy +// +// This file is the bundled default policy for the Solr security agent. It should not be edited +// by operators. To grant additional permissions for your deployment, use the operator extension +// file at: ${server.dir}/etc/agent-security-extra.policy (or the path set by +// SOLR_SECURITY_AGENT_EXTRA_POLICY / -Dsolr.security.agent.extra.policy). +// +// Syntax: JDK-style .policy files with Solr variable substitution. +// Supported variables: ${solr.solr.home}, ${solr.data.home}, ${solr.logs.dir}, ${solr.install.dir}, +// ${solr.install.symDir}, ${java.io.tmpdir}, ${java.home}, +// ${solr.port.listen}, ${solr.zk.port} +// +// For documentation see the Solr Reference Guide: Security Agent. + +// --------------------------------------------------------------------------- +// Global grant — applies to all code running in the Solr JVM +// --------------------------------------------------------------------------- +grant { + + // --- File system access --- + + // Solr installation directory: read-only (JARs, modules, Jetty config, etc.) + // Both the real path and the symlinked path are permitted so that installations where + // /opt/solr -> /opt/solr-X.Y.Z/ work correctly (FileInterceptor uses toAbsolutePath(), + // not toRealPath(), so accesses via the symlink path must be explicitly allowed). + permission java.io.FilePermission "${solr.install.dir}/-", "read"; + permission java.io.FilePermission "${solr.install.symDir}/-", "read"; + + // Solr home: read-only (config files, schema, solrconfig.xml, etc.) + permission java.io.FilePermission "${solr.solr.home}/-", "read"; + + // Solr data and index directories: full read/write/delete + permission java.io.FilePermission "${solr.data.home}/-", "read,write,delete"; + + // Log directory: write access for log files + permission java.io.FilePermission "${solr.logs.dir}/-", "read,write,delete"; + + // Temporary files: full access (sort buffers, merged segments, etc.) + permission java.io.FilePermission "${java.io.tmpdir}/-", "read,write,delete"; + + // JDK runtime libraries: read-only (class loading, native libs, etc.) + permission java.io.FilePermission "${java.home}/-", "read"; + + // Linux pseudo-filesystems: read-only access for JVM internals (e.g. memory-mapping checks, + // network configuration, kernel parameters). These paths do not exist on macOS/Windows and + // their presence in a cross-platform policy is harmless. + permission java.io.FilePermission "/proc/-", "read"; + permission java.io.FilePermission "/sys/-", "read"; + + // --- Outbound network access --- + + // Loopback addresses: unconditionally permitted (inter-thread, localhost HTTP, embedded ZK) + permission java.net.SocketPermission "localhost:1-65535", "connect,resolve"; + permission java.net.SocketPermission "127.0.0.1:1-65535", "connect,resolve"; + permission java.net.SocketPermission "[::1]:1-65535", "connect,resolve"; + + // Intra-cluster connectivity + // Any host on the Solr HTTP port is permitted, covering all current and future cluster nodes + // without requiring a static node list at startup. Similarly for the ZooKeeper port. + permission java.net.SocketPermission "*:${solr.port.listen}", "connect,resolve"; + permission java.net.SocketPermission "*:${solr.zk.port}", "connect,resolve"; + +}; Review Comment: The default policy contains no `java.lang.RuntimePermission "exitVM"` grants for Solr's own legitimate exit callers (`org.apache.solr.cli.SolrCLI`, `org.apache.solr.servlet.SolrDispatchFilter`, etc.). With default warn mode this only produces a `SECURITY VIOLATION [SYSTEM_EXIT]` log entry on every Solr shutdown — but once an operator flips `SOLR_SECURITY_AGENT_MODE=enforce`, Solr's normal shutdown paths will throw `SecurityException` instead of exiting. Per the dev guide and `ApprovedCallSite` Javadoc this is supposed to be pre-permitted; please add explicit `grant codeBase` blocks for the known legitimate exit callers (and verify them against the codeBase-matching bug noted on `PolicyLoader`). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
