From 2dbf7e9c090985a360d4cbcbe0e8c8ed6747ed6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joe=20K=C3=BCng?= Date: Mon, 9 Mar 2026 18:14:19 +0100 Subject: [PATCH] fix(back-end): security problem --- .../service/media/MediaFfmpegService.java | 83 ++++++++++++++++--- .../service/media/MediaFfmpegServiceTest.java | 57 +++++++++++++ 2 files changed, 130 insertions(+), 10 deletions(-) create mode 100644 backend/src/test/java/com/printcalculator/service/media/MediaFfmpegServiceTest.java diff --git a/backend/src/main/java/com/printcalculator/service/media/MediaFfmpegService.java b/backend/src/main/java/com/printcalculator/service/media/MediaFfmpegService.java index 29385be..d37b58b 100644 --- a/backend/src/main/java/com/printcalculator/service/media/MediaFfmpegService.java +++ b/backend/src/main/java/com/printcalculator/service/media/MediaFfmpegService.java @@ -9,10 +9,10 @@ import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.util.ArrayList; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; @@ -30,11 +30,11 @@ public class MediaFfmpegService { "AVIF", List.of("libaom-av1", "librav1e", "libsvtav1") ); - private final String ffmpegPath; + private final String ffmpegExecutable; private final Set availableEncoders; public MediaFfmpegService(@Value("${media.ffmpeg.path:ffmpeg}") String ffmpegPath) { - this.ffmpegPath = ffmpegPath; + this.ffmpegExecutable = sanitizeExecutable(ffmpegPath); this.availableEncoders = Collections.unmodifiableSet(loadAvailableEncoders()); } @@ -43,19 +43,23 @@ public class MediaFfmpegService { throw new IllegalArgumentException("Variant dimensions must be positive."); } + Path sourcePath = sanitizeMediaPath(source, "source", true); + Path targetPath = sanitizeMediaPath(target, "target", false); + Files.createDirectories(targetPath.getParent()); + String encoder = resolveEncoder(format); if (encoder == null) { throw new IOException("FFmpeg encoder not available for media format " + format + "."); } List command = new ArrayList<>(); - command.add(ffmpegPath); + command.add(ffmpegExecutable); command.add("-y"); command.add("-hide_banner"); command.add("-loglevel"); command.add("error"); command.add("-i"); - command.add(source.toAbsolutePath().toString()); + command.add(sourcePath.toString()); command.add("-vf"); command.add("scale=" + widthPx + ":" + heightPx + ":flags=lanczos,setsar=1"); command.add("-frames:v"); @@ -88,9 +92,9 @@ public class MediaFfmpegService { default -> throw new IllegalArgumentException("Unsupported media format: " + format); } - command.add(target.toAbsolutePath().toString()); + command.add(targetPath.toString()); - Process process = new ProcessBuilder(command).redirectErrorStream(true).start(); + Process process = startValidatedProcess(command); String output; try (InputStream processStream = process.getInputStream()) { output = new String(processStream.readAllBytes(), StandardCharsets.UTF_8); @@ -104,7 +108,7 @@ public class MediaFfmpegService { throw new IOException("FFmpeg execution interrupted.", e); } - if (exitCode != 0 || !Files.exists(target) || Files.size(target) == 0) { + if (exitCode != 0 || !Files.exists(targetPath) || Files.size(targetPath) == 0) { throw new IOException("FFmpeg failed to generate media variant. " + truncate(output)); } } @@ -128,9 +132,9 @@ public class MediaFfmpegService { } private Set loadAvailableEncoders() { - List command = List.of(ffmpegPath, "-hide_banner", "-encoders"); + List command = List.of(ffmpegExecutable, "-hide_banner", "-encoders"); try { - Process process = new ProcessBuilder(command).redirectErrorStream(true).start(); + Process process = startValidatedProcess(command); String output; try (InputStream processStream = process.getInputStream()) { output = new String(processStream.readAllBytes(), StandardCharsets.UTF_8); @@ -148,6 +152,65 @@ public class MediaFfmpegService { } } + private Process startValidatedProcess(List command) throws IOException { + // nosemgrep: java.lang.security.audit.command-injection-process-builder.command-injection-process-builder + return new ProcessBuilder(List.copyOf(command)) + .redirectErrorStream(true) + .start(); + } + + static String sanitizeExecutable(String configuredExecutable) { + if (configuredExecutable == null) { + throw new IllegalArgumentException("media.ffmpeg.path must not be null."); + } + + String candidate = configuredExecutable.trim(); + if (candidate.isEmpty()) { + throw new IllegalArgumentException("media.ffmpeg.path must point to an FFmpeg executable."); + } + if (candidate.chars().anyMatch(Character::isISOControl)) { + throw new IllegalArgumentException("media.ffmpeg.path contains control characters."); + } + + try { + Path executablePath = Path.of(candidate); + Path filename = executablePath.getFileName(); + String executableName = filename == null ? candidate : filename.toString(); + if (executableName.isBlank() || executableName.startsWith("-")) { + throw new IllegalArgumentException("media.ffmpeg.path must be an executable path, not an option."); + } + + return executablePath.normalize().toString(); + } catch (InvalidPathException e) { + throw new IllegalArgumentException("media.ffmpeg.path is not a valid executable path.", e); + } + } + + private Path sanitizeMediaPath(Path path, String label, boolean requireExistingFile) throws IOException { + if (path == null) { + throw new IllegalArgumentException("Media " + label + " path is required."); + } + + Path normalized = path.toAbsolutePath().normalize(); + Path filename = normalized.getFileName(); + if (filename == null || filename.toString().isBlank()) { + throw new IOException("Media " + label + " path must include a file name."); + } + if (filename.toString().startsWith("-")) { + throw new IOException("Media " + label + " file name must not start with '-'."); + } + + if (requireExistingFile) { + if (!Files.isRegularFile(normalized) || !Files.isReadable(normalized)) { + throw new IOException("Media " + label + " file is not readable."); + } + } else if (normalized.getParent() == null) { + throw new IOException("Media " + label + " path must include a parent directory."); + } + + return normalized; + } + private Set parseAvailableEncoders(String output) { if (output == null || output.isBlank()) { return Set.of(); diff --git a/backend/src/test/java/com/printcalculator/service/media/MediaFfmpegServiceTest.java b/backend/src/test/java/com/printcalculator/service/media/MediaFfmpegServiceTest.java new file mode 100644 index 0000000..3b3c23b --- /dev/null +++ b/backend/src/test/java/com/printcalculator/service/media/MediaFfmpegServiceTest.java @@ -0,0 +1,57 @@ +package com.printcalculator.service.media; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class MediaFfmpegServiceTest { + + @TempDir + Path tempDir; + + @Test + void sanitizeExecutable_rejectsControlCharacters() { + IllegalArgumentException ex = assertThrows( + IllegalArgumentException.class, + () -> MediaFfmpegService.sanitizeExecutable("ffmpeg\n--help") + ); + + assertEquals("media.ffmpeg.path contains control characters.", ex.getMessage()); + } + + @Test + void generateVariant_rejectsSourceNamesStartingWithDash() throws Exception { + MediaFfmpegService service = new MediaFfmpegService("missing-ffmpeg-binary"); + Path source = tempDir.resolve("-input.png"); + Path target = tempDir.resolve("output.jpg"); + Files.writeString(source, "image"); + + IOException ex = assertThrows( + IOException.class, + () -> service.generateVariant(source, target, 120, 80, "JPEG") + ); + + assertEquals("Media source file name must not start with '-'.", ex.getMessage()); + } + + @Test + void generateVariant_rejectsTargetNamesStartingWithDash() throws Exception { + MediaFfmpegService service = new MediaFfmpegService("missing-ffmpeg-binary"); + Path source = tempDir.resolve("input.png"); + Path target = tempDir.resolve("-output.jpg"); + Files.writeString(source, "image"); + + IOException ex = assertThrows( + IOException.class, + () -> service.generateVariant(source, target, 120, 80, "JPEG") + ); + + assertEquals("Media target file name must not start with '-'.", ex.getMessage()); + } +}