fix(security): restrict temp file permissions and validate memory_limit#368
Conversation
- chmod 0600 on CreateTemp files in restore, delete, and duckdb profiling to prevent world-readable parquet data and profile output (closes #362) - validate memory_limit with regex before SQL interpolation in compaction subprocess to prevent injection via config (closes #363) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds explicit file permission settings for temporary files and introduces regex-based validation for DuckDB memory limits. The review feedback points out that the os.Chmod calls are redundant as os.CreateTemp already defaults to 0600 permissions. Additionally, the reviewer suggests centralizing the memory limit validation logic to ensure consistency and prevent potential SQL injection in other database configurations.
| if err := os.Chmod(tempFile.Name(), 0600); err != nil { | ||
| tempFile.Close() | ||
| os.Remove(tempFile.Name()) | ||
| return 0, fmt.Errorf("failed to secure temp file: %w", err) | ||
| } |
There was a problem hiding this comment.
The call to os.Chmod is redundant here. In Go, os.CreateTemp already creates files with 0600 permissions (readable and writable only by the owner) by default. Adding an explicit Chmod adds unnecessary complexity and an extra error handling path for a condition that is already guaranteed by the standard library.
| if err := os.Chmod(tmpFile.Name(), 0600); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpFile.Name()) | ||
| return 0, fmt.Errorf("failed to secure temp file: %w", err) | ||
| } |
There was a problem hiding this comment.
This os.Chmod call is redundant. Go's os.CreateTemp implementation ensures that the temporary file is created with 0600 permissions. You can safely remove this block to simplify the code and avoid unnecessary system calls.
| if !memLimitRe.MatchString(config.MemoryLimit) { | ||
| return nil, fmt.Errorf("invalid memory_limit value: %q", config.MemoryLimit) | ||
| } |
There was a problem hiding this comment.
The addition of memory_limit validation is a significant security improvement to prevent SQL injection. However, to ensure consistency and prevent similar vulnerabilities elsewhere, consider centralizing this regex (e.g., in a shared utility package) and applying the same validation to the main database configuration in internal/database/duckdb.go, which currently interpolates the MemoryLimit value without similar validation.
| if err := os.Chmod(tmpFile.Name(), 0600); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpFile.Name()) | ||
| // Fall back to regular query | ||
| rows, err := conn.QueryContext(ctx, query) | ||
| if err != nil { | ||
| conn.Close() | ||
| return nil, nil, nil, err | ||
| } | ||
| return rows, conn, nil, nil | ||
| } |
There was a problem hiding this comment.
This block is redundant and introduces significant code duplication. os.CreateTemp already ensures 0600 permissions. Furthermore, the error handling logic here (lines 564-572) is a duplicate of the fallback logic already implemented for the os.CreateTemp failure above (lines 556-561). Removing this block improves maintainability without compromising security.
Centralize memory_limit validation in config.Load() with a regex guard instead of at each call site. os.CreateTemp already creates 0600 files so chmod calls were redundant and have been removed. Addresses Gemini review feedback on #368. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed Gemini feedback:
Net: +7 -27 lines. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
os.Chmod(0600)on allos.CreateTemp()calls inrestore.go,delete.go, andduckdb.go— temp files were created 0644 (world-readable), exposing Parquet data and DuckDB profile outputmemory_limitconfig value against^\d+(\.\d+)?\s*(B|KB|MB|GB|TB|%)?$before interpolating intoSET memory_limit='...'SQL in compaction subprocessTest plan
go build ./...passesgo test ./internal/backup/... ./internal/api/... ./internal/database/... ./internal/compaction/...all passmemory_limitvalues (e.g."1GB; DROP TABLE") are rejected at startup