From 6cda8dca01ef6eb689eaaa15364271e1b485c7be Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 15 Jan 2020 20:53:53 +0100 Subject: [PATCH 1/3] cli,log: change the default file permissions Prior to this patch, files created by a CockroachDB node were created with the default umask (usualy 0777) and permission bits 0644, i.e. `-rw-r--r--`. This situation is defective because in most common deployments umask does not exclude the "other" bits and the resulting data, log and temp files are world-readable. This is unsafe because any of these files can contain sensitive information. As Aaron puts it: > If another service on the system has an arbitrary file read > vulnerability but it is running under its own user account, limiting > these permissions would limit the potential impact of such a > vulnerability. Whereas in the current setup, an attacker could real > CRDB sensitive information from any other compromised user access. This patch rectifies this situation by enforcing at least bits 0007 in the umask early in `cockroach start`, so that created files, directories, symlinks etc have at most `-rw-rw----` (most will be `-rw-r-----`). Release note (security update): A CockroachDB node process (`start` / `start-single-node`) now configures its umask to create all its files without unix permission bits for "others", so that data/log/etc files do not become world-readable by default in systems that do not otherwise customize umask to enforce log file visibility. The files produced by other `cockroach` commands (e.g. the CLI commands) do not force their umask. Note that after upgrading to this release, in sites where permissions matter administrators should be careful to run `chmod -R o-rwx` in directories where files were created by a previous version. Release note (backward-incompatible change): CockroachDB now creates files without read permissions for the "others" group. Sites that automate file management (e.g. log collection) using multiple user accounts now must ensure that the CockroachDB server and the management tools running on the same system are part of a shared unix group. --- pkg/cli/start.go | 9 +++++++++ pkg/cli/start_unix.go | 6 ++++++ pkg/cli/start_windows.go | 4 ++++ pkg/util/log/file.go | 2 +- 4 files changed, 20 insertions(+), 1 deletion(-) diff --git a/pkg/cli/start.go b/pkg/cli/start.go index 97a54642d6be..796750450ad2 100644 --- a/pkg/cli/start.go +++ b/pkg/cli/start.go @@ -445,6 +445,15 @@ func runStart(cmd *cobra.Command, args []string, disableReplication bool) error return err } + // Change the permission mask for all created files. + // + // We're considering everything produced by a cockroach node + // to potentially contain sensitive information, so it should + // not be world-readable. + disableOtherPermissionBits() + + // TODO(knz): the following call is not in the right place. + // See: https://github.com/cockroachdb/cockroach/issues/44041 if s, err := serverCfg.Stores.GetPreventedStartupMessage(); err != nil { return err } else if s != "" { diff --git a/pkg/cli/start_unix.go b/pkg/cli/start_unix.go index b3e50afb4e9f..0e73c99ad9b6 100644 --- a/pkg/cli/start_unix.go +++ b/pkg/cli/start_unix.go @@ -88,3 +88,9 @@ func maybeRerunBackground() (bool, error) { } return false, nil } + +func disableOtherPermissionBits() { + mask := unix.Umask(0000) + mask |= 00007 + _ = unix.Umask(mask) +} diff --git a/pkg/cli/start_windows.go b/pkg/cli/start_windows.go index e4e10f700d24..b3549eb1fec4 100644 --- a/pkg/cli/start_windows.go +++ b/pkg/cli/start_windows.go @@ -25,3 +25,7 @@ func handleSignalDuringShutdown(os.Signal) { func maybeRerunBackground() (bool, error) { return false, nil } + +func disableOtherPermissionBits() { + // No-op on windows, which does not support umask. +} diff --git a/pkg/util/log/file.go b/pkg/util/log/file.go index 197c33acd313..836e238b7ecd 100644 --- a/pkg/util/log/file.go +++ b/pkg/util/log/file.go @@ -246,7 +246,7 @@ func create( fname := filepath.Join(dir, name) // Open the file os.O_APPEND|os.O_CREATE rather than use os.Create. // Append is almost always more efficient than O_RDRW on most modern file systems. - f, err = os.OpenFile(fname, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0664) + f, err = os.OpenFile(fname, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) if err == nil { symlink := filepath.Join(dir, link) From c787f74c8f61a5d0000c783c840a405b332f6dc2 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 15 Jan 2020 22:51:18 +0100 Subject: [PATCH 2/3] cli/haproxy: don't make the output file executable Release note (bug fix): The file generated by `cockroach gen haproxy` does not get an executable bit any more. The executable bit was previously placed in error. --- pkg/cli/haproxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cli/haproxy.go b/pkg/cli/haproxy.go index c8baaa0e87a2..b1e861d3e6ef 100644 --- a/pkg/cli/haproxy.go +++ b/pkg/cli/haproxy.go @@ -236,7 +236,7 @@ func runGenHAProxyCmd(cmd *cobra.Command, args []string) error { var f *os.File if haProxyPath == "-" { w = os.Stdout - } else if f, err = os.OpenFile(haProxyPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0755); err != nil { + } else if f, err = os.OpenFile(haProxyPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644); err != nil { return err } else { w = f From 3b9cffc82a285d34585f03418f1d3b43747fd16d Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 15 Jan 2020 22:52:16 +0100 Subject: [PATCH 3/3] cli: minor perm fixups Release note: None --- pkg/cli/sql_test.go | 2 +- pkg/cli/systembench/disk_bench.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cli/sql_test.go b/pkg/cli/sql_test.go index 412ad5281006..e56c59b3c28b 100644 --- a/pkg/cli/sql_test.go +++ b/pkg/cli/sql_test.go @@ -75,7 +75,7 @@ select ''' for _, test := range tests { // Populate the test input. - if f, err = os.OpenFile(fname, os.O_WRONLY, 0666); err != nil { + if f, err = os.OpenFile(fname, os.O_WRONLY, 0644); err != nil { fmt.Fprintln(stderr, err) return } diff --git a/pkg/cli/systembench/disk_bench.go b/pkg/cli/systembench/disk_bench.go index c2af2aa38f44..65d67610fd2a 100644 --- a/pkg/cli/systembench/disk_bench.go +++ b/pkg/cli/systembench/disk_bench.go @@ -151,7 +151,7 @@ func newTempFile(dir string) (*os.File, error) { } return os.OpenFile(tempFileName, - os.O_RDWR|os.O_APPEND, 0660) + os.O_RDWR|os.O_APPEND, 0640) } // Run runs I/O benchmarks specified by diskOpts.