From 53a5d00fffca113d9ea21c91cbb9634d2528b074 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 25 Jun 2024 23:25:44 -0400 Subject: [PATCH] net/dns: ensure /etc/resolv.conf is world-readable even with a umask Previously, if we had a umask set (e.g. 0027) that prevented creating a world-readable file, /etc/resolv.conf would be created without the o+r bit and thus other users may be unable to resolve DNS. Since a umask only applies to file creation, chmod the file after creation and before renaming it to ensure that it has the appropriate permissions. Updates #12609 Signed-off-by: Andrew Dunham Change-Id: I2a05d64f4f3a8ee8683a70be17a7da0e70933137 --- net/dns/direct.go | 27 +++++++++++++++++++--- net/dns/direct_unix_test.go | 43 +++++++++++++++++++++++++++++++++++ net/dns/manager_linux_test.go | 5 ++-- net/dns/wsl_windows.go | 4 ++++ 4 files changed, 74 insertions(+), 5 deletions(-) create mode 100644 net/dns/direct_unix_test.go diff --git a/net/dns/direct.go b/net/dns/direct.go index 23a9a04df..aaff18fcb 100644 --- a/net/dns/direct.go +++ b/net/dns/direct.go @@ -276,6 +276,14 @@ func (m *directManager) rename(old, new string) error { return fmt.Errorf("writing to %q in rename of %q: %w", new, old, err) } + // Explicitly set the permissions on the new file. This ensures that + // if we have a umask set which prevents creating world-readable files, + // the file will still have the correct permissions once it's renamed + // into place. See #12609. + if err := m.fs.Chmod(new, 0644); err != nil { + return fmt.Errorf("chmod %q in rename of %q: %w", new, old, err) + } + if err := m.fs.Remove(old); err != nil { err2 := m.fs.Truncate(old) if err2 != nil { @@ -467,6 +475,14 @@ func (m *directManager) atomicWriteFile(fs wholeFileFS, filename string, data [] if err := fs.WriteFile(tmpName, data, perm); err != nil { return fmt.Errorf("atomicWriteFile: %w", err) } + // Explicitly set the permissions on the temporary file before renaming + // it. This ensures that if we have a umask set which prevents creating + // world-readable files, the file will still have the correct + // permissions once it's renamed into place. See #12609. + if err := fs.Chmod(tmpName, perm); err != nil { + return fmt.Errorf("atomicWriteFile: Chmod: %w", err) + } + return m.rename(tmpName, filename) } @@ -475,10 +491,11 @@ func (m *directManager) atomicWriteFile(fs wholeFileFS, filename string, data [] // // All name parameters are absolute paths. type wholeFileFS interface { - Stat(name string) (isRegular bool, err error) - Rename(oldName, newName string) error - Remove(name string) error + Chmod(name string, mode os.FileMode) error ReadFile(name string) ([]byte, error) + Remove(name string) error + Rename(oldName, newName string) error + Stat(name string) (isRegular bool, err error) Truncate(name string) error WriteFile(name string, contents []byte, perm os.FileMode) error } @@ -502,6 +519,10 @@ func (fs directFS) Stat(name string) (isRegular bool, err error) { return fi.Mode().IsRegular(), nil } +func (fs directFS) Chmod(name string, mode os.FileMode) error { + return os.Chmod(fs.path(name), mode) +} + func (fs directFS) Rename(oldName, newName string) error { return os.Rename(fs.path(oldName), fs.path(newName)) } diff --git a/net/dns/direct_unix_test.go b/net/dns/direct_unix_test.go new file mode 100644 index 000000000..bffa6ade9 --- /dev/null +++ b/net/dns/direct_unix_test.go @@ -0,0 +1,43 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +//go:build unix + +package dns + +import ( + "context" + "os" + "path/filepath" + "syscall" + "testing" +) + +func TestWriteFileUmask(t *testing.T) { + // Set a umask that disallows world-readable files for the duration of + // this test. + oldUmask := syscall.Umask(0027) + defer syscall.Umask(oldUmask) + + tmp := t.TempDir() + fs := directFS{prefix: tmp} + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + m := directManager{logf: t.Logf, fs: fs, ctx: ctx, ctxClose: cancel} + + const perms = 0644 + if err := m.atomicWriteFile(fs, "resolv.conf", []byte("nameserver 8.8.8.8\n"), perms); err != nil { + t.Fatal(err) + } + + // Ensure that the created file has the world-readable bit set. + fi, err := os.Stat(filepath.Join(tmp, "resolv.conf")) + if err != nil { + t.Fatal(err) + } + if got := fi.Mode().Perm(); got != perms { + t.Fatalf("file mode: got 0o%o, want 0o%o", got, perms) + } +} diff --git a/net/dns/manager_linux_test.go b/net/dns/manager_linux_test.go index c60701295..605344c06 100644 --- a/net/dns/manager_linux_test.go +++ b/net/dns/manager_linux_test.go @@ -313,8 +313,9 @@ func (m memFS) Stat(name string) (isRegular bool, err error) { return false, nil } -func (m memFS) Rename(oldName, newName string) error { panic("TODO") } -func (m memFS) Remove(name string) error { panic("TODO") } +func (m memFS) Chmod(name string, mode os.FileMode) error { panic("TODO") } +func (m memFS) Rename(oldName, newName string) error { panic("TODO") } +func (m memFS) Remove(name string) error { panic("TODO") } func (m memFS) ReadFile(name string) ([]byte, error) { v, ok := m[name] if !ok { diff --git a/net/dns/wsl_windows.go b/net/dns/wsl_windows.go index b769405fb..8b0780f55 100644 --- a/net/dns/wsl_windows.go +++ b/net/dns/wsl_windows.go @@ -159,6 +159,10 @@ func (fs wslFS) Stat(name string) (isRegular bool, err error) { return true, nil } +func (fs wslFS) Chmod(name string, perm os.FileMode) error { + return wslRun(fs.cmd("chmod", "--", fmt.Sprintf("%04o", perm), name)) +} + func (fs wslFS) Rename(oldName, newName string) error { return wslRun(fs.cmd("mv", "--", oldName, newName)) }