glog: have createInDir fail if the file already exists
This prevents an attack like the one described
[here](https://owasp.org/www-community/vulnerabilities/Insecure_Temporary_File#:~:text=On%20Unix%20based,with%20elevated%20permissions.).
An unprivileged attacker could use symlinks to trick a privileged
logging process to follow a symlink from the log dir and write logs over
an arbitrary file.
The components of the log names are program, host, username, tag, date,
time and PID. These are all predictable. It's not at all unusual for the
logdir to be writable by unprivileged users, and one of the fallback
directories (/tmp) traditionally has broad write privs with the sticky
bit set on Unix systems.
As a concrete example, let's say I've got a glog-enabled binary running
as a root cronjob. I can gauge when that cron job will run and then use
a bash script to spray the log dir with glog-looking symlinks to
`/etc/shadow` with predicted times and PIDs. When the cronjob runs, the
`os.Create` call will follow the symlink, truncate `/etc/shadow` and
then fill it with logs.
This change defeats that by setting `O_EXCL`, which will cause the open
call to fail if the file already exists.
Fixes CVE-2024-45339
cl/712795111 (google-internal)
diff --git a/glog_file.go b/glog_file.go
index 9a56a69..b54bd40 100644
--- a/glog_file.go
+++ b/glog_file.go
@@ -143,7 +143,12 @@
func createInDir(dir, tag string, t time.Time) (f *os.File, name string, err error) {
name, link := logName(tag, t)
fname := filepath.Join(dir, name)
- f, err = os.Create(fname)
+ // O_EXCL is important here, as it prevents a vulnerability. The general idea is that logs often
+ // live in an insecure directory (like /tmp), so an unprivileged attacker could create fname in
+ // advance as a symlink to a file the logging process can access, but the attacker cannot. O_EXCL
+ // fails the open if it already exists, thus prevent our this code from opening the existing file
+ // the attacker points us to.
+ f, err = os.OpenFile(fname, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
if err == nil {
symlink := filepath.Join(dir, link)
os.Remove(symlink) // ignore err
diff --git a/glog_test.go b/glog_test.go
index c1bf553..8fd15cd 100644
--- a/glog_test.go
+++ b/glog_test.go
@@ -772,3 +772,14 @@
len(c), logsink.MaxLogMessageLen, c)
}
}
+
+func TestCreateFailsIfExists(t *testing.T) {
+ tmp := t.TempDir()
+ now := time.Now()
+ if _, _, err := create("INFO", now, tmp); err != nil {
+ t.Errorf("create() failed on first call: %v", err)
+ }
+ if _, _, err := create("INFO", now, tmp); err == nil {
+ t.Errorf("create() succeeded on second call, want error")
+ }
+}
