Increased the scope of File Interceptor to intercept methods from FileSystemProvider.class

CHANGELOG.md

@@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Update API of Message in index to add the timestamp for lag calculation in ingestion polling ([#17977](https://github.com/opensearch-project/OpenSearch/pull/17977/))
 - Add composite directory factory ([#17988](https://github.com/opensearch-project/OpenSearch/pull/17988))
 - Add pull-based ingestion error metrics and make internal queue size configurable ([#18088](https://github.com/opensearch-project/OpenSearch/pull/18088))
+- [Security Manager Replacement] Enhance Java Agent to intercept newByteChannel ([#17989](https://github.com/opensearch-project/OpenSearch/pull/17989))
 - Enabled Async Shard Batch Fetch by default ([#18139](https://github.com/opensearch-project/OpenSearch/pull/18139))
 - Allow to get the search request from the QueryCoordinatorContext ([#17818](https://github.com/opensearch-project/OpenSearch/pull/17818))
 

buildSrc/src/main/java/org/opensearch/gradle/OpenSearchTestBasePlugin.java

@@ -166,7 +166,6 @@ public void execute(Task t) {
                 "java.security.properties",
                 project.getRootProject().getLayout().getProjectDirectory() + "/distribution/src/config/" + securityFile
             );
-
             // don't track these as inputs since they contain absolute paths and break cache relocatability
             File gradleHome = project.getGradle().getGradleUserHomeDir();
             String gradleVersion = project.getGradle().getGradleVersion();

distribution/archives/integ-test-zip/src/test/resources/plugin-security.policy

@@ -11,5 +11,5 @@
 
 grant {
   // Needed to read the log file
-  permission java.io.FilePermission "${tests.logfile}", "read";
+  permission java.io.FilePermission "${{tests.logfile}}", "read";
 };

libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/policy/PolicyFile.java

@@ -137,7 +137,13 @@
         while ((b = pe.name().indexOf("${{", startIndex)) != -1 && (e = pe.name().indexOf("}}", b)) != -1) {
             sb.append(pe.name(), startIndex, b);
             String value = pe.name().substring(b + 3, e);
-            sb.append("${{").append(value).append("}}");
+            String propertyValue = System.getProperty(value);
+            if (propertyValue != null) {
+                sb.append(propertyValue);
+            } else {
+                // replacement not found
+                sb.append("${{").append(value).append("}}");
+            }
             startIndex = e + 2;
         }
 

libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/Agent.java

@@ -15,6 +15,7 @@
 import java.nio.channels.FileChannel;
 import java.nio.channels.SocketChannel;
 import java.nio.file.Files;
+import java.nio.file.spi.FileSystemProvider;
 import java.util.Map;
 
 import net.bytebuddy.ByteBuddy;
@@ -77,6 +78,7 @@
             .or(ElementMatchers.isSubTypeOf(Socket.class));
         final Junction<TypeDescription> pathType = ElementMatchers.isSubTypeOf(Files.class);
         final Junction<TypeDescription> fileChannelType = ElementMatchers.isSubTypeOf(FileChannel.class);
+        final Junction<TypeDescription> fileSystemProviderType = ElementMatchers.isSubTypeOf(FileSystemProvider.class);
 
         final AgentBuilder.Transformer socketTransformer = (b, typeDescription, classLoader, module, pd) -> b.visit(
             Advice.to(SocketChannelInterceptor.class)
@@ -106,7 +108,7 @@
             .ignore(ElementMatchers.nameContains("$MockitoMock$")) /* ingore all Mockito mocks */
             .type(socketType)
             .transform(socketTransformer)
-            .type(pathType.or(fileChannelType))
+            .type(pathType.or(fileChannelType).or(fileSystemProviderType))
             .transform(fileTransformer)
             .type(ElementMatchers.is(java.lang.System.class))
             .transform(

libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/FileInterceptor.java

@@ -21,6 +21,7 @@
 import java.security.Policy;
 import java.security.ProtectionDomain;
 import java.util.Collection;
+import java.util.Set;
 
 import net.bytebuddy.asm.Advice;
 
@@ -85,14 +86,33 @@
             String targetFilePath = null;
             if (isMutating == false && isDelete == false) {
                 if (name.equals("newByteChannel") == true || name.equals("open") == true) {
-                    if (args.length > 1 && args[1] instanceof OpenOption[] opts) {
-                        for (final OpenOption opt : opts) {
-                            if (opt != StandardOpenOption.READ) {
-                                isMutating = true;
-                                break;
+                    if (args.length > 1 && args instanceof Object) {
+                        if (args instanceof OpenOption[] opts) {
+                            for (final OpenOption opt : opts) {
+                                if (opt != StandardOpenOption.READ) {
+                                    isMutating = true;
+                                    break;
+                                }
                             }
+                        } else if (args[1] instanceof Set<?> opts) {
+                            @SuppressWarnings("unchecked")
+                            final Set<OpenOption> options = (Set<OpenOption>) args[1];
+                            for (final OpenOption opt : options) {
+                                if (opt != StandardOpenOption.READ) {
+                                    isMutating = true;
+                                    break;
+                                }
+                            }
+                        } else if (args[1] instanceof Object[] opts) {
+                            for (final Object opt : opts) {
+                                if (opt != StandardOpenOption.READ) {
+                                    isMutating = true;
+                                    break;
+                                }
+                            }
+                        } else {
+                            throw new SecurityException("Unsupported argument type: " + args[1].getClass().getName());
                         }
-
                     }
                 } else if (name.equals("copy") == true) {
                     if (args.length > 1 && args[1] instanceof String pathStr) {
@@ -106,7 +126,7 @@
             // Check each permission separately
             for (final ProtectionDomain domain : callers) {
                 // Handle FileChannel.open() separately to check read/write permissions properly
-                if (method.getName().equals("open")) {
+                if (method.getName().equals("open") || method.getName().equals("newByteChannel")) {
                     if (isMutating == true && !policy.implies(domain, new FilePermission(filePath, "read,write"))) {
                         throw new SecurityException("Denied OPEN (read/write) access to file: " + filePath + ", domain: " + domain);
                     } else if (!policy.implies(domain, new FilePermission(filePath, "read"))) {

libs/agent-sm/agent/src/test/java/org/opensearch/javaagent/FileInterceptorIntegTests.java

@@ -15,6 +15,7 @@
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FilePermission;
+import java.io.OutputStream;
 import java.nio.ByteBuffer;
 import java.nio.channels.FileChannel;
 import java.nio.charset.StandardCharsets;
@@ -247,4 +248,57 @@ public void testDelete() throws Exception {
             Files.deleteIfExists(tempPath);
         }
     }
+
+    @Test
+    public void testReadAllLines() throws Exception {
+        Path tmpDir = getTestDir();
+        Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt");
+
+        try {
+            // Create a file with some content
+            String content = "test content";
+            Files.write(tempPath, content.getBytes(StandardCharsets.UTF_8));
+
+            // Verify file exists before deletion
+            assertTrue("File should exist before deletion", Files.exists(tempPath));
+            assertEquals("File should have correct content", content, Files.readString(tempPath, StandardCharsets.UTF_8));
+
+            // Test delete operation - FileInterceptor should intercept this
+            String line = Files.readAllLines(tempPath).getFirst();
+
+            // Verify readAllLines
+            assertEquals("File contents should be returned", "test content", line);
+
+        } finally {
+            // Cleanup in case test fails
+            Files.deleteIfExists(tempPath);
+        }
+    }
+
+    @Test
+    public void testNewOutputStream() throws Exception {
+        Path tmpDir = getTestDir();
+        Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt");
+
+        try {
+            // Create an empty file
+            String content = "";
+            Files.write(tempPath, content.getBytes(StandardCharsets.UTF_8));
+
+            // Verify file exists before deletion
+            assertTrue("File should exist before deletion", Files.exists(tempPath));
+            assertEquals("File should have correct content", content, Files.readString(tempPath, StandardCharsets.UTF_8));
+
+            // Test for new OutputStream
+            try (OutputStream os = Files.newOutputStream(tempPath)) {
+                os.write("test content".getBytes(StandardCharsets.UTF_8));
+            }
+            String line = Files.readAllLines(tempPath).getFirst();
+            assertEquals("File contents should be returned", "test content", line);
+
+        } finally {
+            // Cleanup in case test fails
+            Files.deleteIfExists(tempPath);
+        }
+    }
 }

libs/agent-sm/agent/src/test/java/org/opensearch/javaagent/FileInterceptorNegativeIntegTests.java

@@ -0,0 +1,157 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.javaagent;
+
+import org.opensearch.javaagent.bootstrap.AgentPolicy;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.nio.channels.FileChannel;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+import java.security.PermissionCollection;
+import java.security.Permissions;
+import java.security.Policy;
+import java.security.ProtectionDomain;
+import java.util.UUID;
+
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+@SuppressWarnings("removal")
+public class FileInterceptorNegativeIntegTests {
+    private static Path getTestDir() {
+        Path baseDir = Path.of(System.getProperty("user.dir"));
+        Path integTestFiles = baseDir.resolve("integ-test-files").normalize();
+        return integTestFiles;
+    }
+
+    private String randomAlphaOfLength(int length) {
+        // Using UUID to generate random string and taking first 'length' characters
+        return UUID.randomUUID().toString().replaceAll("-", "").substring(0, length);
+    }
+
+    @BeforeClass
+    public static void setUp() throws Exception {
+        Policy policy = new Policy() {
+            @Override
+            public PermissionCollection getPermissions(ProtectionDomain domain) {
+                Permissions permissions = new Permissions();
+                return permissions;
+            }
+        };
+        AgentPolicy.setPolicy(policy);
+    }
+
+    @Test
+    public void testFileInputStream() {
+        Path tmpDir = getTestDir();
+
+        // Create a unique file name
+        String fileName = "test-" + randomAlphaOfLength(8) + ".txt";
+        Path tempPath = tmpDir.resolve(fileName);
+
+        // Verify error when writing Content
+        String content = "test content";
+        SecurityException exception = assertThrows("", SecurityException.class, () -> {
+            Files.write(tempPath, content.getBytes(StandardCharsets.UTF_8));
+        });
+        assertTrue(exception.getMessage().contains("Denied WRITE access to file"));
+    }
+
+    @Test
+    public void testOpenForReadAndWrite() {
+        Path tmpDir = getTestDir();
+
+        // Create a unique file name
+        String fileName = "test-" + randomAlphaOfLength(8) + ".txt";
+        Path tempPath = tmpDir.resolve(fileName);
+
+        // Verify error when opening file
+        SecurityException exception = assertThrows("", SecurityException.class, () -> {
+            FileChannel.open(tempPath, StandardOpenOption.CREATE, StandardOpenOption.READ, StandardOpenOption.WRITE);
+        });
+        assertTrue(exception.getMessage().contains("Denied OPEN (read/write) access to file"));
+    }
+
+    @Test
+    public void testCopy() {
+        Path tmpDir = getTestDir();
+        Path sourcePath = tmpDir.resolve("test-source-" + randomAlphaOfLength(8) + ".txt");
+        Path targetPath = tmpDir.resolve("test-target-" + randomAlphaOfLength(8) + ".txt");
+
+        // Verify Error when copying file
+        SecurityException exception = assertThrows(SecurityException.class, () -> Files.copy(sourcePath, targetPath));
+        assertTrue(exception.getMessage().contains("Denied COPY (read) access to file"));
+    }
+
+    @Test
+    public void testCreateFile() {
+        Path tmpDir = getTestDir();
+        Path tempPath = tmpDir.resolve("test-create-" + randomAlphaOfLength(8) + ".txt");
+
+        // Verify Error when creating file
+        SecurityException exception = assertThrows(SecurityException.class, () -> Files.createFile(tempPath));
+        assertTrue(exception.getMessage().contains("Denied WRITE access to file"));
+    }
+
+    @Test
+    public void testMove() {
+        Path tmpDir = getTestDir();
+        Path sourcePath = tmpDir.resolve("test-source-" + randomAlphaOfLength(8) + ".txt");
+        Path targetPath = tmpDir.resolve("test-target-" + randomAlphaOfLength(8) + ".txt");
+
+        // Verify Error when moving file
+        SecurityException exception = assertThrows(SecurityException.class, () -> Files.move(sourcePath, targetPath));
+        assertTrue(exception.getMessage().contains("Denied WRITE access to file"));
+    }
+
+    @Test
+    public void testCreateLink() throws Exception {
+        Path tmpDir = getTestDir();
+        Path originalPath = tmpDir.resolve("test-original-" + randomAlphaOfLength(8) + ".txt");
+        Path linkPath = tmpDir.resolve("test-link-" + randomAlphaOfLength(8) + ".txt");
+
+        // Verify Error when creating link
+        SecurityException exception = assertThrows(SecurityException.class, () -> Files.createLink(linkPath, originalPath));
+        assertTrue(exception.getMessage().contains("Denied WRITE access to file"));
+    }
+
+    @Test
+    public void testDelete() throws Exception {
+        Path tmpDir = getTestDir();
+        Path tempPath = tmpDir.resolve("test-delete-" + randomAlphaOfLength(8) + ".txt");
+
+        // Verify Error when deleting file
+        SecurityException exception = assertThrows(SecurityException.class, () -> Files.delete(tempPath));
+        assertTrue(exception.getMessage().contains("Denied DELETE access to file"));
+    }
+
+    @Test
+    public void testReadAllLines() throws Exception {
+        Path tmpDir = getTestDir();
+        Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt");
+
+        // Verify Error when reading file
+        SecurityException exception = assertThrows(SecurityException.class, () -> Files.readAllLines(tempPath));
+        assertTrue(exception.getMessage().contains("Denied OPEN (read) access to file"));
+    }
+
+    @Test
+    public void testNewOutputStream() throws Exception {
+        Path tmpDir = getTestDir();
+        Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt");
+
+        // Verify error when calling newOutputStream
+        SecurityException exception = assertThrows(SecurityException.class, () -> Files.newOutputStream(tempPath));
+        assertTrue(exception.getMessage().contains("Denied OPEN (read/write) access to file"));
+    }
+}

qa/logging-config/src/test/resources/plugin-security.policy

@@ -11,5 +11,5 @@
 
 grant {
   // Needed to read the log file
-  permission java.io.FilePermission "${tests.logfile}", "read";
+  permission java.io.FilePermission "${{tests.logfile}}", "read";
 };

qa/unconfigured-node-name/src/test/resources/plugin-security.policy

@@ -11,5 +11,5 @@
 
 grant {
   // Needed to read the log file
-  permission java.io.FilePermission "${tests.logfile}", "read";
+  permission java.io.FilePermission "${{tests.logfile}}", "read";
 };