Avoid crashtracking temp-script path hijack

dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/CrashUploaderScriptInitializer.java

@@ -4,6 +4,7 @@
 import static datadog.crashtracking.Initializer.LOG;
 import static datadog.crashtracking.Initializer.findAgentJar;
 import static datadog.crashtracking.Initializer.getCrashUploaderTemplate;
+import static datadog.crashtracking.Initializer.isOwnedAndPrivate;
 import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY;
 import static java.util.Locale.ROOT;
 
@@ -27,16 +28,16 @@ public final class CrashUploaderScriptInitializer {
   private CrashUploaderScriptInitializer() {}
 
   @VisibleForTesting
-  static void initialize(String onErrorVal, String onErrorFile) {
-    initialize(onErrorVal, onErrorFile, null);
+  static boolean initialize(String onErrorVal, String onErrorFile) {
+    return initialize(onErrorVal, onErrorFile, null);
   }
 
   @VisibleForTesting
-  static void initialize(String onErrorVal, String onErrorFile, String javacorePath) {
+  static boolean initialize(String onErrorVal, String onErrorFile, String javacorePath) {
     if (onErrorVal == null || onErrorVal.isEmpty()) {
       LOG.debug(
           SEND_TELEMETRY, "'-XX:OnError' argument was not provided. Crash tracking is disabled.");
-      return;
+      return false;
     }
     if (onErrorFile == null || onErrorFile.isEmpty()) {
       onErrorFile = SystemProperties.get("user.dir") + "/hs_err_pid" + PidHelper.getPid() + ".log";
@@ -48,21 +49,22 @@ static void initialize(String onErrorVal, String onErrorFile, String javacorePat
     String agentJar = findAgentJar();
     if (agentJar == null) {
       LOG.warn(SEND_TELEMETRY, "Unable to locate the agent jar. " + SETUP_FAILURE_MESSAGE);
-      return;
+      return false;
     }
 
     File scriptFile = new File(onErrorVal.replace(" %p", ""));
     boolean isDDCrashUploader =
         scriptFile.getName().toLowerCase(ROOT).contains("dd_crash_uploader");
     if (isDDCrashUploader && !copyCrashUploaderScript(scriptFile, onErrorFile, agentJar)) {
-      return;
+      return false;
     }
 
     if (javacorePath != null && !javacorePath.isEmpty()) {
       writeConfigToPath(scriptFile, "agent", agentJar, "javacore_path", javacorePath);
     } else {
       writeConfigToPath(scriptFile, "agent", agentJar, "hs_err", onErrorFile);
     }
+    return true;
   }
 
   private static boolean copyCrashUploaderScript(
@@ -76,16 +78,17 @@ private static boolean copyCrashUploaderScript(
             scriptDirectory);
         return false;
       }
-      boolean permissionFailure = false;
-      permissionFailure |= !scriptDirectory.setReadable(true, false);
-      permissionFailure |= !scriptDirectory.setWritable(true, false);
-      permissionFailure |= !scriptDirectory.setExecutable(true, false);
-      if (permissionFailure) {
+      scriptDirectory.setReadable(true, true);
+      scriptDirectory.setWritable(true, true);
+      scriptDirectory.setExecutable(true, true);
+    } else {
+      if (!isOwnedAndPrivate(scriptDirectory)) {
         LOG.warn(
             SEND_TELEMETRY,
-            "Failed to set permissions on crash tracking script folder {}. {}",
-            scriptDirectory,
-            SETUP_FAILURE_MESSAGE);
+            "Untrusted crash tracking script folder {} (wrong owner or group/world bits set). "
+                + SETUP_FAILURE_MESSAGE,
+            scriptDirectory);
+        return false;
       }
     }
     if (!scriptDirectory.canWrite()) {
@@ -95,6 +98,13 @@ private static boolean copyCrashUploaderScript(
     try {
       LOG.debug("Writing crash uploader script: {}", scriptFile);
       writeCrashUploaderScript(getCrashUploaderTemplate(), scriptFile, agentJar, onErrorFile);
+    } catch (UntrustedScriptException e) {
+      LOG.warn(
+          SEND_TELEMETRY,
+          "Untrusted crash uploader script {} (wrong owner or group/world-writable). "
+              + SETUP_FAILURE_MESSAGE,
+          scriptFile);
+      return false;
     } catch (IOException e) {
       LOG.warn(
           SEND_TELEMETRY,
@@ -105,6 +115,13 @@ private static boolean copyCrashUploaderScript(
     return true;
   }
 
+  static class UntrustedScriptException extends IOException {}
+
+  /**
+   * Writes the crash uploader script if it does not already exist. When the script already exists
+   * it is validated for POSIX ownership and permissions before reuse; an untrusted script causes
+   * this method to throw {@link UntrustedScriptException} so the caller can return {@code false}.
+   */
   private static void writeCrashUploaderScript(
       InputStream template, File scriptFile, String execClass, String crashFile)
       throws IOException {
@@ -120,9 +137,13 @@ private static void writeCrashUploaderScript(
           bw.newLine();
         }
       }
-      scriptFile.setReadable(true, false);
+      scriptFile.setReadable(true, true);
       scriptFile.setWritable(false, false);
-      scriptFile.setExecutable(true, false);
+      scriptFile.setExecutable(true, true);
+    } else {
+      if (!isOwnedAndPrivate(scriptFile)) {
+        throw new UntrustedScriptException();
+      }
     }
   }
 

dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/Initializer.java

@@ -12,11 +12,18 @@
 import datadog.trace.api.Platform;
 import datadog.trace.util.TempLocationManager;
 import java.io.File;
+import java.io.IOException;
 import java.io.InputStream;
 import java.lang.management.ManagementFactory;
 import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.UserPrincipal;
 import java.util.Arrays;
+import java.util.EnumSet;
 import java.util.List;
+import java.util.Set;
 import java.util.StringTokenizer;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -154,7 +161,8 @@ private static boolean initializeJ9() {
         return false;
       }
     } catch (Throwable t) {
-      logInitializationError(
+      LOG.warn(
+          SEND_TELEMETRY,
           "Unexpected exception while initializing J9 crash tracking. Crash tracking will not work.",
           t);
     }
@@ -361,18 +369,19 @@ private static void initializeCrashUploader(FlagAccess flags) {
         }
       }
 
-      // set the JVM flag
-      boolean rslt = flags.setValue("OnError", onErrorVal);
-      if (!rslt && LOG.isDebugEnabled()) {
-        LOG.debug(
-            SEND_TELEMETRY,
-            "Unable to set OnError flag to {}. Crash-tracking may not work.",
-            onErrorVal);
+      if (CrashUploaderScriptInitializer.initialize(uploadScript, onErrorFile)) {
+        // set the JVM flag only if the script was successfully initialized
+        boolean rslt = flags.setValue("OnError", onErrorVal);
+        if (!rslt && LOG.isDebugEnabled()) {
+          LOG.debug(
+              SEND_TELEMETRY,
+              "Unable to set OnError flag to {}. Crash-tracking may not work.",
+              onErrorVal);
+        }
       }
-
-      CrashUploaderScriptInitializer.initialize(uploadScript, onErrorFile);
     } catch (Throwable t) {
-      logInitializationError(
+      LOG.warn(
+          SEND_TELEMETRY,
           "Unexpected exception while creating custom crash upload script. Crash tracking will not work properly.",
           t);
     }
@@ -401,19 +410,21 @@ private static void initializeOOMENotifier(FlagAccess flags) {
         }
       }
 
-      // set the JVM flag
-      boolean rslt = flags.setValue("OnOutOfMemoryError", onOutOfMemoryVal);
-      if (!rslt && LOG.isDebugEnabled()) {
-        LOG.debug(
-            SEND_TELEMETRY,
-            "Unable to set OnOutOfMemoryError flag to {}. OOME tracking may not work.",
-            onOutOfMemoryVal);
+      if (OOMENotifierScriptInitializer.initialize(notifierScript)) {
+        // set the JVM flag only if the script was successfully initialized
+        boolean rslt = flags.setValue("OnOutOfMemoryError", onOutOfMemoryVal);
+        if (!rslt && LOG.isDebugEnabled()) {
+          LOG.debug(
+              SEND_TELEMETRY,
+              "Unable to set OnOutOfMemoryError flag to {}. OOME tracking may not work.",
+              onOutOfMemoryVal);
+        }
       }
-
-      OOMENotifierScriptInitializer.initialize(notifierScript);
     } catch (Throwable t) {
-      logInitializationError(
-          "Unexpected exception while initializing OOME notifier. OOMEs will not be tracked.", t);
+      LOG.warn(
+          SEND_TELEMETRY,
+          "Unexpected exception while initializing OOME notifier. OOMEs will not be tracked.",
+          t);
     }
   }
 
@@ -428,15 +439,37 @@ private static String getScriptFileName(String scriptName) {
     return scriptName + "." + (OperatingSystem.isWindows() ? "bat" : "sh");
   }
 
-  private static void logInitializationError(String msg, Throwable t) {
-    if (LOG.isDebugEnabled()) {
-      LOG.warn(SEND_TELEMETRY, msg, t);
-    } else {
-      LOG.warn(
-          SEND_TELEMETRY,
-          "{} [{}] (Change the logging level to debug to see the full stacktrace)",
-          msg,
-          t.getMessage());
+  private static final Set<PosixFilePermission> GROUP_WORLD_BITS =
+      EnumSet.of(
+          PosixFilePermission.GROUP_READ,
+          PosixFilePermission.GROUP_WRITE,
+          PosixFilePermission.GROUP_EXECUTE,
+          PosixFilePermission.OTHERS_READ,
+          PosixFilePermission.OTHERS_WRITE,
+          PosixFilePermission.OTHERS_EXECUTE);
+
+  /**
+   * Returns {@code true} when {@code f} is safe to trust: on non-POSIX file systems always returns
+   * {@code true}; on POSIX returns {@code true} only when the path is owned by the current JVM user
+   * and has no group or world permission bits set (effective {@code 0700} for dirs, {@code 0600} or
+   * stricter for files).
+   */
+  static boolean isOwnedAndPrivate(File f) {
+    if (OperatingSystem.isWindows()) {
+      return true;
+    }
+    try {
+      Path path = f.toPath();
+      UserPrincipal owner = Files.getOwner(path);
+      UserPrincipal jvmUser = Files.getOwner(TempLocationManager.getInstance().getTempDir());
+      if (!jvmUser.equals(owner)) {
+        return false;
+      }
+      Set<PosixFilePermission> perms = Files.getPosixFilePermissions(path);
+      return perms.stream().noneMatch(GROUP_WORLD_BITS::contains);
+    } catch (IOException | IllegalStateException e) {
+      LOG.debug("Unable to check ownership/permissions for {}: {}", f, e.getMessage());
+      return false;
     }
   }
 }

dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/OOMENotifierScriptInitializer.java

@@ -5,6 +5,7 @@
 import static datadog.crashtracking.Initializer.findAgentJar;
 import static datadog.crashtracking.Initializer.getOomeNotifierTemplate;
 import static datadog.crashtracking.Initializer.getScriptPathFromArg;
+import static datadog.crashtracking.Initializer.isOwnedAndPrivate;
 import static datadog.crashtracking.Initializer.pidFromSpecialFileName;
 import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY;
 
@@ -22,32 +23,33 @@ public final class OOMENotifierScriptInitializer {
   private OOMENotifierScriptInitializer() {}
 
   @VisibleForTesting
-  static void initialize(String onOutOfMemoryVal) {
+  static boolean initialize(String onOutOfMemoryVal) {
     if (onOutOfMemoryVal == null || onOutOfMemoryVal.isEmpty()) {
       LOG.debug(
           SEND_TELEMETRY,
           "'-XX:OnOutOfMemoryError' argument was not provided. OOME tracking is disabled.");
-      return;
+      return false;
     }
     File scriptFile = getOOMEScriptFile(onOutOfMemoryVal);
     if (scriptFile == null) {
       LOG.error(
           SEND_TELEMETRY,
           "OOME notifier script value ({}) does not follow the expected format: <path>/dd_oome_notifier.(sh|bat) %p. OOME tracking is disabled.",
           onOutOfMemoryVal);
-      return;
+      return false;
     }
     String agentJar = findAgentJar();
     if (agentJar == null) {
       LOG.warn(
           SEND_TELEMETRY,
           "Unable to locate the agent jar. OOME notification will not work properly.");
-      return;
+      return false;
     }
     if (!copyOOMEscript(scriptFile)) {
-      return;
+      return false;
     }
     writeConfigToPath(scriptFile, "agent", agentJar);
+    return true;
   }
 
   private static File getOOMEScriptFile(String onOutOfMemoryVal) {
@@ -58,12 +60,17 @@ private static File getOOMEScriptFile(String onOutOfMemoryVal) {
   private static boolean copyOOMEscript(File scriptFile) {
     File scriptDirectory = scriptFile.getParentFile();
 
-    // cleanup all stale process-specific generated files in the parent folder of the given OOME
-    // notifier script
-    runScriptCleanup(scriptDirectory);
-
     if (scriptDirectory.exists()) {
-      // can be safely ignored; if the folder exists we will just reuse it
+      if (!isOwnedAndPrivate(scriptDirectory)) {
+        LOG.warn(
+            SEND_TELEMETRY,
+            "Untrusted OOME script folder {} (wrong owner or group/world bits set). OOME notification will not work properly.",
+            scriptDirectory);
+        return false;
+      }
+      // cleanup all stale process-specific generated files in the parent folder of the given OOME
+      // notifier script
+      runScriptCleanup(scriptDirectory);
       if (!scriptDirectory.canWrite()) {
         LOG.warn(
             SEND_TELEMETRY,
@@ -79,19 +86,27 @@ private static boolean copyOOMEscript(File scriptFile) {
             scriptDirectory);
         return false;
       }
-      scriptDirectory.setReadable(true, false);
-      scriptDirectory.setWritable(true, false);
-      scriptDirectory.setExecutable(true, false);
+      scriptDirectory.setReadable(true, true);
+      scriptDirectory.setWritable(true, true);
+      scriptDirectory.setExecutable(true, true);
     }
 
     try {
       // do not overwrite existing
       if (!scriptFile.exists()) {
         copyStream(getOomeNotifierTemplate(), scriptFile);
+        scriptFile.setReadable(true, true);
+        scriptFile.setWritable(false, false);
+        scriptFile.setExecutable(true, true);
+      } else {
+        if (!isOwnedAndPrivate(scriptFile)) {
+          LOG.warn(
+              SEND_TELEMETRY,
+              "Untrusted OOME script {} (wrong owner or group/world-writable). OOME notification will not work properly.",
+              scriptFile);
+          return false;
+        }
       }
-      scriptFile.setReadable(true, false);
-      scriptFile.setWritable(false, false);
-      scriptFile.setExecutable(true, false);
     } catch (IOException e) {
       LOG.warn(
           SEND_TELEMETRY,