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..a4a9207ea81 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 @@ -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(); } } 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..f8b68e1c1a9 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 @@ -448,28 +448,128 @@ private static String getScriptFileName(String scriptName) { PosixFilePermission.OTHERS_WRITE, PosixFilePermission.OTHERS_EXECUTE); + private static final Set 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 + * write bit set; on non-POSIX file systems always returns {@code true}. 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 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 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 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 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 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; + } + } } 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..a9e0753fed2 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 @@ -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; 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..c17d0416e08 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; @@ -67,6 +69,43 @@ void crashUploaderFreshDirIsOwnerRestricted() throws Exception { assertNoGroupWorldWriteBit(tempDir); } + @Test + void crashUploaderRepairsPreviouslyGeneratedScriptFile() throws Exception { + // Simulate a script file left behind by a pre-upgrade version of the initializer that did not + // clear group/world bits: owned by us and executable, but group/world readable (0555). + Path scriptFile = tempDir.resolve("dd_crash_uploader.sh"); + Files.createFile(scriptFile); + Files.setPosixFilePermissions(scriptFile, PosixFilePermissions.fromString("r-xr-xr-x")); + + assertTrue( + CrashUploaderScriptInitializer.initialize(scriptFile.toString(), "/tmp/hs_err.log"), + "Initializer should repair the stale script file instead of refusing it"); + + // Repair strips the group/world bits while preserving the owner's own bits + assertPermissions( + scriptFile, EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_EXECUTE)); + + // The repaired file must pass validation on the next JVM start as well + assertTrue(CrashUploaderScriptInitializer.initialize(scriptFile.toString(), "/tmp/hs_err.log")); + } + + @Test + void oomeNotifierRepairsPreviouslyGeneratedScriptFile() throws Exception { + Path scriptFile = tempDir.resolve("dd_oome_notifier.sh"); + Files.createFile(scriptFile); + Files.setPosixFilePermissions(scriptFile, PosixFilePermissions.fromString("r-xr-xr-x")); + + assertTrue( + OOMENotifierScriptInitializer.initialize(scriptFile + " %p"), + "Initializer should repair the stale script file instead of refusing it"); + + assertPermissions( + scriptFile, EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_EXECUTE)); + + // The repaired file must pass validation on the next JVM start as well + assertTrue(OOMENotifierScriptInitializer.initialize(scriptFile + " %p")); + } + @Test void crashUploaderHijackedScriptIsRefused() throws Exception { Path scriptFile = tempDir.resolve("dd_crash_uploader.sh"); @@ -108,6 +147,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 owner-only restriction must strip + // any inherited group/other bits, not just overlay owner bits on top of them, otherwise a + // later JVM start refuses the script as untrusted. + 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 +313,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 :