Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## 2024-05-24 - [Insecure Temporary File Creation]
**Vulnerability:** Predictable temporary file names (e.g., `path + ".tmp"`) were used with `os.WriteFile` in shared directories like `os.TempDir()`. This allows symlink attacks where a malicious user could pre-create the predictable symlink, causing `os.WriteFile` to overwrite arbitrary files the application user has access to.
**Learning:** `os.WriteFile` follows symlinks. When performing atomic file writes via temporary files, predictable names should not be used in shared temporary directories.
**Prevention:** Always use `os.CreateTemp` to generate unpredictable, securely-created temporary files (it uses `O_EXCL` when opening) before writing data and subsequently calling `os.Rename`.
22 changes: 20 additions & 2 deletions internal/burnrate/burnrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,28 @@ func LoadOrCreateSnapshotAt(sessionID string, currentCost float64, now time.Time
return nil, false, err
}

tmpPath := path + ".tmp"
if err := os.WriteFile(tmpPath, data, 0644); err != nil {
// Use os.CreateTemp for secure temporary file creation
tmpFile, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+"-*")
if err != nil {
return nil, false, err
}
tmpPath := tmpFile.Name()

if _, err := tmpFile.Write(data); err != nil {
tmpFile.Close()
os.Remove(tmpPath)
return nil, false, err
}
if err := tmpFile.Close(); err != nil {
os.Remove(tmpPath)
return nil, false, err
}

if err := os.Chmod(tmpPath, 0644); err != nil {
os.Remove(tmpPath)
return nil, false, err
}

if err := os.Rename(tmpPath, path); err != nil {
os.Remove(tmpPath)
return nil, false, err
Expand Down
27 changes: 24 additions & 3 deletions internal/sparkline/sparkline.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,32 @@ func Save(sessionID, metric string, b *Buffer) {
if err != nil {
return
}
tmp := path + ".tmp"
if err := os.WriteFile(tmp, data, 0644); err != nil {

// Use os.CreateTemp for secure temporary file creation
tmpFile, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+"-*")
if err != nil {
return
}
tmp := tmpFile.Name()

if _, err := tmpFile.Write(data); err != nil {
tmpFile.Close()
os.Remove(tmp)
return
}
if err := tmpFile.Close(); err != nil {
os.Remove(tmp)
return
}

if err := os.Chmod(tmp, 0644); err != nil {
os.Remove(tmp)
return
}
os.Rename(tmp, path)

if err := os.Rename(tmp, path); err != nil {
os.Remove(tmp)
}
Comment on lines +147 to +166
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the changes in burnrate.go, consider using tmpFile.Sync() for durability and defer os.Remove(tmp) to simplify the cleanup logic. This makes the atomic write process more robust.

	tmp := tmpFile.Name()
	defer os.Remove(tmp)

	if _, err := tmpFile.Write(data); err != nil {
		tmpFile.Close()
		return
	}
	if err := tmpFile.Sync(); err != nil {
		tmpFile.Close()
		return
	}
	if err := tmpFile.Close(); err != nil {
		return
	}

	if err := os.Chmod(tmp, 0644); err != nil {
		return
	}

	if err := os.Rename(tmp, path); err != nil {
		return
	}

}

// PushAndSave is a convenience that loads, pushes, saves, and returns the buffer
Expand Down
Loading