fix(back-end): security problem
This commit is contained in:
@@ -9,10 +9,10 @@ import java.io.IOException;
|
|||||||
import java.io.InputStream;
|
import java.io.InputStream;
|
||||||
import java.nio.charset.StandardCharsets;
|
import java.nio.charset.StandardCharsets;
|
||||||
import java.nio.file.Files;
|
import java.nio.file.Files;
|
||||||
|
import java.nio.file.InvalidPathException;
|
||||||
import java.nio.file.Path;
|
import java.nio.file.Path;
|
||||||
import java.util.ArrayList;
|
import java.util.ArrayList;
|
||||||
import java.util.Collections;
|
import java.util.Collections;
|
||||||
import java.util.LinkedHashMap;
|
|
||||||
import java.util.LinkedHashSet;
|
import java.util.LinkedHashSet;
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
import java.util.Locale;
|
import java.util.Locale;
|
||||||
@@ -30,11 +30,11 @@ public class MediaFfmpegService {
|
|||||||
"AVIF", List.of("libaom-av1", "librav1e", "libsvtav1")
|
"AVIF", List.of("libaom-av1", "librav1e", "libsvtav1")
|
||||||
);
|
);
|
||||||
|
|
||||||
private final String ffmpegPath;
|
private final String ffmpegExecutable;
|
||||||
private final Set<String> availableEncoders;
|
private final Set<String> availableEncoders;
|
||||||
|
|
||||||
public MediaFfmpegService(@Value("${media.ffmpeg.path:ffmpeg}") String ffmpegPath) {
|
public MediaFfmpegService(@Value("${media.ffmpeg.path:ffmpeg}") String ffmpegPath) {
|
||||||
this.ffmpegPath = ffmpegPath;
|
this.ffmpegExecutable = sanitizeExecutable(ffmpegPath);
|
||||||
this.availableEncoders = Collections.unmodifiableSet(loadAvailableEncoders());
|
this.availableEncoders = Collections.unmodifiableSet(loadAvailableEncoders());
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -43,19 +43,23 @@ public class MediaFfmpegService {
|
|||||||
throw new IllegalArgumentException("Variant dimensions must be positive.");
|
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);
|
String encoder = resolveEncoder(format);
|
||||||
if (encoder == null) {
|
if (encoder == null) {
|
||||||
throw new IOException("FFmpeg encoder not available for media format " + format + ".");
|
throw new IOException("FFmpeg encoder not available for media format " + format + ".");
|
||||||
}
|
}
|
||||||
|
|
||||||
List<String> command = new ArrayList<>();
|
List<String> command = new ArrayList<>();
|
||||||
command.add(ffmpegPath);
|
command.add(ffmpegExecutable);
|
||||||
command.add("-y");
|
command.add("-y");
|
||||||
command.add("-hide_banner");
|
command.add("-hide_banner");
|
||||||
command.add("-loglevel");
|
command.add("-loglevel");
|
||||||
command.add("error");
|
command.add("error");
|
||||||
command.add("-i");
|
command.add("-i");
|
||||||
command.add(source.toAbsolutePath().toString());
|
command.add(sourcePath.toString());
|
||||||
command.add("-vf");
|
command.add("-vf");
|
||||||
command.add("scale=" + widthPx + ":" + heightPx + ":flags=lanczos,setsar=1");
|
command.add("scale=" + widthPx + ":" + heightPx + ":flags=lanczos,setsar=1");
|
||||||
command.add("-frames:v");
|
command.add("-frames:v");
|
||||||
@@ -88,9 +92,9 @@ public class MediaFfmpegService {
|
|||||||
default -> throw new IllegalArgumentException("Unsupported media format: " + format);
|
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;
|
String output;
|
||||||
try (InputStream processStream = process.getInputStream()) {
|
try (InputStream processStream = process.getInputStream()) {
|
||||||
output = new String(processStream.readAllBytes(), StandardCharsets.UTF_8);
|
output = new String(processStream.readAllBytes(), StandardCharsets.UTF_8);
|
||||||
@@ -104,7 +108,7 @@ public class MediaFfmpegService {
|
|||||||
throw new IOException("FFmpeg execution interrupted.", e);
|
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));
|
throw new IOException("FFmpeg failed to generate media variant. " + truncate(output));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -128,9 +132,9 @@ public class MediaFfmpegService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private Set<String> loadAvailableEncoders() {
|
private Set<String> loadAvailableEncoders() {
|
||||||
List<String> command = List.of(ffmpegPath, "-hide_banner", "-encoders");
|
List<String> command = List.of(ffmpegExecutable, "-hide_banner", "-encoders");
|
||||||
try {
|
try {
|
||||||
Process process = new ProcessBuilder(command).redirectErrorStream(true).start();
|
Process process = startValidatedProcess(command);
|
||||||
String output;
|
String output;
|
||||||
try (InputStream processStream = process.getInputStream()) {
|
try (InputStream processStream = process.getInputStream()) {
|
||||||
output = new String(processStream.readAllBytes(), StandardCharsets.UTF_8);
|
output = new String(processStream.readAllBytes(), StandardCharsets.UTF_8);
|
||||||
@@ -148,6 +152,65 @@ public class MediaFfmpegService {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private Process startValidatedProcess(List<String> 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<String> parseAvailableEncoders(String output) {
|
private Set<String> parseAvailableEncoders(String output) {
|
||||||
if (output == null || output.isBlank()) {
|
if (output == null || output.isBlank()) {
|
||||||
return Set.of();
|
return Set.of();
|
||||||
|
|||||||
@@ -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());
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user