Fix FileInterceptor to properly check read / write sides path separation (#17836)

Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Andrew Ross <[email protected]>
Co-authored-by: Andrew Ross <[email protected]>

Author: reta

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

@@ -61,18 +61,27 @@ public static void intercept(@Advice.AllArguments Object[] args, @Advice.Origin
         final Collection<ProtectionDomain> callers = walker.walk(StackCallerProtectionDomainChainExtractor.INSTANCE);
 
         final String name = method.getName();
-        boolean isMutating = name.equals("copy") || name.equals("move") || name.equals("write") || name.startsWith("create");
+        boolean isMutating = name.equals("move") || name.equals("write") || name.startsWith("create");
         final boolean isDelete = isMutating == false ? name.startsWith("delete") : false;
 
-        if (isMutating == false && isDelete == false && name.equals("newByteChannel") == true) {
-            if (args.length > 1 && args[1] instanceof OpenOption[] opts) {
-                for (final OpenOption opt : opts) {
-                    if (opt != StandardOpenOption.READ) {
-                        isMutating = true;
-                        break;
+        String targetFilePath = null;
+        if (isMutating == false && isDelete == false) {
+            if (name.equals("newByteChannel") == true) {
+                if (args.length > 1 && args[1] instanceof OpenOption[] opts) {
+                    for (final OpenOption opt : opts) {
+                        if (opt != StandardOpenOption.READ) {
+                            isMutating = true;
+                            break;
+                        }
                     }
-                }
 
+                }
+            } else if (name.equals("copy") == true) {
+                if (args.length > 1 && args[1] instanceof String pathStr) {
+                    targetFilePath = Paths.get(pathStr).toAbsolutePath().toString();
+                } else if (args.length > 1 && args[1] instanceof Path path) {
+                    targetFilePath = path.toAbsolutePath().toString();
+                }
             }
         }
 
@@ -85,6 +94,19 @@ public static void intercept(@Advice.AllArguments Object[] args, @Advice.Origin
                 }
             }
 
+            // Handle Files.copy() separately to check read/write permissions properly
+            if (method.getName().equals("copy")) {
+                if (!policy.implies(domain, new FilePermission(filePath, "read"))) {
+                    throw new SecurityException("Denied OPEN access to file: " + filePath + ", domain: " + domain);
+                }
+
+                if (targetFilePath != null) {
+                    if (!policy.implies(domain, new FilePermission(targetFilePath, "write"))) {
+                        throw new SecurityException("Denied OPEN access to file: " + targetFilePath + ", domain: " + domain);
+                    }
+                }
+            }
+
             // File mutating operations
             if (isMutating && !policy.implies(domain, new FilePermission(filePath, "write"))) {
                 throw new SecurityException("Denied WRITE access to file: " + filePath + ", domain: " + domain);

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

@@ -29,6 +29,7 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
 @SuppressWarnings("removal")
@@ -144,6 +145,10 @@ public void testCopy() throws Exception {
 
             // Test copy operation
             Files.copy(sourcePath, targetPath);
+            assertThrows(
+                SecurityException.class,
+                () -> Files.copy(sourcePath, tmpDir.getRoot().resolve("test-target-" + randomAlphaOfLength(8) + ".txt"))
+            );
 
             // Verify copy
             assertTrue("Target file should exist", Files.exists(targetPath));