From 8559e20237313ca0d7874673ecfe4c644aa0e3f3 Mon Sep 17 00:00:00 2001
From: Jakob Borg <jakob@kastelo.net>
Date: Wed, 23 Nov 2016 14:06:08 +0000
Subject: [PATCH] lib/osutil: Don't chmod in atomic file creation (fixes #2472)

Instead, trust (and test) that the temp file has appropriate permissions
from the start. The only place where this changes our behavior is for
ignores which go from 0644 to 0600. I'm OK with that.

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3756
---
 cmd/syncthing/gui_csrf.go      |  2 +-
 lib/config/wrapper.go          |  2 +-
 lib/model/model.go             |  2 +-
 lib/osutil/atomic.go           | 20 +++++++---------
 lib/osutil/atomic_test.go      |  4 ++--
 lib/osutil/atomic_unix_test.go | 44 ++++++++++++++++++++++++++++++++++
 6 files changed, 57 insertions(+), 17 deletions(-)
 create mode 100644 lib/osutil/atomic_unix_test.go

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())
+	}
+}