This is an automated email from the ASF dual-hosted git repository.
jongyoul pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new f2f34cd548 [HOTFIX] Avoid shell evaluation of interpreter launch
arguments
f2f34cd548 is described below
commit f2f34cd548ef03ca7b0c438e63709d0cc12a9de4
Author: Jongyoul Lee <[email protected]>
AuthorDate: Thu Aug 20 16:58:46 2026 +0900
[HOTFIX] Avoid shell evaluation of interpreter launch arguments
### What is this PR for?
This PR makes interpreter launch argument handling predictable across
configuration and impersonation modes.
The dependency downloader command is now executed directly as an argument
array instead of being evaluated as a shell command. Classpath wildcards,
spaces, JVM options, and other configured values therefore remain literal
process arguments.
When user impersonation is enabled, `%conf` and session-scoped
configuration reject new environment-variable-style overrides. Existing
operator-provided interpreter settings remain unchanged, and rejected updates
are not partially applied.
### What type of PR is it?
Hot Fix
### Todos
* [x] Execute downloader arguments without shell evaluation
* [x] Preserve classpath and JVM argument behavior
* [x] Validate user-provided environment overrides in impersonation mode
* [x] Add unit and shell-level regression tests
### What is the Jira issue?
N/A
### How should this be tested?
```bash
ZEPPELIN_LOCAL_IP=127.0.0.1 ./mvnw -pl zeppelin-server \
-Dtest=ConfInterpreterTest,SessionConfInterpreterTest,InterpreterShellScriptTest,StandardInterpreterLauncherTest
test
./mvnw -pl zeppelin-server -DskipTests -Prat apache-rat:check
```
The tests cover literal argument handling for classpaths, wildcards, JVM
options, rejected environment overrides with impersonation enabled, allowed
updates without impersonation, and preservation of existing operator
configuration.
### Screenshots (if appropriate)
N/A
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Closes #5429 from jongyoul/codex/security-interpreter-eval.
Signed-off-by: Jongyoul Lee <[email protected]>
---
bin/interpreter.sh | 8 +-
.../zeppelin/interpreter/ConfInterpreter.java | 21 ++++
.../interpreter/SessionConfInterpreter.java | 6 ++
.../zeppelin/interpreter/ConfInterpreterTest.java | 39 +++++++
.../interpreter/SessionConfInterpreterTest.java | 51 ++++++++-
.../launcher/InterpreterShellScriptTest.java | 119 +++++++++++++++++++++
6 files changed, 238 insertions(+), 6 deletions(-)
diff --git a/bin/interpreter.sh b/bin/interpreter.sh
index 00ff030731..857f7637a0 100755
--- a/bin/interpreter.sh
+++ b/bin/interpreter.sh
@@ -25,12 +25,14 @@ function usage() {
}
function downloadInterpreterLibraries() {
- mkdir -p ${LOCAL_INTERPRETER_REPO}
+ mkdir -p "${LOCAL_INTERPRETER_REPO}"
+ local -a JAVA_INTP_OPTS_ARRAY=()
+ local -a INTERPRETER_DOWNLOAD_COMMAND=()
+ local
ZEPPELIN_DOWNLOADER="org.apache.zeppelin.interpreter.remote.RemoteInterpreterDownloader"
IFS=' ' read -r -a JAVA_INTP_OPTS_ARRAY <<< "${JAVA_INTP_OPTS}"
-
ZEPPELIN_DOWNLOADER="org.apache.zeppelin.interpreter.remote.RemoteInterpreterDownloader"
INTERPRETER_DOWNLOAD_COMMAND+=("${ZEPPELIN_RUNNER}"
"${JAVA_INTP_OPTS_ARRAY[@]}" "-cp"
"${ZEPPELIN_INTP_CLASSPATH_OVERRIDES}:${ZEPPELIN_INTP_CLASSPATH}"
"${ZEPPELIN_DOWNLOADER}" "${CALLBACK_HOST}" "${PORT}"
"${INTERPRETER_SETTING_NAME}" "${LOCAL_INTERPRETER_REPO}")
echo "Interpreter download command: ${INTERPRETER_DOWNLOAD_COMMAND[@]}"
- eval "${INTERPRETER_DOWNLOAD_COMMAND[@]}"
+ "${INTERPRETER_DOWNLOAD_COMMAND[@]}"
}
# pre-requisites for checking that we're running in container
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/ConfInterpreter.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/ConfInterpreter.java
index 9c5d02debb..ba3d85bfd0 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/ConfInterpreter.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/ConfInterpreter.java
@@ -18,11 +18,13 @@
package org.apache.zeppelin.interpreter;
import org.apache.commons.lang3.exception.ExceptionUtils;
+import org.apache.zeppelin.interpreter.remote.RemoteInterpreterUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.io.StringReader;
+import java.util.Optional;
import java.util.Properties;
/**
@@ -67,6 +69,10 @@ public class ConfInterpreter extends Interpreter {
finalProperties.putAll(getProperties());
Properties newProperties = new Properties();
newProperties.load(new StringReader(st));
+ Optional<InterpreterResult> validationError =
validateUpdatedProperties(newProperties);
+ if (validationError.isPresent()) {
+ return validationError.get();
+ }
for (String key : newProperties.stringPropertyNames()) {
finalProperties.put(key.trim(), newProperties.getProperty(key).trim());
}
@@ -79,6 +85,21 @@ public class ConfInterpreter extends Interpreter {
}
}
+ protected Optional<InterpreterResult> validateUpdatedProperties(Properties
updatedProperties) {
+ if (!interpreterSetting.getOption().isUserImpersonate()) {
+ return Optional.empty();
+ }
+ return updatedProperties.stringPropertyNames().stream()
+ .map(String::trim)
+ .filter(RemoteInterpreterUtils::isEnvString)
+ .sorted()
+ .findFirst()
+ .map(environmentVariable -> new
InterpreterResult(InterpreterResult.Code.ERROR,
+ "Environment variable '" + environmentVariable
+ + "' cannot be overridden with %conf when user impersonation
is enabled. "
+ + "Configure it in the interpreter setting instead."));
+ }
+
@Override
public void cancel(InterpreterContext context) throws InterpreterException {
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/SessionConfInterpreter.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/SessionConfInterpreter.java
index 0718b07384..2b1b733338 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/SessionConfInterpreter.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/SessionConfInterpreter.java
@@ -25,6 +25,7 @@ import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.io.StringReader;
import java.util.List;
+import java.util.Optional;
import java.util.Properties;
public class SessionConfInterpreter extends ConfInterpreter {
@@ -46,6 +47,11 @@ public class SessionConfInterpreter extends ConfInterpreter {
finalProperties.putAll(this.properties);
Properties updatedProperties = new Properties();
updatedProperties.load(new StringReader(st));
+ Optional<InterpreterResult> validationError =
+ validateUpdatedProperties(updatedProperties);
+ if (validationError.isPresent()) {
+ return validationError.get();
+ }
finalProperties.putAll(updatedProperties);
LOGGER.debug("Properties for Session: {}:{}", sessionId,
finalProperties);
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/ConfInterpreterTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/ConfInterpreterTest.java
index 785c30122f..8bf258f1ce 100644
---
a/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/ConfInterpreterTest.java
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/ConfInterpreterTest.java
@@ -148,4 +148,43 @@ class ConfInterpreterTest extends AbstractInterpreterTest {
assertEquals(InterpreterResult.Code.ERROR, result.code);
}
+ @Test
+ void testRejectEnvironmentVariableOverrideWithImpersonation() throws
InterpreterException {
+ InterpreterSetting interpreterSetting =
+ interpreterSettingManager.getInterpreterSettingByName("test");
+ interpreterSetting.getOption().setUserImpersonate(true);
+ ConfInterpreter confInterpreter =
+ (ConfInterpreter) interpreterFactory.getInterpreter("test.conf",
executionContext);
+ RemoteInterpreter remoteInterpreter =
+ (RemoteInterpreter) interpreterFactory.getInterpreter("test",
executionContext);
+
+ InterpreterResult result = confInterpreter.interpret(
+ "property_1\tnew_value\n BASH_ENV \t/tmp/user-controlled.sh",
+ createDummyInterpreterContext());
+
+ assertEquals(InterpreterResult.Code.ERROR, result.code);
+ assertTrue(result.toString().contains("BASH_ENV"), result.toString());
+ assertEquals("value_1", remoteInterpreter.getProperty("property_1"));
+ assertNull(remoteInterpreter.getProperty("BASH_ENV"));
+
+ result = confInterpreter.interpret(
+ "property_1\tnew_value", createDummyInterpreterContext());
+ assertEquals(InterpreterResult.Code.SUCCESS, result.code);
+ assertEquals("new_value", remoteInterpreter.getProperty("property_1"));
+ }
+
+ @Test
+ void testAllowEnvironmentVariableOverrideWithoutImpersonation() throws
InterpreterException {
+ ConfInterpreter confInterpreter =
+ (ConfInterpreter) interpreterFactory.getInterpreter("test.conf",
executionContext);
+ RemoteInterpreter remoteInterpreter =
+ (RemoteInterpreter) interpreterFactory.getInterpreter("test",
executionContext);
+
+ InterpreterResult result = confInterpreter.interpret(
+ "BASH_ENV\t/tmp/user-controlled.sh", createDummyInterpreterContext());
+
+ assertEquals(InterpreterResult.Code.SUCCESS, result.code);
+ assertEquals("/tmp/user-controlled.sh",
remoteInterpreter.getProperty("BASH_ENV"));
+ }
+
}
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/SessionConfInterpreterTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/SessionConfInterpreterTest.java
index 44701cf549..05eba5fc6e 100644
---
a/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/SessionConfInterpreterTest.java
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/SessionConfInterpreterTest.java
@@ -27,6 +27,8 @@ import java.util.List;
import java.util.Properties;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -38,6 +40,7 @@ class SessionConfInterpreterTest {
InterpreterSetting mockInterpreterSetting = mock(InterpreterSetting.class);
ManagedInterpreterGroup mockInterpreterGroup =
mock(ManagedInterpreterGroup.class);
when(mockInterpreterSetting.getInterpreterGroup("group_1")).thenReturn(mockInterpreterGroup);
+ when(mockInterpreterSetting.getOption()).thenReturn(new
InterpreterOption());
Properties properties = new Properties();
properties.setProperty("property_1", "value_1");
@@ -54,18 +57,60 @@ class SessionConfInterpreterTest {
when(mockInterpreterGroup.get("session_1")).thenReturn(interpreters);
InterpreterResult result =
-
confInterpreter.interpret("property_1\tupdated_value_1\nproperty_3\tvalue_3",
+ confInterpreter.interpret(
+ "property_1\tupdated_value_1\nproperty_3\tvalue_3\nENV_1\tVALUE_1",
mock(InterpreterContext.class));
assertEquals(InterpreterResult.Code.SUCCESS, result.code);
- assertEquals(3, remoteInterpreter.getProperties().size());
+ assertEquals(4, remoteInterpreter.getProperties().size());
assertEquals("updated_value_1",
remoteInterpreter.getProperty("property_1"));
assertEquals("value_2", remoteInterpreter.getProperty("property_2"));
assertEquals("value_3", remoteInterpreter.getProperty("property_3"));
+ assertEquals("VALUE_1", remoteInterpreter.getProperty("ENV_1"));
remoteInterpreter.setOpened(true);
result =
-
confInterpreter.interpret("property_1\tupdated_value_1\nproperty_3\tvalue_3",
+ confInterpreter.interpret(
+ "property_1\tupdated_value_1\nproperty_3\tvalue_3\nENV_1\tVALUE_1",
mock(InterpreterContext.class));
assertEquals(InterpreterResult.Code.ERROR, result.code);
}
+
+ @Test
+ void testRejectEnvironmentVariableOverrideWithImpersonation() throws
InterpreterException {
+ InterpreterSetting mockInterpreterSetting = mock(InterpreterSetting.class);
+ ManagedInterpreterGroup mockInterpreterGroup =
mock(ManagedInterpreterGroup.class);
+ InterpreterOption option = new InterpreterOption();
+ option.setUserImpersonate(true);
+ when(mockInterpreterSetting.getOption()).thenReturn(option);
+
when(mockInterpreterSetting.getInterpreterGroup("group_1")).thenReturn(mockInterpreterGroup);
+
+ Properties properties = new Properties();
+ properties.setProperty("property_1", "value_1");
+ properties.setProperty("JAVA_HOME", "/operator/java");
+ SessionConfInterpreter confInterpreter = new SessionConfInterpreter(
+ properties, "session_1", "group_1", mockInterpreterSetting);
+ RemoteInterpreter remoteInterpreter =
+ new RemoteInterpreter(properties, "session_1", "className", "user1",
+ ZeppelinConfiguration.load());
+ List<Interpreter> interpreters = new ArrayList<>();
+ interpreters.add(confInterpreter);
+ interpreters.add(remoteInterpreter);
+ when(mockInterpreterGroup.get("session_1")).thenReturn(interpreters);
+
+ InterpreterResult result = confInterpreter.interpret(
+ "property_1\tupdated_value\nBASH_ENV\t/tmp/user-controlled.sh",
+ mock(InterpreterContext.class));
+
+ assertEquals(InterpreterResult.Code.ERROR, result.code);
+ assertTrue(result.toString().contains("BASH_ENV"), result.toString());
+ assertEquals("value_1", remoteInterpreter.getProperty("property_1"));
+ assertEquals("/operator/java", remoteInterpreter.getProperty("JAVA_HOME"));
+ assertNull(remoteInterpreter.getProperty("BASH_ENV"));
+
+ result = confInterpreter.interpret(
+ "property_1\tupdated_value", mock(InterpreterContext.class));
+ assertEquals(InterpreterResult.Code.SUCCESS, result.code);
+ assertEquals("updated_value", remoteInterpreter.getProperty("property_1"));
+ assertEquals("/operator/java", remoteInterpreter.getProperty("JAVA_HOME"));
+ }
}
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/launcher/InterpreterShellScriptTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/launcher/InterpreterShellScriptTest.java
new file mode 100644
index 0000000000..fd3d41639d
--- /dev/null
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/interpreter/launcher/InterpreterShellScriptTest.java
@@ -0,0 +1,119 @@
+/*
+ * 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.zeppelin.interpreter.launcher;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class InterpreterShellScriptTest {
+
+ @Test
+ void doesNotEvaluateDownloaderArguments(
+ @TempDir Path temporaryDirectory) throws Exception {
+ Path zeppelinHome = Paths.get("..").toAbsolutePath().normalize();
+ Path javaHome =
Files.createDirectories(temporaryDirectory.resolve("java-home/bin"))
+ .getParent();
+ Path captureFile = temporaryDirectory.resolve("java-arguments");
+ Path fakeJava = javaHome.resolve("bin/java");
+ Files.writeString(fakeJava,
+ "#!/bin/bash\n"
+ + "if [[ \"$1\" == \"-version\" ]]; then\n"
+ + " echo 'openjdk version \"11.0.0\"' >&2\n"
+ + " exit 0\n"
+ + "fi\n"
+ + "printf 'BEGIN\\n' >> \"${CAPTURE_FILE}\"\n"
+ + "printf '<%s>\\n' \"$@\" >> \"${CAPTURE_FILE}\"\n"
+ + "printf 'END\\n' >> \"${CAPTURE_FILE}\"\n",
+ StandardCharsets.UTF_8);
+ fakeJava.toFile().setExecutable(true);
+
+ Path classpathMarker = temporaryDirectory.resolve("classpath-injected");
+ Path classpathOverridesMarker =
temporaryDirectory.resolve("classpath-overrides-injected");
+ Path javaOptionsMarker =
temporaryDirectory.resolve("java-options-injected");
+ Path memoryMarker = temporaryDirectory.resolve("memory-injected");
+ Path wildcardDirectory =
Files.createDirectory(temporaryDirectory.resolve("wildcard jars"));
+ Files.write(wildcardDirectory.resolve("dependency.jar"), new byte[] {1});
+ String maliciousClasspath = wildcardDirectory + "/*;touch " +
classpathMarker + ";";
+ String maliciousClasspathOverrides =
+ "override;touch " + classpathOverridesMarker + ";";
+ String maliciousJavaOptions = "-Dsafe=true $(touch " + javaOptionsMarker +
")";
+ String maliciousMemory = "-Xmx128m;touch " + memoryMarker + ";";
+
+ Path confDirectory =
Files.createDirectory(temporaryDirectory.resolve("conf"));
+ Path interpreterDirectory =
Files.createDirectory(temporaryDirectory.resolve("test"));
+ Path localRepository =
Files.createDirectory(temporaryDirectory.resolve("local-repo"));
+ Path logDirectory =
Files.createDirectory(temporaryDirectory.resolve("logs"));
+ Path pidDirectory =
Files.createDirectory(temporaryDirectory.resolve("run"));
+
+ ProcessBuilder processBuilder = new ProcessBuilder(
+ zeppelinHome.resolve("bin/interpreter.sh").toString(),
+ "-p", "12345",
+ "-r", ":",
+ "-i", "group-id",
+ "-d", interpreterDirectory.toString(),
+ "-l", localRepository.toString(),
+ "-g", "test");
+ processBuilder.redirectErrorStream(true);
+ Map<String, String> environment = processBuilder.environment();
+ environment.put("JAVA_HOME", javaHome.toString());
+ environment.put("ZEPPELIN_HOME", zeppelinHome.toString());
+ environment.put("ZEPPELIN_CONF_DIR", confDirectory.toString());
+ environment.put("ZEPPELIN_LOG_DIR", logDirectory.toString());
+ environment.put("ZEPPELIN_PID_DIR", pidDirectory.toString());
+ environment.put("INTERPRETER_GROUP_ID", "group-id");
+ environment.put("CAPTURE_FILE", captureFile.toString());
+ environment.put("ZEPPELIN_INTP_CLASSPATH", maliciousClasspath);
+ environment.put("ZEPPELIN_INTP_CLASSPATH_OVERRIDES",
maliciousClasspathOverrides);
+ environment.put("ZEPPELIN_INTP_JAVA_OPTS", maliciousJavaOptions);
+ environment.put("ZEPPELIN_INTP_MEM", maliciousMemory);
+
+ Process process = processBuilder.start();
+ assertTrue(process.waitFor(30, TimeUnit.SECONDS));
+ String output = new String(process.getInputStream().readAllBytes(),
StandardCharsets.UTF_8);
+
+ assertEquals(0, process.exitValue(), output);
+ assertFalse(Files.exists(classpathMarker), output);
+ assertFalse(Files.exists(classpathOverridesMarker), output);
+ assertFalse(Files.exists(javaOptionsMarker), output);
+ assertFalse(Files.exists(memoryMarker), output);
+ List<String> capturedArguments = Files.readAllLines(captureFile,
StandardCharsets.UTF_8);
+ assertEquals(2, capturedArguments.stream().filter("BEGIN"::equals).count(),
+ capturedArguments.toString());
+ assertEquals(2,
capturedArguments.stream().filter("<-Dsafe=true>"::equals).count(),
+ capturedArguments.toString());
+ assertEquals(2,
capturedArguments.stream().filter("<$(touch>"::equals).count(),
+ capturedArguments.toString());
+ assertEquals(2, capturedArguments.stream()
+ .filter(argument -> argument.startsWith("<" +
maliciousClasspathOverrides + ":"))
+ .filter(argument -> argument.contains(maliciousClasspath))
+ .count(), capturedArguments.toString());
+ assertTrue(capturedArguments.contains("<-Xmx128m;touch>"),
capturedArguments.toString());
+ }
+}
