azkaban-aplcache

Conditional workflow - Restrict arbitrary code from running

8/13/2018 6:38:59 PM

Details

diff --git a/azkaban-exec-server/src/main/java/azkaban/execapp/AzkabanExecutorServer.java b/azkaban-exec-server/src/main/java/azkaban/execapp/AzkabanExecutorServer.java
index f50f5c9..99031f2 100644
--- a/azkaban-exec-server/src/main/java/azkaban/execapp/AzkabanExecutorServer.java
+++ b/azkaban-exec-server/src/main/java/azkaban/execapp/AzkabanExecutorServer.java
@@ -59,6 +59,9 @@ import java.lang.reflect.Constructor;
 import java.net.InetAddress;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Paths;
+import java.security.Permission;
+import java.security.Policy;
+import java.security.ProtectionDomain;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.TimeZone;
@@ -130,6 +133,17 @@ public class AzkabanExecutorServer {
     StdOutErrRedirect.redirectOutAndErrToLog();
 
     logger.info("Starting Jetty Azkaban Executor...");
+
+    if (System.getSecurityManager() == null) {
+      Policy.setPolicy(new Policy() {
+        @Override
+        public boolean implies(final ProtectionDomain domain, final Permission permission) {
+          return true; // allow all
+        }
+      });
+      System.setSecurityManager(new SecurityManager());
+    }
+
     final Props props = AzkabanServer.loadProps(args);
 
     if (props == null) {
diff --git a/azkaban-exec-server/src/main/java/azkaban/execapp/FlowRunner.java b/azkaban-exec-server/src/main/java/azkaban/execapp/FlowRunner.java
index df5f3c5..a8984ec 100644
--- a/azkaban-exec-server/src/main/java/azkaban/execapp/FlowRunner.java
+++ b/azkaban-exec-server/src/main/java/azkaban/execapp/FlowRunner.java
@@ -61,6 +61,10 @@ import com.google.common.io.Files;
 import java.io.File;
 import java.io.FilenameFilter;
 import java.io.IOException;
+import java.security.AccessControlContext;
+import java.security.AccessController;
+import java.security.PrivilegedExceptionAction;
+import java.security.ProtectionDomain;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
@@ -982,14 +986,29 @@ public class FlowRunner extends EventHandler implements Runnable {
 
   private boolean evaluateExpression(final String expression) {
     boolean result = false;
+    final ScriptEngineManager sem = new ScriptEngineManager();
+    final ScriptEngine se = sem.getEngineByName("JavaScript");
+
+    // Restrict permission using the two-argument form of doPrivileged()
     try {
-      final ScriptEngineManager sem = new ScriptEngineManager();
-      final ScriptEngine se = sem.getEngineByName("JavaScript");
-      result = (boolean) se.eval(expression);
-      this.logger.info("Evaluate expression result: " + result);
-    } catch (final ScriptException e) {
-      this.logger.error("Invalid expression.", e);
+      final Object object = AccessController.doPrivileged(
+          new PrivilegedExceptionAction<Object>() {
+            @Override
+            public Object run() throws ScriptException {
+              return se.eval(expression);
+            }
+          },
+          new AccessControlContext(
+              new ProtectionDomain[]{new ProtectionDomain(null, null)}) // no permissions
+      );
+      if (object != null) {
+        result = (boolean) object;
+      }
+    } catch (final Exception e) {
+      this.logger.error("Failed to evaluate the expression.", e);
     }
+
+    this.logger.info("Evaluate expression result: " + result);
     return result;
   }
 
diff --git a/azkaban-exec-server/src/test/java/azkaban/execapp/FlowRunnerConditionalFlowTest.java b/azkaban-exec-server/src/test/java/azkaban/execapp/FlowRunnerConditionalFlowTest.java
index 3836072..077c556 100644
--- a/azkaban-exec-server/src/test/java/azkaban/execapp/FlowRunnerConditionalFlowTest.java
+++ b/azkaban-exec-server/src/test/java/azkaban/execapp/FlowRunnerConditionalFlowTest.java
@@ -27,7 +27,11 @@ import azkaban.project.Project;
 import azkaban.test.executions.ExecutionsTestUtil;
 import azkaban.utils.Props;
 import java.io.File;
+import java.security.Permission;
+import java.security.Policy;
+import java.security.ProtectionDomain;
 import java.util.HashMap;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -40,6 +44,7 @@ public class FlowRunnerConditionalFlowTest extends FlowRunnerTestBase {
   private static final String CONDITIONAL_FLOW_4 = "conditional_flow4";
   private static final String CONDITIONAL_FLOW_5 = "conditional_flow5";
   private static final String CONDITIONAL_FLOW_6 = "conditional_flow6";
+  private static final String CONDITIONAL_FLOW_7 = "conditional_flow7";
   private FlowRunnerTestUtil testUtil;
   private Project project;
 
@@ -47,6 +52,16 @@ public class FlowRunnerConditionalFlowTest extends FlowRunnerTestBase {
   public void setUp() throws Exception {
     this.testUtil = new FlowRunnerTestUtil(FLOW_YAML_DIR, this.temporaryFolder);
     this.project = this.testUtil.getProject();
+
+    if (System.getSecurityManager() == null) {
+      Policy.setPolicy(new Policy() {
+        @Override
+        public boolean implies(final ProtectionDomain domain, final Permission permission) {
+          return true; // allow all
+        }
+      });
+      System.setSecurityManager(new SecurityManager());
+    }
   }
 
   @Test
@@ -147,6 +162,19 @@ public class FlowRunnerConditionalFlowTest extends FlowRunnerTestBase {
     assertFlowStatus(flow, Status.FAILED);
   }
 
+  @Test
+  public void runFlowOnArbitraryCondition() throws Exception {
+    final HashMap<String, String> flowProps = new HashMap<>();
+    setUp(CONDITIONAL_FLOW_7, flowProps);
+    final ExecutableFlow flow = this.runner.getExecutableFlow();
+    assertStatus(flow, "jobA", Status.SUCCEEDED);
+    assertStatus(flow, "jobB", Status.CANCELLED);
+    assertFlowStatus(flow, Status.KILLED);
+    // The arbitrary code should be restricted from creating a new file.
+    final File file = new File("new.txt");
+    Assert.assertFalse(file.exists());
+  }
+
   private void setUp(final String flowName, final HashMap<String, String> flowProps)
       throws Exception {
     final String flowYamlFile = flowName + ".flow";
diff --git a/azkaban-solo-server/src/main/java/azkaban/soloserver/AzkabanSingleServer.java b/azkaban-solo-server/src/main/java/azkaban/soloserver/AzkabanSingleServer.java
index 6e10639..33a05d7 100644
--- a/azkaban-solo-server/src/main/java/azkaban/soloserver/AzkabanSingleServer.java
+++ b/azkaban-solo-server/src/main/java/azkaban/soloserver/AzkabanSingleServer.java
@@ -31,6 +31,9 @@ import com.google.inject.Guice;
 import com.google.inject.Injector;
 import java.io.File;
 import java.io.IOException;
+import java.security.Permission;
+import java.security.Policy;
+import java.security.ProtectionDomain;
 import javax.inject.Inject;
 import org.apache.commons.io.FileUtils;
 import org.apache.log4j.Logger;
@@ -53,6 +56,16 @@ public class AzkabanSingleServer {
   public static void main(String[] args) throws Exception {
     log.info("Starting Azkaban Server");
 
+    if (System.getSecurityManager() == null) {
+      Policy.setPolicy(new Policy() {
+        @Override
+        public boolean implies(final ProtectionDomain domain, final Permission permission) {
+          return true; // allow all
+        }
+      });
+      System.setSecurityManager(new SecurityManager());
+    }
+
     if (args.length == 0) {
       args = prepareDefaultConf();
     }
diff --git a/test/execution-test-data/conditionalflowyamltest/conditional_flow7.flow b/test/execution-test-data/conditionalflowyamltest/conditional_flow7.flow
new file mode 100644
index 0000000..d2533f0
--- /dev/null
+++ b/test/execution-test-data/conditionalflowyamltest/conditional_flow7.flow
@@ -0,0 +1,17 @@
+nodes:
+  - name: jobB
+    type: test
+    config:
+      fail: false
+      seconds: 0
+
+    condition: var fImport = new JavaImporter(java.io.File); with(fImport) { var f = new File('new'); f.createNewFile(); }
+
+    dependsOn:
+      - jobA
+
+  - name: jobA
+    type: test
+    config:
+      fail: false
+      seconds: 0