diff --git a/internal/adapters/audio/ffmpeg_test.go b/internal/adapters/audio/ffmpeg_test.go index 4442c80..9a4a852 100644 --- a/internal/adapters/audio/ffmpeg_test.go +++ b/internal/adapters/audio/ffmpeg_test.go @@ -58,3 +58,22 @@ func TestFFmpegToWAV_MissingBinaryIsAnError(t *testing.T) { require.Error(t, err) assert.False(t, errors.Is(err, context.Canceled)) } + +func TestFFmpegToWAV_RestrictsWhatTheFileMayMakeFFmpegOpen(t *testing.T) { + rec := filepath.Join(t.TempDir(), "argv") + bin := stub(t, `printf '%s\n' "$@" > `+rec+` +for a; do last="$a"; done +printf data > "$last"`) + + err := (&audio.FFmpeg{Bin: bin}).ToWAV(context.Background(), "/spool/in", filepath.Join(t.TempDir(), "o.wav")) + + require.NoError(t, err) + args := argvOf(t, rec) + pw, fw, in := indexOf(args, "-protocol_whitelist"), indexOf(args, "-format_whitelist"), indexOf(args, "-i") + require.GreaterOrEqual(t, pw, 0, "argv: %v", args) + require.GreaterOrEqual(t, fw, 0, "argv: %v", args) + assert.Equal(t, "file,pipe", args[pw+1]) + assert.Equal(t, audio.AllowedDemuxers, args[fw+1]) + assert.Less(t, pw, in, "input options must come BEFORE -i or ffmpeg applies them to the output") + assert.Less(t, fw, in) +} diff --git a/internal/adapters/audio/ffprobe_test.go b/internal/adapters/audio/ffprobe_test.go index 609a84c..214320b 100644 --- a/internal/adapters/audio/ffprobe_test.go +++ b/internal/adapters/audio/ffprobe_test.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" "time" @@ -127,3 +128,59 @@ func TestFFprobeProbe_StopsWhenTheContextIsCancelled(t *testing.T) { assert.Less(t, time.Since(start), 5*time.Second) assert.False(t, errors.Is(err, uploads.ErrUnreadable), "a timeout is not a verdict on the file") } + +// argvStub records the arguments it was called with into a file and then behaves as +// a successful ffprobe, so a test can assert exactly what the process was asked. +func argvStub(t *testing.T) (bin, argvFile string) { + t.Helper() + argvFile = filepath.Join(t.TempDir(), "argv") + bin = stub(t, `printf '%s\n' "$@" > `+argvFile+` +echo '{"streams":[{"codec_type":"audio"}],"format":{"duration":"1"}}'`) + return bin, argvFile +} + +func argvOf(t *testing.T, file string) []string { + t.Helper() + raw, err := os.ReadFile(file) + require.NoError(t, err) + return strings.Split(strings.TrimSpace(string(raw)), "\n") +} + +func indexOf(args []string, want string) int { + for i, a := range args { + if a == want { + return i + } + } + return -1 +} + +func TestFFprobeProbe_RestrictsWhatTheFileMayMakeFFmpegOpen(t *testing.T) { + // The input is attacker-supplied. ffmpeg can be told by a file's CONTENT to open + // other files or URLs (HLS and concat playlists); recent versions refuse by + // default, but that is version luck. Pin it: local files and pipes only, and + // only the demuxers real audio recordings use. + bin, argvFile := argvStub(t) + + _, err := (&audio.FFprobe{Bin: bin}).Probe(context.Background(), "/spool/upload-1") + + require.NoError(t, err) + args := argvOf(t, argvFile) + pw := indexOf(args, "-protocol_whitelist") + require.GreaterOrEqual(t, pw, 0, "argv: %v", args) + assert.Equal(t, "file,pipe", args[pw+1]) + fw := indexOf(args, "-format_whitelist") + require.GreaterOrEqual(t, fw, 0, "argv: %v", args) + assert.Equal(t, audio.AllowedDemuxers, args[fw+1]) + assert.Equal(t, "/spool/upload-1", args[len(args)-1], "the path stays the last argument") +} + +func TestAllowedDemuxers_ExcludeThePlaylistFormats(t *testing.T) { + for _, bad := range []string{"concat", "hls", "applehttp", "dash", "rtsp", "sdp", "image2"} { + assert.NotContains(t, strings.Split(audio.AllowedDemuxers, ","), bad, + "%s can make ffmpeg open other files or URLs named inside the upload", bad) + } + for _, good := range []string{"wav", "mov", "mp4", "m4a", "mp3", "ogg", "matroska", "flac"} { + assert.Contains(t, strings.Split(audio.AllowedDemuxers, ","), good) + } +}