Browse Source

libs/os: avoid CopyFile truncating destination before checking if regular file (#6428)

This change fixes a potential exploitable vulnerability
that can cause the WAL to be consistently truncated by falsely
supplying the WAL path which would be any arbitrary dirrectory.

Fixes #6427
pull/6437/head
Emmanuel T Odeke 4 years ago
committed by GitHub
parent
commit
6fdf665385
No known key found for this signature in database GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 48 additions and 3 deletions
  1. +7
    -3
      libs/os/os.go
  2. +41
    -0
      libs/os/os_test.go

+ 7
- 3
libs/os/os.go View File

@ -1,6 +1,7 @@
package os
import (
"errors"
"fmt"
"io"
"os"
@ -50,16 +51,19 @@ func FileExists(filePath string) bool {
// CopyFile copies a file. It truncates the destination file if it exists.
func CopyFile(src, dst string) error {
info, err := os.Stat(src)
srcfile, err := os.Open(src)
if err != nil {
return err
}
defer srcfile.Close()
srcfile, err := os.Open(src)
info, err := srcfile.Stat()
if err != nil {
return err
}
defer srcfile.Close()
if info.IsDir() {
return errors.New("cannot read from directories")
}
// create new file, truncate if exists and apply same permissions as the original one
dstfile, err := os.OpenFile(dst, os.O_RDWR|os.O_CREATE|os.O_TRUNC, info.Mode().Perm())


+ 41
- 0
libs/os/os_test.go View File

@ -135,3 +135,44 @@ func newTestProgram(t *testing.T, environVar string) (cmd *exec.Cmd, stdout *byt
return
}
// Ensure that using CopyFile does not truncate the destination file before
// the origin is positively a non-directory and that it is ready for copying.
// See https://github.com/tendermint/tendermint/issues/6427
func TestTrickedTruncation(t *testing.T) {
tmpDir, err := ioutil.TempDir(os.TempDir(), "pwn_truncate")
if err != nil {
t.Fatal(err)
}
defer os.Remove(tmpDir)
originalWALPath := filepath.Join(tmpDir, "wal")
originalWALContent := []byte("I AM BECOME DEATH, DESTROYER OF ALL WORLDS!")
if err := ioutil.WriteFile(originalWALPath, originalWALContent, 0755); err != nil {
t.Fatal(err)
}
// 1. Sanity check.
readWAL, err := ioutil.ReadFile(originalWALPath)
if err != nil {
t.Fatal(err)
}
if !bytes.Equal(readWAL, originalWALContent) {
t.Fatalf("Cannot proceed as the content does not match\nGot: %q\nWant: %q", readWAL, originalWALContent)
}
// 2. Now cause the truncation of the original file.
// It is absolutely legal to invoke os.Open on a directory.
if err := tmos.CopyFile(tmpDir, originalWALPath); err == nil {
t.Fatal("Expected an error")
}
// 3. Check the WAL's content
reReadWAL, err := ioutil.ReadFile(originalWALPath)
if err != nil {
t.Fatal(err)
}
if !bytes.Equal(reReadWAL, originalWALContent) {
t.Fatalf("Oops, the WAL's content was changed :(\nGot: %q\nWant: %q", reReadWAL, originalWALContent)
}
}

Loading…
Cancel
Save