security: hardening against TOCTOU/symlink attacks by sjinks · Pull Request #56 · Automattic/cron-control-runner

sjinks · 2026-05-18T01:35:28Z

Fix insecure temporary log file handling in remote command execution.

Previously, the code performed a check-then-remove sequence before creating /tmp/wp-cli- log files:

stat(path) -> remove(path) -> open(path, O_CREATE)

That pattern created a TOCTOU window where an attacker could swap in a symlink and cause writes/truncation against an unintended target.

This change hardens all relevant file-open paths:

Replace stat/remove/create flow with openOrCreateLogFileForWrite().
Create new files atomically using O_CREATE|O_EXCL.
Add O_NOFOLLOW to prevent following symlinks on create/read/truncate paths.
When reusing an existing GUID file, open with O_TRUNC|O_NOFOLLOW and verify the opened file is a regular file before use.
Update streamLogs to open directly with O_NOFOLLOW and remove the separate pre-open existence check.

Added regression tests in remote/log_file_security_test.go:

creates a new log file safely
truncates an existing regular file safely
rejects symlink log paths and confirms target content is unchanged

Fix insecure temporary log file handling in remote command execution. Previously, the code performed a check-then-remove sequence before creating /tmp/wp-cli-<GUID> log files: - stat(path) -> remove(path) -> open(path, O_CREATE) That pattern created a TOCTOU window where an attacker could swap in a symlink and cause writes/truncation against an unintended target. This change hardens all relevant file-open paths: - Replace stat/remove/create flow with openOrCreateLogFileForWrite(). - Create new files atomically using O_CREATE|O_EXCL. - Add O_NOFOLLOW to prevent following symlinks on create/read/truncate paths. - When reusing an existing GUID file, open with O_TRUNC|O_NOFOLLOW and verify the opened file is a regular file before use. - Update streamLogs to open directly with O_NOFOLLOW and remove the separate pre-open existence check. Added regression tests in remote/log_file_security_test.go: - creates a new log file safely - truncates an existing regular file safely - rejects symlink log paths and confirms target content is unchanged

Copilot

Pull request overview

Hardens the WP-CLI remote log file handling in /tmp/wp-cli-<GUID> against TOCTOU/symlink attacks by replacing the stat-then-remove-then-create sequence with an atomic open helper, and adds O_NOFOLLOW to related read paths. Regression tests cover create, truncate-existing, and symlink-rejection cases.

Changes:

Introduce openOrCreateLogFileForWrite that uses O_CREATE|O_EXCL|O_NOFOLLOW, falling back to O_TRUNC|O_NOFOLLOW plus a post-open IsRegular() check.
Add syscall.O_NOFOLLOW to the read-side opens in runWpCliCmdRemote and streamLogs, and drop the now-redundant pre-open os.Stat check in streamLogs.
Add remote/log_file_security_test.go covering create, truncate, and symlink-rejection behaviors.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
remote/remote.go	Replaces unsafe stat/remove/create pattern with atomic helper; adds `O_NOFOLLOW` to read paths.
remote/log_file_security_test.go	New tests verifying the helper's behavior on new files, existing regular files, and symlinks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

remote/remote.go:1074

Manually calling logFile.Truncate(0) after opening leaves a small window where another writer (or a stale process holding the same path) could observe a non-empty file. Passing os.O_TRUNC in the OpenFile flags would truncate atomically at open time and is functionally equivalent here (the file-type check has already gated us to a regular file via O_NOFOLLOW+fstat). The explicit Truncate also makes O_APPEND semantics on the fd slightly confusing because the offset/size are mutated out of band.

	if err := logFile.Truncate(0); err != nil {
		_ = logFile.Close()
		return nil, err
	}

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

remote/remote.go:1140

The PR description states the reuse path should "open with O_TRUNC|O_NOFOLLOW", but the implementation here opens with O_APPEND|O_WRONLY|O_SYNC|O_NOFOLLOW|O_NONBLOCK (no O_TRUNC) and then calls Truncate(0) after a regular-file check. Either the description or the implementation should be updated so the two agree. The implementation choice (open + stat + truncate) is fine for safety, but readers comparing the description to the code will be confused.

	logFile, err = os.OpenFile(logFileName, os.O_APPEND|os.O_WRONLY|os.O_SYNC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0)
	if err != nil {
		return nil, err
	}

	fileInfo, err := logFile.Stat()
	if err != nil {
		_ = logFile.Close()
		return nil, err
	}

	if !fileInfo.Mode().IsRegular() {
		_ = logFile.Close()
		return nil, fmt.Errorf("log file path %q is not a regular file", logFileName)
	}

	stat, ok := fileInfo.Sys().(*syscall.Stat_t)
	if !ok {
		_ = logFile.Close()
		return nil, fmt.Errorf("could not determine owner for log file path %q", logFileName)
	}

	if int(stat.Uid) != os.Geteuid() {
		_ = logFile.Close()
		return nil, fmt.Errorf("log file path %q is not owned by current user", logFileName)
	}

	if err := syscall.SetNonblock(int(logFile.Fd()), false); err != nil {
		_ = logFile.Close()
		return nil, err
	}

	if err := logFile.Truncate(0); err != nil {

+
+func openOrCreateLogFileForWrite(logFileName string) (*os.File, error) {
+	// Use atomic create without following symlinks to avoid TOCTOU attacks on /tmp paths.
+	logFile, err := os.OpenFile(logFileName, os.O_APPEND|os.O_WRONLY|os.O_CREATE|os.O_EXCL|os.O_SYNC|syscall.O_NOFOLLOW, 0600)


+func TestStreamLogs_MissingFileMessage(t *testing.T) {
+	serverConn, clientConn := net.Pipe()
+	defer serverConn.Close()
+	defer clientConn.Close()
+
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		streamLogs(serverConn, "guid-that-does-not-exist")
+	}()
+
+	if err := clientConn.SetReadDeadline(time.Now().Add(2 * time.Second)); err != nil {
+		t.Fatalf("setReadDeadline() error = %v", err)
+	}
+
+	buf := make([]byte, 512)
+	n, err := clientConn.Read(buf)
+	if err != nil {
+		t.Fatalf("read() error = %v", err)
+	}
+
+	got := string(buf[:n])
+	if got != "The WP CLI log file for GUID guid-that-does-not-exist does not exist\n" {
+		t.Fatalf("unexpected message: %q", got)
+	}
+
+	select {
+	case <-done:
+	case <-time.After(2 * time.Second):
+		t.Fatal("streamLogs did not return")
+	}
+}


 	for {
 		read, err = logFile.Read(buf)
 		if io.EOF == err {
 			break
 		}
-		conn.Write(buf[:read])
+		if err != nil {
+			log.Printf("error reading the WP CLI log file: %s\n", err.Error())
+			break
+		}
+		if err = writeAll(conn, buf[:read]); err != nil {
+			log.Printf("error writing the WP CLI log stream: %s\n", err.Error())
+			break
+		}
 	}


+		conn.Write([]byte("unable to launch the remote WP CLI process"))
 		conn.Close()
 		return fmt.Errorf("runWpCliCmdRemote: error creating the WP CLI log file: %s", err.Error())
 	}

 	watcher, err := fsnotify.NewWatcher()
 	if err != nil {
-		conn.Write([]byte("unable to launch the remote WP CLI process: " + err.Error()))
+		conn.Write([]byte("unable to launch the remote WP CLI process"))


+	logFile, err = os.OpenFile(logFileName, os.O_APPEND|os.O_WRONLY|os.O_SYNC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0)
+	if err != nil {
+		return nil, err
+	}
+
+	fileInfo, err := logFile.Stat()
+	if err != nil {
+		_ = logFile.Close()
+		return nil, err
+	}
+
+	if !fileInfo.Mode().IsRegular() {
+		_ = logFile.Close()
+		return nil, fmt.Errorf("log file path %q is not a regular file", logFileName)
+	}
+
+	stat, ok := fileInfo.Sys().(*syscall.Stat_t)
+	if !ok {
+		_ = logFile.Close()
+		return nil, fmt.Errorf("could not determine owner for log file path %q", logFileName)
+	}
+
+	if int(stat.Uid) != os.Geteuid() {
+		_ = logFile.Close()
+		return nil, fmt.Errorf("log file path %q is not owned by current user", logFileName)
+	}
+
+	if err := syscall.SetNonblock(int(logFile.Fd()), false); err != nil {
+		_ = logFile.Close()
+		return nil, err
+	}
+
+	if err := logFile.Truncate(0); err != nil {
+		_ = logFile.Close()
+		return nil, err
+	}
+
+	return logFile, nil


+func openLogFileForRead(logFileName string) (*os.File, error) {
+	// Validate with a non-blocking open first to avoid hanging on FIFOs/special files.
+	logFile, err := os.OpenFile(logFileName, os.O_RDONLY|os.O_SYNC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0)
+	if err != nil {
+		return nil, err
+	}
+
+	fileInfo, err := logFile.Stat()
+	if err != nil {
+		_ = logFile.Close()
+		return nil, err
+	}
+
+	if !fileInfo.Mode().IsRegular() {
+		_ = logFile.Close()
+		return nil, fmt.Errorf("log file path %q is not a regular file", logFileName)
+	}
+
+	if err := syscall.SetNonblock(int(logFile.Fd()), false); err != nil {
+		_ = logFile.Close()
+		return nil, err
+	}
+
+	return logFile, nil
+}


sjinks self-assigned this May 18, 2026

sjinks marked this pull request as ready for review May 18, 2026 01:36

Copilot AI review requested due to automatic review settings May 18, 2026 01:36

Copilot started reviewing on behalf of sjinks May 18, 2026 01:36 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread remote/remote.go Outdated

fix: address code review comments

aa2561e

sjinks requested a review from Copilot May 18, 2026 01:41

Copilot started reviewing on behalf of sjinks May 18, 2026 01:42 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread remote/remote.go

Comment thread remote/remote.go Outdated

Comment thread remote/remote.go

fix: address code review comments

f396fc9

sjinks requested a review from Copilot May 18, 2026 02:01

Copilot started reviewing on behalf of sjinks May 18, 2026 02:01 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

sjinks requested a review from Copilot May 18, 2026 02:04

Copilot started reviewing on behalf of sjinks May 18, 2026 02:04 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security: hardening against TOCTOU/symlink attacks#56

security: hardening against TOCTOU/symlink attacks#56
sjinks wants to merge 3 commits into
trunkfrom
pltfrm-2258-toctou-race-condition-on-temporary-log-files

sjinks commented May 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sjinks commented May 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants