diff --git a/otdfctl/cmd/tdf/decrypt.go b/otdfctl/cmd/tdf/decrypt.go index 05096d7ec0..29e3fa0b2e 100644 --- a/otdfctl/cmd/tdf/decrypt.go +++ b/otdfctl/cmd/tdf/decrypt.go @@ -76,7 +76,7 @@ func decryptRun(cmd *cobra.Command, args []string) { // straight to it rather than accumulating in memory first. var dest io.Writer = os.Stdout if output != "" { - outFile, err = streamio.NewOutputFile(output, decryptedOutputFileMode) + outFile, err = streamio.NewOutputFile(output, decryptedOutputFileMode, in) if err != nil { fail("Failed to write decrypted data to file", err) } diff --git a/otdfctl/cmd/tdf/encrypt.go b/otdfctl/cmd/tdf/encrypt.go index d4c09aa805..641aa67a2e 100644 --- a/otdfctl/cmd/tdf/encrypt.go +++ b/otdfctl/cmd/tdf/encrypt.go @@ -120,7 +120,9 @@ func encryptRun(cmd *cobra.Command, args []string) { // The SDK seeks to the end of the payload to size it, so the input has to be // seekable. A file already is; a pipe is spooled to disk, which trades the // temporary file for the memory a whole-payload read used to cost. - var in io.ReadSeeker + // Held as *os.File rather than io.ReadSeeker so NewOutputFile can check the + // destination against it. + var in *os.File var cleanup func() if filePath != "" { f, err := os.Open(filePath) @@ -148,7 +150,7 @@ func encryptRun(cmd *cobra.Command, args []string) { if !strings.HasSuffix(out, ".tdf") { out += ".tdf" } - tdfFile, err = streamio.NewOutputFile(out, encryptedOutputFileMode) + tdfFile, err = streamio.NewOutputFile(out, encryptedOutputFileMode, in) if err != nil { cleanup() cli.ExitWithError("Failed to write encrypted file "+out, err) diff --git a/otdfctl/e2e/streaming.bats b/otdfctl/e2e/streaming.bats index 9bf1c69a28..96912d8bae 100755 --- a/otdfctl/e2e/streaming.bats +++ b/otdfctl/e2e/streaming.bats @@ -124,7 +124,7 @@ assert_no_leftovers() { assert_failure [ ! -f "$TDF_OUT" ] - assert_no_leftovers "$BATS_TEST_TMPDIR/.payload.txt.tdf.tmp-*" + assert_no_leftovers "$BATS_TEST_TMPDIR/.otdfctl.tmp-*" } @test "decrypt leaves no output behind when it fails" { @@ -135,7 +135,7 @@ assert_no_leftovers() { assert_failure [ ! -f "$RESULT" ] - assert_no_leftovers "$BATS_TEST_TMPDIR/.payload.out.tmp-*" + assert_no_leftovers "$BATS_TEST_TMPDIR/.otdfctl.tmp-*" } # The payoff of writing to a temp sibling: a failed decrypt over an existing @@ -163,6 +163,33 @@ assert_no_leftovers() { [ -c /dev/null ] } +# The other side of writing through: -o at a symlink resolving to the input +# would truncate the input at open, before it has been read. Both commands open +# their input first, so both could destroy a file they were only asked to read. +@test "decrypt refuses an output symlink pointing at its input" { + ./otdfctl encrypt -o "$TDF_OUT" $COMMON "$PLAIN" + ln -s "$TDF_OUT" "$BATS_TEST_TMPDIR/alias.tdf" + + run ./otdfctl decrypt -o "$BATS_TEST_TMPDIR/alias.tdf" $COMMON "$TDF_OUT" + assert_failure + assert_output --partial "output would overwrite the input" + + # The TDF is still a TDF, not a zero-byte file. + ./otdfctl decrypt -o "$RESULT" $COMMON "$TDF_OUT" + diff "$PLAIN" "$RESULT" +} + +@test "encrypt refuses an output symlink pointing at its input" { + ln -s "$PLAIN" "$BATS_TEST_TMPDIR/alias.tdf" + + run ./otdfctl encrypt -o "$BATS_TEST_TMPDIR/alias.tdf" $COMMON "$PLAIN" + assert_failure + assert_output --partial "output would overwrite the input" + + run cat "$PLAIN" + assert_output "$SECRET_TEXT" +} + # The point of DSPX-4499: peak RSS is bounded by segment size, not payload size. # Needs GNU time for 'Maximum resident set size'; BSD/shell time cannot report it. @test "encrypt and decrypt peak memory stay bounded on a large payload" { diff --git a/otdfctl/pkg/streamio/output.go b/otdfctl/pkg/streamio/output.go index 04dc602dde..d7ba52cafb 100644 --- a/otdfctl/pkg/streamio/output.go +++ b/otdfctl/pkg/streamio/output.go @@ -13,10 +13,24 @@ import ( // OutputFile that had already been committed or discarded. var ErrOutputFileFinished = errors.New("streamio: output file already committed or discarded") +// ErrOutputAliasesInput reports that the destination resolves to the file being +// read. Opening it would truncate the input before it has been read. +var ErrOutputAliasesInput = errors.New("streamio: output would overwrite the input") + // tempFileAttempts bounds the search for an unused temporary name, so a // pathological directory cannot spin here forever. const tempFileAttempts = 1000 +// tempFilePrefix names the temporary files created beside a destination. +// +// Deliberately fixed rather than derived from the destination: a component name +// is capped (255 bytes on most filesystems), and a prefix carrying the +// destination's own basename overflows that cap for a destination whose name is +// itself valid, failing the open before a single byte has been read. The random +// suffix is what distinguishes concurrent writers; the prefix only has to be +// recognizable enough to sweep up after a crash. +const tempFilePrefix = ".otdfctl.tmp-" + // OutputFile writes to a temporary file alongside the destination and renames // it into place only once the write has succeeded, so an interrupted or failed // run leaves no partial output where a complete file is expected. @@ -50,12 +64,29 @@ type OutputFile struct { // destination after the process umask has been applied. A destination written // through directly is opened with mode, which the umask likewise applies to, // and which has no effect at all on a destination that already exists. -func NewOutputFile(path string, mode os.FileMode) (*OutputFile, error) { +// +// input is the file the caller is reading, already open, and is what the +// destination is checked against — pass nil when there is none. Callers open +// their input before their output precisely so a failure to read costs nothing; +// without this the write-through open would undo that. +func NewOutputFile(path string, mode os.FileMode, input *os.File) (*OutputFile, error) { direct, err := isDirectDestination(path) if err != nil { return nil, err } if direct { + // Only the write-through open is destructive. An ordinary destination + // naming the input goes to a temp sibling and is renamed over it once + // the read has finished, which is in-place decryption working as asked + // rather than a footgun. + alias, err := aliasesInput(path, input) + if err != nil { + return nil, err + } + if alias { + return nil, fmt.Errorf("%w: %s", ErrOutputAliasesInput, path) + } + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode) if err != nil { return nil, err @@ -63,13 +94,38 @@ func NewOutputFile(path string, mode os.FileMode) (*OutputFile, error) { return &OutputFile{f: f, path: path, direct: true}, nil } - f, err := createTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-", mode) + f, err := createTemp(filepath.Dir(path), tempFilePrefix, mode) if err != nil { return nil, err } return &OutputFile{f: f, path: path}, nil } +// aliasesInput reports whether a write-through open of path would land on +// input. +// +// os.Stat, not os.Lstat: a symlink destination is the case that matters, since +// the open follows it, and the link's own identity is never what gets written. +// A dangling symlink resolves to nothing and aliases nothing — the open creates +// the target. +func aliasesInput(path string, input *os.File) (bool, error) { + if input == nil { + return false, nil + } + inputInfo, err := input.Stat() + if err != nil { + return false, err + } + destInfo, err := os.Stat(path) + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + if err != nil { + return false, err + } + return os.SameFile(inputInfo, destInfo), nil +} + // createTemp is os.CreateTemp with a caller-chosen mode. os.CreateTemp hardcodes // 0600, and correcting that afterwards with Chmod would ignore the umask, so // this opens the file directly to let mode reach the kernel. diff --git a/otdfctl/pkg/streamio/output_test.go b/otdfctl/pkg/streamio/output_test.go index eb4957ef9b..b58a7971f0 100644 --- a/otdfctl/pkg/streamio/output_test.go +++ b/otdfctl/pkg/streamio/output_test.go @@ -1,6 +1,7 @@ package streamio import ( + "io" "os" "path/filepath" "strings" @@ -16,7 +17,7 @@ func TestOutputFileCommitRenamesIntoPlace(t *testing.T) { dir := t.TempDir() dest := filepath.Join(dir, "out.tdf") - o, err := NewOutputFile(dest, testOutputFileMode) + o, err := NewOutputFile(dest, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("payload")) @@ -31,14 +32,14 @@ func TestOutputFileCommitRenamesIntoPlace(t *testing.T) { got, err := os.ReadFile(dest) require.NoError(t, err) assert.Equal(t, "payload", string(got)) - assert.Empty(t, tempSiblings(t, dir, "out.tdf"), "temp file should be gone after Commit") + assert.Empty(t, tempSiblings(t, dir), "temp file should be gone after Commit") } func TestOutputFileCleanupLeavesNoPartialOutput(t *testing.T) { dir := t.TempDir() dest := filepath.Join(dir, "out.tdf") - o, err := NewOutputFile(dest, testOutputFileMode) + o, err := NewOutputFile(dest, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("partial")) require.NoError(t, err) @@ -48,14 +49,14 @@ func TestOutputFileCleanupLeavesNoPartialOutput(t *testing.T) { _, err = os.Stat(dest) require.ErrorIs(t, err, os.ErrNotExist, "a failed run must not leave a partial file") - assert.Empty(t, tempSiblings(t, dir, "out.tdf"), "a failed run must not leave a temp file") + assert.Empty(t, tempSiblings(t, dir), "a failed run must not leave a temp file") } func TestOutputFileCleanupAfterCommitIsNoop(t *testing.T) { dir := t.TempDir() dest := filepath.Join(dir, "out.tdf") - o, err := NewOutputFile(dest, testOutputFileMode) + o, err := NewOutputFile(dest, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("payload")) require.NoError(t, err) @@ -74,7 +75,7 @@ func TestOutputFileTempIsSiblingOfDestination(t *testing.T) { dir := t.TempDir() dest := filepath.Join(dir, "out.tdf") - o, err := NewOutputFile(dest, testOutputFileMode) + o, err := NewOutputFile(dest, testOutputFileMode, nil) require.NoError(t, err) defer o.Cleanup() @@ -88,7 +89,7 @@ func TestOutputFileCommitSetsReadableMode(t *testing.T) { dir := t.TempDir() dest := filepath.Join(dir, "out.tdf") - o, err := NewOutputFile(dest, testOutputFileMode) + o, err := NewOutputFile(dest, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("payload")) require.NoError(t, err) @@ -107,7 +108,7 @@ func TestOutputFileTempNamesDoNotCollide(t *testing.T) { const concurrent = 16 names := make(map[string]bool, concurrent) for range concurrent { - o, err := NewOutputFile(dest, testOutputFileMode) + o, err := NewOutputFile(dest, testOutputFileMode, nil) require.NoError(t, err) defer o.Cleanup() @@ -120,7 +121,7 @@ func TestOutputFileCommitSetsRequestedMode(t *testing.T) { dir := t.TempDir() dest := filepath.Join(dir, "plaintext.txt") - o, err := NewOutputFile(dest, 0o600) + o, err := NewOutputFile(dest, 0o600, nil) require.NoError(t, err) _, err = o.Write([]byte("plaintext")) require.NoError(t, err) @@ -136,7 +137,7 @@ func TestOutputFileCommitReplacesExistingFileWithRequestedMode(t *testing.T) { dest := filepath.Join(dir, "plaintext.txt") require.NoError(t, os.WriteFile(dest, []byte("old"), 0o644)) - o, err := NewOutputFile(dest, 0o600) + o, err := NewOutputFile(dest, 0o600, nil) require.NoError(t, err) _, err = o.Write([]byte("new")) require.NoError(t, err) @@ -148,7 +149,7 @@ func TestOutputFileCommitReplacesExistingFileWithRequestedMode(t *testing.T) { info, err := os.Stat(dest) require.NoError(t, err) assert.Equal(t, plainCreateMode(t, dir, 0o600), info.Mode().Perm()) - assert.Empty(t, tempSiblings(t, dir, "plaintext.txt")) + assert.Empty(t, tempSiblings(t, dir)) } func TestOutputFileCleanupPreservesExistingFile(t *testing.T) { @@ -156,7 +157,7 @@ func TestOutputFileCleanupPreservesExistingFile(t *testing.T) { dest := filepath.Join(dir, "plaintext.txt") require.NoError(t, os.WriteFile(dest, []byte("original"), 0o600)) - o, err := NewOutputFile(dest, 0o600) + o, err := NewOutputFile(dest, 0o600, nil) require.NoError(t, err) _, err = o.Write([]byte("partial")) require.NoError(t, err) @@ -165,14 +166,14 @@ func TestOutputFileCleanupPreservesExistingFile(t *testing.T) { got, err := os.ReadFile(dest) require.NoError(t, err) assert.Equal(t, "original", string(got)) - assert.Empty(t, tempSiblings(t, dir, "plaintext.txt")) + assert.Empty(t, tempSiblings(t, dir)) } func TestOutputFileCommitAfterCommitReturnsError(t *testing.T) { dir := t.TempDir() dest := filepath.Join(dir, "out.tdf") - o, err := NewOutputFile(dest, testOutputFileMode) + o, err := NewOutputFile(dest, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("payload")) require.NoError(t, err) @@ -189,7 +190,7 @@ func TestOutputFileCommitAfterCleanupReturnsError(t *testing.T) { dir := t.TempDir() dest := filepath.Join(dir, "out.tdf") - o, err := NewOutputFile(dest, testOutputFileMode) + o, err := NewOutputFile(dest, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("payload")) require.NoError(t, err) @@ -201,6 +202,65 @@ func TestOutputFileCommitAfterCleanupReturnsError(t *testing.T) { require.ErrorIs(t, err, os.ErrNotExist, "Commit after Cleanup must not create the destination") } +// An ordinary destination that happens to be the input is not the destructive +// case: the payload goes to a temp sibling and is renamed over the input only +// once the read has finished. Decrypting a file in place is a reasonable thing +// to ask for, so the guard must not reject it. +func TestOutputFileAllowsRegularDestinationNamingInput(t *testing.T) { + dir := t.TempDir() + dest := filepath.Join(dir, "in-place.txt") + require.NoError(t, os.WriteFile(dest, []byte("ciphertext"), 0o600)) + + input, err := os.Open(dest) + require.NoError(t, err) + defer input.Close() + + o, err := NewOutputFile(dest, testOutputFileMode, input) + require.NoError(t, err) + + // The input stays readable while the output is open, which is what makes + // the temp-and-rename route safe here. + got, err := io.ReadAll(input) + require.NoError(t, err) + assert.Equal(t, "ciphertext", string(got)) + + _, err = o.Write([]byte("plaintext")) + require.NoError(t, err) + require.NoError(t, o.Commit()) + + final, err := os.ReadFile(dest) + require.NoError(t, err) + assert.Equal(t, "plaintext", string(final)) +} + +// The temporary name is bounded by the fixed prefix, not by the destination's, +// so a destination whose own name is as long as the filesystem allows still +// opens. Deriving the prefix from the basename overflowed NAME_MAX here and +// failed before a byte had been read. +func TestOutputFileAcceptsMaximumLengthDestinationName(t *testing.T) { + dir := t.TempDir() + dest := filepath.Join(dir, strings.Repeat("n", 255)) + + // Not every filesystem allows a 255-byte component; probe rather than + // assume, so this reports the temp-name bug and nothing else. + probe, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { + t.Skipf("filesystem will not hold a 255-byte name: %v", err) + } + require.NoError(t, probe.Close()) + require.NoError(t, os.Remove(dest)) + + o, err := NewOutputFile(dest, testOutputFileMode, nil) + require.NoError(t, err) + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + require.NoError(t, o.Commit()) + + got, err := os.ReadFile(dest) + require.NoError(t, err) + assert.Equal(t, "payload", string(got)) +} + // plainCreateMode reports the permissions a plain create with mode produces in // dir under whatever umask the test process happens to be running with, which // is what a committed OutputFile is expected to match. Probing beats hardcoding @@ -220,15 +280,16 @@ func plainCreateMode(t *testing.T, dir string, mode os.FileMode) os.FileMode { } // tempSiblings returns any leftover temp files NewOutputFile would have created -// for dest in dir. -func tempSiblings(t *testing.T, dir, dest string) []string { +// in dir. The prefix carries no destination name, so this cannot be narrowed to +// one destination — which is fine, since every caller wants dir swept clean. +func tempSiblings(t *testing.T, dir string) []string { t.Helper() entries, err := os.ReadDir(dir) require.NoError(t, err) var found []string for _, e := range entries { - if strings.HasPrefix(e.Name(), "."+dest+".tmp-") { + if strings.HasPrefix(e.Name(), tempFilePrefix) { found = append(found, e.Name()) } } diff --git a/otdfctl/pkg/streamio/output_umask_unix_test.go b/otdfctl/pkg/streamio/output_umask_unix_test.go index f1535c904f..02371cb573 100644 --- a/otdfctl/pkg/streamio/output_umask_unix_test.go +++ b/otdfctl/pkg/streamio/output_umask_unix_test.go @@ -34,7 +34,7 @@ func TestOutputFileCommitHonorsUmask(t *testing.T) { dir := t.TempDir() dest := filepath.Join(dir, "out.tdf") - o, err := NewOutputFile(dest, testOutputFileMode) + o, err := NewOutputFile(dest, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("payload")) require.NoError(t, err) diff --git a/otdfctl/pkg/streamio/output_unix_test.go b/otdfctl/pkg/streamio/output_unix_test.go index cf3a8a5a6b..bfb8d89af4 100644 --- a/otdfctl/pkg/streamio/output_unix_test.go +++ b/otdfctl/pkg/streamio/output_unix_test.go @@ -3,6 +3,7 @@ package streamio import ( + "io" "os" "path/filepath" "testing" @@ -15,7 +16,7 @@ import ( // device node. `-o /dev/null` is a routine way to benchmark or smoke-test a // decrypt, and worked before the switch to atomic output. func TestOutputFileWritesThroughDevNull(t *testing.T) { - o, err := NewOutputFile(os.DevNull, testOutputFileMode) + o, err := NewOutputFile(os.DevNull, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("payload")) @@ -30,7 +31,7 @@ func TestOutputFileWritesThroughDevNull(t *testing.T) { // Cleanup removes the temp file it created; it must not remove a destination it // was only writing through, which is not its to delete. func TestOutputFileCleanupLeavesDirectDestination(t *testing.T) { - o, err := NewOutputFile(os.DevNull, testOutputFileMode) + o, err := NewOutputFile(os.DevNull, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("partial")) @@ -53,7 +54,7 @@ func TestOutputFileWritesThroughSymlink(t *testing.T) { require.NoError(t, os.WriteFile(target, []byte("stale"), 0o600)) require.NoError(t, os.Symlink(target, link)) - o, err := NewOutputFile(link, testOutputFileMode) + o, err := NewOutputFile(link, testOutputFileMode, nil) require.NoError(t, err) _, err = o.Write([]byte("payload")) require.NoError(t, err) @@ -68,6 +69,84 @@ func TestOutputFileWritesThroughSymlink(t *testing.T) { assert.Equal(t, "payload", string(got), "the write must land on the symlink's target") } +// The cost of writing through a symlink: the O_TRUNC open follows it, so a link +// resolving to the input empties the input before it has been read. The caller +// opened the input first precisely so that a failure costs nothing, and this is +// the one open that can undo that, so it must not happen. +func TestOutputFileRejectsSymlinkToInput(t *testing.T) { + dir := t.TempDir() + source := filepath.Join(dir, "source.tdf") + link := filepath.Join(dir, "link.tdf") + + require.NoError(t, os.WriteFile(source, []byte("ciphertext"), 0o600)) + require.NoError(t, os.Symlink(source, link)) + + input, err := os.Open(source) + require.NoError(t, err) + defer input.Close() + + _, err = NewOutputFile(link, testOutputFileMode, input) + require.ErrorIs(t, err, ErrOutputAliasesInput) + + got, err := io.ReadAll(input) + require.NoError(t, err) + assert.Equal(t, "ciphertext", string(got), "the input must still be readable after a rejected destination") +} + +// The guard has to be narrow enough to leave the case it was added beside +// working: a symlink to some other file is still written through. +func TestOutputFileWritesThroughSymlinkToOtherFile(t *testing.T) { + dir := t.TempDir() + source := filepath.Join(dir, "source.tdf") + target := filepath.Join(dir, "target.txt") + link := filepath.Join(dir, "link.txt") + + require.NoError(t, os.WriteFile(source, []byte("ciphertext"), 0o600)) + require.NoError(t, os.WriteFile(target, []byte("stale"), 0o600)) + require.NoError(t, os.Symlink(target, link)) + + input, err := os.Open(source) + require.NoError(t, err) + defer input.Close() + + o, err := NewOutputFile(link, testOutputFileMode, input) + require.NoError(t, err) + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + require.NoError(t, o.Commit()) + + got, err := os.ReadFile(target) + require.NoError(t, err) + assert.Equal(t, "payload", string(got)) +} + +// A dangling link resolves to nothing, so it aliases nothing and the open +// creates the target. Worth pinning: the alias check stats through the link, +// and a missing target must read as "no alias" rather than as an error. +func TestOutputFileWritesThroughDanglingSymlink(t *testing.T) { + dir := t.TempDir() + source := filepath.Join(dir, "source.tdf") + target := filepath.Join(dir, "not-yet.txt") + link := filepath.Join(dir, "link.txt") + + require.NoError(t, os.WriteFile(source, []byte("ciphertext"), 0o600)) + require.NoError(t, os.Symlink(target, link)) + + input, err := os.Open(source) + require.NoError(t, err) + defer input.Close() + + o, err := NewOutputFile(link, testOutputFileMode, input) + require.NoError(t, err) + _, err = o.Write([]byte("payload")) + require.NoError(t, err) + require.NoError(t, o.Commit()) + + got, err := os.ReadFile(target) + require.NoError(t, err) + assert.Equal(t, "payload", string(got)) +} + // A fifo has no seekable identity to rename over either, and opening one for // writing blocks until a reader arrives — so this only checks the routing // decision, not a full write. @@ -98,6 +177,6 @@ func TestDirectDestinationDetection(t *testing.T) { // A directory reaches the direct path and then fails to open, which is a better // outcome than os.CreateTemp succeeding inside it and the rename failing later. func TestOutputFileRejectsDirectoryDestination(t *testing.T) { - _, err := NewOutputFile(t.TempDir(), testOutputFileMode) + _, err := NewOutputFile(t.TempDir(), testOutputFileMode, nil) require.Error(t, err) }