Fix crash-tracking script/directory permission lockdown

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

@@ -5,6 +5,9 @@
 import static datadog.crashtracking.Initializer.findAgentJar;
 import static datadog.crashtracking.Initializer.getCrashUploaderTemplate;
 import static datadog.crashtracking.Initializer.isOwnedAndPrivate;
+import static datadog.crashtracking.Initializer.isSafeToRepairDirectory;
+import static datadog.crashtracking.Initializer.restrictDirectoryToOwnerOnly;
+import static datadog.crashtracking.Initializer.stripGroupAndWorldBits;
 import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY;
 import static java.util.Locale.ROOT;
 
@@ -78,18 +81,20 @@ private static boolean copyCrashUploaderScript(
             scriptDirectory);
         return false;
       }
-      scriptDirectory.setReadable(true, true);
-      scriptDirectory.setWritable(true, true);
-      scriptDirectory.setExecutable(true, true);
+      restrictDirectoryToOwnerOnly(scriptDirectory);
     } else {
-      if (!isOwnedAndPrivate(scriptDirectory)) {
+      if (!isSafeToRepairDirectory(scriptDirectory)) {
         LOG.warn(
             SEND_TELEMETRY,
-            "Untrusted crash tracking script folder {} (wrong owner or group/world bits set). "
+            "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
+      stripGroupAndWorldBits(scriptDirectory);
     }
     if (!scriptDirectory.canWrite()) {
       LOG.warn(SEND_TELEMETRY, "Read only directory {}. " + SETUP_FAILURE_MESSAGE, scriptDirectory);
@@ -137,8 +142,13 @@ private static void writeCrashUploaderScript(
           bw.newLine();
         }
       }
-      scriptFile.setReadable(true, true);
+      // first clear all privileges
+      scriptFile.setReadable(false, false);
       scriptFile.setWritable(false, false);
+      scriptFile.setExecutable(false, false);
+      // then set them only for the owner
+      scriptFile.setReadable(true, true);
+      // do not restore the writable
       scriptFile.setExecutable(true, true);
     } else {
       if (!isOwnedAndPrivate(scriptFile)) {

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

@@ -460,9 +460,7 @@ static boolean isOwnedAndPrivate(File f) {
     }
     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);
@@ -472,4 +470,78 @@ static boolean isOwnedAndPrivate(File f) {
       return false;
     }
   }
+
+  private static final Set<PosixFilePermission> GROUP_WORLD_WRITE_BITS =
+      EnumSet.of(PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE);
+
+  /**
+   * Returns {@code true} when {@code dir} 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}. Unlike {@link
+   * #isOwnedAndPrivate(File)}, 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 directory here: those bits are safe to tighten in
+   * place with {@link #restrictDirectoryToOwnerOnly(File)} rather than treating the directory 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 isSafeToRepairDirectory(File dir) {
+    if (OperatingSystem.isWindows()) {
+      return true;
+    }
+    try {
+      Path path = dir.toPath();
+      if (!isJvmOwner(path)) {
+        return false;
+      }
+      Set<PosixFilePermission> perms = Files.getPosixFilePermissions(path);
+      return perms.stream().noneMatch(GROUP_WORLD_WRITE_BITS::contains);
+    } catch (IOException | IllegalStateException e) {
+      LOG.debug("Unable to check ownership/permissions for {}: {}", dir, 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);
+  }
+
+  /**
+   * Clears all permission bits on {@code dir} and then sets read/write/execute for the owner only
+   * (effective {@code 0700}). Used both when a script directory is freshly created (to strip any
+   * group/world bits left over from the process umask) and to repair a pre-existing directory that
+   * is owned by the JVM user but was created by an older, less restrictive version.
+   */
+  static void restrictDirectoryToOwnerOnly(File dir) {
+    // first clear all privileges
+    dir.setReadable(false, false);
+    dir.setWritable(false, false);
+    dir.setExecutable(false, false);
+    // then set them only for the owner
+    dir.setReadable(true, true);
+    dir.setWritable(true, true);
+    dir.setExecutable(true, true);
+  }
+
+  /**
+   * Removes any group/world permission bits from {@code dir} while leaving the owner's own bits
+   * untouched. Unlike {@link #restrictDirectoryToOwnerOnly(File)}, this never adds a permission
+   * (e.g. owner write) that the directory did not already have, so a directory an operator
+   * deliberately made non-writable for the owner stays non-writable after repair. On non-POSIX file
+   * systems this is a no-op.
+   */
+  static void stripGroupAndWorldBits(File dir) {
+    if (OperatingSystem.isWindows()) {
+      return;
+    }
+    try {
+      Path path = dir.toPath();
+      Set<PosixFilePermission> perms = EnumSet.copyOf(Files.getPosixFilePermissions(path));
+      perms.removeAll(GROUP_WORLD_BITS);
+      Files.setPosixFilePermissions(path, perms);
+    } catch (IOException | IllegalStateException e) {
+      LOG.debug("Unable to strip group/world permissions for {}: {}", dir, e.getMessage());
+    }
+  }
 }

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

@@ -6,7 +6,10 @@
 import static datadog.crashtracking.Initializer.getOomeNotifierTemplate;
 import static datadog.crashtracking.Initializer.getScriptPathFromArg;
 import static datadog.crashtracking.Initializer.isOwnedAndPrivate;
+import static datadog.crashtracking.Initializer.isSafeToRepairDirectory;
 import static datadog.crashtracking.Initializer.pidFromSpecialFileName;
+import static datadog.crashtracking.Initializer.restrictDirectoryToOwnerOnly;
+import static datadog.crashtracking.Initializer.stripGroupAndWorldBits;
 import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY;
 
 import datadog.trace.api.internal.VisibleForTesting;
@@ -61,13 +64,17 @@ private static boolean copyOOMEscript(File scriptFile) {
     File scriptDirectory = scriptFile.getParentFile();
 
     if (scriptDirectory.exists()) {
-      if (!isOwnedAndPrivate(scriptDirectory)) {
+      if (!isSafeToRepairDirectory(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;
       }
+      // 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
+      stripGroupAndWorldBits(scriptDirectory);
       // cleanup all stale process-specific generated files in the parent folder of the given OOME
       // notifier script
       runScriptCleanup(scriptDirectory);
@@ -86,17 +93,20 @@ private static boolean copyOOMEscript(File scriptFile) {
             scriptDirectory);
         return false;
       }
-      scriptDirectory.setReadable(true, true);
-      scriptDirectory.setWritable(true, true);
-      scriptDirectory.setExecutable(true, true);
+      restrictDirectoryToOwnerOnly(scriptDirectory);
     }
 
     try {
       // do not overwrite existing
       if (!scriptFile.exists()) {
         copyStream(getOomeNotifierTemplate(), scriptFile);
-        scriptFile.setReadable(true, true);
+        // first clear all privileges
+        scriptFile.setReadable(false, false);
         scriptFile.setWritable(false, false);
+        scriptFile.setExecutable(false, false);
+        // then set them only for the owner
+        scriptFile.setReadable(true, true);
+        // do not restore the writable
         scriptFile.setExecutable(true, true);
       } else {
         if (!isOwnedAndPrivate(scriptFile)) {

dd-java-agent/agent-crashtracking/src/test/java/datadog/crashtracking/ScriptInitializerSecurityTest.java

@@ -1,6 +1,7 @@
 package datadog.crashtracking;
 
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
@@ -12,6 +13,7 @@
 import java.nio.file.attribute.PosixFilePermission;
 import java.nio.file.attribute.PosixFilePermissions;
 import java.util.Comparator;
+import java.util.EnumSet;
 import java.util.Set;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.AfterEach;
@@ -108,6 +110,104 @@ void crashUploaderHijackedDirectoryIsRefused() throws Exception {
     assertFalse(Files.exists(scriptFile), "Script must not be written into a hijacked directory");
   }
 
+  @Test
+  void crashUploaderRepairsPreviouslyGeneratedDirectory() throws Exception {
+    // Simulate a directory left behind by a pre-upgrade version of the initializer that did not
+    // lock down permissions: owned by us, but group/world readable+executable (0755) with no
+    // write bits set for group/other.
+    Path scriptDir = tempDir.resolve("legacy_crash_dir");
+    Files.createDirectories(scriptDir);
+    Files.setPosixFilePermissions(scriptDir, PosixFilePermissions.fromString("rwxr-xr-x"));
+
+    Path scriptFile = scriptDir.resolve("dd_crash_uploader.sh");
+    CrashUploaderScriptInitializer.initialize(scriptFile.toString(), "/tmp/hs_err.log");
+
+    assertTrue(Files.exists(scriptFile), "Script should have been created in the repaired dir");
+    assertPermissions(
+        scriptDir,
+        EnumSet.of(
+            PosixFilePermission.OWNER_READ,
+            PosixFilePermission.OWNER_WRITE,
+            PosixFilePermission.OWNER_EXECUTE));
+  }
+
+  @Test
+  void oomeNotifierRepairsPreviouslyGeneratedDirectory() throws Exception {
+    Path scriptDir = tempDir.resolve("legacy_oome_dir");
+    Files.createDirectories(scriptDir);
+    Files.setPosixFilePermissions(scriptDir, PosixFilePermissions.fromString("rwxr-xr-x"));
+
+    Path scriptFile = scriptDir.resolve("dd_oome_notifier.sh");
+    OOMENotifierScriptInitializer.initialize(scriptFile + " %p");
+
+    assertTrue(Files.exists(scriptFile), "Script should have been created in the repaired dir");
+    assertPermissions(
+        scriptDir,
+        EnumSet.of(
+            PosixFilePermission.OWNER_READ,
+            PosixFilePermission.OWNER_WRITE,
+            PosixFilePermission.OWNER_EXECUTE));
+  }
+
+  @Test
+  void crashUploaderFreshDirHasExactlyOwnerPermissions() throws Exception {
+    // Place the script under a child directory that does not exist yet, so the initializer
+    // must go through its mkdirs()/permission-reset branch rather than the existing-directory
+    // (already owner-only) branch that tempDir itself would take.
+    Path scriptFile = tempDir.resolve("fresh-crash-dir").resolve("dd_crash_uploader.sh");
+    CrashUploaderScriptInitializer.initialize(scriptFile.toString(), "/tmp/hs_err.log");
+
+    // The directory is created by mkdirs() (subject to the process umask) before the
+    // owner-only bits are applied, so any stray group/other bits left over from that
+    // umask must have been cleared, not just overlaid with the owner bits.
+    assertPermissions(
+        scriptFile.getParent(),
+        EnumSet.of(
+            PosixFilePermission.OWNER_READ,
+            PosixFilePermission.OWNER_WRITE,
+            PosixFilePermission.OWNER_EXECUTE));
+  }
+
+  @Test
+  void oomeNotifierFreshDirHasExactlyOwnerPermissions() throws Exception {
+    // Place the script under a child directory that does not exist yet, so the initializer
+    // must go through its mkdirs()/permission-reset branch rather than the existing-directory
+    // (already owner-only) branch that tempDir itself would take.
+    Path scriptFile = tempDir.resolve("fresh-oome-dir").resolve("dd_oome_notifier.sh");
+    OOMENotifierScriptInitializer.initialize(scriptFile + " %p");
+
+    assertPermissions(
+        scriptFile.getParent(),
+        EnumSet.of(
+            PosixFilePermission.OWNER_READ,
+            PosixFilePermission.OWNER_WRITE,
+            PosixFilePermission.OWNER_EXECUTE));
+  }
+
+  @Test
+  void crashUploaderScriptFileHasNoGroupOrWorldReadBit() throws Exception {
+    Path scriptFile = tempDir.resolve("dd_crash_uploader.sh");
+    CrashUploaderScriptInitializer.initialize(scriptFile.toString(), "/tmp/hs_err.log");
+
+    // The script is created with FileOutputStream, so its initial mode is subject to the
+    // process umask (e.g. 0644 under a typical 0022 umask). The clear-then-set-owner-only
+    // sequence must strip any inherited group/other bits, not just overlay owner bits on top
+    // of them, otherwise a later JVM start rejects the script via isOwnedAndPrivate().
+    assertPermissions(
+        scriptFile, EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_EXECUTE));
+  }
+
+  @Test
+  void oomeNotifierScriptFileIsNotOwnerWritable() throws Exception {
+    Path scriptFile = tempDir.resolve("dd_oome_notifier.sh");
+    OOMENotifierScriptInitializer.initialize(scriptFile + " %p");
+
+    // The write bit is deliberately not restored after the clear/set-owner-only sequence,
+    // so the copied script must end up read+execute only, even for the owner.
+    assertPermissions(
+        scriptFile, EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_EXECUTE));
+  }
+
   @Test
   void oomeNotifierFreshDirIsOwnerRestricted() throws Exception {
     Path scriptFile = tempDir.resolve("dd_oome_notifier.sh");
@@ -176,6 +276,20 @@ void cleanPosixTreeEndToEndInitProducesScriptsAndConfigs() throws Exception {
     assertTrue(crashCfgWritten, "Crash uploader .cfg file must be written in the clean flow");
   }
 
+  private static void assertPermissions(Path path, Set<PosixFilePermission> expected)
+      throws IOException {
+    Set<PosixFilePermission> actual = Files.getPosixFilePermissions(path);
+    assertEquals(
+        actual,
+        expected,
+        "Expected permissions "
+            + PosixFilePermissions.toString(expected)
+            + " but found "
+            + PosixFilePermissions.toString(actual)
+            + " on "
+            + path);
+  }
+
   private static void assertNoGroupWorldWriteBit(Path path) throws IOException {
     Set<PosixFilePermission> perms = Files.getPosixFilePermissions(path);
     for (PosixFilePermission bit :