Fix crash-tracking script/directory permission lockdown

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

@@ -4,7 +4,10 @@
 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.crashtracking.Initializer.isSafeToRepair;
+import static datadog.crashtracking.Initializer.restrictDirectoryToOwnerOnly;
+import static datadog.crashtracking.Initializer.restrictScriptToOwnerOnly;
+import static datadog.crashtracking.Initializer.stripGroupAndWorldBits;
 import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY;
 import static java.util.Locale.ROOT;
 
@@ -78,14 +81,30 @@ private static boolean copyCrashUploaderScript(
             scriptDirectory);
         return false;
       }
-      scriptDirectory.setReadable(true, true);
-      scriptDirectory.setWritable(true, true);
-      scriptDirectory.setExecutable(true, true);
+      if (!restrictDirectoryToOwnerOnly(scriptDirectory)) {
+        LOG.warn(
+            SEND_TELEMETRY,
+            "Unable to restrict crash tracking script folder {} to owner-only permissions. "
+                + SETUP_FAILURE_MESSAGE,
+            scriptDirectory);
+        return false;
+      }
     } else {
-      if (!isOwnedAndPrivate(scriptDirectory)) {
+      if (!isSafeToRepair(scriptDirectory)) {
+        LOG.warn(
+            SEND_TELEMETRY,
+            "Untrusted crash tracking script folder {} (wrong owner or group/world-writable). "
+                + SETUP_FAILURE_MESSAGE,
+            scriptDirectory);
+        return false;
+      }
+      // owned by us but possibly left over from an older, less restrictive version: strip any
+      // stray group/world bits without touching the owner's own bits, so a directory an operator
+      // deliberately made non-writable stays non-writable
+      if (!stripGroupAndWorldBits(scriptDirectory)) {
         LOG.warn(
             SEND_TELEMETRY,
-            "Untrusted crash tracking script folder {} (wrong owner or group/world bits set). "
+            "Unable to strip group/world permissions from crash tracking script folder {}. "
                 + SETUP_FAILURE_MESSAGE,
             scriptDirectory);
         return false;
@@ -101,7 +120,8 @@ private static boolean copyCrashUploaderScript(
     } catch (UntrustedScriptException e) {
       LOG.warn(
           SEND_TELEMETRY,
-          "Untrusted crash uploader script {} (wrong owner or group/world-writable). "
+          "Untrusted or unprotectable crash uploader script {} (wrong owner, group/world-writable,"
+              + " or unable to restrict permissions). "
               + SETUP_FAILURE_MESSAGE,
           scriptFile);
       return false;
@@ -118,9 +138,14 @@ private static boolean copyCrashUploaderScript(
   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}.
+   * Writes the crash uploader script if it does not already exist. A freshly written script is
+   * immediately restricted to owner-only permissions; a script that cannot be locked down is
+   * discarded. When the script already exists it is validated for POSIX ownership and repairable
+   * permissions before reuse: a script owned by the JVM user without group/world write bits is
+   * repaired in place by stripping stray group/world bits (e.g. left behind by an older, less
+   * restrictive version of this initializer), anything else is untrusted. Failure to trust or
+   * repair the 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)
@@ -137,11 +162,13 @@ private static void writeCrashUploaderScript(
           bw.newLine();
         }
       }
-      scriptFile.setReadable(true, true);
-      scriptFile.setWritable(false, false);
-      scriptFile.setExecutable(true, true);
+      // fail closed: never leave a freshly written script we could not lock down
+      if (!restrictScriptToOwnerOnly(scriptFile)) {
+        scriptFile.delete();
+        throw new UntrustedScriptException();
+      }
     } else {
-      if (!isOwnedAndPrivate(scriptFile)) {
+      if (!isSafeToRepair(scriptFile) || !stripGroupAndWorldBits(scriptFile)) {
         throw new UntrustedScriptException();
       }
     }

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

@@ -448,28 +448,128 @@ private static String getScriptFileName(String scriptName) {
           PosixFilePermission.OTHERS_WRITE,
           PosixFilePermission.OTHERS_EXECUTE);
 
+  private static final Set<PosixFilePermission> GROUP_WORLD_WRITE_BITS =
+      EnumSet.of(PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE);
+
+  /**
+   * Returns {@code true} when {@code f} is owned by the current JVM user and has no group/world
+   * <em>write</em> bit set; on non-POSIX file systems always returns {@code true}. Stray
+   * group/world <em>read</em> or <em>execute</em> bits (e.g. the {@code 0755} a pre-upgrade version
+   * of this initializer, which did not lock down permissions, could have left behind) do not
+   * disqualify the path here: those bits are safe to tighten in place with {@link
+   * #stripGroupAndWorldBits(File)} rather than treating the path as untrusted. A group/world write
+   * bit is still treated as a sign of possible tampering and causes this method to return {@code
+   * false}.
+   */
+  static boolean isSafeToRepair(File f) {
+    return isOwnedWithoutBits(f, GROUP_WORLD_WRITE_BITS);
+  }
+
   /**
-   * 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).
+   * Returns {@code true} when {@code f} is owned by the current JVM user and has none of {@code
+   * forbiddenBits} set. On non-POSIX file systems always returns {@code true}.
    */
-  static boolean isOwnedAndPrivate(File f) {
+  private static boolean isOwnedWithoutBits(File f, Set<PosixFilePermission> forbiddenBits) {
     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)) {
+      if (!isJvmOwner(path)) {
         return false;
       }
       Set<PosixFilePermission> perms = Files.getPosixFilePermissions(path);
-      return perms.stream().noneMatch(GROUP_WORLD_BITS::contains);
-    } catch (IOException | IllegalStateException e) {
+      return perms.stream().noneMatch(forbiddenBits::contains);
+    } catch (IOException | IllegalStateException | UnsupportedOperationException e) {
       LOG.debug("Unable to check ownership/permissions for {}: {}", f, e.getMessage());
       return false;
     }
   }
+
+  private static boolean isJvmOwner(Path path) throws IOException {
+    UserPrincipal owner = Files.getOwner(path);
+    UserPrincipal jvmUser = Files.getOwner(TempLocationManager.getInstance().getTempDir());
+    return jvmUser.equals(owner);
+  }
+
+  /**
+   * Sets read/write/execute for the owner only on a freshly created script directory (effective
+   * {@code 0700}, stripping any group/world bits left over from the process umask). Returns {@code
+   * true} when the permissions were applied; on failure the caller must treat the directory as
+   * unusable.
+   */
+  static boolean restrictDirectoryToOwnerOnly(File dir) {
+    return setOwnerOnlyPermissions(dir, true);
+  }
+
+  /**
+   * Sets read/execute (but not write) for the owner only on a freshly created script file. Returns
+   * {@code true} when the permissions were applied; on failure the caller must discard the file.
+   */
+  static boolean restrictScriptToOwnerOnly(File scriptFile) {
+    return setOwnerOnlyPermissions(scriptFile, false);
+  }
+
+  /**
+   * Applies owner-only permissions to {@code f}: all group/world bits are cleared and the owner
+   * keeps read and execute, plus write iff {@code ownerWritable}. On POSIX file systems this is a
+   * single atomic operation whose failure makes the caller refuse the path. On file systems without
+   * POSIX permission bits (e.g. Windows) there is nothing to verify, so restriction is best-effort
+   * through the legacy API and the method returns {@code true} even on partial failure.
+   */
+  private static boolean setOwnerOnlyPermissions(File f, boolean ownerWritable) {
+    Path path = f.toPath();
+    if (path.getFileSystem().supportedFileAttributeViews().contains("posix")) {
+      try {
+        Set<PosixFilePermission> perms =
+            EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_EXECUTE);
+        if (ownerWritable) {
+          perms.add(PosixFilePermission.OWNER_WRITE);
+        }
+        Files.setPosixFilePermissions(path, perms);
+        return true;
+      } catch (IOException | IllegalStateException | UnsupportedOperationException e) {
+        LOG.debug("Unable to restrict permissions for {}: {}", f, e.getMessage());
+        return false;
+      }
+    }
+    boolean ok = f.setReadable(false, false);
+    ok &= f.setWritable(false, false);
+    ok &= f.setExecutable(false, false);
+    ok &= f.setReadable(true, true);
+    if (ownerWritable) {
+      ok &= f.setWritable(true, true);
+    }
+    ok &= f.setExecutable(true, true);
+    if (!ok) {
+      LOG.debug(
+          "Unable to fully restrict permissions for {} on a file system without POSIX support", f);
+    }
+    return true;
+  }
+
+  /**
+   * Removes any group/world permission bits from {@code f} while leaving the owner's own bits
+   * untouched, in a single atomic operation. Unlike {@link #restrictDirectoryToOwnerOnly(File)},
+   * this never adds a permission (e.g. owner write) that {@code f} did not already have, so a path
+   * an operator deliberately made non-writable for the owner stays non-writable after repair.
+   * Returns {@code true} when the bits were applied or nothing needed stripping; on failure the
+   * caller must treat {@code f} as unusable. On Windows this is a no-op that returns {@code true}.
+   */
+  static boolean stripGroupAndWorldBits(File f) {
+    if (OperatingSystem.isWindows()) {
+      return true;
+    }
+    try {
+      Path path = f.toPath();
+      Set<PosixFilePermission> perms = EnumSet.noneOf(PosixFilePermission.class);
+      perms.addAll(Files.getPosixFilePermissions(path));
+      perms.removeAll(GROUP_WORLD_BITS);
+      Files.setPosixFilePermissions(path, perms);
+      return true;
+    } catch (IOException | IllegalStateException | UnsupportedOperationException e) {
+      LOG.debug("Unable to strip group/world permissions for {}: {}", f, e.getMessage());
+      return false;
+    }
+  }
 }

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

@@ -5,8 +5,11 @@
 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.isSafeToRepair;
 import static datadog.crashtracking.Initializer.pidFromSpecialFileName;
+import static datadog.crashtracking.Initializer.restrictDirectoryToOwnerOnly;
+import static datadog.crashtracking.Initializer.restrictScriptToOwnerOnly;
+import static datadog.crashtracking.Initializer.stripGroupAndWorldBits;
 import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY;
 
 import datadog.trace.api.internal.VisibleForTesting;
@@ -61,10 +64,20 @@ private static boolean copyOOMEscript(File scriptFile) {
     File scriptDirectory = scriptFile.getParentFile();
 
     if (scriptDirectory.exists()) {
-      if (!isOwnedAndPrivate(scriptDirectory)) {
+      if (!isSafeToRepair(scriptDirectory)) {
         LOG.warn(
             SEND_TELEMETRY,
-            "Untrusted OOME script folder {} (wrong owner or group/world bits set). OOME notification will not work properly.",
+            "Untrusted OOME script folder {} (wrong owner or group/world-writable). OOME notification will not work properly.",
+            scriptDirectory);
+        return false;
+      }
+      // owned by us but possibly left over from an older, less restrictive version: strip any
+      // stray group/world bits without touching the owner's own bits, so a directory an operator
+      // deliberately made non-writable stays non-writable
+      if (!stripGroupAndWorldBits(scriptDirectory)) {
+        LOG.warn(
+            SEND_TELEMETRY,
+            "Unable to strip group/world permissions from OOME script folder {}. OOME notification will not work properly.",
             scriptDirectory);
         return false;
       }
@@ -86,32 +99,48 @@ private static boolean copyOOMEscript(File scriptFile) {
             scriptDirectory);
         return false;
       }
-      scriptDirectory.setReadable(true, true);
-      scriptDirectory.setWritable(true, true);
-      scriptDirectory.setExecutable(true, true);
+      if (!restrictDirectoryToOwnerOnly(scriptDirectory)) {
+        LOG.warn(
+            SEND_TELEMETRY,
+            "Unable to restrict OOME script folder {} to owner-only permissions. OOME notification will not work properly.",
+            scriptDirectory);
+        return false;
+      }
     }
 
     try {
       // do not overwrite existing
       if (!scriptFile.exists()) {
         copyStream(getOomeNotifierTemplate(), scriptFile);
-        scriptFile.setReadable(true, true);
-        scriptFile.setWritable(false, false);
-        scriptFile.setExecutable(true, true);
+        // fail closed: never leave a freshly written script we could not lock down
+        if (!restrictScriptToOwnerOnly(scriptFile)) {
+          scriptFile.delete();
+          throw new IOException("Unable to restrict OOME script permissions");
+        }
       } else {
-        if (!isOwnedAndPrivate(scriptFile)) {
+        // owned by us but possibly left over from an older, less restrictive version: repair in
+        // place by stripping stray group/world bits, preserving the owner's own bits
+        if (!isSafeToRepair(scriptFile)) {
           LOG.warn(
               SEND_TELEMETRY,
               "Untrusted OOME script {} (wrong owner or group/world-writable). OOME notification will not work properly.",
               scriptFile);
           return false;
         }
+        if (!stripGroupAndWorldBits(scriptFile)) {
+          LOG.warn(
+              SEND_TELEMETRY,
+              "Unable to strip group/world permissions from OOME script {}. OOME notification will not work properly.",
+              scriptFile);
+          return false;
+        }
       }
     } catch (IOException e) {
       LOG.warn(
           SEND_TELEMETRY,
-          "Failed to copy OOME script {}. OOME notification will not work properly.",
-          scriptFile);
+          "Failed to copy OOME script {} ({}). OOME notification will not work properly.",
+          scriptFile,
+          e.getMessage());
       return false;
     }
     return true;