This is an automated email from the ASF dual-hosted git repository. jongyoul pushed a commit to branch branch-0.12 in repository https://gitbox.apache.org/repos/asf/zeppelin.git
commit 1cde1eb6f77f371ec96a3d95f546385441714295 Author: Jongyoul Lee <[email protected]> AuthorDate: Thu Aug 20 16:58:46 2026 +0900 [HOTFIX] Avoid shell evaluation of interpreter launch arguments 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. Hot Fix * [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 N/A ```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. N/A * 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]> (cherry picked from commit f2f34cd548ef03ca7b0c438e63709d0cc12a9de4) 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 19a6fe7119..6666d13413 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-zengine/src/main/java/org/apache/zeppelin/interpreter/ConfInterpreter.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/ConfInterpreter.java index 9c5d02debb..ba3d85bfd0 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/ConfInterpreter.java +++ b/zeppelin-zengine/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-zengine/src/main/java/org/apache/zeppelin/interpreter/SessionConfInterpreter.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/SessionConfInterpreter.java index 0718b07384..2b1b733338 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/SessionConfInterpreter.java +++ b/zeppelin-zengine/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-zengine/src/test/java/org/apache/zeppelin/interpreter/ConfInterpreterTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/ConfInterpreterTest.java index 785c30122f..8bf258f1ce 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/ConfInterpreterTest.java +++ b/zeppelin-zengine/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-zengine/src/test/java/org/apache/zeppelin/interpreter/SessionConfInterpreterTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/SessionConfInterpreterTest.java index 44701cf549..05eba5fc6e 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/SessionConfInterpreterTest.java +++ b/zeppelin-zengine/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-zengine/src/test/java/org/apache/zeppelin/interpreter/launcher/InterpreterShellScriptTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/launcher/InterpreterShellScriptTest.java new file mode 100644 index 0000000000..fd3d41639d --- /dev/null +++ b/zeppelin-zengine/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()); + } +}
