Skip to content

Commit 70e890a

Browse files
pranu2502cwperks
pranu2502
and
cwperks
authored
Increased the scope of File Interceptor to intercept methods from FileSystemProvider.class (#17989)
* Increased the scope o File Interceptor ro intercept methods from FileSystemProvider.class Signed-off-by: Pranav Reddy <pranavrd@amazon.com> * Added negative Integ Tests to validate the correct Security Exception being thrown Signed-off-by: Pranav Reddy <pranavrd@amazon.com> * Fixed failing negative tests because of file exist assertions Signed-off-by: Pranav Reddy <pranavrd@amazon.com> * Added a deny default Security Exception for unknown arfumen types in File Interceptor Signed-off-by: Pranav Reddy <pranavrd@amazon.com> * Fixed File Permission substitution for Integ test Log files Signed-off-by: Pranav Reddy <pranavrd@amazon.com> * Fixes issue with system property substition in policy file parsing Signed-off-by: Pranav Reddy <pranavrd@amazon.com> * Added ChangeLog for newByteChannel Interception Signed-off-by: Pranav Reddy <pranavrd@amazon.com> --------- Signed-off-by: Pranav Reddy <pranavrd@amazon.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>
1 parent aed1264 commit 70e890a

File tree

10 files changed

+252
-13
lines changed

10 files changed

+252
-13
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
1212
- 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/))
1313
- Add composite directory factory ([#17988](https://github.com/opensearch-project/OpenSearch/pull/17988))
1414
- Add pull-based ingestion error metrics and make internal queue size configurable ([#18088](https://github.com/opensearch-project/OpenSearch/pull/18088))
15+
- [Security Manager Replacement] Enhance Java Agent to intercept newByteChannel ([#17989](https://github.com/opensearch-project/OpenSearch/pull/17989))
1516
- Enabled Async Shard Batch Fetch by default ([#18139](https://github.com/opensearch-project/OpenSearch/pull/18139))
1617
- Allow to get the search request from the QueryCoordinatorContext ([#17818](https://github.com/opensearch-project/OpenSearch/pull/17818))
1718

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,6 @@ public void execute(Task t) {
166166
"java.security.properties",
167167
project.getRootProject().getLayout().getProjectDirectory() + "/distribution/src/config/" + securityFile
168168
);
169-
170169
// don't track these as inputs since they contain absolute paths and break cache relocatability
171170
File gradleHome = project.getGradle().getGradleUserHomeDir();
172171
String gradleVersion = project.getGradle().getGradleVersion();

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,5 +11,5 @@
1111

1212
grant {
1313
// Needed to read the log file
14-
permission java.io.FilePermission "${tests.logfile}", "read";
14+
permission java.io.FilePermission "${{tests.logfile}}", "read";
1515
};

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,13 @@ private static PermissionEntry expandPermissionName(PermissionEntry pe) {
137137
while ((b = pe.name().indexOf("${{", startIndex)) != -1 && (e = pe.name().indexOf("}}", b)) != -1) {
138138
sb.append(pe.name(), startIndex, b);
139139
String value = pe.name().substring(b + 3, e);
140-
sb.append("${{").append(value).append("}}");
140+
String propertyValue = System.getProperty(value);
141+
if (propertyValue != null) {
142+
sb.append(propertyValue);
143+
} else {
144+
// replacement not found
145+
sb.append("${{").append(value).append("}}");
146+
}
141147
startIndex = e + 2;
142148
}
143149

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import java.nio.channels.FileChannel;
1616
import java.nio.channels.SocketChannel;
1717
import java.nio.file.Files;
18+
import java.nio.file.spi.FileSystemProvider;
1819
import java.util.Map;
1920

2021
import net.bytebuddy.ByteBuddy;
@@ -77,6 +78,7 @@ private static AgentBuilder createAgentBuilder() throws Exception {
7778
.or(ElementMatchers.isSubTypeOf(Socket.class));
7879
final Junction<TypeDescription> pathType = ElementMatchers.isSubTypeOf(Files.class);
7980
final Junction<TypeDescription> fileChannelType = ElementMatchers.isSubTypeOf(FileChannel.class);
81+
final Junction<TypeDescription> fileSystemProviderType = ElementMatchers.isSubTypeOf(FileSystemProvider.class);
8082

8183
final AgentBuilder.Transformer socketTransformer = (b, typeDescription, classLoader, module, pd) -> b.visit(
8284
Advice.to(SocketChannelInterceptor.class)
@@ -106,7 +108,7 @@ private static AgentBuilder createAgentBuilder() throws Exception {
106108
.ignore(ElementMatchers.nameContains("$MockitoMock$")) /* ingore all Mockito mocks */
107109
.type(socketType)
108110
.transform(socketTransformer)
109-
.type(pathType.or(fileChannelType))
111+
.type(pathType.or(fileChannelType).or(fileSystemProviderType))
110112
.transform(fileTransformer)
111113
.type(ElementMatchers.is(java.lang.System.class))
112114
.transform(

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

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.security.Policy;
2222
import java.security.ProtectionDomain;
2323
import java.util.Collection;
24+
import java.util.Set;
2425

2526
import net.bytebuddy.asm.Advice;
2627

@@ -85,14 +86,33 @@ public static void intercept(@Advice.AllArguments Object[] args, @Advice.Origin
8586
String targetFilePath = null;
8687
if (isMutating == false && isDelete == false) {
8788
if (name.equals("newByteChannel") == true || name.equals("open") == true) {
88-
if (args.length > 1 && args[1] instanceof OpenOption[] opts) {
89-
for (final OpenOption opt : opts) {
90-
if (opt != StandardOpenOption.READ) {
91-
isMutating = true;
92-
break;
89+
if (args.length > 1 && args instanceof Object) {
90+
if (args instanceof OpenOption[] opts) {
91+
for (final OpenOption opt : opts) {
92+
if (opt != StandardOpenOption.READ) {
93+
isMutating = true;
94+
break;
95+
}
9396
}
97+
} else if (args[1] instanceof Set<?> opts) {
98+
@SuppressWarnings("unchecked")
99+
final Set<OpenOption> options = (Set<OpenOption>) args[1];
100+
for (final OpenOption opt : options) {
101+
if (opt != StandardOpenOption.READ) {
102+
isMutating = true;
103+
break;
104+
}
105+
}
106+
} else if (args[1] instanceof Object[] opts) {
107+
for (final Object opt : opts) {
108+
if (opt != StandardOpenOption.READ) {
109+
isMutating = true;
110+
break;
111+
}
112+
}
113+
} else {
114+
throw new SecurityException("Unsupported argument type: " + args[1].getClass().getName());
94115
}
95-
96116
}
97117
} else if (name.equals("copy") == true) {
98118
if (args.length > 1 && args[1] instanceof String pathStr) {
@@ -106,7 +126,7 @@ public static void intercept(@Advice.AllArguments Object[] args, @Advice.Origin
106126
// Check each permission separately
107127
for (final ProtectionDomain domain : callers) {
108128
// Handle FileChannel.open() separately to check read/write permissions properly
109-
if (method.getName().equals("open")) {
129+
if (method.getName().equals("open") || method.getName().equals("newByteChannel")) {
110130
if (isMutating == true && !policy.implies(domain, new FilePermission(filePath, "read,write"))) {
111131
throw new SecurityException("Denied OPEN (read/write) access to file: " + filePath + ", domain: " + domain);
112132
} else if (!policy.implies(domain, new FilePermission(filePath, "read"))) {

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import java.io.File;
1616
import java.io.FileInputStream;
1717
import java.io.FilePermission;
18+
import java.io.OutputStream;
1819
import java.nio.ByteBuffer;
1920
import java.nio.channels.FileChannel;
2021
import java.nio.charset.StandardCharsets;
@@ -247,4 +248,57 @@ public void testDelete() throws Exception {
247248
Files.deleteIfExists(tempPath);
248249
}
249250
}
251+
252+
@Test
253+
public void testReadAllLines() throws Exception {
254+
Path tmpDir = getTestDir();
255+
Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt");
256+
257+
try {
258+
// Create a file with some content
259+
String content = "test content";
260+
Files.write(tempPath, content.getBytes(StandardCharsets.UTF_8));
261+
262+
// Verify file exists before deletion
263+
assertTrue("File should exist before deletion", Files.exists(tempPath));
264+
assertEquals("File should have correct content", content, Files.readString(tempPath, StandardCharsets.UTF_8));
265+
266+
// Test delete operation - FileInterceptor should intercept this
267+
String line = Files.readAllLines(tempPath).getFirst();
268+
269+
// Verify readAllLines
270+
assertEquals("File contents should be returned", "test content", line);
271+
272+
} finally {
273+
// Cleanup in case test fails
274+
Files.deleteIfExists(tempPath);
275+
}
276+
}
277+
278+
@Test
279+
public void testNewOutputStream() throws Exception {
280+
Path tmpDir = getTestDir();
281+
Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt");
282+
283+
try {
284+
// Create an empty file
285+
String content = "";
286+
Files.write(tempPath, content.getBytes(StandardCharsets.UTF_8));
287+
288+
// Verify file exists before deletion
289+
assertTrue("File should exist before deletion", Files.exists(tempPath));
290+
assertEquals("File should have correct content", content, Files.readString(tempPath, StandardCharsets.UTF_8));
291+
292+
// Test for new OutputStream
293+
try (OutputStream os = Files.newOutputStream(tempPath)) {
294+
os.write("test content".getBytes(StandardCharsets.UTF_8));
295+
}
296+
String line = Files.readAllLines(tempPath).getFirst();
297+
assertEquals("File contents should be returned", "test content", line);
298+
299+
} finally {
300+
// Cleanup in case test fails
301+
Files.deleteIfExists(tempPath);
302+
}
303+
}
250304
}

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

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
9+
package org.opensearch.javaagent;
10+
11+
import org.opensearch.javaagent.bootstrap.AgentPolicy;
12+
import org.junit.BeforeClass;
13+
import org.junit.Test;
14+
15+
import java.nio.channels.FileChannel;
16+
import java.nio.charset.StandardCharsets;
17+
import java.nio.file.Files;
18+
import java.nio.file.Path;
19+
import java.nio.file.StandardOpenOption;
20+
import java.security.PermissionCollection;
21+
import java.security.Permissions;
22+
import java.security.Policy;
23+
import java.security.ProtectionDomain;
24+
import java.util.UUID;
25+
26+
import static org.junit.Assert.assertThrows;
27+
import static org.junit.Assert.assertTrue;
28+
29+
@SuppressWarnings("removal")
30+
public class FileInterceptorNegativeIntegTests {
31+
private static Path getTestDir() {
32+
Path baseDir = Path.of(System.getProperty("user.dir"));
33+
Path integTestFiles = baseDir.resolve("integ-test-files").normalize();
34+
return integTestFiles;
35+
}
36+
37+
private String randomAlphaOfLength(int length) {
38+
// Using UUID to generate random string and taking first 'length' characters
39+
return UUID.randomUUID().toString().replaceAll("-", "").substring(0, length);
40+
}
41+
42+
@BeforeClass
43+
public static void setUp() throws Exception {
44+
Policy policy = new Policy() {
45+
@Override
46+
public PermissionCollection getPermissions(ProtectionDomain domain) {
47+
Permissions permissions = new Permissions();
48+
return permissions;
49+
}
50+
};
51+
AgentPolicy.setPolicy(policy);
52+
}
53+
54+
@Test
55+
public void testFileInputStream() {
56+
Path tmpDir = getTestDir();
57+
58+
// Create a unique file name
59+
String fileName = "test-" + randomAlphaOfLength(8) + ".txt";
60+
Path tempPath = tmpDir.resolve(fileName);
61+
62+
// Verify error when writing Content
63+
String content = "test content";
64+
SecurityException exception = assertThrows("", SecurityException.class, () -> {
65+
Files.write(tempPath, content.getBytes(StandardCharsets.UTF_8));
66+
});
67+
assertTrue(exception.getMessage().contains("Denied WRITE access to file"));
68+
}
69+
70+
@Test
71+
public void testOpenForReadAndWrite() {
72+
Path tmpDir = getTestDir();
73+
74+
// Create a unique file name
75+
String fileName = "test-" + randomAlphaOfLength(8) + ".txt";
76+
Path tempPath = tmpDir.resolve(fileName);
77+
78+
// Verify error when opening file
79+
SecurityException exception = assertThrows("", SecurityException.class, () -> {
80+
FileChannel.open(tempPath, StandardOpenOption.CREATE, StandardOpenOption.READ, StandardOpenOption.WRITE);
81+
});
82+
assertTrue(exception.getMessage().contains("Denied OPEN (read/write) access to file"));
83+
}
84+
85+
@Test
86+
public void testCopy() {
87+
Path tmpDir = getTestDir();
88+
Path sourcePath = tmpDir.resolve("test-source-" + randomAlphaOfLength(8) + ".txt");
89+
Path targetPath = tmpDir.resolve("test-target-" + randomAlphaOfLength(8) + ".txt");
90+
91+
// Verify Error when copying file
92+
SecurityException exception = assertThrows(SecurityException.class, () -> Files.copy(sourcePath, targetPath));
93+
assertTrue(exception.getMessage().contains("Denied COPY (read) access to file"));
94+
}
95+
96+
@Test
97+
public void testCreateFile() {
98+
Path tmpDir = getTestDir();
99+
Path tempPath = tmpDir.resolve("test-create-" + randomAlphaOfLength(8) + ".txt");
100+
101+
// Verify Error when creating file
102+
SecurityException exception = assertThrows(SecurityException.class, () -> Files.createFile(tempPath));
103+
assertTrue(exception.getMessage().contains("Denied WRITE access to file"));
104+
}
105+
106+
@Test
107+
public void testMove() {
108+
Path tmpDir = getTestDir();
109+
Path sourcePath = tmpDir.resolve("test-source-" + randomAlphaOfLength(8) + ".txt");
110+
Path targetPath = tmpDir.resolve("test-target-" + randomAlphaOfLength(8) + ".txt");
111+
112+
// Verify Error when moving file
113+
SecurityException exception = assertThrows(SecurityException.class, () -> Files.move(sourcePath, targetPath));
114+
assertTrue(exception.getMessage().contains("Denied WRITE access to file"));
115+
}
116+
117+
@Test
118+
public void testCreateLink() throws Exception {
119+
Path tmpDir = getTestDir();
120+
Path originalPath = tmpDir.resolve("test-original-" + randomAlphaOfLength(8) + ".txt");
121+
Path linkPath = tmpDir.resolve("test-link-" + randomAlphaOfLength(8) + ".txt");
122+
123+
// Verify Error when creating link
124+
SecurityException exception = assertThrows(SecurityException.class, () -> Files.createLink(linkPath, originalPath));
125+
assertTrue(exception.getMessage().contains("Denied WRITE access to file"));
126+
}
127+
128+
@Test
129+
public void testDelete() throws Exception {
130+
Path tmpDir = getTestDir();
131+
Path tempPath = tmpDir.resolve("test-delete-" + randomAlphaOfLength(8) + ".txt");
132+
133+
// Verify Error when deleting file
134+
SecurityException exception = assertThrows(SecurityException.class, () -> Files.delete(tempPath));
135+
assertTrue(exception.getMessage().contains("Denied DELETE access to file"));
136+
}
137+
138+
@Test
139+
public void testReadAllLines() throws Exception {
140+
Path tmpDir = getTestDir();
141+
Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt");
142+
143+
// Verify Error when reading file
144+
SecurityException exception = assertThrows(SecurityException.class, () -> Files.readAllLines(tempPath));
145+
assertTrue(exception.getMessage().contains("Denied OPEN (read) access to file"));
146+
}
147+
148+
@Test
149+
public void testNewOutputStream() throws Exception {
150+
Path tmpDir = getTestDir();
151+
Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt");
152+
153+
// Verify error when calling newOutputStream
154+
SecurityException exception = assertThrows(SecurityException.class, () -> Files.newOutputStream(tempPath));
155+
assertTrue(exception.getMessage().contains("Denied OPEN (read/write) access to file"));
156+
}
157+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,5 +11,5 @@
1111

1212
grant {
1313
// Needed to read the log file
14-
permission java.io.FilePermission "${tests.logfile}", "read";
14+
permission java.io.FilePermission "${{tests.logfile}}", "read";
1515
};

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,5 +11,5 @@
1111

1212
grant {
1313
// Needed to read the log file
14-
permission java.io.FilePermission "${tests.logfile}", "read";
14+
permission java.io.FilePermission "${{tests.logfile}}", "read";
1515
};

0 commit comments

Comments
 (0)