diff --git a/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/CrashUploaderScriptInitializer.java b/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/CrashUploaderScriptInitializer.java index f2e7ac706bb..bd57687d643 100644 --- a/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/CrashUploaderScriptInitializer.java +++ b/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)) { diff --git a/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/Initializer.java b/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/Initializer.java index 8e1ba25f423..d0f7cac1ad8 100644 --- a/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/Initializer.java +++ b/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 perms = Files.getPosixFilePermissions(path); @@ -472,4 +470,78 @@ static boolean isOwnedAndPrivate(File f) { return false; } } + + private static final Set 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 + * write bit set; on non-POSIX file systems always returns {@code true}. Unlike {@link + * #isOwnedAndPrivate(File)}, stray group/world read or execute 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 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 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()); + } + } } diff --git a/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/OOMENotifierScriptInitializer.java b/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/OOMENotifierScriptInitializer.java index 91420eda112..e4ae6fda673 100644 --- a/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/OOMENotifierScriptInitializer.java +++ b/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)) { diff --git a/dd-java-agent/agent-crashtracking/src/test/java/datadog/crashtracking/ScriptInitializerSecurityTest.java b/dd-java-agent/agent-crashtracking/src/test/java/datadog/crashtracking/ScriptInitializerSecurityTest.java index 8f0cc003948..625e15031ea 100644 --- a/dd-java-agent/agent-crashtracking/src/test/java/datadog/crashtracking/ScriptInitializerSecurityTest.java +++ b/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 expected) + throws IOException { + Set 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 perms = Files.getPosixFilePermissions(path); for (PosixFilePermission bit :