diff --git a/cmd/syncthing/gui_csrf.go b/cmd/syncthing/gui_csrf.go index 481d8a36..30520d31 100644 --- a/cmd/syncthing/gui_csrf.go +++ b/cmd/syncthing/gui_csrf.go @@ -116,7 +116,7 @@ func saveCsrfTokens() { // nothing relevant we can do about them anyway... name := locations[locCsrfTokens] - f, err := osutil.CreateAtomic(name, 0600) + f, err := osutil.CreateAtomic(name) if err != nil { return } diff --git a/lib/config/wrapper.go b/lib/config/wrapper.go index a911e591..d6699d7e 100644 --- a/lib/config/wrapper.go +++ b/lib/config/wrapper.go @@ -325,7 +325,7 @@ func (w *Wrapper) Device(id protocol.DeviceID) (DeviceConfiguration, bool) { // Save writes the configuration to disk, and generates a ConfigSaved event. func (w *Wrapper) Save() error { - fd, err := osutil.CreateAtomic(w.path, 0600) + fd, err := osutil.CreateAtomic(w.path) if err != nil { l.Debugln("CreateAtomic:", err) return err diff --git a/lib/model/model.go b/lib/model/model.go index 644f0744..80018aba 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -1248,7 +1248,7 @@ func (m *Model) SetIgnores(folder string, content []string) error { path := filepath.Join(cfg.Path(), ".stignore") - fd, err := osutil.CreateAtomic(path, 0644) + fd, err := osutil.CreateAtomic(path) if err != nil { l.Warnln("Saving .stignore:", err) return err diff --git a/lib/osutil/atomic.go b/lib/osutil/atomic.go index a5385c95..ebdabe24 100644 --- a/lib/osutil/atomic.go +++ b/lib/osutil/atomic.go @@ -29,23 +29,19 @@ type AtomicWriter struct { err error } -// CreateAtomic is like os.Create with a FileMode, except a temporary file -// name is used instead of the given name. -func CreateAtomic(path string, mode os.FileMode) (*AtomicWriter, error) { +// CreateAtomic is like os.Create, except a temporary file name is used +// instead of the given name. The file is created with secure (0600) +// permissions. +func CreateAtomic(path string) (*AtomicWriter, error) { + // The security of this depends on the tempfile having secure + // permissions, 0600, from the beginning. This is what ioutil.TempFile + // does. We have a test that verifies that that is the case, should this + // ever change in the standard library in the future. fd, err := ioutil.TempFile(filepath.Dir(path), TempPrefix) if err != nil { return nil, err } - // chmod fails on Android so don't even try - if runtime.GOOS != "android" { - if err := os.Chmod(fd.Name(), mode); err != nil { - fd.Close() - os.Remove(fd.Name()) - return nil, err - } - } - w := &AtomicWriter{ path: path, next: fd, diff --git a/lib/osutil/atomic_test.go b/lib/osutil/atomic_test.go index b07838b9..28c6aad8 100644 --- a/lib/osutil/atomic_test.go +++ b/lib/osutil/atomic_test.go @@ -21,7 +21,7 @@ func TestCreateAtomicCreate(t *testing.T) { t.Fatal(err) } - w, err := CreateAtomic("testdata/file", 0644) + w, err := CreateAtomic("testdata/file") if err != nil { t.Fatal(err) } @@ -63,7 +63,7 @@ func TestCreateAtomicReplace(t *testing.T) { t.Fatal(err) } - w, err := CreateAtomic("testdata/file", 0644) + w, err := CreateAtomic("testdata/file") if err != nil { t.Fatal(err) } diff --git a/lib/osutil/atomic_unix_test.go b/lib/osutil/atomic_unix_test.go new file mode 100644 index 00000000..c9d88096 --- /dev/null +++ b/lib/osutil/atomic_unix_test.go @@ -0,0 +1,44 @@ +// Copyright (C) 2016 The Syncthing Authors. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +//+build !windows + +// (No syscall.Umask or the equivalent on Windows) + +package osutil + +import ( + "io/ioutil" + "os" + "syscall" + "testing" +) + +func TestTempFilePermissions(t *testing.T) { + // Set a zero umask, so any files created will have the permission bits + // asked for in the create call and nothing less. + oldMask := syscall.Umask(0) + defer syscall.Umask(oldMask) + + fd, err := ioutil.TempFile("", "test") + if err != nil { + t.Fatal(err) + } + + info, err := fd.Stat() + if err != nil { + t.Fatal(err) + } + defer os.Remove(fd.Name()) + defer fd.Close() + + // The temp file should have 0600 permissions at the most, or we have a + // security problem in CreateAtomic. + t.Logf("Got 0%03o", info.Mode()) + if info.Mode()&^0600 != 0 { + t.Errorf("Permission 0%03o is too generous", info.Mode()) + } +}