From 8ae333442342f627d1f78d463bfe1f575cc257ae Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Mon, 17 Sep 2018 12:38:29 +0200 Subject: [PATCH] [libs/autofile & db/fsdb] Throw error if file permissions change (#2286) * Enforce file permissions in case they've changed * test behaviour for autofile * use testify in tests and rename `fInf` to `fileInfo` * return an error if file permissions have changed - if we can't read the file, we'll still panic * get rid of "github.com/pkg/errors" dependency * address review comments: - prefix instead of suffix - add state to err and construct formatting in Error() method * address review comments: - move error to libs/errors --- libs/autofile/autofile.go | 15 ++++++- libs/autofile/autofile_test.go | 73 +++++++++++++++++++++------------- libs/db/fsdb.go | 9 +++++ libs/errors/errors.go | 26 ++++++++++++ 4 files changed, 93 insertions(+), 30 deletions(-) create mode 100644 libs/errors/errors.go diff --git a/libs/autofile/autofile.go b/libs/autofile/autofile.go index b00585285..fa1eab20b 100644 --- a/libs/autofile/autofile.go +++ b/libs/autofile/autofile.go @@ -6,6 +6,7 @@ import ( "time" cmn "github.com/tendermint/tendermint/libs/common" + "github.com/tendermint/tendermint/libs/errors" ) /* AutoFile usage @@ -30,7 +31,10 @@ if err != nil { } */ -const autoFileOpenDuration = 1000 * time.Millisecond +const ( + autoFileOpenDuration = 1000 * time.Millisecond + autoFilePerms = os.FileMode(0600) +) // Automatically closes and re-opens file for writing. // This is useful for using a log file with the logrotate tool. @@ -116,10 +120,17 @@ func (af *AutoFile) Sync() error { } func (af *AutoFile) openFile() error { - file, err := os.OpenFile(af.Path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0600) + file, err := os.OpenFile(af.Path, os.O_RDWR|os.O_CREATE|os.O_APPEND, autoFilePerms) if err != nil { return err } + fileInfo, err := file.Stat() + if err != nil { + return err + } + if fileInfo.Mode() != autoFilePerms { + return errors.NewErrPermissionsChanged(file.Name(), fileInfo.Mode(), autoFilePerms) + } af.file = file return nil } diff --git a/libs/autofile/autofile_test.go b/libs/autofile/autofile_test.go index 67397380b..e8a9b3e4d 100644 --- a/libs/autofile/autofile_test.go +++ b/libs/autofile/autofile_test.go @@ -8,41 +8,33 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + cmn "github.com/tendermint/tendermint/libs/common" + "github.com/tendermint/tendermint/libs/errors" ) func TestSIGHUP(t *testing.T) { - // First, create an AutoFile writing to a tempfile dir file, err := ioutil.TempFile("", "sighup_test") - if err != nil { - t.Fatalf("Error creating tempfile: %v", err) - } - if err := file.Close(); err != nil { - t.Fatalf("Error closing tempfile: %v", err) - } + require.NoError(t, err) + err = file.Close() + require.NoError(t, err) name := file.Name() + // Here is the actual AutoFile af, err := OpenAutoFile(name) - if err != nil { - t.Fatalf("Error creating autofile: %v", err) - } + require.NoError(t, err) // Write to the file. _, err = af.Write([]byte("Line 1\n")) - if err != nil { - t.Fatalf("Error writing to autofile: %v", err) - } + require.NoError(t, err) _, err = af.Write([]byte("Line 2\n")) - if err != nil { - t.Fatalf("Error writing to autofile: %v", err) - } + require.NoError(t, err) // Move the file over err = os.Rename(name, name+"_old") - if err != nil { - t.Fatalf("Error moving autofile: %v", err) - } + require.NoError(t, err) // Send SIGHUP to self. oldSighupCounter := atomic.LoadInt32(&sighupCounter) @@ -55,16 +47,11 @@ func TestSIGHUP(t *testing.T) { // Write more to the file. _, err = af.Write([]byte("Line 3\n")) - if err != nil { - t.Fatalf("Error writing to autofile: %v", err) - } + require.NoError(t, err) _, err = af.Write([]byte("Line 4\n")) - if err != nil { - t.Fatalf("Error writing to autofile: %v", err) - } - if err := af.Close(); err != nil { - t.Fatalf("Error closing autofile") - } + require.NoError(t, err) + err = af.Close() + require.NoError(t, err) // Both files should exist if body := cmn.MustReadFile(name + "_old"); string(body) != "Line 1\nLine 2\n" { @@ -74,3 +61,33 @@ func TestSIGHUP(t *testing.T) { t.Errorf("Unexpected body %s", body) } } + +// Manually modify file permissions, close, and reopen using autofile: +// We expect the file permissions to be changed back to the intended perms. +func TestOpenAutoFilePerms(t *testing.T) { + file, err := ioutil.TempFile("", "permission_test") + require.NoError(t, err) + err = file.Close() + require.NoError(t, err) + name := file.Name() + + // open and change permissions + af, err := OpenAutoFile(name) + require.NoError(t, err) + err = af.file.Chmod(0755) + require.NoError(t, err) + err = af.Close() + require.NoError(t, err) + + // reopen and expect an ErrPermissionsChanged as Cause + af, err = OpenAutoFile(name) + require.Error(t, err) + if e, ok := err.(*errors.ErrPermissionsChanged); ok { + t.Logf("%v", e) + } else { + t.Errorf("unexpected error %v", e) + } + + err = af.Close() + require.NoError(t, err) +} diff --git a/libs/db/fsdb.go b/libs/db/fsdb.go index fc861decc..92c059d42 100644 --- a/libs/db/fsdb.go +++ b/libs/db/fsdb.go @@ -10,7 +10,9 @@ import ( "sync" "github.com/pkg/errors" + cmn "github.com/tendermint/tendermint/libs/common" + tmerrors "github.com/tendermint/tendermint/libs/errors" ) const ( @@ -205,6 +207,13 @@ func write(path string, d []byte) error { return err } defer f.Close() + fInfo, err := f.Stat() + if err != nil { + return err + } + if fInfo.Mode() != keyPerm { + return tmerrors.NewErrPermissionsChanged(f.Name(), keyPerm, fInfo.Mode()) + } _, err = f.Write(d) if err != nil { return err diff --git a/libs/errors/errors.go b/libs/errors/errors.go new file mode 100644 index 000000000..ae5d94392 --- /dev/null +++ b/libs/errors/errors.go @@ -0,0 +1,26 @@ +// Package errors contains errors that are thrown across packages. +package errors + +import ( + "fmt" + "os" +) + +// ErrPermissionsChanged occurs if the file permission have changed since the file was created. +type ErrPermissionsChanged struct { + name string + got, want os.FileMode +} + +func NewErrPermissionsChanged(name string, got, want os.FileMode) *ErrPermissionsChanged { + return &ErrPermissionsChanged{name: name, got: got, want: want} +} + +func (e ErrPermissionsChanged) Error() string { + return fmt.Sprintf( + "file: [%v]\nexpected file permissions: %v, got: %v", + e.name, + e.want, + e.got, + ) +}